Re: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Grygorii Strashko]

On 03/27/2017 02:54 PM, Alexandre Bailon wrote:
> On 03/27/2017 07:38 PM, Grygorii Strashko wrote:
>>
>>
>> On 03/27/2017 11:39 AM, Alexandre Bailon wrote:
>>> Hello Grygorii,
>>> On 03/24/2017 06:26 PM, Grygorii Strashko wrote:


 On 03/24/2017 09:36 AM, Alexandre Bailon wrote:
> Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
> Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
> CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
> to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
> the clock by using PM runtime.
>
> Signed-off-by: Alexandre Bailon 
> ---
>  drivers/usb/musb/da8xx.c | 27 ---
>  1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index ed28afd..89e12f6 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -30,7 +30,6 @@
>   */
>

 [...]

>
> @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device
 *pdev)
>  if (!glue)
>  return -ENOMEM;
>
> -clk = devm_clk_get(>dev, "usb20");
> -if (IS_ERR(clk)) {
> -dev_err(>dev, "failed to get clock\n");
> -return PTR_ERR(clk);
> -}
> -
>  glue->phy = devm_phy_get(>dev, "usb-phy");
>  if (IS_ERR(glue->phy)) {
>  if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
> @@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device
 *pdev)
>  }
>
>  glue->dev= >dev;
> -glue->clk= clk;
>
>  if (IS_ENABLED(CONFIG_OF) && np) {
>  pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> @@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device
 *pdev)
>  pinfo.data = pdata;
>  pinfo.size_data = sizeof(*pdata);
>
> +pm_runtime_enable(>dev);
> +
>  glue->musb = platform_device_register_full();
>  ret = PTR_ERR_OR_ZERO(glue->musb);
>  if (ret) {
> @@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device
 *pdev)
>
>  platform_device_unregister(glue->musb);
>  usb_phy_generic_unregister(glue->usb_phy);
> +pm_runtime_disable(>dev);
>
>  return 0;
>  }
> @@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
>  ret = phy_power_off(glue->phy);
>  if (ret)
>  return ret;
> -clk_disable_unprepare(glue->clk);
> +pm_runtime_put_sync(dev);

 This, most probably will do nothing as Suspend framework will increase
 ref counter.
 Better way might be to use PM runtime force API.
 pm_runtime_force_suspend()
>>> Good catch. Effectively, the device remain active.
>>> But we can't use pm_runtime_force_suspend() because it expect that all
>>> child have been
>>> runtime suspended which is usually not the case.
>>
>> If this is the parent - it should be suspended the last and any
>> children are
>> not expected to be accessible after that.
> Yes but suspended doesn't mean runtime suspended.
> In the case of system suspend, the MUSB core will be suspended but its
> runtime_status
> will remain active and so pm_runtime_force_suspend() will refuse to work
> because it will
> not consider the MUSB core as suspend.
>>
>> Also, if there are will be force_suspend() here and force_resume() in
>> da8xx_resume()
>> then parent should always be active before any child.
>>
>> So, I seems didn't get your point :(
> I think with an example and some logs it should be more clear:
> rtcwake -d /dev/rtc0 -m mem -s 1
> rtcwake: assuming RTC uses UTC ...
> rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Mar 22 00:43:07 2017
> PM: Syncing filesystems ... done.
> Freezing user space processes ... (elapsed 0.002 seconds) done.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> Suspending console(s) (use no_console_suspend to debug)
> davinci_mdio davinci_mdio.0: resetting idled controller
> musb-da8xx musb-da8xx: runtime PM trying to suspend device but active child
> PM: suspend of devices complete after 167.287 msecs
> PM: late suspend of devices complete after 8.752 msecs
> PM: noirq suspend of devices complete after 8.389 msecs
> PM: noirq resume of devices complete after 4.385 msecs
> PM: early resume of devices complete after 5.880 msecs
> davinci_mdio davinci_mdio.0: resetting idled controller
> SMSC LAN8710/LAN8720 davinci_mdio.0:07: attached PHY driver [SMSC
> LAN8710/LAN8720] (mii_bus:phy_addr=davinci_mdio.0:07, irq=-1)
> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x0161): FIFO underflow
> Suspended for 1.454 seconds
> davinci_emac davinci_emac.1 eth0: Link is Up - 100Mbps/Full - flow
> control off
> PM: resume of devices complete after 4178.211 msecs
> Restarting tasks ...

Re: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Sekhar Nori]

On Tuesday 28 March 2017 06:09 AM, Kevin Hilman wrote:
> Alexandre Bailon  writes:
> 
>> Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
>> Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
>> CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
>> to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
>> the clock by using PM runtime.
>>
>> Signed-off-by: Alexandre Bailon 
> 
> Dumb question: even if the clock is shared with cppi4, doesn't the
> use-couting in the clock API handle it so that things should function
> fine even without this patch?
> 
> Some other comments/questions below...
> 
>> ---
>>  drivers/usb/musb/da8xx.c | 27 ---
>>  1 file changed, 8 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
>> index ed28afd..89e12f6 100644
>> --- a/drivers/usb/musb/da8xx.c
>> +++ b/drivers/usb/musb/da8xx.c
>> @@ -30,7 +30,6 @@
>>   */
>>  
>>  #include 
>> -#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -86,7 +85,6 @@ struct da8xx_glue {
>>  struct device   *dev;
>>  struct platform_device  *musb;
>>  struct platform_device  *usb_phy;
>> -struct clk  *clk;
>>  struct phy  *phy;
>>  };
>>  
>> @@ -376,11 +374,7 @@ static int da8xx_musb_init(struct musb *musb)
>>  
>>  musb->mregs += DA8XX_MENTOR_CORE_OFFSET;
>>  
>> -ret = clk_prepare_enable(glue->clk);
>> -if (ret) {
>> -dev_err(glue->dev, "failed to enable clock\n");
>> -return ret;
>> -}
>> +pm_runtime_get_sync(musb->controller->parent);
>>  
>>  /* Returns zero if e.g. not clocked */
>>  rev = musb_readl(reg_base, DA8XX_USB_REVISION_REG);
>> @@ -423,7 +417,7 @@ static int da8xx_musb_init(struct musb *musb)
>>  err_phy_power_on:
>>  phy_exit(glue->phy);
>>  fail:
>> -clk_disable_unprepare(glue->clk);
>> +pm_runtime_put(musb->controller->parent);
>>  return ret;
>>  }
>>  
>> @@ -435,7 +429,7 @@ static int da8xx_musb_exit(struct musb *musb)
>>  
>>  phy_power_off(glue->phy);
>>  phy_exit(glue->phy);
>> -clk_disable_unprepare(glue->clk);
>> +pm_runtime_put(musb->controller->parent);
>>  
>>  usb_put_phy(musb->xceiv);
>>  
>> @@ -519,7 +513,6 @@ static int da8xx_probe(struct platform_device *pdev)
>>  struct musb_hdrc_platform_data  *pdata = dev_get_platdata(>dev);
>>  struct da8xx_glue   *glue;
>>  struct platform_device_info pinfo;
>> -struct clk  *clk;
>>  struct device_node  *np = pdev->dev.of_node;
>>  int ret;
>>  
>> @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
>>  if (!glue)
>>  return -ENOMEM;
>>  
>> -clk = devm_clk_get(>dev, "usb20");
>> -if (IS_ERR(clk)) {
>> -dev_err(>dev, "failed to get clock\n");
>> -return PTR_ERR(clk);
>> -}
> 
> Something isn't quite right here.
> 
> This clk_get uses a con_id "usb20", but when converting to runtime PM,
> we rely on the pm_clk layer (used on davinci for runtime PM) to do clock
> lookups by the default con_ids.  I guess this still probably works
> because we fallback to the NULL con_id.

Right, since DaVinci uses pm_clk layer for runtime PM, we support
runtime PM on only two types of clocks. Those with con_id in the list of
con_ids recognized by pm_clk layer or those with NULL con_id. For
devices which use a single clock, NULL con_id is preferred.

Thanks,
Sekhar
--
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: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Kevin Hilman]

Alexandre Bailon  writes:

> Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
> Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
> CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
> to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
> the clock by using PM runtime.
>
> Signed-off-by: Alexandre Bailon 

Dumb question: even if the clock is shared with cppi4, doesn't the
use-couting in the clock API handle it so that things should function
fine even without this patch?

Some other comments/questions below...

> ---
>  drivers/usb/musb/da8xx.c | 27 ---
>  1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index ed28afd..89e12f6 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -30,7 +30,6 @@
>   */
>  
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -86,7 +85,6 @@ struct da8xx_glue {
>   struct device   *dev;
>   struct platform_device  *musb;
>   struct platform_device  *usb_phy;
> - struct clk  *clk;
>   struct phy  *phy;
>  };
>  
> @@ -376,11 +374,7 @@ static int da8xx_musb_init(struct musb *musb)
>  
>   musb->mregs += DA8XX_MENTOR_CORE_OFFSET;
>  
> - ret = clk_prepare_enable(glue->clk);
> - if (ret) {
> - dev_err(glue->dev, "failed to enable clock\n");
> - return ret;
> - }
> + pm_runtime_get_sync(musb->controller->parent);
>  
>   /* Returns zero if e.g. not clocked */
>   rev = musb_readl(reg_base, DA8XX_USB_REVISION_REG);
> @@ -423,7 +417,7 @@ static int da8xx_musb_init(struct musb *musb)
>  err_phy_power_on:
>   phy_exit(glue->phy);
>  fail:
> - clk_disable_unprepare(glue->clk);
> + pm_runtime_put(musb->controller->parent);
>   return ret;
>  }
>  
> @@ -435,7 +429,7 @@ static int da8xx_musb_exit(struct musb *musb)
>  
>   phy_power_off(glue->phy);
>   phy_exit(glue->phy);
> - clk_disable_unprepare(glue->clk);
> + pm_runtime_put(musb->controller->parent);
>  
>   usb_put_phy(musb->xceiv);
>  
> @@ -519,7 +513,6 @@ static int da8xx_probe(struct platform_device *pdev)
>   struct musb_hdrc_platform_data  *pdata = dev_get_platdata(>dev);
>   struct da8xx_glue   *glue;
>   struct platform_device_info pinfo;
> - struct clk  *clk;
>   struct device_node  *np = pdev->dev.of_node;
>   int ret;
>  
> @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
>   if (!glue)
>   return -ENOMEM;
>  
> - clk = devm_clk_get(>dev, "usb20");
> - if (IS_ERR(clk)) {
> - dev_err(>dev, "failed to get clock\n");
> - return PTR_ERR(clk);
> - }

Something isn't quite right here.

This clk_get uses a con_id "usb20", but when converting to runtime PM,
we rely on the pm_clk layer (used on davinci for runtime PM) to do clock
lookups by the default con_ids.  I guess this still probably works
because we fallback to the NULL con_id.

>   glue->phy = devm_phy_get(>dev, "usb-phy");
>   if (IS_ERR(glue->phy)) {
>   if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
> @@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device *pdev)
>   }
>  
>   glue->dev   = >dev;
> - glue->clk   = clk;
>  
>   if (IS_ENABLED(CONFIG_OF) && np) {
>   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> @@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device *pdev)
>   pinfo.data = pdata;
>   pinfo.size_data = sizeof(*pdata);
>  
> + pm_runtime_enable(>dev);
> +
>   glue->musb = platform_device_register_full();
>   ret = PTR_ERR_OR_ZERO(glue->musb);
>   if (ret) {
> @@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device *pdev)
>  
>   platform_device_unregister(glue->musb);
>   usb_phy_generic_unregister(glue->usb_phy);
> + pm_runtime_disable(>dev);
>  
>   return 0;
>  }
> @@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
>   ret = phy_power_off(glue->phy);
>   if (ret)
>   return ret;
> - clk_disable_unprepare(glue->clk);
> + pm_runtime_put_sync(dev);

I'm a bit lots in the MUSB layering here, but does this 'dev' correspond
to musb->controller->parent?

Kevin


>   return 0;
>  }
> @@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
>   int ret;
>   struct da8xx_glue *glue = dev_get_drvdata(dev);
>  
> - ret = clk_prepare_enable(glue->clk);
> + ret = pm_runtime_get_sync(dev);
>   if (ret)
>   return ret;
>   return phy_power_on(glue->phy);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to 

Re: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Alexandre Bailon]

On 03/27/2017 07:38 PM, Grygorii Strashko wrote:



On 03/27/2017 11:39 AM, Alexandre Bailon wrote:

Hello Grygorii,
On 03/24/2017 06:26 PM, Grygorii Strashko wrote:



On 03/24/2017 09:36 AM, Alexandre Bailon wrote:

Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
the clock by using PM runtime.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/da8xx.c | 27 ---
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index ed28afd..89e12f6 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -30,7 +30,6 @@
  */



[...]



@@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device

*pdev)

 if (!glue)
 return -ENOMEM;

-clk = devm_clk_get(>dev, "usb20");
-if (IS_ERR(clk)) {
-dev_err(>dev, "failed to get clock\n");
-return PTR_ERR(clk);
-}
-
 glue->phy = devm_phy_get(>dev, "usb-phy");
 if (IS_ERR(glue->phy)) {
 if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
@@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device

*pdev)

 }

 glue->dev= >dev;
-glue->clk= clk;

 if (IS_ENABLED(CONFIG_OF) && np) {
 pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
@@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device

*pdev)

 pinfo.data = pdata;
 pinfo.size_data = sizeof(*pdata);

+pm_runtime_enable(>dev);
+
 glue->musb = platform_device_register_full();
 ret = PTR_ERR_OR_ZERO(glue->musb);
 if (ret) {
@@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device

*pdev)


 platform_device_unregister(glue->musb);
 usb_phy_generic_unregister(glue->usb_phy);
+pm_runtime_disable(>dev);

 return 0;
 }
@@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
 ret = phy_power_off(glue->phy);
 if (ret)
 return ret;
-clk_disable_unprepare(glue->clk);
+pm_runtime_put_sync(dev);


This, most probably will do nothing as Suspend framework will increase
ref counter.
Better way might be to use PM runtime force API.
pm_runtime_force_suspend()

Good catch. Effectively, the device remain active.
But we can't use pm_runtime_force_suspend() because it expect that all
child have been
runtime suspended which is usually not the case.


If this is the parent - it should be suspended the last and any children are
not expected to be accessible after that.

Yes but suspended doesn't mean runtime suspended.
In the case of system suspend, the MUSB core will be suspended but its 
runtime_status
will remain active and so pm_runtime_force_suspend() will refuse to work 
because it will

not consider the MUSB core as suspend.


Also, if there are will be force_suspend() here and force_resume() in 
da8xx_resume()
then parent should always be active before any child.

So, I seems didn't get your point :(

I think with an example and some logs it should be more clear:
rtcwake -d /dev/rtc0 -m mem -s 1
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Mar 22 00:43:07 2017
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.002 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
davinci_mdio davinci_mdio.0: resetting idled controller
musb-da8xx musb-da8xx: runtime PM trying to suspend device but active child
PM: suspend of devices complete after 167.287 msecs
PM: late suspend of devices complete after 8.752 msecs
PM: noirq suspend of devices complete after 8.389 msecs
PM: noirq resume of devices complete after 4.385 msecs
PM: early resume of devices complete after 5.880 msecs
davinci_mdio davinci_mdio.0: resetting idled controller
SMSC LAN8710/LAN8720 davinci_mdio.0:07: attached PHY driver [SMSC 
LAN8710/LAN8720] (mii_bus:phy_addr=davinci_mdio.0:07, irq=-1)

tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x0161): FIFO underflow
Suspended for 1.454 seconds
davinci_emac davinci_emac.1 eth0: Link is Up - 100Mbps/Full - flow 
control off

PM: resume of devices complete after 4178.211 msecs
Restarting tasks ...
usb 2-1: USB disconnect, device number 3
done.

I'm using rtcwake to test suspend / resume.
As you can see in the log, musb-da8xx doesn't complete the suspend 
because it child is active

(though it doesn't prevent the suspend to happen).
On resume, the USB device disconnects and from here the USB controller 
is dead.
It will not detect any connect / disconnect anymore. This happens 
because pm_runtime_force_resume()

fails and the resume callback exit before to turn on the OTG phy.








 return 0;
 }
@@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
  

Re: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Grygorii Strashko]

On 03/27/2017 11:39 AM, Alexandre Bailon wrote:
> Hello Grygorii,
> On 03/24/2017 06:26 PM, Grygorii Strashko wrote:
>>
>>
>> On 03/24/2017 09:36 AM, Alexandre Bailon wrote:
>> > Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
>> > Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
>> > CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
>> > to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
>> > the clock by using PM runtime.
>> >
>> > Signed-off-by: Alexandre Bailon 
>> > ---
>> >  drivers/usb/musb/da8xx.c | 27 ---
>> >  1 file changed, 8 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
>> > index ed28afd..89e12f6 100644
>> > --- a/drivers/usb/musb/da8xx.c
>> > +++ b/drivers/usb/musb/da8xx.c
>> > @@ -30,7 +30,6 @@
>> >   */
>> >
>>
>> [...]
>>
>> >
>> > @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device
>> *pdev)
>> >  if (!glue)
>> >  return -ENOMEM;
>> >
>> > -clk = devm_clk_get(>dev, "usb20");
>> > -if (IS_ERR(clk)) {
>> > -dev_err(>dev, "failed to get clock\n");
>> > -return PTR_ERR(clk);
>> > -}
>> > -
>> >  glue->phy = devm_phy_get(>dev, "usb-phy");
>> >  if (IS_ERR(glue->phy)) {
>> >  if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
>> > @@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device
>> *pdev)
>> >  }
>> >
>> >  glue->dev= >dev;
>> > -glue->clk= clk;
>> >
>> >  if (IS_ENABLED(CONFIG_OF) && np) {
>> >  pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
>> > @@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device
>> *pdev)
>> >  pinfo.data = pdata;
>> >  pinfo.size_data = sizeof(*pdata);
>> >
>> > +pm_runtime_enable(>dev);
>> > +
>> >  glue->musb = platform_device_register_full();
>> >  ret = PTR_ERR_OR_ZERO(glue->musb);
>> >  if (ret) {
>> > @@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device
>> *pdev)
>> >
>> >  platform_device_unregister(glue->musb);
>> >  usb_phy_generic_unregister(glue->usb_phy);
>> > +pm_runtime_disable(>dev);
>> >
>> >  return 0;
>> >  }
>> > @@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
>> >  ret = phy_power_off(glue->phy);
>> >  if (ret)
>> >  return ret;
>> > -clk_disable_unprepare(glue->clk);
>> > +pm_runtime_put_sync(dev);
>>
>> This, most probably will do nothing as Suspend framework will increase
>> ref counter.
>> Better way might be to use PM runtime force API.
>> pm_runtime_force_suspend()
> Good catch. Effectively, the device remain active.
> But we can't use pm_runtime_force_suspend() because it expect that all
> child have been
> runtime suspended which is usually not the case.

If this is the parent - it should be suspended the last and any children are
not expected to be accessible after that. 

Also, if there are will be force_suspend() here and force_resume() in 
da8xx_resume()
then parent should always be active before any child. 

So, I seems didn't get your point :(



>>
>> >
>> >  return 0;
>> >  }
>> > @@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
>> >  int ret;
>> >  struct da8xx_glue *glue = dev_get_drvdata(dev);
>> >
>> > -ret = clk_prepare_enable(glue->clk);
>> > +ret = pm_runtime_get_sync(dev);
>> >  if (ret)
>> >  return ret;
>>
>> Better way might be to use PM runtime force API.
> Again, it will not work. Because the forced runtime suspend will not
> complete
> because child are not runtime suspended then the resume will not happen.
>>
>> >  return phy_power_on(glue->phy);
>> >
>>
> Do you have any other suggestions to fix it?
> My original intent was to give a way to CPPI 4.1 DMA driver to enable or
> disable the usb20 clock owned DA8xx USB glue driver.
> 
> Thanks,
> Alexandre

-- 
regards,
-grygorii
--
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: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Alexandre Bailon]

Hello Grygorii,
On 03/24/2017 06:26 PM, Grygorii Strashko wrote:



On 03/24/2017 09:36 AM, Alexandre Bailon wrote:
> Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
> Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
> CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
> to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
> the clock by using PM runtime.
>
> Signed-off-by: Alexandre Bailon 
> ---
>  drivers/usb/musb/da8xx.c | 27 ---
>  1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index ed28afd..89e12f6 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -30,7 +30,6 @@
>   */
>

[...]

>
> @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
>  if (!glue)
>  return -ENOMEM;
>
> -clk = devm_clk_get(>dev, "usb20");
> -if (IS_ERR(clk)) {
> -dev_err(>dev, "failed to get clock\n");
> -return PTR_ERR(clk);
> -}
> -
>  glue->phy = devm_phy_get(>dev, "usb-phy");
>  if (IS_ERR(glue->phy)) {
>  if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
> @@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device *pdev)
>  }
>
>  glue->dev= >dev;
> -glue->clk= clk;
>
>  if (IS_ENABLED(CONFIG_OF) && np) {
>  pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> @@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device *pdev)
>  pinfo.data = pdata;
>  pinfo.size_data = sizeof(*pdata);
>
> +pm_runtime_enable(>dev);
> +
>  glue->musb = platform_device_register_full();
>  ret = PTR_ERR_OR_ZERO(glue->musb);
>  if (ret) {
> @@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device *pdev)
>
>  platform_device_unregister(glue->musb);
>  usb_phy_generic_unregister(glue->usb_phy);
> +pm_runtime_disable(>dev);
>
>  return 0;
>  }
> @@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
>  ret = phy_power_off(glue->phy);
>  if (ret)
>  return ret;
> -clk_disable_unprepare(glue->clk);
> +pm_runtime_put_sync(dev);

This, most probably will do nothing as Suspend framework will increase ref 
counter.
Better way might be to use PM runtime force API. pm_runtime_force_suspend()

Good catch. Effectively, the device remain active.
But we can't use pm_runtime_force_suspend() because it expect that all child 
have been
runtime suspended which is usually not the case.


>
>  return 0;
>  }
> @@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
>  int ret;
>  struct da8xx_glue *glue = dev_get_drvdata(dev);
>
> -ret = clk_prepare_enable(glue->clk);
> +ret = pm_runtime_get_sync(dev);
>  if (ret)
>  return ret;

Better way might be to use PM runtime force API.

Again, it will not work. Because the forced runtime suspend will not complete
because child are not runtime suspended then the resume will not happen.


>  return phy_power_on(glue->phy);
>


Do you have any other suggestions to fix it?
My original intent was to give a way to CPPI 4.1 DMA driver to enable or
disable the usb20 clock owned DA8xx USB glue driver.

Thanks,
Alexandre
--
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: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Grygorii Strashko]

On 03/24/2017 09:36 AM, Alexandre Bailon wrote:
> Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
> Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
> CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
> to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
> the clock by using PM runtime.
> 
> Signed-off-by: Alexandre Bailon 
> ---
>  drivers/usb/musb/da8xx.c | 27 ---
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index ed28afd..89e12f6 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -30,7 +30,6 @@
>   */
>  

[...]

>  
> @@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
>   if (!glue)
>   return -ENOMEM;
>  
> - clk = devm_clk_get(>dev, "usb20");
> - if (IS_ERR(clk)) {
> - dev_err(>dev, "failed to get clock\n");
> - return PTR_ERR(clk);
> - }
> -
>   glue->phy = devm_phy_get(>dev, "usb-phy");
>   if (IS_ERR(glue->phy)) {
>   if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
> @@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device *pdev)
>   }
>  
>   glue->dev   = >dev;
> - glue->clk   = clk;
>  
>   if (IS_ENABLED(CONFIG_OF) && np) {
>   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> @@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device *pdev)
>   pinfo.data = pdata;
>   pinfo.size_data = sizeof(*pdata);
>  
> + pm_runtime_enable(>dev);
> +
>   glue->musb = platform_device_register_full();
>   ret = PTR_ERR_OR_ZERO(glue->musb);
>   if (ret) {
> @@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device *pdev)
>  
>   platform_device_unregister(glue->musb);
>   usb_phy_generic_unregister(glue->usb_phy);
> + pm_runtime_disable(>dev);
>  
>   return 0;
>  }
> @@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
>   ret = phy_power_off(glue->phy);
>   if (ret)
>   return ret;
> - clk_disable_unprepare(glue->clk);
> + pm_runtime_put_sync(dev);

This, most probably will do nothing as Suspend framework will increase ref 
counter.
Better way might be to use PM runtime force API. pm_runtime_force_suspend()

>  
>   return 0;
>  }
> @@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
>   int ret;
>   struct da8xx_glue *glue = dev_get_drvdata(dev);
>  
> - ret = clk_prepare_enable(glue->clk);
> + ret = pm_runtime_get_sync(dev);
>   if (ret)
>   return ret;

Better way might be to use PM runtime force API.

>   return phy_power_on(glue->phy);
> 

-- 
regards,
-grygorii
--
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

[RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Alexandre Bailon]

Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
the clock by using PM runtime.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/da8xx.c | 27 ---
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index ed28afd..89e12f6 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -30,7 +30,6 @@
  */
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,7 +85,6 @@ struct da8xx_glue {
struct device   *dev;
struct platform_device  *musb;
struct platform_device  *usb_phy;
-   struct clk  *clk;
struct phy  *phy;
 };
 
@@ -376,11 +374,7 @@ static int da8xx_musb_init(struct musb *musb)
 
musb->mregs += DA8XX_MENTOR_CORE_OFFSET;
 
-   ret = clk_prepare_enable(glue->clk);
-   if (ret) {
-   dev_err(glue->dev, "failed to enable clock\n");
-   return ret;
-   }
+   pm_runtime_get_sync(musb->controller->parent);
 
/* Returns zero if e.g. not clocked */
rev = musb_readl(reg_base, DA8XX_USB_REVISION_REG);
@@ -423,7 +417,7 @@ static int da8xx_musb_init(struct musb *musb)
 err_phy_power_on:
phy_exit(glue->phy);
 fail:
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put(musb->controller->parent);
return ret;
 }
 
@@ -435,7 +429,7 @@ static int da8xx_musb_exit(struct musb *musb)
 
phy_power_off(glue->phy);
phy_exit(glue->phy);
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put(musb->controller->parent);
 
usb_put_phy(musb->xceiv);
 
@@ -519,7 +513,6 @@ static int da8xx_probe(struct platform_device *pdev)
struct musb_hdrc_platform_data  *pdata = dev_get_platdata(>dev);
struct da8xx_glue   *glue;
struct platform_device_info pinfo;
-   struct clk  *clk;
struct device_node  *np = pdev->dev.of_node;
int ret;
 
@@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
if (!glue)
return -ENOMEM;
 
-   clk = devm_clk_get(>dev, "usb20");
-   if (IS_ERR(clk)) {
-   dev_err(>dev, "failed to get clock\n");
-   return PTR_ERR(clk);
-   }
-
glue->phy = devm_phy_get(>dev, "usb-phy");
if (IS_ERR(glue->phy)) {
if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
@@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device *pdev)
}
 
glue->dev   = >dev;
-   glue->clk   = clk;
 
if (IS_ENABLED(CONFIG_OF) && np) {
pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
@@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device *pdev)
pinfo.data = pdata;
pinfo.size_data = sizeof(*pdata);
 
+   pm_runtime_enable(>dev);
+
glue->musb = platform_device_register_full();
ret = PTR_ERR_OR_ZERO(glue->musb);
if (ret) {
@@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device *pdev)
 
platform_device_unregister(glue->musb);
usb_phy_generic_unregister(glue->usb_phy);
+   pm_runtime_disable(>dev);
 
return 0;
 }
@@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
ret = phy_power_off(glue->phy);
if (ret)
return ret;
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put_sync(dev);
 
return 0;
 }
@@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
int ret;
struct da8xx_glue *glue = dev_get_drvdata(dev);
 
-   ret = clk_prepare_enable(glue->clk);
+   ret = pm_runtime_get_sync(dev);
if (ret)
return ret;
return phy_power_on(glue->phy);
-- 
2.10.2

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

[PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime

[Alexandre Bailon]

Currently, MUSB DA8xx glue driver doesn't have PM runtime support.
Because the CPPI 4.1 is using the same clock as MUSB DA8xx and
CPPI 4.1 is a child of MUSB DA8xx glue, add support of PM runtime
to the DA8xx glue driver in order to let the CPPI 4.1 driver manage
the clock by using PM runtime.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/da8xx.c | 27 ---
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index ed28afd..89e12f6 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -30,7 +30,6 @@
  */
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,7 +85,6 @@ struct da8xx_glue {
struct device   *dev;
struct platform_device  *musb;
struct platform_device  *usb_phy;
-   struct clk  *clk;
struct phy  *phy;
 };
 
@@ -376,11 +374,7 @@ static int da8xx_musb_init(struct musb *musb)
 
musb->mregs += DA8XX_MENTOR_CORE_OFFSET;
 
-   ret = clk_prepare_enable(glue->clk);
-   if (ret) {
-   dev_err(glue->dev, "failed to enable clock\n");
-   return ret;
-   }
+   pm_runtime_get_sync(musb->controller->parent);
 
/* Returns zero if e.g. not clocked */
rev = musb_readl(reg_base, DA8XX_USB_REVISION_REG);
@@ -423,7 +417,7 @@ static int da8xx_musb_init(struct musb *musb)
 err_phy_power_on:
phy_exit(glue->phy);
 fail:
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put(musb->controller->parent);
return ret;
 }
 
@@ -435,7 +429,7 @@ static int da8xx_musb_exit(struct musb *musb)
 
phy_power_off(glue->phy);
phy_exit(glue->phy);
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put(musb->controller->parent);
 
usb_put_phy(musb->xceiv);
 
@@ -519,7 +513,6 @@ static int da8xx_probe(struct platform_device *pdev)
struct musb_hdrc_platform_data  *pdata = dev_get_platdata(>dev);
struct da8xx_glue   *glue;
struct platform_device_info pinfo;
-   struct clk  *clk;
struct device_node  *np = pdev->dev.of_node;
int ret;
 
@@ -527,12 +520,6 @@ static int da8xx_probe(struct platform_device *pdev)
if (!glue)
return -ENOMEM;
 
-   clk = devm_clk_get(>dev, "usb20");
-   if (IS_ERR(clk)) {
-   dev_err(>dev, "failed to get clock\n");
-   return PTR_ERR(clk);
-   }
-
glue->phy = devm_phy_get(>dev, "usb-phy");
if (IS_ERR(glue->phy)) {
if (PTR_ERR(glue->phy) != -EPROBE_DEFER)
@@ -541,7 +528,6 @@ static int da8xx_probe(struct platform_device *pdev)
}
 
glue->dev   = >dev;
-   glue->clk   = clk;
 
if (IS_ENABLED(CONFIG_OF) && np) {
pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
@@ -587,6 +573,8 @@ static int da8xx_probe(struct platform_device *pdev)
pinfo.data = pdata;
pinfo.size_data = sizeof(*pdata);
 
+   pm_runtime_enable(>dev);
+
glue->musb = platform_device_register_full();
ret = PTR_ERR_OR_ZERO(glue->musb);
if (ret) {
@@ -603,6 +591,7 @@ static int da8xx_remove(struct platform_device *pdev)
 
platform_device_unregister(glue->musb);
usb_phy_generic_unregister(glue->usb_phy);
+   pm_runtime_disable(>dev);
 
return 0;
 }
@@ -616,7 +605,7 @@ static int da8xx_suspend(struct device *dev)
ret = phy_power_off(glue->phy);
if (ret)
return ret;
-   clk_disable_unprepare(glue->clk);
+   pm_runtime_put_sync(dev);
 
return 0;
 }
@@ -626,7 +615,7 @@ static int da8xx_resume(struct device *dev)
int ret;
struct da8xx_glue *glue = dev_get_drvdata(dev);
 
-   ret = clk_prepare_enable(glue->clk);
+   ret = pm_runtime_get_sync(dev);
if (ret)
return ret;
return phy_power_on(glue->phy);
-- 
2.10.2

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