Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571202 > unrolled thread

Re: [PATCH 2/8] power: add power sequence library

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/8] power: add power sequence library "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-01 00:20 +0100
    Re: [PATCH 2/8] power: add power sequence library Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-01 09:10 +0100
      Re: [PATCH 2/8] power: add power sequence library Peter Chen <hzpeterchen@gmail.com> - 2017-02-03 09:20 +0100
        Re: [PATCH 2/8] power: add power sequence library "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-03 22:20 +0100
          Re: [PATCH 2/8] power: add power sequence library Peter Chen <hzpeterchen@gmail.com> - 2017-02-04 03:00 +0100

#1571202 — Re: [PATCH 2/8] power: add power sequence library

On Tue, Jan 3, 2017 at 7:33 AM, Peter Chen <peter.chen@nxp.com> wrote:
> We have an well-known problem that the device needs to do some power
> sequence before it can be recognized by related host, the typical
> example like hard-wired mmc devices and usb devices.
>
> This power sequence is hard to be described at device tree and handled by
> related host driver, so we have created a common power sequence
> library to cover this requirement. The core code has supplied
> some common helpers for host driver, and individual power sequence
> libraries handle kinds of power sequence for devices. The pwrseq
> librares always need to allocate extra instance for compatible
> string match.
>
> pwrseq_generic is intended for general purpose of power sequence, which
> handles gpios and clocks currently, and can cover other controls in
> future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> if only one power sequence is needed, else call of_pwrseq_on_list
> /of_pwrseq_off_list instead (eg, USB hub driver).
>
> For new power sequence library, it can add its compatible string
> to pwrseq_of_match_table, then the pwrseq core will match it with
> DT's, and choose this library at runtime.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>

Quite honestly, I have a really hard time with trying to follow this
code and the total lack of documentation makes it even harder.  In
particular, the generic power sequence code is not even commented at
all, so it really is hard to say how this is going to work, let alone
deciding whether or not to apply it.

Plus, of course, the USB core changes must be acked by the maintainer
thereof for me to be able to handle the series.

But at this point I basically need you to explain the design to me, please.

Thanks,
Rafael

[toc] | [next] | [standalone]

#1571318

On Wed, Feb 01, 2017 at 12:10:17AM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 3, 2017 at 7:33 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > We have an well-known problem that the device needs to do some power
> > sequence before it can be recognized by related host, the typical
> > example like hard-wired mmc devices and usb devices.
> >
> > This power sequence is hard to be described at device tree and handled by
> > related host driver, so we have created a common power sequence
> > library to cover this requirement. The core code has supplied
> > some common helpers for host driver, and individual power sequence
> > libraries handle kinds of power sequence for devices. The pwrseq
> > librares always need to allocate extra instance for compatible
> > string match.
> >
> > pwrseq_generic is intended for general purpose of power sequence, which
> > handles gpios and clocks currently, and can cover other controls in
> > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > if only one power sequence is needed, else call of_pwrseq_on_list
> > /of_pwrseq_off_list instead (eg, USB hub driver).
> >
> > For new power sequence library, it can add its compatible string
> > to pwrseq_of_match_table, then the pwrseq core will match it with
> > DT's, and choose this library at runtime.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Quite honestly, I have a really hard time with trying to follow this
> code and the total lack of documentation makes it even harder.  In
> particular, the generic power sequence code is not even commented at
> all, so it really is hard to say how this is going to work, let alone
> deciding whether or not to apply it.
> 
> Plus, of course, the USB core changes must be acked by the maintainer
> thereof for me to be able to handle the series.

Ah crap, I wanted you to explain it as I too couldn't figure it out :)

> But at this point I basically need you to explain the design to me, please.

Same here.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1572937

On Wed, Feb 01, 2017 at 09:08:17AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 01, 2017 at 12:10:17AM +0100, Rafael J. Wysocki wrote:
> > On Tue, Jan 3, 2017 at 7:33 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > We have an well-known problem that the device needs to do some power
> > > sequence before it can be recognized by related host, the typical
> > > example like hard-wired mmc devices and usb devices.
> > >
> > > This power sequence is hard to be described at device tree and handled by
> > > related host driver, so we have created a common power sequence
> > > library to cover this requirement. The core code has supplied
> > > some common helpers for host driver, and individual power sequence
> > > libraries handle kinds of power sequence for devices. The pwrseq
> > > librares always need to allocate extra instance for compatible
> > > string match.
> > >
> > > pwrseq_generic is intended for general purpose of power sequence, which
> > > handles gpios and clocks currently, and can cover other controls in
> > > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > > if only one power sequence is needed, else call of_pwrseq_on_list
> > > /of_pwrseq_off_list instead (eg, USB hub driver).
> > >
> > > For new power sequence library, it can add its compatible string
> > > to pwrseq_of_match_table, then the pwrseq core will match it with
> > > DT's, and choose this library at runtime.
> > >
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > Quite honestly, I have a really hard time with trying to follow this
> > code and the total lack of documentation makes it even harder. 

Sorry about that, Is it ok I add the design doc at:
Documentation/power/power-sequence/design.rst?

> > particular, the generic power sequence code is not even commented at
> > all, 

The generic power sequence code just implements the APIs which are 
called at power/pwrseq/core.c, and those API are commented at
include/linux/power/pwrseq.h. Anyway, I will add more comments at it.
	
-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1573451

On Friday, February 03, 2017 04:16:15 PM Peter Chen wrote:
> On Wed, Feb 01, 2017 at 09:08:17AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 01, 2017 at 12:10:17AM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Jan 3, 2017 at 7:33 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > We have an well-known problem that the device needs to do some power
> > > > sequence before it can be recognized by related host, the typical
> > > > example like hard-wired mmc devices and usb devices.
> > > >
> > > > This power sequence is hard to be described at device tree and handled by
> > > > related host driver, so we have created a common power sequence
> > > > library to cover this requirement. The core code has supplied
> > > > some common helpers for host driver, and individual power sequence
> > > > libraries handle kinds of power sequence for devices. The pwrseq
> > > > librares always need to allocate extra instance for compatible
> > > > string match.
> > > >
> > > > pwrseq_generic is intended for general purpose of power sequence, which
> > > > handles gpios and clocks currently, and can cover other controls in
> > > > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > > > if only one power sequence is needed, else call of_pwrseq_on_list
> > > > /of_pwrseq_off_list instead (eg, USB hub driver).
> > > >
> > > > For new power sequence library, it can add its compatible string
> > > > to pwrseq_of_match_table, then the pwrseq core will match it with
> > > > DT's, and choose this library at runtime.
> > > >
> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > > 
> > > Quite honestly, I have a really hard time with trying to follow this
> > > code and the total lack of documentation makes it even harder. 
> 
> Sorry about that, Is it ok I add the design doc at:
> Documentation/power/power-sequence/design.rst?

You can do that if you think it will address the request to explain the design.

> > > particular, the generic power sequence code is not even commented at
> > > all, 
> 
> The generic power sequence code just implements the APIs which are 
> called at power/pwrseq/core.c, and those API are commented at
> include/linux/power/pwrseq.h. Anyway, I will add more comments at it.

It actually seems to be doing more than that and I'm not sure why the code in
core.c is necessary at all.  The "generic" thing seems to be the only user of
it anyway and the callbacks seem to be tailored to its needs.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1573572

On Fri, Feb 03, 2017 at 10:14:31PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 03, 2017 04:16:15 PM Peter Chen wrote:
> > On Wed, Feb 01, 2017 at 09:08:17AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Feb 01, 2017 at 12:10:17AM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Jan 3, 2017 at 7:33 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > We have an well-known problem that the device needs to do some power
> > > > > sequence before it can be recognized by related host, the typical
> > > > > example like hard-wired mmc devices and usb devices.
> > > > >
> > > > > This power sequence is hard to be described at device tree and handled by
> > > > > related host driver, so we have created a common power sequence
> > > > > library to cover this requirement. The core code has supplied
> > > > > some common helpers for host driver, and individual power sequence
> > > > > libraries handle kinds of power sequence for devices. The pwrseq
> > > > > librares always need to allocate extra instance for compatible
> > > > > string match.
> > > > >
> > > > > pwrseq_generic is intended for general purpose of power sequence, which
> > > > > handles gpios and clocks currently, and can cover other controls in
> > > > > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > > > > if only one power sequence is needed, else call of_pwrseq_on_list
> > > > > /of_pwrseq_off_list instead (eg, USB hub driver).
> > > > >
> > > > > For new power sequence library, it can add its compatible string
> > > > > to pwrseq_of_match_table, then the pwrseq core will match it with
> > > > > DT's, and choose this library at runtime.
> > > > >
> > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > > > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > > > 
> > > > Quite honestly, I have a really hard time with trying to follow this
> > > > code and the total lack of documentation makes it even harder. 
> > 
> > Sorry about that, Is it ok I add the design doc at:
> > Documentation/power/power-sequence/design.rst?
> 
> You can do that if you think it will address the request to explain the design.
> 
> > > > particular, the generic power sequence code is not even commented at
> > > > all, 
> > 
> > The generic power sequence code just implements the APIs which are 
> > called at power/pwrseq/core.c, and those API are commented at
> > include/linux/power/pwrseq.h. Anyway, I will add more comments at it.
> 
> It actually seems to be doing more than that and I'm not sure why the code in
> core.c is necessary at all.  The "generic" thing seems to be the only user of
> it anyway and the callbacks seem to be tailored to its needs.
> 

Currently, the "generic" pwrseq is the only use case. But some devices may have
custom power sequence [1], and MMC devices (mmc card/wifi) have two
power sequences (drivers/mmc/core/pwrseq_emmc.c, drivers/mmc/core/pwrseq_simple.c)

I have an example use case at v7 named pwrseq_compatible_sample.c for
those custom use case [2].

[1] https://www.spinics.net/lists/devicetree/msg139756.html
[2] https://lkml.org/lkml/2016/9/19/972

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web