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 <abailon@xxxxxxxxxxxx>
>>>>> ---
>>>>> 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(&pdev->dev, "usb20");
>>>>> - if (IS_ERR(clk)) {
>>>>> - dev_err(&pdev->dev, "failed to get clock\n");
>>>>> - return PTR_ERR(clk);
>>>>> - }
>>>>> -
>>>>> glue->phy = devm_phy_get(&pdev->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 = &pdev->dev;
>>>>> - glue->clk = clk;
>>>>>
>>>>> if (IS_ENABLED(CONFIG_OF) && np) {
>>>>> pdata = devm_kzalloc(&pdev->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(&pdev->dev);
>>>>> +
>>>>> glue->musb = platform_device_register_full(&pinfo);
>>>>> 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(&pdev->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(0x00000161): 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.
>>
More simple way to test it (at least on am335x-evm):
33 echo platform > /sys/power/pm_test
34 echo 1 > /sys/power/pm_print_times
35 echo mem > /sys/power/state
I was able to reproduce and play with it on am335x-evm, unfortunately you are right
and pm_runtime_force_x() APIs will not work out of the box, because USB framework
keeps a lot of devices in its hierarchy in PM runtime Active state
(and this hierarchy is not static - depends on what is plugged in port during suspend).
So, I see two option here:
1) use pm_clk_suspend/pm_clk_resume() directly in .suspend()/.resume()
2) do some hacks as in diff below
you might not need get/put in suspend as you going do get in .probe() without put.
>From ac0178455f8dfda635d8d45e8235d73b936a19a9 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@xxxxxx>
Date: Mon, 3 Apr 2017 14:53:57 -0500
Subject: [PATCH] [draft] drivers/usb/musb/musb_dsps: do suspend
Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
---
drivers/usb/musb/musb_dsps.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 9c7ee26..43306a7 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -1011,6 +1011,8 @@ static int dsps_suspend(struct device *dev)
struct musb *musb = platform_get_drvdata(glue->musb);
void __iomem *mbase;
+ pm_runtime_get_sync(dev);
+
del_timer_sync(&glue->timer);
if (!musb)
@@ -1027,8 +1029,9 @@ static int dsps_suspend(struct device *dev)
glue->context.rx_mode = musb_readl(mbase, wrp->rx_mode);
dsps_dma_controller_suspend(glue);
+ pm_suspend_ignore_children(dev, true);
- return 0;
+ return pm_runtime_force_suspend(dev);
}
static int dsps_resume(struct device *dev)
@@ -1040,6 +1043,8 @@ static int dsps_resume(struct device *dev)
if (!musb)
return 0;
+ pm_suspend_ignore_children(dev, false);
+ pm_runtime_force_resume(dev);
dsps_dma_controller_resume(glue);
@@ -1055,6 +1060,8 @@ static int dsps_resume(struct device *dev)
musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
dsps_mod_timer(glue, -1);
+ pm_runtime_put(dev);
+
return 0;
}
#endif
--
2.10.1
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
