From patchwork Mon Apr 3 20:14:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grygorii Strashko X-Patchwork-Id: 9660517 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E40CC6016C for ; Mon, 3 Apr 2017 20:16:35 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CFDF22815E for ; Mon, 3 Apr 2017 20:16:35 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C1336284EC; Mon, 3 Apr 2017 20:16:35 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BAEAF2815E for ; Mon, 3 Apr 2017 20:16:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751924AbdDCUOy (ORCPT ); Mon, 3 Apr 2017 16:14:54 -0400 Received: from fllnx210.ext.ti.com ([198.47.19.17]:30451 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751842AbdDCUOR (ORCPT ); Mon, 3 Apr 2017 16:14:17 -0400 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by fllnx210.ext.ti.com (8.15.1/8.15.1) with ESMTP id v33KECHj031773; Mon, 3 Apr 2017 15:14:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com; s=ti-com-17Q1; t=1491250452; bh=jM8P47Vh9xHRC9tpyvcgjPaamZVy5bcWx7fywUJKc1Y=; h=Subject:To:References:CC:From:Date:In-Reply-To; b=xbBh8EaI+rBRijCIKSgylbrEIGh9VZuaqQ9ocA+boZUDH72cywk5+JIze4mSdI456 l+N84XonDdoOtcWJ4eftbnQv8KfUcBwzsg/nyH03xaVVqp59N4ctUBL8jTH/2U6pR4 vkE6kZOH19iQotqjUAVLWLAWPuCojYA4Ie9/Txc8= Received: from DLEE70.ent.ti.com (dlee70.ent.ti.com [157.170.170.113]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id v33KECvD013694; Mon, 3 Apr 2017 15:14:12 -0500 Received: from [128.247.83.96] (128.247.83.96) by DLEE70.ent.ti.com (157.170.170.113) with Microsoft SMTP Server id 14.3.294.0; Mon, 3 Apr 2017 15:14:12 -0500 Subject: Re: [RESEND PATCH v6 4/4] usb: musb: da8xx: Add a primary support of PM runtime To: Alexandre Bailon References: <20170324143600.4704-1-abailon@baylibre.com> <20170324143600.4704-5-abailon@baylibre.com> <703e5e95-8bde-2b1c-0d63-4ed3e03f53b8@ti.com> <03cc95cd-5967-c0c7-06a7-89cf08dbde47@baylibre.com> <1552a8e0-9efe-3658-e511-76e056ed5588@ti.com> <322bdb6b-72c4-01e1-8ae7-bf04ea89a1ee@baylibre.com> CC: , , , , , , , From: Grygorii Strashko Message-ID: <66c2ba1d-8da2-3932-afc7-5fe00eaad932@ti.com> Date: Mon, 3 Apr 2017 15:14:08 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <322bdb6b-72c4-01e1-8ae7-bf04ea89a1ee@baylibre.com> X-Originating-IP: [128.247.83.96] Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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(&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 Date: Mon, 3 Apr 2017 14:53:57 -0500 Subject: [PATCH] [draft] drivers/usb/musb/musb_dsps: do suspend Signed-off-by: Grygorii Strashko --- 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