Hi,

On Tue, Apr 2, 2013 at 1:53 PM, Felipe Balbi <[email protected]> wrote:
> On Mon, Apr 01, 2013 at 07:24:00PM +0530, Vivek Gautam wrote:
>> Adding APIs to handle runtime power management on PHY
>> devices. PHY consumers may need to wake-up/suspend PHYs
>> when they work across autosuspend.
>>
>> Signed-off-by: Vivek Gautam <[email protected]>
>> ---
>> include/linux/usb/phy.h | 141 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 141 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
>> index 6b5978f..01bf9c1 100644
>> --- a/include/linux/usb/phy.h
>> +++ b/include/linux/usb/phy.h
>> @@ -297,4 +297,145 @@ static inline const char *usb_phy_type_string(enum usb_phy_type type)
>> return "UNKNOWN PHY TYPE";
>> }
>> }
>> +
>> +static inline void usb_phy_autopm_enable(struct usb_phy *x)
>> +{
>> + if (!x || !x->dev) {
>> + dev_err(x->dev, "no PHY or attached device available\n");
>> + return;
>> + }
>
> wrong indentation, also, I'm not sure we should allow calls with NULL
> pointers. Perhaps a WARN() so we get API offenders early enough ?

True, bad coding style :-(
We should be handling dev_err with a NULL pointer.
Will just keep here:
if (WARN_ON(!x->dev))
return .... ;

--
Thanks & Regards
Vivek

2013-04-02 12:11:14

Hi,

On Wed, Apr 03, 2013 at 11:48:39AM +0530, Vivek Gautam wrote:
> >> Adding APIs to handle runtime power management on PHY
> >> devices. PHY consumers may need to wake-up/suspend PHYs
> >> when they work across autosuspend.
> >>
> >> Signed-off-by: Vivek Gautam <[email protected]>
> >> ---
> >> include/linux/usb/phy.h | 141
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 files changed, 141 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> >> index 6b5978f..01bf9c1 100644
> >> --- a/include/linux/usb/phy.h
> >> +++ b/include/linux/usb/phy.h
> >> @@ -297,4 +297,145 @@ static inline const char *usb_phy_type_string(enum
> >> usb_phy_type type)
> >> return "UNKNOWN PHY TYPE";
> >> }
> >> }
> >> +
> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x)
> >> +{
> >> + if (!x || !x->dev) {
> >> + dev_err(x->dev, "no PHY or attached device available\n");
> >> + return;
> >> + }
> >> +
> >> + pm_runtime_enable(x->dev);
> >> +}
> >
> >
> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable
> > here. Generally runtime_enable and runtime_disable is done in probe and
> > remove of a driver respectively. So it's better to leave the
> > runtime_enable/runtime_disable to be done in *phy provider* driver than
> > having an API for it to be done by *phy user* driver. Felipe, what do you
> > think?
>
> Thanks!!
> That's very true, runtime_enable() and runtime_disable() calls are made by
> *phy_provider* only. But a querry here.
> Wouldn't in any case a PHY consumer might want to disable runtime_pm on PHY ?
> Say, when consumer failed to suspend the PHY properly
> (*put_sync(phy->dev)* fails), how much sure is the consumer about the
> state of PHY ?

no no, wait a minute. We might not want to enable runtime pm for the PHY
until the UDC says it can handle runtime pm, no ? I guess this makes a
bit of sense (at least in my head :-p).

Imagine if PHY is runtime suspended but e.g. DWC3 isn't runtime pm
enabled... Does it make sense to leave that control to the USB
controller drivers ?

I'm open for suggestions

--
balbi

Attachments:

(No filename) (2.17 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-04-03 08:17:28

HI,

On Wed, Apr 03, 2013 at 06:42:48PM +0530, Vivek Gautam wrote:
> >> >> Adding APIs to handle runtime power management on PHY
> >> >> devices. PHY consumers may need to wake-up/suspend PHYs
> >> >> when they work across autosuspend.
> >> >>
> >> >> Signed-off-by: Vivek Gautam <[email protected]>
> >> >> ---
> >> >> include/linux/usb/phy.h | 141
> >> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> >> 1 files changed, 141 insertions(+), 0 deletions(-)
> >> >>
> >> >> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> >> >> index 6b5978f..01bf9c1 100644
> >> >> --- a/include/linux/usb/phy.h
> >> >> +++ b/include/linux/usb/phy.h
> >> >> @@ -297,4 +297,145 @@ static inline const char *usb_phy_type_string(enum
> >> >> usb_phy_type type)
> >> >> return "UNKNOWN PHY TYPE";
> >> >> }
> >> >> }
> >> >> +
> >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x)
> >> >> +{
> >> >> + if (!x || !x->dev) {
> >> >> + dev_err(x->dev, "no PHY or attached device available\n");
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> + pm_runtime_enable(x->dev);
> >> >> +}
> >> >
> >> >
> >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable
> >> > here. Generally runtime_enable and runtime_disable is done in probe and
> >> > remove of a driver respectively. So it's better to leave the
> >> > runtime_enable/runtime_disable to be done in *phy provider* driver than
> >> > having an API for it to be done by *phy user* driver. Felipe, what do you
> >> > think?
> >>
> >> Thanks!!
> >> That's very true, runtime_enable() and runtime_disable() calls are made by
> >> *phy_provider* only. But a querry here.
> >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on PHY ?
> >> Say, when consumer failed to suspend the PHY properly
> >> (*put_sync(phy->dev)* fails), how much sure is the consumer about the
> >> state of PHY ?
> >
> > no no, wait a minute. We might not want to enable runtime pm for the PHY
> > until the UDC says it can handle runtime pm, no ? I guess this makes a
> > bit of sense (at least in my head :-p).
> >
> > Imagine if PHY is runtime suspended but e.g. DWC3 isn't runtime pm
> > enabled... Does it make sense to leave that control to the USB
> > controller drivers ?
> >
> > I'm open for suggestions
>
> Of course unless the PHY consumer can handle runtime PM for PHY,
> PHY should not ideally be going into runtime_suspend.
>
> Actually trying out few things, here are my observations
>
> Enabling runtime_pm on PHY pushes PHY to go into runtime_suspend state.
> But a device detection wakes up DWC3 controller, and if i don't wake
> up PHY (using get_sync(phy->dev)) here
> in runtime_resume() callback of DWC3, i don't get PHY back in active state.
> So it becomes the duty of DWC3 controller to handle PHY's sleep and wake-up.
> Thereby it becomes logical that DWC3 controller has the right to
> enable runtime_pm
> of PHY.
>
> But there's a catch here. if there are multiple consumers of PHY (like
> USB2 type PHY can
> have DWC3 controller as well as EHCI/OHCI or even HSGadget) then in that case,
> only one of the consumer can enable runtime_pm on PHY. So who decides this.
>
> Aargh!! lot of confusion here :-(

hmmm, maybe add a flag to struct usb_phy and check it on
usb_phy_autopm_enable() ??

How does usbcore handle it ? They request class drivers to pass
supports_autosuspend, but while we should have a similar flag, that's
not enough. We also need a flag to tell us when pm_runtime has already
been enabled.

So how about:

usb_phy_autopm_enable()
{
if (!phy->suports_autosuspend)
return -ENOSYS;

if (phy->autosuspend_enabled)
return 0;

phy->autosuspend_enabled = true;
return pm_runtime_enable(phy->dev);
}

???

--
balbi

Attachments:

(No filename) (3.75 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-04-03 13:57:22

2013-04-04 07:18:56

Hi,

On Thu, Apr 04, 2013 at 02:26:51PM +0530, Vivek Gautam wrote:
> >> > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy,
> >> > > it will try to go into suspend state and thereby call runtime_suspend(), if any.
> >> > > And PHY will come to active state only when its consumer wakes it up,
> >> > > and this consumer is operational
> >> > > only when its related PHY is in fully functional state.
> >> > > So do we have a situation in which this PHY goes into low power state
> >> > > in its runtime_suspend(),
> >> > > resulting in non-detection of devices on further attach (since PHY is
> >> > > in low power state) ?
> >> > >
> >> > > Will the controller (like EHCI/OHCI) be functional now ?
> >> >
> >> > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(),
> >> > right ? (so does DWC3 :-)
> >>
> >> Maybe you guys have already got this all figured out -- if so, feel
> >> free to ignore this email.
> >>
> >> Some subsystems handle this issue by calling pm_runtime_get_sync()
> >> before probing a driver and pm_runtime_put_sync() after unbinding the
> >> driver. If the driver is runtime-PM-enabled, it then does its own
> >> put_sync near the end of its probe routine and get_sync in its release
> >> routine.
> >
> > sounds a bit 'fishy' to me... So a separate entity would call
> > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops,
> > then drivers need to check if runtime_pm is enabled and call
> > pm_runtime_put*() conditionally before returning from probe(). One
> > remove, we might have another issue: device is already runtime_suspended
> > (due to e.g. autosuspend) when module is removed, a call to
> > pm_runtime_put_sync() will be unbalanced. No ?
>
> May be i am misinterpreting !!
> If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*),
> then the consumers
> need to call pm_runtime_get_sync whever they want to access PHY.

Alright, so here's my understanding:

I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said
that it could be done before that so that DWC3 sees an enabled PHY
during probe.

--
balbi

Attachments:

(No filename) (2.06 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-04-04 14:46:23

by Alan Stern

[permalink] [raw]

Subject: Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management

On Thu, 4 Apr 2013, Felipe Balbi wrote:

> > >> Some subsystems handle this issue by calling pm_runtime_get_sync()
> > >> before probing a driver and pm_runtime_put_sync() after unbinding the
> > >> driver. If the driver is runtime-PM-enabled, it then does its own
> > >> put_sync near the end of its probe routine and get_sync in its release
> > >> routine.
> > >
> > > sounds a bit 'fishy' to me... So a separate entity would call
> > > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops,

I don't know what you mean by "separate entity". The PHY's subsystem
would handle this. After all, the subsystem has to handle registering
the PHY in the first place.

If the PHY doesn't have a dev_pm_ops, why are you talking about doing
runtime PM on it? That doesn't make any sense.

> > > then drivers need to check if runtime_pm is enabled and call
> > > pm_runtime_put*() conditionally before returning from probe(). One

They don't have to check. If CONFIG_PM_RUNTIME isn't set or the target
is runtime-PM-disabled then pm_runtime_put* is essentially a no-op (in
the disabled case it decrements the usage counter but doesn't do
anything else).

One possible complication I did not mention: The scheme described above
assumes the device that uses the PHY will always be registered on the
same type of bus. If the device can be used on multiple bus types (and
hence in multiple subsystems) then things aren't so simple, because
some of the subsystems might support runtime PM and others might not.
(You may very well run into this problem with USB controllers that are
sometimes on a PCI bus and sometimes on a platform bus.) In this case,
the driver needs to adapt to the subsystem's capabilities. Presumably
the bus-glue part of the driver takes care of this.

> > > remove, we might have another issue: device is already runtime_suspended
> > > (due to e.g. autosuspend) when module is removed, a call to
> > > pm_runtime_put_sync() will be unbalanced. No ?

No. I left out some of the details. For one thing, the subsystem is
careful to do a runtime resume before calling the driver's remove
method. (Also, if you look over my original description carefully,
you'll see that there are no unbalanced calls -- even if the device is
already runtime suspended when the driver is unbound.)

For another, the subsystem needs to check before calling the driver's
runtime-PM methods. There are two brief windows during which the
driver isn't in charge of the device even though dev->driver is set.
Those windows occur in the subsystem's probe routine (before it calls
the driver's probe method) and in the subsystem's remove routine
(after it calls the driver's remove method). At such times, the
subsystem's runtime-PM handlers must be careful _not_ to call the
driver's runtime-PM routines.

> > May be i am misinterpreting !!
> > If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*),
> > then the consumers
> > need to call pm_runtime_get_sync whever they want to access PHY.

No, because in addition to being runtime-PM enabled, the PHY should
automatically be runtime resumed when the consumer registers itself as
a user of the PHY. Therefore the consumer doesn't need to do anything
at all -- which is good for consumers that aren't runtime-PM aware.

> Alright, so here's my understanding:
>
> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said
> that it could be done before that so that DWC3 sees an enabled PHY
> during probe.

Basically right. Help me to understand the overall situation a little
better:

What code registers the PHY initially?

What routine does the DWC3 driver call to register itself
as a consumer of the PHY?

Likewise, what routine does it call to unregister itself?

Alan Stern

2013-04-04 16:40:24

by Sarah Sharp

[permalink] [raw]

Subject: Re: [PATCH v3 00/11] usb: dwc3/xhci/phy: Enable runtime power management

On Thu, Apr 04, 2013 at 01:02:45PM +0530, Vivek Gautam wrote:
> On Thu, Apr 4, 2013 at 12:40 PM, Felipe Balbi <[email protected]> wrote:
> > On Thu, Apr 04, 2013 at 10:34:57AM +0530, Vivek Gautam wrote:
> >> Hi Sarah,
> >>
> >>
> >> On Wed, Apr 3, 2013 at 10:57 PM, Sarah Sharp
> >> <[email protected]> wrote:
> >> > Question: Do you still need this patch for 3.10?
> >>
> >> Felipe's 'next' is closed for 3.10, so this series won't be making it
> >> to 3.10 now, as a whole. :-(
> >
> > right, besides we're still discussing what to do with the whole PHY
> > part, right ?
>
> Right ofcourse. :-)

Ok, so it sounds like I shouldn't merge that patch for 3.10. Please
include that patch in your next round of revisions instead. And now I'm
glad I'm slow. :)

Sarah Sharp

2013-04-18 11:50:35

by Kishon Vijay Abraham I

[permalink] [raw]

Subject: Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management

On Tuesday 02 April 2013 06:10 PM, Vivek Gautam wrote:
> Hi,
>
>
> On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <[email protected]> wrote:
>> Hi,
>>
>> On Tue, Apr 02, 2013 at 04:04:01PM +0530, Vivek Gautam wrote:
>>>> On Mon, Apr 01, 2013 at 07:24:00PM +0530, Vivek Gautam wrote:
>>>>> Adding APIs to handle runtime power management on PHY
>>>>> devices. PHY consumers may need to wake-up/suspend PHYs
>>>>> when they work across autosuspend.
>>>>>
>>>>> Signed-off-by: Vivek Gautam <[email protected]>
>>>>> ---
>>>>> include/linux/usb/phy.h | 141 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 files changed, 141 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
>>>>> index 6b5978f..01bf9c1 100644
>>>>> --- a/include/linux/usb/phy.h
>>>>> +++ b/include/linux/usb/phy.h
>>>>> @@ -297,4 +297,145 @@ static inline const char *usb_phy_type_string(enum usb_phy_type type)
>>>>> return "UNKNOWN PHY TYPE";
>>>>> }
>>>>> }
>>>>> +
>>>>> +static inline void usb_phy_autopm_enable(struct usb_phy *x)
>>>>> +{
>>>>> + if (!x || !x->dev) {
>>>>> + dev_err(x->dev, "no PHY or attached device available\n");
>>>>> + return;
>>>>> + }
>>>>
>>>> wrong indentation, also, I'm not sure we should allow calls with NULL
>>>> pointers. Perhaps a WARN() so we get API offenders early enough ?
>>>
>>> True, bad coding style :-(
>>> We should be handling dev_err with a NULL pointer.
>>> Will just keep here:
>>> if (WARN_ON(!x->dev))
>>> return .... ;
>>
>> right, but I guess:
>>
>> if (WARN(!x || !x->dev, "Invalid parameters\n"))
>> return -EINVAL;
>>
>> would be better ??

btw, shouldn't it be IS_ERR(x)?

Thanks
Kishon

2013-04-23 11:16:20

On Tue, Apr 23, 2013 at 11:42 PM, Alan Stern <[email protected]> wrote:
> On Tue, 23 Apr 2013, Vivek Gautam wrote:
>
>> Hi,
>>
>>
>> On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <[email protected]> wrote:
>> > On Tue, 23 Apr 2013, Vivek Gautam wrote:
>> >
>> >> >> Alright, so here's my understanding:
>> >> >>
>> >> >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said
>> >> >> that it could be done before that so that DWC3 sees an enabled PHY
>> >> >> during probe.
>> >> >
>> >> > Basically right. Help me to understand the overall situation a little
>> >> > better:
>> >> >
>> >> > What code registers the PHY initially?
>> >> PHY is added to global list by PHY drivers (like
>> >> phy-samsung-usb2.c/phy-omap-usb2.c)
>> >> by usb_add_phy() API
>> >
>> > Then this routine should initialize the PHY. The initialized state
>> > could be either active or suspended, your choice. Suspended would be
>> > best, in case the PHY never gets used.
>>
>> Fair enough.
>>
>> >
>> >> > What routine does the DWC3 driver call to register itself
>> >> > as a consumer of the PHY?
>> >> I think DWC3 doesn't registers itself as consumer of PHY,
>> >> rather it gets that PHY from
>> >> the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API.
>> >> DWC3 can now call PHY's initialization sequence using usb_phy_init().
>> >> So, before DWC3 initializes the PHY, PHYs should be in active state.
>> >
>> > Then usb_phy_init should make sure the PHY is in the active state. If
>> > usb_add_phy() left the PHY suspended, then this routine should call
>> > pm_runtime_get_sync().
>>
>> Right
>>
>> >
>> > After DWC3 (or any other driver) has acquired the PHY, it can call
>> > pm_runtime_put/get() however it likes, so long as the calls balance
>> > properly. If the driver isn't runtime-PM aware then it won't use any
>> > of these calls, and the PHY will remain active the entire time.
>>
>> Alright, so DWC3 (or any other consumer of PHY) should do minimal to
>> handle runtime state of PHYs; get() when accessing PHY and put() when it's done
>> with it.
>
> Yes. The first operation will generally be a put, because
> usb_phy_init() will leave the PHY in an active state.

Alright.

>
>> >> > Likewise, what routine does it call to unregister itself?
>> >> Once DWC3's remove() is called PHYs are put.
>> >
>> > Is there a routine analogous to usb_phy_init() that gets called when
>> > PHY is released? That routine would do the opposite of usb_phy_init(),
>> > putting the PHY back into its initialized state.
>>
>> Yes, ofcourse there's a routine usb_phy_shutdown(). So this will be
>> calling put_sync()
>> to put PHYs back to its initialized state. Right ?
>
> Correct.

Hmm.

Thanks for explaining things here and keeping patience to my queries :-)

Felipe, thanks to you too for the discussion :-)
I shall update the patchset asap.

--
Best Regards
Vivek