Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support

[Tony Lindgren]

* Alan Stern  [170518 10:24]:
> On Wed, 17 May 2017, Tony Lindgren wrote:
> 
> > This is needed in preparation of adding support for omap3 and
> > later OHCI. The runtime PM will only do something on platforms
> > that implement it.
> 
> > @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device 
> > *dev)
> > struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
> > int clk, ret, phy_num;
> >  
> > +   ret = pm_runtime_get_sync(>dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> 
> I don't remember how probing for platform device drivers is set up.
> So although this is definitely the wrong place for 
> pm_runtime_get_sync(), it may turn out that you need to do a 
> pm_runtime_get_noresume() before calling pm_runtime_enable(), and a 
> pm_runtime_put() at the end of ohci_platform_probe().  Otherwise 
> runtime PM might kick in during the middle of the probe sequence -- not 
> what you want.

Just removing those parts seems to work for probing.

> Similarly, you may need pm_runtime_get_sync() at the start of 
> ohci_platform_remove() and pm_runtime_put_noidle() at the end.

Yup that I noticed that too trying to rmmod it with no devices
connected.

While testing I also saw once "external abort on non-linefetch":

(ohci_hub_status_data [ohci_hcd]) from []
(usb_hcd_poll_rh_status+0x3c/0x128 [usbcore])
(usb_hcd_poll_rh_status [usbcore]) from []
(call_timer_fn+0xb0/0x3f8)
(call_timer_fn) from [] (expire_timers+0xe4/0x220)
(expire_timers) from [] (run_timer_softirq+0x94/0x19c)
(run_timer_softirq) from [] (__do_softirq+0x140/0x570)

Also I'm still checking if legacy usb_phy vs phy needs handling.
So far it seems that no need to do anything because most devices
need a USB hub anyways unless they have USB serial transceiver
configured with drivers/mfd/omap-usb-host.c for LS/FS.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support

[Alan Stern]

On Wed, 17 May 2017, Tony Lindgren wrote:

> This is needed in preparation of adding support for omap3 and
> later OHCI. The runtime PM will only do something on platforms
> that implement it.

> @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device 
> *dev)
>   struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
>   int clk, ret, phy_num;
>  
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0)
> + return ret;
> +

I don't remember how probing for platform device drivers is set up.
So although this is definitely the wrong place for 
pm_runtime_get_sync(), it may turn out that you need to do a 
pm_runtime_get_noresume() before calling pm_runtime_enable(), and a 
pm_runtime_put() at the end of ohci_platform_probe().  Otherwise 
runtime PM might kick in during the middle of the probe sequence -- not 
what you want.

Similarly, you may need pm_runtime_get_sync() at the start of 
ohci_platform_remove() and pm_runtime_put_noidle() at the end.

Alan Stern

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

Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support

[Tony Lindgren]

* Alan Stern  [170518 08:28]:
> On Wed, 17 May 2017, Tony Lindgren wrote:
> 
> > This is needed in preparation of adding support for omap3 and
> > later OHCI. The runtime PM will only do something on platforms
> > that implement it.
> 
> This patch isn't correct.  See below.
> 
> > Cc: devicet...@vger.kernel.org
> > Cc: Hans de Goede 
> > Cc: Rob Herring 
> > Cc: Roger Quadros 
> > Cc: Yoshihiro Shimoda 
> > Signed-off-by: Tony Lindgren 
> > ---
> >  drivers/usb/host/ohci-platform.c | 11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/usb/host/ohci-platform.c 
> > b/drivers/usb/host/ohci-platform.c
> > --- a/drivers/usb/host/ohci-platform.c
> > +++ b/drivers/usb/host/ohci-platform.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device 
> > *dev)
> > struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
> > int clk, ret, phy_num;
> >  
> > +   ret = pm_runtime_get_sync(>dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
> > ret = clk_prepare_enable(priv->clks[clk]);
> > if (ret)
> > @@ -96,6 +101,8 @@ static void ohci_platform_power_off(struct 
> > platform_device *dev)
> > for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
> > if (priv->clks[clk])
> > clk_disable_unprepare(priv->clks[clk]);
> > +
> > +   pm_runtime_put_sync(>dev);
> >  }
> 
> ohci_platform_power_on() is invoked (by default) by the runtime-resume
> callback routine ohci_platform_resume(), through the pdata->power_on
> method pointer.  Likewise, ohci_platform_power_off() is invoked by the
> runtime-suspend callback routine.
> 
> This means you shouldn't do pm_runtime_get/put calls from within these 
> routines.

Oh OK great, so the above should not be needed at all.

> Is there any particular reason you wanted to add these calls?  In
> general, USB host controllers are expected to go into runtime suspend
> whenever there aren't any children keeping them awake.  Hence they
> usually don't need to worry about initiating their own suspends and
> resumes.

No particular reason as it sounds things work without it :) I'll check.

> >  static struct hc_driver __read_mostly ohci_platform_hc_driver;
> > @@ -242,6 +249,7 @@ static int ohci_platform_probe(struct platform_device 
> > *dev)
> > }
> >  #endif
> >  
> > +   pm_runtime_enable(>dev);
> 
> There should be a pm_runtime_set_active() just before this.

OK

> > if (pdata->power_on) {
> > err = pdata->power_on(dev);
> > if (err < 0)
> > @@ -271,6 +279,7 @@ static int ohci_platform_probe(struct platform_device 
> > *dev)
> > if (pdata->power_off)
> > pdata->power_off(dev);
> >  err_reset:
> > +   pm_runtime_disable(>dev);
> > while (--rst >= 0)
> > reset_control_assert(priv->resets[rst]);
> >  err_put_clks:
> > @@ -305,6 +314,8 @@ static int ohci_platform_remove(struct platform_device 
> > *dev)
> >  
> > usb_put_hcd(hcd);
> >  
> > +   pm_runtime_disable(>dev);
> > +
> > if (pdata == _platform_defaults)
> > dev->dev.platform_data = NULL;
> 
> These changes make sense.

OK thanks for looking.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support

[Alan Stern]

On Wed, 17 May 2017, Tony Lindgren wrote:

> This is needed in preparation of adding support for omap3 and
> later OHCI. The runtime PM will only do something on platforms
> that implement it.

This patch isn't correct.  See below.

> Cc: devicet...@vger.kernel.org
> Cc: Hans de Goede 
> Cc: Rob Herring 
> Cc: Roger Quadros 
> Cc: Yoshihiro Shimoda 
> Signed-off-by: Tony Lindgren 
> ---
>  drivers/usb/host/ohci-platform.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-platform.c 
> b/drivers/usb/host/ohci-platform.c
> --- a/drivers/usb/host/ohci-platform.c
> +++ b/drivers/usb/host/ohci-platform.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device 
> *dev)
>   struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
>   int clk, ret, phy_num;
>  
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0)
> + return ret;
> +
>   for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
>   ret = clk_prepare_enable(priv->clks[clk]);
>   if (ret)
> @@ -96,6 +101,8 @@ static void ohci_platform_power_off(struct platform_device 
> *dev)
>   for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
>   if (priv->clks[clk])
>   clk_disable_unprepare(priv->clks[clk]);
> +
> + pm_runtime_put_sync(>dev);
>  }

ohci_platform_power_on() is invoked (by default) by the runtime-resume
callback routine ohci_platform_resume(), through the pdata->power_on
method pointer.  Likewise, ohci_platform_power_off() is invoked by the
runtime-suspend callback routine.

This means you shouldn't do pm_runtime_get/put calls from within these 
routines.

Is there any particular reason you wanted to add these calls?  In
general, USB host controllers are expected to go into runtime suspend
whenever there aren't any children keeping them awake.  Hence they
usually don't need to worry about initiating their own suspends and
resumes.

>  static struct hc_driver __read_mostly ohci_platform_hc_driver;
> @@ -242,6 +249,7 @@ static int ohci_platform_probe(struct platform_device 
> *dev)
>   }
>  #endif
>  
> + pm_runtime_enable(>dev);

There should be a pm_runtime_set_active() just before this.

>   if (pdata->power_on) {
>   err = pdata->power_on(dev);
>   if (err < 0)
> @@ -271,6 +279,7 @@ static int ohci_platform_probe(struct platform_device 
> *dev)
>   if (pdata->power_off)
>   pdata->power_off(dev);
>  err_reset:
> + pm_runtime_disable(>dev);
>   while (--rst >= 0)
>   reset_control_assert(priv->resets[rst]);
>  err_put_clks:
> @@ -305,6 +314,8 @@ static int ohci_platform_remove(struct platform_device 
> *dev)
>  
>   usb_put_hcd(hcd);
>  
> + pm_runtime_disable(>dev);
> +
>   if (pdata == _platform_defaults)
>   dev->dev.platform_data = NULL;

These changes make sense.

Alan Stern

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

Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support

[Roger Quadros]

On 18/05/17 01:59, Tony Lindgren wrote:
> This is needed in preparation of adding support for omap3 and
> later OHCI. The runtime PM will only do something on platforms
> that implement it.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Hans de Goede 
> Cc: Rob Herring 
> Cc: Roger Quadros 
> Cc: Yoshihiro Shimoda 
> Signed-off-by: Tony Lindgren 

Acked-by: Roger Quadros 

> ---
>  drivers/usb/host/ohci-platform.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-platform.c 
> b/drivers/usb/host/ohci-platform.c
> --- a/drivers/usb/host/ohci-platform.c
> +++ b/drivers/usb/host/ohci-platform.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device 
> *dev)
>   struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
>   int clk, ret, phy_num;
>  
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0)
> + return ret;
> +
>   for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
>   ret = clk_prepare_enable(priv->clks[clk]);
>   if (ret)
> @@ -96,6 +101,8 @@ static void ohci_platform_power_off(struct platform_device 
> *dev)
>   for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
>   if (priv->clks[clk])
>   clk_disable_unprepare(priv->clks[clk]);
> +
> + pm_runtime_put_sync(>dev);
>  }
>  
>  static struct hc_driver __read_mostly ohci_platform_hc_driver;
> @@ -242,6 +249,7 @@ static int ohci_platform_probe(struct platform_device 
> *dev)
>   }
>  #endif
>  
> + pm_runtime_enable(>dev);
>   if (pdata->power_on) {
>   err = pdata->power_on(dev);
>   if (err < 0)
> @@ -271,6 +279,7 @@ static int ohci_platform_probe(struct platform_device 
> *dev)
>   if (pdata->power_off)
>   pdata->power_off(dev);
>  err_reset:
> + pm_runtime_disable(>dev);
>   while (--rst >= 0)
>   reset_control_assert(priv->resets[rst]);
>  err_put_clks:
> @@ -305,6 +314,8 @@ static int ohci_platform_remove(struct platform_device 
> *dev)
>  
>   usb_put_hcd(hcd);
>  
> + pm_runtime_disable(>dev);
> +
>   if (pdata == _platform_defaults)
>   dev->dev.platform_data = NULL;
>  
> 

-- 
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html