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

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

[Vivek Gautam]

This patch-series enables runtime power management on xhci-plat,
dwc3-core, dwc3-exynos as well as on Samsung's USB 2.0 type and
USB 3.0 type PHYs.

Based on 'next' branch of Felipe Balbi's USB tree.

Changes from v2:
 - Using separate functions for USB PHY runtime power management, instead of
   using macros.
 - Adding 'pm_runtime_set_suspended()' api call in dwc3 core layer before
   enabling runtime pm. (Ideally, we should be explicitly make device
   'suspended' or 'active' before enabling runtime pm on it).
 - Checking return code for 'put_sync' and 'get_sync' of USB-PHYs when
   waking up or suspending them from dwc3 core's runtime_pm callbacks.
 - Removed buggy pm_runtime_put() calls from driver's (xhci, dwc3 and PHY)
   remove functions.
 - Adding a patch to enable runtime power management of Samsung's USB 2.0 PHY
   (usb: phy: samsung: Enable runtime power management on usb2phy)

Changes from v1:
 - Adding required PHY APIs to handle runtime power management
   instead of directly twiddling with phy->dev.
 - Handling runtime power management of usb PHYs in dwc3 core
   driver instead of in any glue layer.
 - Splitting the patch:
   [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb
   into required number to bifurcate functionality.

Vivek Gautam (11):
  usb: phy: Add APIs for runtime power management
  USB: dwc3: Adjust runtime pm to allow autosuspend
  usb: dwc3: Enable runtime pm only after PHYs are initialized
  usb: dwc3: Add runtime power management callbacks
  usb: dwc3: exynos: Enable runtime power management
  usb: xhci: Enable runtime pm in xhci-plat
  usb: phy: samsung: Enable runtime power management on usb2phy
  usb: phy: samsung: Enable runtime power management on usb3phy
  usb: phy: samsung: Add support for external reference clock
  usb: phy: samsung: Add support for PHY ref_clk gpio
  usb: phy: samsung: Add support for PHY refclk switching

 drivers/usb/dwc3/core.c            |   59 ++++++++++++++--
 drivers/usb/dwc3/dwc3-exynos.c     |   12 +++
 drivers/usb/host/xhci-plat.c       |    6 ++
 drivers/usb/phy/phy-samsung-usb.c  |   26 +++++++
 drivers/usb/phy/phy-samsung-usb.h  |    1 +
 drivers/usb/phy/phy-samsung-usb2.c |    5 ++
 drivers/usb/phy/phy-samsung-usb3.c |  119 +++++++++++++++++++++++++++++--
 include/linux/usb/phy.h            |  141 ++++++++++++++++++++++++++++++++++++
 8 files changed, 358 insertions(+), 11 deletions(-)

-- 
1.7.6.5

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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);
+}
+
+static inline void usb_phy_autopm_disable(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return;
+	}
+
+	pm_runtime_disable(x->dev);
+}
+
+static inline int usb_phy_autopm_get(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_get(x->dev);
+}
+
+static inline int usb_phy_autopm_get_sync(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_get_sync(x->dev);
+}
+
+static inline int usb_phy_autopm_put(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_put(x->dev);
+}
+
+static inline int usb_phy_autopm_put_sync(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_put_sync(x->dev);
+}
+
+static inline void usb_phy_autopm_allow(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return;
+	}
+
+	pm_runtime_allow(x->dev);
+}
+
+static inline void usb_phy_autopm_forbid(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return;
+	}
+
+	pm_runtime_forbid(x->dev);
+}
+
+static inline int usb_phy_autopm_set_active(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_set_active(x->dev);
+}
+
+static inline void usb_phy_autopm_set_suspended(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return;
+	}
+
+	pm_runtime_set_suspended(x->dev);
+}
+
+static inline bool usb_phy_autopm_suspended(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return 0;
+	}
+
+	return pm_runtime_suspended(x->dev);
+}
+
+static inline bool usb_phy_autopm_active(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return 0;
+	}
+
+	return pm_runtime_active(x->dev);
+}
+
+static inline int usb_phy_autopm_suspend(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_suspend(x->dev);
+}
+
+static inline int usb_phy_autopm_resume(struct usb_phy *x)
+{
+	if (!x || !x->dev) {
+		dev_err(x->dev, "no PHY or attached device available\n");
+		return -ENODEV;
+	}
+
+	return pm_runtime_resume(x->dev);
+}
+
 #endif /* __LINUX_USB_PHY_H */
-- 
1.7.6.5

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

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 <gautam.vivek@samsung.com>
> ---
>  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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

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 <gautam.vivek@samsung.com>
> ---
>  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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 1:53 PM, Felipe Balbi <balbi@ti.com> 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 <gautam.vivek@samsung.com>
>> ---
>>  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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

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 <gautam.vivek@samsung.com>
> >> ---
> >>  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 ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

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 <gautam.vivek@samsung.com>
> >> ---
> >>  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 ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <balbi@ti.com> 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 <gautam.vivek@samsung.com>
>> >> ---
>> >>  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 ??

Yea, better. Thanks
Will amend this accordingly.


-- 
Thanks & Regards
Vivek

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

[Kishon Vijay Abraham I]

On Tuesday 02 April 2013 06:10 PM, Vivek Gautam wrote:
> Hi,
>
>
> On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <balbi@ti.com> 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 <gautam.vivek@samsung.com>
>>>>> ---
>>>>>   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

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

[Kishon Vijay Abraham I]

On Tuesday 02 April 2013 06:10 PM, Vivek Gautam wrote:
> Hi,
>
>
> On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <balbi@ti.com> 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 <gautam.vivek@samsung.com>
>>>>> ---
>>>>>   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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

Hi,

On Thu, Apr 18, 2013 at 05:20:11PM +0530, Kishon Vijay Abraham I 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 <gautam.vivek@samsung.com>
> >>>>>---
> >>>>>  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)?

not in this case, since we're trying to catch users passing NULL to as
the phy argument.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

Hi,

On Thu, Apr 18, 2013 at 05:20:11PM +0530, Kishon Vijay Abraham I 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 <gautam.vivek@samsung.com>
> >>>>>---
> >>>>>  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)?

not in this case, since we're trying to catch users passing NULL to as
the phy argument.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Kishon Vijay Abraham I]

Hi,

On Monday 01 April 2013 07:24 PM, 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 <gautam.vivek@samsung.com>
> ---
>   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
Kishon

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

[Kishon Vijay Abraham I]

Hi,

On Monday 01 April 2013 07:24 PM, 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 <gautam.vivek@samsung.com>
> ---
>   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
Kishon

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

[Vivek Gautam]

Hi Kishon,


On Wed, Apr 3, 2013 at 10:38 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi,
>
>
> On Monday 01 April 2013 07:24 PM, 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 <gautam.vivek@samsung.com>
>> ---
>>   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 ?


-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

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 <gautam.vivek@samsung.com>
> >> ---
> >>   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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

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 <gautam.vivek@samsung.com>
> >> ---
> >>   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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi Felipe,


On Wed, Apr 3, 2013 at 1:45 PM, Felipe Balbi <balbi@ti.com> wrote:
> 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 <gautam.vivek@samsung.com>
>> >> ---
>> >>   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 :-(


>
> --
> balbi



-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

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 <gautam.vivek@samsung.com>
> >> >> ---
> >> >>   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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

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 <gautam.vivek@samsung.com>
> >> >> ---
> >> >>   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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

Hi,

On Wed, Apr 03, 2013 at 04:54:14PM +0300, Felipe Balbi wrote:
> > >> >> +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);
> }
> 
> ???

and of course I missed the fact that pm_runtime_enable returns nothing
:-) But you got my point.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

Hi,

On Wed, Apr 03, 2013 at 04:54:14PM +0300, Felipe Balbi wrote:
> > >> >> +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);
> }
> 
> ???

and of course I missed the fact that pm_runtime_enable returns nothing
:-) But you got my point.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi,


On Wed, Apr 3, 2013 at 7:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Apr 03, 2013 at 04:54:14PM +0300, Felipe Balbi wrote:
>> > >> >> +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.

True

>>
>> 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);
>> }
>>
>> ???

Great

>
> and of course I missed the fact that pm_runtime_enable returns nothing
> :-) But you got my point.

Yea, this is a way around this. :-)

Just one more query :-)

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 ?



-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]

Hi,

On Wed, Apr 03, 2013 at 07:40:44PM +0530, Vivek Gautam wrote:
> >> > >> >> +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.
> 
> True
> 
> >>
> >> 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);
> >> }
> >>
> >> ???
> 
> Great
> 
> >
> > and of course I missed the fact that pm_runtime_enable returns nothing
> > :-) But you got my point.
> 
> Yea, this is a way around this. :-)
> 
> Just one more query :-)
> 
> 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 :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]

Hi,

On Wed, Apr 03, 2013 at 07:40:44PM +0530, Vivek Gautam wrote:
> >> > >> >> +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.
> 
> True
> 
> >>
> >> 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);
> >> }
> >>
> >> ???
> 
> Great
> 
> >
> > and of course I missed the fact that pm_runtime_enable returns nothing
> > :-) But you got my point.
> 
> Yea, this is a way around this. :-)
> 
> Just one more query :-)
> 
> 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 :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

On Wed, Apr 3, 2013 at 7:48 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Apr 03, 2013 at 07:40:44PM +0530, Vivek Gautam wrote:
>> >> > >> >> +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.
>>
>> True
>>
>> >>
>> >> 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);
>> >> }
>> >>
>> >> ???
>>
>> Great
>>
>> >
>> > and of course I missed the fact that pm_runtime_enable returns nothing
>> > :-) But you got my point.
>>
>> Yea, this is a way around this. :-)
>>
>> Just one more query :-)
>>
>> 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 :-)

Yes ofcourse.
So PHYs (in their runtime_suspend) should not be pulled into a state
wherein even the controllers become in-operational.
Thereby no attach-detection, and controller doesn't wake up to be able
to usb_phy_autopm_get_sync()

Sorry for so much noise, i am acting like slow study ;-)

>
> --
> balbi



-- 
Thanks & Regards
Vivek

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

[Alan Stern]

On Wed, 3 Apr 2013, Felipe Balbi 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.

With PHYs you don't have probing and releasing, but you do have 
consumers registering and unregistering themselves, which is about the 
same thing.

Alan Stern

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

[Alan Stern]

On Wed, 3 Apr 2013, Felipe Balbi 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.

With PHYs you don't have probing and releasing, but you do have 
consumers registering and unregistering themselves, which is about the 
same thing.

Alan Stern

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

Hi,

On Wed, Apr 03, 2013 at 02:14:02PM -0400, Alan Stern 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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

Hi,

On Wed, Apr 03, 2013 at 02:14:02PM -0400, Alan Stern 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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi,


On Thu, Apr 4, 2013 at 12:48 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Apr 03, 2013 at 02:14:02PM -0400, Alan Stern 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.
Besides PHYs also need to *put_sync* just before their probe is
finishing, so that it's
availbale for autosuspend.

I, however didn't understand the need of PHY to *get_sync* itself in
release routine.



-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Stern]

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

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

[Alan Stern]

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

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

[Vivek Gautam]

Hi,


On Thu, Apr 4, 2013 at 8:16 PM, Alan Stern <stern@rowland.harvard.edu> wrote:

Apologies for delay in replying.

> 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?
           PHY is added to global list by PHY drivers (like
phy-samsung-usb2.c/phy-omap-usb2.c)
           by usb_add_phy() API

>
>         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.

>
>         Likewise, what routine does it call to unregister itself?
           Once DWC3's remove() is called PHYs are put.



-- 
Best Regards
Vivek

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

[Alan Stern]

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.

> >         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().

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.

> >         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.

Alan Stern

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

[Alan Stern]

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.

> >         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().

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.

> >         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.

Alan Stern

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

[Vivek Gautam]

Hi,


On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> 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.

>
>> >         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 ?



-- 
Best Regards
Vivek

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

[Vivek Gautam]

Hi,


On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> 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.

>
>> >         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 ?



-- 
Best Regards
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alan Stern]

On Tue, 23 Apr 2013, Vivek Gautam wrote:

> Hi,
> 
> 
> On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> 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.

> >> >         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.

Alan Stern

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

[Alan Stern]

On Tue, 23 Apr 2013, Vivek Gautam wrote:

> Hi,
> 
> 
> On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> 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.

> >> >         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.

Alan Stern

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

[Vivek Gautam]

On Tue, Apr 23, 2013 at 11:42 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 23 Apr 2013, Vivek Gautam wrote:
>
>> Hi,
>>
>>
>> On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> 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

[PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Vivek Gautam]

The current code in the dwc3 probe effectively disables runtime pm
from ever working because it calls a get() that was never put() until
device removal.  Change the runtime pm code to match the standard
formula and allow runtime pm to function.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/dwc3/core.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e2325ad..3a6993c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
 
+	/* Setting device state as 'suspended' initially,
+	 * to make sure we know device state prior to
+	 * pm_runtime_enable
+	 */
+	pm_runtime_set_suspended(dev);
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	pm_runtime_forbid(dev);
@@ -566,6 +571,7 @@ static int dwc3_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+	pm_runtime_put_sync(dev);
 	pm_runtime_allow(dev);
 
 	return 0;
@@ -595,6 +601,7 @@ err1:
 
 err0:
 	dwc3_free_event_buffers(dwc);
+	pm_runtime_disable(&pdev->dev);
 
 	return ret;
 }
@@ -606,7 +613,6 @@ static int dwc3_remove(struct platform_device *pdev)
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
 
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	dwc3_debugfs_exit(dwc);
-- 
1.7.6.5

Re: [PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

On Mon, Apr 01, 2013 at 07:24:01PM +0530, Vivek Gautam wrote:
> The current code in the dwc3 probe effectively disables runtime pm
> from ever working because it calls a get() that was never put() until
> device removal.  Change the runtime pm code to match the standard
> formula and allow runtime pm to function.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc3/core.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e2325ad..3a6993c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>  
> +	/* Setting device state as 'suspended' initially,

wrong comment style.

> +	 * to make sure we know device state prior to
> +	 * pm_runtime_enable
> +	 */
> +	pm_runtime_set_suspended(dev);

didn't Alan mention this should be done at the Bus level ? In that case,
shouldn't you have call pm_runtime_set_active/suspended() based on
DT's status=okay or status=disabled ? Or something similar ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

On Mon, Apr 01, 2013 at 07:24:01PM +0530, Vivek Gautam wrote:
> The current code in the dwc3 probe effectively disables runtime pm
> from ever working because it calls a get() that was never put() until
> device removal.  Change the runtime pm code to match the standard
> formula and allow runtime pm to function.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc3/core.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e2325ad..3a6993c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>  
> +	/* Setting device state as 'suspended' initially,

wrong comment style.

> +	 * to make sure we know device state prior to
> +	 * pm_runtime_enable
> +	 */
> +	pm_runtime_set_suspended(dev);

didn't Alan mention this should be done at the Bus level ? In that case,
shouldn't you have call pm_runtime_set_active/suspended() based on
DT's status=okay or status=disabled ? Or something similar ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Vivek Gautam]

Hi Felipe,


On Tue, Apr 2, 2013 at 1:59 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Apr 01, 2013 at 07:24:01PM +0530, Vivek Gautam wrote:
>> The current code in the dwc3 probe effectively disables runtime pm
>> from ever working because it calls a get() that was never put() until
>> device removal.  Change the runtime pm code to match the standard
>> formula and allow runtime pm to function.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> CC: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/usb/dwc3/core.c |    8 +++++++-
>>  1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index e2325ad..3a6993c 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
>>
>>       dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>>
>> +     /* Setting device state as 'suspended' initially,
>
> wrong comment style.
Yea :-(  will fix this.

>
>> +      * to make sure we know device state prior to
>> +      * pm_runtime_enable
>> +      */
>> +     pm_runtime_set_suspended(dev);
>
> didn't Alan mention this should be done at the Bus level ? In that case,
> shouldn't you have call pm_runtime_set_active/suspended() based on
> DT's status=okay or status=disabled ? Or something similar ?

True, we should be doing this at bus level. But he did also mention to
let pm core
know of the state of the device before enabling the runtime pm. Right ?
Moreover immediately after pm_runtime_enable(), we do pm_runtime_get_sync()
so that device comes to active state.
I am possibly missing things out here, not able to grab this whole
picture completely :-(

Wouldn't DT's status=disabled actually be disabling the device as a whole ?
So, how much will runtime power management on the device be affecting ?



-- 
Thanks & Regards
Vivek

Re: [PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

Hi,

On Wed, Apr 03, 2013 at 11:35:43AM +0530, Vivek Gautam wrote:
> >> The current code in the dwc3 probe effectively disables runtime pm
> >> from ever working because it calls a get() that was never put() until
> >> device removal.  Change the runtime pm code to match the standard
> >> formula and allow runtime pm to function.
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> CC: Doug Anderson <dianders@chromium.org>
> >> ---
> >>  drivers/usb/dwc3/core.c |    8 +++++++-
> >>  1 files changed, 7 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index e2325ad..3a6993c 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
> >>
> >>       dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
> >>
> >> +     /* Setting device state as 'suspended' initially,
> >
> > wrong comment style.
> Yea :-(  will fix this.
> 
> >
> >> +      * to make sure we know device state prior to
> >> +      * pm_runtime_enable
> >> +      */
> >> +     pm_runtime_set_suspended(dev);
> >
> > didn't Alan mention this should be done at the Bus level ? In that case,
> > shouldn't you have call pm_runtime_set_active/suspended() based on
> > DT's status=okay or status=disabled ? Or something similar ?
> 
> True, we should be doing this at bus level. But he did also mention to
> let pm core
> know of the state of the device before enabling the runtime pm. Right ?
> Moreover immediately after pm_runtime_enable(), we do pm_runtime_get_sync()
> so that device comes to active state.
> I am possibly missing things out here, not able to grab this whole
> picture completely :-(
> 
> Wouldn't DT's status=disabled actually be disabling the device as a whole ?
> So, how much will runtime power management on the device be affecting ?

indeed, maybe we can keep it like this, but it would be nice to have OF
core handle this for us based on whatever data.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 02/11] USB: dwc3: Adjust runtime pm to allow autosuspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

Hi,

On Wed, Apr 03, 2013 at 11:35:43AM +0530, Vivek Gautam wrote:
> >> The current code in the dwc3 probe effectively disables runtime pm
> >> from ever working because it calls a get() that was never put() until
> >> device removal.  Change the runtime pm code to match the standard
> >> formula and allow runtime pm to function.
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> CC: Doug Anderson <dianders@chromium.org>
> >> ---
> >>  drivers/usb/dwc3/core.c |    8 +++++++-
> >>  1 files changed, 7 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index e2325ad..3a6993c 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev)
> >>
> >>       dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
> >>
> >> +     /* Setting device state as 'suspended' initially,
> >
> > wrong comment style.
> Yea :-(  will fix this.
> 
> >
> >> +      * to make sure we know device state prior to
> >> +      * pm_runtime_enable
> >> +      */
> >> +     pm_runtime_set_suspended(dev);
> >
> > didn't Alan mention this should be done at the Bus level ? In that case,
> > shouldn't you have call pm_runtime_set_active/suspended() based on
> > DT's status=okay or status=disabled ? Or something similar ?
> 
> True, we should be doing this at bus level. But he did also mention to
> let pm core
> know of the state of the device before enabling the runtime pm. Right ?
> Moreover immediately after pm_runtime_enable(), we do pm_runtime_get_sync()
> so that device comes to active state.
> I am possibly missing things out here, not able to grab this whole
> picture completely :-(
> 
> Wouldn't DT's status=disabled actually be disabling the device as a whole ?
> So, how much will runtime power management on the device be affecting ?

indeed, maybe we can keep it like this, but it would be nice to have OF
core handle this for us based on whatever data.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 03/11] usb: dwc3: Enable runtime pm only after PHYs are initialized

[Vivek Gautam]

Allow dwc3 to enable auto power management only after its PHYs
are initialized so that any further PHY handling by dwc3's
runtime power management callbacks is fine.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/dwc3/core.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3a6993c..e250828 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -491,15 +491,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
 
-	/* Setting device state as 'suspended' initially,
-	 * to make sure we know device state prior to
-	 * pm_runtime_enable
-	 */
-	pm_runtime_set_suspended(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
-	pm_runtime_forbid(dev);
-
 	dwc3_cache_hwparams(dwc);
 
 	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
@@ -521,6 +512,15 @@ static int dwc3_probe(struct platform_device *pdev)
 		goto err1;
 	}
 
+	/* Setting device state as 'suspended' initially,
+	 * to make sure we know device state prior to
+	 * pm_runtime_enable
+	 */
+	pm_runtime_set_suspended(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	pm_runtime_forbid(dev);
+
 	if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
 		mode = DWC3_MODE_HOST;
 	else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
-- 
1.7.6.5

[PATCH v3 04/11] usb: dwc3: Add runtime power management callbacks

[Vivek Gautam]

Right now it doesn't handle full runtime suspend/resume
functionality. However it allows to handle PHYs' sleep
and wakeup across runtime suspend/resume.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/dwc3/core.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e250828..65a3adf 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -745,11 +745,54 @@ static int dwc3_resume(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int dwc3_runtime_suspend(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = usb_phy_autopm_put_sync(dwc->usb2_phy);
+	if (ret)
+		dev_warn(dev, "Can't autosuspend usb2-phy\n");
+
+	ret = usb_phy_autopm_put_sync(dwc->usb3_phy);
+	if (ret)
+		dev_warn(dev, "Can't autosuspend usb3-phy\n");
+
+	return ret;
+}
+
+static int dwc3_runtime_resume(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = usb_phy_autopm_get_sync(dwc->usb2_phy);
+	if (ret) {
+		dev_err(dev, "usb2-phy: get sync failed with err %d\n", ret);
+		return ret;
+	}
+
+	ret = usb_phy_autopm_get_sync(dwc->usb3_phy);
+	if (ret) {
+		dev_err(dev, "usb3-phy: get sync failed with err %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+#else
+#define dwc3_runtime_suspend		NULL
+#define dwc3_runtime_resume		NULL
+#endif
+
 static const struct dev_pm_ops dwc3_dev_pm_ops = {
 	.prepare	= dwc3_prepare,
 	.complete	= dwc3_complete,
 
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
+	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend,
+				dwc3_runtime_resume, NULL)
 };
 
 #define DWC3_PM_OPS	&(dwc3_dev_pm_ops)
-- 
1.7.6.5

Re: [PATCH v3 04/11] usb: dwc3: Add runtime power management callbacks

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hi,

On Mon, Apr 01, 2013 at 07:24:03PM +0530, Vivek Gautam wrote:
> +#else
> +#define dwc3_runtime_suspend		NULL
> +#define dwc3_runtime_resume		NULL

this #else branch is unnecessary. Look at the definition for
SET_RUNTIME_PM_OPS()

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 04/11] usb: dwc3: Add runtime power management callbacks

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hi,

On Mon, Apr 01, 2013 at 07:24:03PM +0530, Vivek Gautam wrote:
> +#else
> +#define dwc3_runtime_suspend		NULL
> +#define dwc3_runtime_resume		NULL

this #else branch is unnecessary. Look at the definition for
SET_RUNTIME_PM_OPS()

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 04/11] usb: dwc3: Add runtime power management callbacks

[Vivek Gautam]

Hi Balbi,


On Tue, Apr 2, 2013 at 2:02 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Apr 01, 2013 at 07:24:03PM +0530, Vivek Gautam wrote:
>> +#else
>> +#define dwc3_runtime_suspend         NULL
>> +#define dwc3_runtime_resume          NULL
>
> this #else branch is unnecessary. Look at the definition for
> SET_RUNTIME_PM_OPS()

Right, will remove this.


-- 
Thanks & Regards
Vivek

[PATCH v3 05/11] usb: dwc3: exynos: Enable runtime power management

[Vivek Gautam]

Enabling runtime power management on dwc3-exynos
letting dwc3 controller to be autosuspended on exynos
platform when not in use.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/dwc3/dwc3-exynos.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 1ea7bd8..1ae81a0 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -19,6 +19,7 @@
 #include <linux/platform_data/dwc3-exynos.h>
 #include <linux/dma-mapping.h>
 #include <linux/clk.h>
+#include <linux/pm_runtime.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/nop-usb-xceiv.h>
 #include <linux/of.h>
@@ -138,6 +139,11 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	exynos->dev	= dev;
 	exynos->clk	= clk;
 
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	pm_runtime_forbid(dev);
+
 	clk_prepare_enable(exynos->clk);
 
 	if (node) {
@@ -152,10 +158,14 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto err2;
 	}
 
+	pm_runtime_put_sync(dev);
+	pm_runtime_allow(dev);
+
 	return 0;
 
 err2:
 	clk_disable_unprepare(clk);
+	pm_runtime_disable(dev);
 err1:
 	return ret;
 }
@@ -164,6 +174,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+
 	platform_device_unregister(exynos->usb2_phy);
 	platform_device_unregister(exynos->usb3_phy);
 	device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
-- 
1.7.6.5

Re: [PATCH v3 05/11] usb: dwc3: exynos: Enable runtime power management

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Mon, Apr 01, 2013 at 07:24:04PM +0530, Vivek Gautam wrote:
> Enabling runtime power management on dwc3-exynos
> letting dwc3 controller to be autosuspended on exynos
> platform when not in use.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 1ea7bd8..1ae81a0 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_data/dwc3-exynos.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/nop-usb-xceiv.h>
>  #include <linux/of.h>
> @@ -138,6 +139,11 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	exynos->dev	= dev;
>  	exynos->clk	= clk;
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_forbid(dev);

don't you want to use autosuspend() to avoid consecutive suspend/resume
calls when controller is under use ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 05/11] usb: dwc3: exynos: Enable runtime power management

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Mon, Apr 01, 2013 at 07:24:04PM +0530, Vivek Gautam wrote:
> Enabling runtime power management on dwc3-exynos
> letting dwc3 controller to be autosuspended on exynos
> platform when not in use.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 1ea7bd8..1ae81a0 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_data/dwc3-exynos.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/nop-usb-xceiv.h>
>  #include <linux/of.h>
> @@ -138,6 +139,11 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	exynos->dev	= dev;
>  	exynos->clk	= clk;
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_forbid(dev);

don't you want to use autosuspend() to avoid consecutive suspend/resume
calls when controller is under use ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 06/11] usb: xhci: Enable runtime pm in xhci-plat

[Vivek Gautam]

By enabling runtime pm in this driver allows users of
xhci-plat to enter into runtime pm. This is not full
runtime pm support (AKA xhci-plat doesn't actually power
anything off when in runtime suspend mode) but,
just basic enablement.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/host/xhci-plat.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index df90fe5..b10573e 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 
@@ -149,6 +150,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto put_usb3_hcd;
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 put_usb3_hcd:
@@ -174,6 +178,8 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
+	pm_runtime_disable(&dev->dev);
+
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
-- 
1.7.6.5

[PATCH v3 07/11] usb: phy: samsung: Enable runtime power management on usb2phy

[Vivek Gautam]

Enable autosuspending of Samsung usb2.0 PHY

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/phy/phy-samsung-usb2.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/phy-samsung-usb2.c b/drivers/usb/phy/phy-samsung-usb2.c
index 45ffe03..d378fe9 100644
--- a/drivers/usb/phy/phy-samsung-usb2.c
+++ b/drivers/usb/phy/phy-samsung-usb2.c
@@ -423,6 +423,9 @@ static int samsung_usb2phy_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sphy);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
 }
 
@@ -432,6 +435,8 @@ static int samsung_usb2phy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	pm_runtime_disable(&pdev->dev);
+
 	if (sphy->pmuregs)
 		iounmap(sphy->pmuregs);
 	if (sphy->sysreg)
-- 
1.7.6.5

[PATCH v3 08/11] usb: phy: samsung: Enable runtime power management on usb3phy

[Vivek Gautam]

Enable autosuspending of Samsung usb3.0 PHY

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/phy/phy-samsung-usb3.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/phy-samsung-usb3.c b/drivers/usb/phy/phy-samsung-usb3.c
index 54f6418..a713585 100644
--- a/drivers/usb/phy/phy-samsung-usb3.c
+++ b/drivers/usb/phy/phy-samsung-usb3.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/usb/samsung_usb_phy.h>
 #include <linux/platform_data/samsung-usbphy.h>
 
@@ -287,6 +288,9 @@ static int samsung_usb3phy_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sphy);
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB3);
 }
 
@@ -296,6 +300,8 @@ static int samsung_usb3phy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	pm_runtime_disable(&pdev->dev);
+
 	if (sphy->pmuregs)
 		iounmap(sphy->pmuregs);
 	if (sphy->sysreg)
-- 
1.7.6.5

[PATCH v3 09/11] usb: phy: samsung: Add support for external reference clock

[Vivek Gautam]

The PHY controller can choose between ref_pad_clk (XusbXTI-external PLL),
or EXTREFCLK (XXTI-internal clock crystal) to generate the required clock.
Adding the provision for ref_pad_clk here.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/phy/phy-samsung-usb3.c |   46 +++++++++++++++++++++++++++++++----
 1 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/phy/phy-samsung-usb3.c b/drivers/usb/phy/phy-samsung-usb3.c
index a713585..8afef9d 100644
--- a/drivers/usb/phy/phy-samsung-usb3.c
+++ b/drivers/usb/phy/phy-samsung-usb3.c
@@ -33,7 +33,7 @@
 /*
  * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
  */
-static u32 samsung_usb3phy_set_refclk(struct samsung_usbphy *sphy)
+static u32 samsung_usb3phy_set_refclk_int(struct samsung_usbphy *sphy)
 {
 	u32 reg;
 	u32 refclk;
@@ -66,7 +66,22 @@ static u32 samsung_usb3phy_set_refclk(struct samsung_usbphy *sphy)
 	return reg;
 }
 
-static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)
+/*
+ * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
+ */
+static u32 samsung_usb3phy_set_refclk_ext(void)
+{
+	u32 reg;
+
+	reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
+		PHYCLKRST_FSEL_PAD_100MHZ |
+		PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
+
+	return reg;
+}
+
+static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy,
+							bool use_ext_clk)
 {
 	void __iomem *regs = sphy->regs;
 	u32 phyparam0;
@@ -81,7 +96,10 @@ static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)
 
 	phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
 	/* Select PHY CLK source */
-	phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
+	if (use_ext_clk)
+		phyparam0 |= PHYPARAM0_REF_USE_PAD;
+	else
+		phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
 	/* Set Loss-of-Signal Detector sensitivity */
 	phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
 	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
@@ -116,7 +134,10 @@ static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)
 	/* UTMI Power Control */
 	writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
 
-	phyclkrst = samsung_usb3phy_set_refclk(sphy);
+	if (use_ext_clk)
+		phyclkrst = samsung_usb3phy_set_refclk_ext();
+	else
+		phyclkrst = samsung_usb3phy_set_refclk_int(sphy);
 
 	phyclkrst |= PHYCLKRST_PORTRESET |
 			/* Digital power supply in normal operating mode */
@@ -164,7 +185,7 @@ static void samsung_exynos5_usb3phy_disable(struct samsung_usbphy *sphy)
 	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
 }
 
-static int samsung_usb3phy_init(struct usb_phy *phy)
+static int samsung_exynos5_usb3phy_init(struct usb_phy *phy, bool use_ext_clk)
 {
 	struct samsung_usbphy *sphy;
 	unsigned long flags;
@@ -188,7 +209,7 @@ static int samsung_usb3phy_init(struct usb_phy *phy)
 	samsung_usbphy_set_isolation(sphy, false);
 
 	/* Initialize usb phy registers */
-	samsung_exynos5_usb3phy_enable(sphy);
+	samsung_exynos5_usb3phy_enable(sphy, use_ext_clk);
 
 	spin_unlock_irqrestore(&sphy->lock, flags);
 
@@ -199,6 +220,19 @@ static int samsung_usb3phy_init(struct usb_phy *phy)
 }
 
 /*
+ * The function passed to the usb driver for phy initialization
+ */
+static int samsung_usb3phy_init(struct usb_phy *phy)
+{
+	/*
+	 * We start with using PHY refclk from external PLL,
+	 * once runtime suspend for the device is called this
+	 * will change to internal core clock
+	 */
+	return samsung_exynos5_usb3phy_init(phy, true);
+}
+
+/*
  * The function passed to the usb driver for phy shutdown
  */
 static void samsung_usb3phy_shutdown(struct usb_phy *phy)
-- 
1.7.6.5

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

[Sarah Sharp]

Question: Do you still need this patch for 3.10?

http://marc.info/?l=linux-usb&m=136057666911621&w=2

Does this patchset build on top of that?

I'm really behind on my patches for 3.10, sorry.

Sarah Sharp

On Mon, Apr 01, 2013 at 07:23:59PM +0530, Vivek Gautam wrote:
> This patch-series enables runtime power management on xhci-plat,
> dwc3-core, dwc3-exynos as well as on Samsung's USB 2.0 type and
> USB 3.0 type PHYs.
> 
> Based on 'next' branch of Felipe Balbi's USB tree.
> 
> Changes from v2:
>  - Using separate functions for USB PHY runtime power management, instead of
>    using macros.
>  - Adding 'pm_runtime_set_suspended()' api call in dwc3 core layer before
>    enabling runtime pm. (Ideally, we should be explicitly make device
>    'suspended' or 'active' before enabling runtime pm on it).
>  - Checking return code for 'put_sync' and 'get_sync' of USB-PHYs when
>    waking up or suspending them from dwc3 core's runtime_pm callbacks.
>  - Removed buggy pm_runtime_put() calls from driver's (xhci, dwc3 and PHY)
>    remove functions.
>  - Adding a patch to enable runtime power management of Samsung's USB 2.0 PHY
>    (usb: phy: samsung: Enable runtime power management on usb2phy)
> 
> Changes from v1:
>  - Adding required PHY APIs to handle runtime power management
>    instead of directly twiddling with phy->dev.
>  - Handling runtime power management of usb PHYs in dwc3 core
>    driver instead of in any glue layer.
>  - Splitting the patch:
>    [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb
>    into required number to bifurcate functionality.
> 
> Vivek Gautam (11):
>   usb: phy: Add APIs for runtime power management
>   USB: dwc3: Adjust runtime pm to allow autosuspend
>   usb: dwc3: Enable runtime pm only after PHYs are initialized
>   usb: dwc3: Add runtime power management callbacks
>   usb: dwc3: exynos: Enable runtime power management
>   usb: xhci: Enable runtime pm in xhci-plat
>   usb: phy: samsung: Enable runtime power management on usb2phy
>   usb: phy: samsung: Enable runtime power management on usb3phy
>   usb: phy: samsung: Add support for external reference clock
>   usb: phy: samsung: Add support for PHY ref_clk gpio
>   usb: phy: samsung: Add support for PHY refclk switching
> 
>  drivers/usb/dwc3/core.c            |   59 ++++++++++++++--
>  drivers/usb/dwc3/dwc3-exynos.c     |   12 +++
>  drivers/usb/host/xhci-plat.c       |    6 ++
>  drivers/usb/phy/phy-samsung-usb.c  |   26 +++++++
>  drivers/usb/phy/phy-samsung-usb.h  |    1 +
>  drivers/usb/phy/phy-samsung-usb2.c |    5 ++
>  drivers/usb/phy/phy-samsung-usb3.c |  119 +++++++++++++++++++++++++++++--
>  include/linux/usb/phy.h            |  141 ++++++++++++++++++++++++++++++++++++
>  8 files changed, 358 insertions(+), 11 deletions(-)
> 
> -- 
> 1.7.6.5
> 

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

[Vivek Gautam]

Hi Sarah,


On Wed, Apr 3, 2013 at 10:57 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> 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. :-(

>
> http://marc.info/?l=linux-usb&m=136057666911621&w=2
>
> Does this patchset build on top of that?

Yea, it builds fine on top of the above mentioned patch.
I would suggest that we pull the xhci-plat patch in this series when the
complete patch-set getting in.
btw, its your decision ;-)

>
> I'm really behind on my patches for 3.10, sorry.
>
> Sarah Sharp
>
> On Mon, Apr 01, 2013 at 07:23:59PM +0530, Vivek Gautam wrote:
>> This patch-series enables runtime power management on xhci-plat,
>> dwc3-core, dwc3-exynos as well as on Samsung's USB 2.0 type and
>> USB 3.0 type PHYs.
>>
>> Based on 'next' branch of Felipe Balbi's USB tree.
>>
>> Changes from v2:
>>  - Using separate functions for USB PHY runtime power management, instead of
>>    using macros.
>>  - Adding 'pm_runtime_set_suspended()' api call in dwc3 core layer before
>>    enabling runtime pm. (Ideally, we should be explicitly make device
>>    'suspended' or 'active' before enabling runtime pm on it).
>>  - Checking return code for 'put_sync' and 'get_sync' of USB-PHYs when
>>    waking up or suspending them from dwc3 core's runtime_pm callbacks.
>>  - Removed buggy pm_runtime_put() calls from driver's (xhci, dwc3 and PHY)
>>    remove functions.
>>  - Adding a patch to enable runtime power management of Samsung's USB 2.0 PHY
>>    (usb: phy: samsung: Enable runtime power management on usb2phy)
>>
>> Changes from v1:
>>  - Adding required PHY APIs to handle runtime power management
>>    instead of directly twiddling with phy->dev.
>>  - Handling runtime power management of usb PHYs in dwc3 core
>>    driver instead of in any glue layer.
>>  - Splitting the patch:
>>    [PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb
>>    into required number to bifurcate functionality.
>>
>> Vivek Gautam (11):
>>   usb: phy: Add APIs for runtime power management
>>   USB: dwc3: Adjust runtime pm to allow autosuspend
>>   usb: dwc3: Enable runtime pm only after PHYs are initialized
>>   usb: dwc3: Add runtime power management callbacks
>>   usb: dwc3: exynos: Enable runtime power management
>>   usb: xhci: Enable runtime pm in xhci-plat
>>   usb: phy: samsung: Enable runtime power management on usb2phy
>>   usb: phy: samsung: Enable runtime power management on usb3phy
>>   usb: phy: samsung: Add support for external reference clock
>>   usb: phy: samsung: Add support for PHY ref_clk gpio
>>   usb: phy: samsung: Add support for PHY refclk switching
>>
>>  drivers/usb/dwc3/core.c            |   59 ++++++++++++++--
>>  drivers/usb/dwc3/dwc3-exynos.c     |   12 +++
>>  drivers/usb/host/xhci-plat.c       |    6 ++
>>  drivers/usb/phy/phy-samsung-usb.c  |   26 +++++++
>>  drivers/usb/phy/phy-samsung-usb.h  |    1 +
>>  drivers/usb/phy/phy-samsung-usb2.c |    5 ++
>>  drivers/usb/phy/phy-samsung-usb3.c |  119 +++++++++++++++++++++++++++++--
>>  include/linux/usb/phy.h            |  141 ++++++++++++++++++++++++++++++++++++
>>  8 files changed, 358 insertions(+), 11 deletions(-)
>>
>> --
>> 1.7.6.5
>>



-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

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
> <sarah.a.sharp@linux.intel.com> 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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

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
> <sarah.a.sharp@linux.intel.com> 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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

On Thu, Apr 4, 2013 at 12:40 PM, Felipe Balbi <balbi@ti.com> 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
>> <sarah.a.sharp@linux.intel.com> 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. :-)



-- 
Thanks & Regards
Vivek

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

[Sarah Sharp]

On Thu, Apr 04, 2013 at 01:02:45PM +0530, Vivek Gautam wrote:
> On Thu, Apr 4, 2013 at 12:40 PM, Felipe Balbi <balbi@ti.com> 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
> >> <sarah.a.sharp@linux.intel.com> 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