| Message ID | 20200513173131.11200-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi: don't lose RPM savings because of manual clk handling | expand |
+ Niklas, Geert, Yamada-san, On Wed, 13 May 2020 at 19:31, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > The SDHI driver en-/disabled clocks on its own during probe() and > remove(). This basically killed all potential RPM power savings. Now, we > just enable the clocks for a short time when we access registers in > probe(). We otherwise leave all handling to RPM. That means, we need to > shift the RPM enabling code in the TMIO core a bit up, so we can access > registers there, too. No, this doesn't sound entirely right to me. However, I do admit that we may need to move the pm_runtime initialization earlier (perhaps even out of tmio_mmc_core), but for slightly different reasons. Let me elaborate. For uniphier-sd, renesas_sdhi_sys_dmac and renesas_sdhi_internal_dmac - they all have assigned the ->clk_enable|disable() ops. Which means they have internal clock management (calling clk_prepare|enable() etc). For tmio_mmc, that's not the case. On top of this, the device may also have a potential PM domain attached. If that is the case, the PM domain may or may not have clock management implemented through genpd's ->start|stop() callbacks. So, in the end we are going to have to rely on clock enable/prepare reference counting, as we have to manage the clock(s) at both the driver and the PM domain level. Taking into account all various combinations (and that CONFIG_PM may not always be set). I have started to hack on some patches, but before I share them, let me ask a few questions. 1. tmio_mmc: - is that used solely with clock management through genpd? Or has no clock management at all? 2. uniphier-sd: Don't have runtime PM callbacks assigned. It looks like it doesn't care about runtime PM, but maybe it does through a PM domain? Can we skip to enable runtime PM for uniphier-sd, no? Kind regards Uffe > > clk_summary before: > sd0 1 1 0 12480000 0 0 50000 > sdif0 2 2 0 12480000 0 0 50000 > > clk_summary after: > sd0 1 1 0 12480000 0 0 50000 > sdif0 1 1 0 12480000 0 0 50000 > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Tested on a Salvator-XS board with R-Car M3-N. > > drivers/mmc/host/renesas_sdhi_core.c | 7 +++---- > drivers/mmc/host/tmio_mmc_core.c | 14 +++++++------- > 2 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index ff72b381a6b3..d581142634f8 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -910,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, > goto efree; > > ver = sd_ctrl_read16(host, CTL_VERSION); > + renesas_sdhi_clk_disable(host); > + > /* GEN2_SDR104 is first known SDHI to use 32bit block count */ > if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) > mmc_data->max_blk_count = U16_MAX; > @@ -920,7 +922,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > ret = tmio_mmc_host_probe(host); > if (ret < 0) > - goto edisclk; > + goto efree; > > /* Enable tuning iff we have an SCC and a supported mode */ > if (of_data && of_data->scc_offset && > @@ -985,8 +987,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > eirq: > tmio_mmc_host_remove(host); > -edisclk: > - renesas_sdhi_clk_disable(host); > efree: > tmio_mmc_host_free(host); > > @@ -999,7 +999,6 @@ int renesas_sdhi_remove(struct platform_device *pdev) > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > tmio_mmc_host_remove(host); > - renesas_sdhi_clk_disable(host); > > return 0; > } > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 9a4ae954553b..6968177dd1cd 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1116,6 +1116,13 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > > _host->set_pwr = pdata->set_pwr; > > + dev_pm_domain_start(&pdev->dev); > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + > ret = tmio_mmc_init_ocr(_host); > if (ret < 0) > return ret; > @@ -1192,13 +1199,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > /* See if we also get DMA */ > tmio_mmc_request_dma(_host, pdata); > > - dev_pm_domain_start(&pdev->dev); > - pm_runtime_get_noresume(&pdev->dev); > - pm_runtime_set_active(&pdev->dev); > - pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > - pm_runtime_use_autosuspend(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > - > ret = mmc_add_host(mmc); > if (ret) > goto remove_host; > -- > 2.20.1 >
Hi Wolfram and Ulf, Thanks for working on this! On 2020-05-15 11:26:06 +0200, Ulf Hansson wrote: > + Niklas, Geert, Yamada-san, > > > On Wed, 13 May 2020 at 19:31, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > > > The SDHI driver en-/disabled clocks on its own during probe() and > > remove(). This basically killed all potential RPM power savings. Now, we > > just enable the clocks for a short time when we access registers in > > probe(). We otherwise leave all handling to RPM. That means, we need to > > shift the RPM enabling code in the TMIO core a bit up, so we can access > > registers there, too. > > No, this doesn't sound entirely right to me. > > However, I do admit that we may need to move the pm_runtime > initialization earlier (perhaps even out of tmio_mmc_core), but for > slightly different reasons. Let me elaborate. > > For uniphier-sd, renesas_sdhi_sys_dmac and renesas_sdhi_internal_dmac > - they all have assigned the ->clk_enable|disable() ops. Which means > they have internal clock management (calling clk_prepare|enable() > etc). For tmio_mmc, that's not the case. > > On top of this, the device may also have a potential PM domain > attached. If that is the case, the PM domain may or may not have clock > management implemented through genpd's ->start|stop() callbacks. > > So, in the end we are going to have to rely on clock enable/prepare > reference counting, as we have to manage the clock(s) at both the > driver and the PM domain level. Taking into account all various > combinations (and that CONFIG_PM may not always be set). I have > started to hack on some patches, but before I share them, let me ask a > few questions. I had a go at this once but gave up as I only have knowledge about one implementation and !CONFIG_PM made it messy for me. I will see if I can digout the patches and see if I can recall exactly what it was. I'm sure it was my lack of understanding and not something technical. > > 1. tmio_mmc: - is that used solely with clock management through > genpd? Or has no clock management at all? > 2. uniphier-sd: Don't have runtime PM callbacks assigned. It looks > like it doesn't care about runtime PM, but maybe it does through a PM > domain? Can we skip to enable runtime PM for uniphier-sd, no? > > Kind regards > Uffe > > > > > clk_summary before: > > sd0 1 1 0 12480000 0 0 50000 > > sdif0 2 2 0 12480000 0 0 50000 > > > > clk_summary after: > > sd0 1 1 0 12480000 0 0 50000 > > sdif0 1 1 0 12480000 0 0 50000 > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > Tested on a Salvator-XS board with R-Car M3-N. > > > > drivers/mmc/host/renesas_sdhi_core.c | 7 +++---- > > drivers/mmc/host/tmio_mmc_core.c | 14 +++++++------- > > 2 files changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > > index ff72b381a6b3..d581142634f8 100644 > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > @@ -910,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > goto efree; > > > > ver = sd_ctrl_read16(host, CTL_VERSION); > > + renesas_sdhi_clk_disable(host); > > + > > /* GEN2_SDR104 is first known SDHI to use 32bit block count */ > > if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) > > mmc_data->max_blk_count = U16_MAX; > > @@ -920,7 +922,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > > > ret = tmio_mmc_host_probe(host); > > if (ret < 0) > > - goto edisclk; > > + goto efree; > > > > /* Enable tuning iff we have an SCC and a supported mode */ > > if (of_data && of_data->scc_offset && > > @@ -985,8 +987,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > > > eirq: > > tmio_mmc_host_remove(host); > > -edisclk: > > - renesas_sdhi_clk_disable(host); > > efree: > > tmio_mmc_host_free(host); > > > > @@ -999,7 +999,6 @@ int renesas_sdhi_remove(struct platform_device *pdev) > > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > > > tmio_mmc_host_remove(host); > > - renesas_sdhi_clk_disable(host); > > > > return 0; > > } > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > > index 9a4ae954553b..6968177dd1cd 100644 > > --- a/drivers/mmc/host/tmio_mmc_core.c > > +++ b/drivers/mmc/host/tmio_mmc_core.c > > @@ -1116,6 +1116,13 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > > > > _host->set_pwr = pdata->set_pwr; > > > > + dev_pm_domain_start(&pdev->dev); > > + pm_runtime_get_noresume(&pdev->dev); > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > > + pm_runtime_use_autosuspend(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + > > ret = tmio_mmc_init_ocr(_host); > > if (ret < 0) > > return ret; > > @@ -1192,13 +1199,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > > /* See if we also get DMA */ > > tmio_mmc_request_dma(_host, pdata); > > > > - dev_pm_domain_start(&pdev->dev); > > - pm_runtime_get_noresume(&pdev->dev); > > - pm_runtime_set_active(&pdev->dev); > > - pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > > - pm_runtime_use_autosuspend(&pdev->dev); > > - pm_runtime_enable(&pdev->dev); > > - > > ret = mmc_add_host(mmc); > > if (ret) > > goto remove_host; > > -- > > 2.20.1 > >
Hi Wolfram, On Wed, May 13, 2020 at 7:31 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > The SDHI driver en-/disabled clocks on its own during probe() and > remove(). This basically killed all potential RPM power savings. Now, we > just enable the clocks for a short time when we access registers in > probe(). We otherwise leave all handling to RPM. That means, we need to > shift the RPM enabling code in the TMIO core a bit up, so we can access > registers there, too. > > clk_summary before: > sd0 1 1 0 12480000 0 0 50000 > sdif0 2 2 0 12480000 0 0 50000 > > clk_summary after: > sd0 1 1 0 12480000 0 0 50000 > sdif0 1 1 0 12480000 0 0 50000 > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Tested on a Salvator-XS board with R-Car M3-N. While this seems to have no ill effects on R-Car Gen3, RZ/A1, and RZ/A2, it does on R-Car Gen2, which prints before probing: sh_mobile_sdhi ee100000.sd: Unbalanced pm_runtime_enable! And on SH-Mobile AG5, R-Mobile A1, and R-Mobile APE6: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 5 at drivers/clk/clk.c:954 clk_core_disable+0x6c/0x2b0 sdhi1 already disabled CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.7.0-rc5-ape6evm-00536-ga65e619d428aff9a-dirty #186 Hardware name: Generic R8A73A4 (Flattened Device Tree) Workqueue: pm pm_runtime_work [<c00169e8>] (unwind_backtrace) from [<c001345c>] (show_stack+0x10/0x14) [<c001345c>] (show_stack) from [<c04b6e34>] (dump_stack+0x88/0xa8) [<c04b6e34>] (dump_stack) from [<c00270b8>] (__warn+0xd0/0xec) [<c00270b8>] (__warn) from [<c0027144>] (warn_slowpath_fmt+0x70/0x9c) [<c0027144>] (warn_slowpath_fmt) from [<c029d53c>] (clk_core_disable+0x6c/0x2b0) [<c029d53c>] (clk_core_disable) from [<c029d798>] (clk_core_disable_lock+0x18/0x24) [<c029d798>] (clk_core_disable_lock) from [<c02fa670>] (pm_clk_suspend+0x64/0x78) [<c02fa670>] (pm_clk_suspend) from [<c02f8918>] (genpd_runtime_suspend+0x110/0x1bc) [<c02f8918>] (genpd_runtime_suspend) from [<c02f01b4>] (__rpm_callback+0x30/0xe0) [<c02f01b4>] (__rpm_callback) from [<c02f02d4>] (rpm_callback+0x70/0x80) [<c02f02d4>] (rpm_callback) from [<c02f0e28>] (rpm_suspend+0x330/0x4a0) [<c02f0e28>] (rpm_suspend) from [<c02f1240>] (pm_runtime_work+0x74/0x8c) [<c02f1240>] (pm_runtime_work) from [<c00402f4>] (process_one_work+0x2cc/0x4ac) [<c00402f4>] (process_one_work) from [<c0040730>] (worker_thread+0x230/0x2f0) [<c0040730>] (worker_thread) from [<c0045b9c>] (kthread+0x12c/0x13c) [<c0045b9c>] (kthread) from [<c0009148>] (ret_from_fork+0x14/0x2c) Exception stack(0xef0cdfb0 to 0xef0cdff8) dfa0: 00000000 00000000 00000000 00000000 dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end trace c1cca51c99a6297a ]--- and later: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 12 at drivers/clk/clk.c:1014 clk_core_enable+0x6c/0x2bc Enabling unprepared sdhi1 CPU: 0 PID: 12 Comm: kworker/0:1 Tainted: G W 5.7.0-rc5-ape6evm-00536-ga65e619d428aff9a-dirty #186 Hardware name: Generic R8A73A4 (Flattened Device Tree) Workqueue: events_freezable mmc_rescan [<c00169e8>] (unwind_backtrace) from [<c001345c>] (show_stack+0x10/0x14) [<c001345c>] (show_stack) from [<c04b6e34>] (dump_stack+0x88/0xa8) [<c04b6e34>] (dump_stack) from [<c00270b8>] (__warn+0xd0/0xec) [<c00270b8>] (__warn) from [<c0027144>] (warn_slowpath_fmt+0x70/0x9c) [<c0027144>] (warn_slowpath_fmt) from [<c029d810>] (clk_core_enable+0x6c/0x2bc) [<c029d810>] (clk_core_enable) from [<c029da78>] (clk_core_enable_lock+0x18/0x2c) [<c029da78>] (clk_core_enable_lock) from [<c02fa6ec>] (pm_clk_resume+0x68/0xa0) [<c02fa6ec>] (pm_clk_resume) from [<c02f8dd0>] (genpd_runtime_resume+0xc8/0x170) [<c02f8dd0>] (genpd_runtime_resume) from [<c02f01b4>] (__rpm_callback+0x30/0xe0) [<c02f01b4>] (__rpm_callback) from [<c02f02d4>] (rpm_callback+0x70/0x80) [<c02f02d4>] (rpm_callback) from [<c02f0a50>] (rpm_resume+0x44c/0x4f4) [<c02f0a50>] (rpm_resume) from [<c02f00f0>] (__pm_runtime_resume+0x64/0x80) [<c02f00f0>] (__pm_runtime_resume) from [<c0342480>] (__mmc_claim_host+0x1c8/0x218) [<c0342480>] (__mmc_claim_host) from [<c03437f0>] (mmc_rescan+0xfc/0x260) [<c03437f0>] (mmc_rescan) from [<c00402f4>] (process_one_work+0x2cc/0x4ac) [<c00402f4>] (process_one_work) from [<c0040730>] (worker_thread+0x230/0x2f0) [<c0040730>] (worker_thread) from [<c0045b9c>] (kthread+0x12c/0x13c) [<c0045b9c>] (kthread) from [<c0009148>] (ret_from_fork+0x14/0x2c) Exception stack(0xef0f1fb0 to 0xef0f1ff8) 1fa0: 00000000 00000000 00000000 00000000 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end trace c1cca51c99a6297b ]--- sh_mobile_sdhi ee120000.sd: __pm_clk_enable: failed to enable clk (ptrval), error -108 Gr{oetje,eeting}s, Geert
On Fri, May 15, 2020 at 6:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > + Niklas, Geert, Yamada-san, > > > On Wed, 13 May 2020 at 19:31, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > > > The SDHI driver en-/disabled clocks on its own during probe() and > > remove(). This basically killed all potential RPM power savings. Now, we > > just enable the clocks for a short time when we access registers in > > probe(). We otherwise leave all handling to RPM. That means, we need to > > shift the RPM enabling code in the TMIO core a bit up, so we can access > > registers there, too. > > No, this doesn't sound entirely right to me. > > However, I do admit that we may need to move the pm_runtime > initialization earlier (perhaps even out of tmio_mmc_core), but for > slightly different reasons. Let me elaborate. > > For uniphier-sd, renesas_sdhi_sys_dmac and renesas_sdhi_internal_dmac > - they all have assigned the ->clk_enable|disable() ops. Which means > they have internal clock management (calling clk_prepare|enable() > etc). For tmio_mmc, that's not the case. > > On top of this, the device may also have a potential PM domain > attached. If that is the case, the PM domain may or may not have clock > management implemented through genpd's ->start|stop() callbacks. > > So, in the end we are going to have to rely on clock enable/prepare > reference counting, as we have to manage the clock(s) at both the > driver and the PM domain level. Taking into account all various > combinations (and that CONFIG_PM may not always be set). I have > started to hack on some patches, but before I share them, let me ask a > few questions. > > 1. tmio_mmc: - is that used solely with clock management through > genpd? Or has no clock management at all? > 2. uniphier-sd: Don't have runtime PM callbacks assigned. It looks > like it doesn't care about runtime PM, but maybe it does through a PM > domain? Can we skip to enable runtime PM for uniphier-sd, no? Right, uniphier-sd does not care about runtime PM. UniPhier SoCs do not have separate power domains.
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index ff72b381a6b3..d581142634f8 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -910,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, goto efree; ver = sd_ctrl_read16(host, CTL_VERSION); + renesas_sdhi_clk_disable(host); + /* GEN2_SDR104 is first known SDHI to use 32bit block count */ if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) mmc_data->max_blk_count = U16_MAX; @@ -920,7 +922,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, ret = tmio_mmc_host_probe(host); if (ret < 0) - goto edisclk; + goto efree; /* Enable tuning iff we have an SCC and a supported mode */ if (of_data && of_data->scc_offset && @@ -985,8 +987,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, eirq: tmio_mmc_host_remove(host); -edisclk: - renesas_sdhi_clk_disable(host); efree: tmio_mmc_host_free(host); @@ -999,7 +999,6 @@ int renesas_sdhi_remove(struct platform_device *pdev) struct tmio_mmc_host *host = platform_get_drvdata(pdev); tmio_mmc_host_remove(host); - renesas_sdhi_clk_disable(host); return 0; } diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 9a4ae954553b..6968177dd1cd 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1116,6 +1116,13 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) _host->set_pwr = pdata->set_pwr; + dev_pm_domain_start(&pdev->dev); + pm_runtime_get_noresume(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_enable(&pdev->dev); + ret = tmio_mmc_init_ocr(_host); if (ret < 0) return ret; @@ -1192,13 +1199,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) /* See if we also get DMA */ tmio_mmc_request_dma(_host, pdata); - dev_pm_domain_start(&pdev->dev); - pm_runtime_get_noresume(&pdev->dev); - pm_runtime_set_active(&pdev->dev); - pm_runtime_set_autosuspend_delay(&pdev->dev, 50); - pm_runtime_use_autosuspend(&pdev->dev); - pm_runtime_enable(&pdev->dev); - ret = mmc_add_host(mmc); if (ret) goto remove_host;
The SDHI driver en-/disabled clocks on its own during probe() and remove(). This basically killed all potential RPM power savings. Now, we just enable the clocks for a short time when we access registers in probe(). We otherwise leave all handling to RPM. That means, we need to shift the RPM enabling code in the TMIO core a bit up, so we can access registers there, too. clk_summary before: sd0 1 1 0 12480000 0 0 50000 sdif0 2 2 0 12480000 0 0 50000 clk_summary after: sd0 1 1 0 12480000 0 0 50000 sdif0 1 1 0 12480000 0 0 50000 Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Tested on a Salvator-XS board with R-Car M3-N. drivers/mmc/host/renesas_sdhi_core.c | 7 +++---- drivers/mmc/host/tmio_mmc_core.c | 14 +++++++------- 2 files changed, 10 insertions(+), 11 deletions(-)