| Message ID | 1387796544-18209-2-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 23, 2013 at 07:02:24PM +0800, Dong Aisheng wrote: > @@ -1160,8 +1160,6 @@ free_sdhci: > static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > { > struct sdhci_host *host = platform_get_drvdata(pdev); > - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > - struct pltfm_imx_data *imx_data = pltfm_host->priv; > int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff); > > sdhci_remove_host(host, dead); > @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > pm_runtime_dont_use_autosuspend(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > - clk_disable_unprepare(imx_data->clk_per); > - clk_disable_unprepare(imx_data->clk_ipg); > - clk_disable_unprepare(imx_data->clk_ahb); It's obviously a bad change to me. We should definitely have these clk_disable_unprepare() calls in sdhci_esdhc_imx_remove() to match the clk_prepare_enable() calls in sdhci_esdhc_imx_probe(). Otherwise, at least for !CONFIG_PM_RUNTIME build, it's broken. I'm looking at sdhci-pxav3 driver. Why can it call clk_disable_unprepare(pltfm_host->clk) in .remove() with runtime PM support in there? Is there anything about runtime PM not being done correctly for our driver? Shawn > - > sdhci_pltfm_free(pdev); > > return 0; > -- > 1.7.2.rc3 > >
On Mon, Dec 23, 2013 at 10:06:15PM +0800, Shawn Guo wrote: > On Mon, Dec 23, 2013 at 07:02:24PM +0800, Dong Aisheng wrote: > > @@ -1160,8 +1160,6 @@ free_sdhci: > > static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > > { > > struct sdhci_host *host = platform_get_drvdata(pdev); > > - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > - struct pltfm_imx_data *imx_data = pltfm_host->priv; > > int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff); > > > > sdhci_remove_host(host, dead); > > @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > > > > - clk_disable_unprepare(imx_data->clk_per); > > - clk_disable_unprepare(imx_data->clk_ipg); > > - clk_disable_unprepare(imx_data->clk_ahb); > > It's obviously a bad change to me. We should definitely have these > clk_disable_unprepare() calls in sdhci_esdhc_imx_remove() to match the > clk_prepare_enable() calls in sdhci_esdhc_imx_probe(). Otherwise, at > least for !CONFIG_PM_RUNTIME build, it's broken. > How about add the !CONFIG_RUMTIME_PM precondition for these code to avoid break non runtime pm case? #ifndef CONFIG_PM_RUNTIME clk_disable_unprepare(imx_data->clk_per); clk_disable_unprepare(imx_data->clk_ipg); clk_disable_unprepare(imx_data->clk_ahb); #endif > I'm looking at sdhci-pxav3 driver. Why can it call > clk_disable_unprepare(pltfm_host->clk) in .remove() with runtime PM > support in there? Is there anything about runtime PM not being done > correctly for our driver? > I checked the code, it seems it calls pm_runtime_get_sync(&pdev->dev); before clk_disable_*. That means the clock is already enabled, Without it, i wonder it may have the same problem. static int sdhci_pxav3_remove(struct platform_device *pdev) { ... pm_runtime_get_sync(&pdev->dev); sdhci_remove_host(host, 1); pm_runtime_disable(&pdev->dev); clk_disable_unprepare(pltfm_host->clk); clk_put(pltfm_host->clk); .. } Regards Dong Aisheng > Shawn > > > - > > sdhci_pltfm_free(pdev); > > > > return 0; > > -- > > 1.7.2.rc3 > > > > >
On Tue, Dec 24, 2013 at 10:34:13AM +0800, Dong Aisheng wrote: > > > @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > > pm_runtime_disable(&pdev->dev); > > > > > > - clk_disable_unprepare(imx_data->clk_per); > > > - clk_disable_unprepare(imx_data->clk_ipg); > > > - clk_disable_unprepare(imx_data->clk_ahb); > > > > It's obviously a bad change to me. We should definitely have these > > clk_disable_unprepare() calls in sdhci_esdhc_imx_remove() to match the > > clk_prepare_enable() calls in sdhci_esdhc_imx_probe(). Otherwise, at > > least for !CONFIG_PM_RUNTIME build, it's broken. > > > > How about add the !CONFIG_RUMTIME_PM precondition for these code to avoid > break non runtime pm case? > > #ifndef CONFIG_PM_RUNTIME > clk_disable_unprepare(imx_data->clk_per); > clk_disable_unprepare(imx_data->clk_ipg); > clk_disable_unprepare(imx_data->clk_ahb); > #endif It's quite ugly. But well, if we do not have anything better, we have to live with it. Shawn
On Tue, Dec 24, 2013 at 11:16:09AM +0800, Shawn Guo wrote: > On Tue, Dec 24, 2013 at 10:34:13AM +0800, Dong Aisheng wrote: > > > > @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > > > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > > > pm_runtime_disable(&pdev->dev); > > > > > > > > - clk_disable_unprepare(imx_data->clk_per); > > > > - clk_disable_unprepare(imx_data->clk_ipg); > > > > - clk_disable_unprepare(imx_data->clk_ahb); > > > > > > It's obviously a bad change to me. We should definitely have these > > > clk_disable_unprepare() calls in sdhci_esdhc_imx_remove() to match the > > > clk_prepare_enable() calls in sdhci_esdhc_imx_probe(). Otherwise, at > > > least for !CONFIG_PM_RUNTIME build, it's broken. > > > > > > > How about add the !CONFIG_RUMTIME_PM precondition for these code to avoid > > break non runtime pm case? > > > > #ifndef CONFIG_PM_RUNTIME > > clk_disable_unprepare(imx_data->clk_per); > > clk_disable_unprepare(imx_data->clk_ipg); > > clk_disable_unprepare(imx_data->clk_ahb); > > #endif > > It's quite ugly. But well, if we do not have anything better, we have > to live with it. To make it look less ugly, we may want to use something like the following. if (!IS_ENABLED(CONFIG_PM_RUNTIME) Shawn
On Tue, Dec 24, 2013 at 11:29 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Tue, Dec 24, 2013 at 11:16:09AM +0800, Shawn Guo wrote: >> On Tue, Dec 24, 2013 at 10:34:13AM +0800, Dong Aisheng wrote: >> > > > @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) >> > > > pm_runtime_dont_use_autosuspend(&pdev->dev); >> > > > pm_runtime_disable(&pdev->dev); >> > > > >> > > > - clk_disable_unprepare(imx_data->clk_per); >> > > > - clk_disable_unprepare(imx_data->clk_ipg); >> > > > - clk_disable_unprepare(imx_data->clk_ahb); >> > > >> > > It's obviously a bad change to me. We should definitely have these >> > > clk_disable_unprepare() calls in sdhci_esdhc_imx_remove() to match the >> > > clk_prepare_enable() calls in sdhci_esdhc_imx_probe(). Otherwise, at >> > > least for !CONFIG_PM_RUNTIME build, it's broken. >> > > >> > >> > How about add the !CONFIG_RUMTIME_PM precondition for these code to avoid >> > break non runtime pm case? >> > >> > #ifndef CONFIG_PM_RUNTIME >> > clk_disable_unprepare(imx_data->clk_per); >> > clk_disable_unprepare(imx_data->clk_ipg); >> > clk_disable_unprepare(imx_data->clk_ahb); >> > #endif >> >> It's quite ugly. But well, if we do not have anything better, we have >> to live with it. > > To make it look less ugly, we may want to use something like the > following. > > if (!IS_ENABLED(CONFIG_PM_RUNTIME) > I'm ok with it. Regards Dong Aisheng > Shawn > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 745ee1e..78b7999 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -1160,8 +1160,6 @@ free_sdhci: static int sdhci_esdhc_imx_remove(struct platform_device *pdev) { struct sdhci_host *host = platform_get_drvdata(pdev); - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct pltfm_imx_data *imx_data = pltfm_host->priv; int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff); sdhci_remove_host(host, dead); @@ -1169,10 +1167,6 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_disable(&pdev->dev); - clk_disable_unprepare(imx_data->clk_per); - clk_disable_unprepare(imx_data->clk_ipg); - clk_disable_unprepare(imx_data->clk_ahb); - sdhci_pltfm_free(pdev); return 0;
Since the clock is managed by runtime pm currently, we do not need disable it again during driver remove function, or it will cause clock disable count mismatch issue since the clocks have already been disabled. The issue can be simply reproduced by unbind the devices via sysfs. mx6slevk:/sys/bus/platform/drivers/sdhci-esdhc-imx# echo 2194000.usdhc > unbind mmc1: card aaaa removed ------------[ cut here ]------------ WARNING: CPU: 0 PID: 657 at drivers/clk/clk.c:842 __clk_disable+0x68/0x88() Modules linked in: CPU: 0 PID: 657 Comm: sh Not tainted 3.13.0-rc1+ #285 Backtrace: [<80012160>] (dump_backtrace+0x0/0x10c) from [<80012438>] (show_stack+0x18/0x1c) r6:80481370 r5:00000000 r4:8088ecc8 r3:00000000 [<80012420>] (show_stack+0x0/0x1c) from [<80616b14>] (dump_stack+0x84/0x9c) [<80616a90>] (dump_stack+0x0/0x9c) from [<80027158>] (warn_slowpath_common+0x70/0x94) r5:00000009 r4:00000000 [<800270e8>] (warn_slowpath_common+0x0/0x94) from [<80027220>] (warn_slowpath_null+0x24/0x2c) r8:bec4ff78 r7:0000000e r6:bf91d800 r5:bf81d080 r4:bf81d080 [<800271fc>] (warn_slowpath_null+0x0/0x2c) from [<80481370>] (__clk_disable+0x68/0x88) [<80481308>] (__clk_disable+0x0/0x88) from [<8048148c>] (clk_disable+0x20/0x2c) r4:200f0113 r3:bf95ec00 [<8048146c>] (clk_disable+0x0/0x2c) from [<80463bd8>] (sdhci_esdhc_imx_remove+0x64/0xa4) r5:bf81d080 r4:bfabb010 [<80463b74>] (sdhci_esdhc_imx_remove+0x0/0xa4) from [<8032e82c>] (platform_drv_remove+0x20/0x24) r6:808ae0e0 r5:808ae0e0 r4:bf91d810 r3:80463b74 [<8032e80c>] (platform_drv_remove+0x0/0x24) from [<8032d010>] (__device_release_driver+0x78/0xd0) [<8032cf98>] (__device_release_driver+0x0/0xd0) from [<8032d090>] (device_release_driver+0x28/0x34) r5:bf91d810 r4:bf91d844 [<8032d068>] (device_release_driver+0x0/0x34) from [<8032c0c8>] (unbind_store+0x80/0xc4) r5:bf91d810 r4:80899ba0 [<8032c048>] (unbind_store+0x0/0xc4) from [<8032b648>] (drv_attr_store+0x28/0x34) r7:bed73100 r6:0000000e r5:00000000 r4:8032b620 [<8032b620>] (drv_attr_store+0x0/0x34) from [<80140580>] (sysfs_write_file+0x1b0/0x1e4) [<801403d0>] (sysfs_write_file+0x0/0x1e4) from [<800dcda0>] (vfs_write+0xb4/0x190) [<800dccec>] (vfs_write+0x0/0x190) from [<800dd3e4>] (SyS_write+0x44/0x80) r9:0000000e r8:00000000 r7:01a00408 r6:bf3b1c00 r5:00000000 r4:00000000 [<800dd3a0>] (SyS_write+0x0/0x80) from [<8000e900>] (ret_fast_syscall+0x0/0x48) r9:bec4e000 r8:8000eac4 r7:00000004 r6:76f5fb40 r5:01a00408 r4:0000000e ---[ end trace a0897d268e6233b2 ]--- Cc: Chris Ball <cjb@laptop.org> Cc: Shawn Guo <shawn.guo@linaro.org> Signed-off-by: Dong Aisheng <b29396@freescale.com> --- drivers/mmc/host/sdhci-esdhc-imx.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)