+ Niklas, Geert, Yamada-san, On Wed, 13 May 2020 at 19:31, Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> > --- > > 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 >
