| Message ID | 20200515140459.15273-1-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] mmc: tmio: Further fixup runtime PM management at remove | expand |
Hi Ulf, On Fri, May 15, 2020 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > If the tmio device is attached to a genpd (PM domain), that genpd may have > ->start|stop() callback assigned to it. To make sure the device is > accessible during ->probe(), genpd's ->start() callback must be invoked, > which is currently managed by tmio_mmc_host_probe(). This is very likely to > be too late for some cases, as registers may be read and written way before > that. > > To fix this behaviour, let's drop the call to dev_pm_domain_start() from > tmio_mmc_host_probe() - and let the tmio clients manage this instead. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> So this moves the call to dev_pm_domain_start(). No new calls are added. If I get it right, dev_pm_domain_start() just calls into genpd_dev_pm_start() through a function pointer, and starts the device through the system-specific PM Domain handler. On R-Car SoCs, that is pm_clk_resume(), i.e. enabling the module clock through the clock domain. I have two questions there: 1. What if the device is already started? There seems to be no reference counting involved. 2. Who stops the device again? I always thought the PM Domain was powered on (if still off), and the device started, by calling pm_runtime_get_sync(). What am I missing? Thanks! Gr{oetje,eeting}s, Geert
On Mon, 18 May 2020 at 22:22, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > On Fri, May 15, 2020 at 04:04:59PM +0200, Ulf Hansson wrote: > > If the tmio device is attached to a genpd (PM domain), that genpd may have > > ->start|stop() callback assigned to it. To make sure the device is > > accessible during ->probe(), genpd's ->start() callback must be invoked, > > which is currently managed by tmio_mmc_host_probe(). This is very likely to > > be too late for some cases, as registers may be read and written way before > > that. > > > > To fix this behaviour, let's drop the call to dev_pm_domain_start() from > > tmio_mmc_host_probe() - and let the tmio clients manage this instead. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Okay, this seems to work on Gen3. Great, thanks! > > > @@ -909,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > if (ret) > > goto efree; > > > > + dev_pm_domain_start(&pdev->dev); > > + > > Can't we put it before the custom clk_enable()? And then clean up > further like this to have the main clock only controlled via RPM? I understand what you want to achieve, but to allow that to work we need to consider the below things first. 1. If the driver is built with CONFIG_PM unset, then runtime PM doesn't work and hence the clock won't be managed by a PM domain. 2. If there is a platform configuration where a PM domain (genpd) isn't going to be attached, then the clock needs to be managed locally in the driver. > > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -83,16 +83,11 @@ static int renesas_sdhi_clk_enable(struct tmio_mmc_host *host) > { > struct mmc_host *mmc = host->mmc; > struct renesas_sdhi *priv = host_to_priv(host); > - int ret = clk_prepare_enable(priv->clk); > - > - if (ret < 0) > - return ret; > + int ret; > > ret = clk_prepare_enable(priv->clk_cd); > - if (ret < 0) { > - clk_disable_unprepare(priv->clk); > + if (ret < 0) > return ret; > - } > > /* > * The clock driver may not know what maximum frequency > @@ -198,7 +193,6 @@ static void renesas_sdhi_clk_disable(struct tmio_mmc_host *host) > { > struct renesas_sdhi *priv = host_to_priv(host); > > - clk_disable_unprepare(priv->clk); > clk_disable_unprepare(priv->clk_cd); > } > > @@ -906,12 +900,12 @@ int renesas_sdhi_probe(struct platform_device *pdev, > /* All SDHI have SDIO status bits which must be 1 */ > mmc_data->flags |= TMIO_MMC_SDIO_STATUS_SETBITS; > > + dev_pm_domain_start(&pdev->dev); > + > ret = renesas_sdhi_clk_enable(host); > if (ret) > goto efree; > > - dev_pm_domain_start(&pdev->dev); > - > ver = sd_ctrl_read16(host, CTL_VERSION); > /* GEN2_SDR104 is first known SDHI to use 32bit block count */ > if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) > > > Again, this is only tested on Gen3. I will check Gen2 tomorrow. > > > diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c > > index 93e83ad25976..b8f5687e10be 100644 > > --- a/drivers/mmc/host/tmio_mmc.c > > +++ b/drivers/mmc/host/tmio_mmc.c > > @@ -17,6 +17,7 @@ > > #include <linux/mmc/host.h> > > #include <linux/module.h> > > #include <linux/pagemap.h> > > +#include <linux/pm_domain.h> > > #include <linux/scatterlist.h> > > > > #include "tmio_mmc.h" > > @@ -172,6 +173,8 @@ static int tmio_mmc_probe(struct platform_device *pdev) > > host->mmc->f_max = pdata->hclk; > > host->mmc->f_min = pdata->hclk / 512; > > > > + dev_pm_domain_start(&pdev->dev); > > + > > I am quite sure tmio_mmc won't need this, but better safe than sorry. Okay! I observed that tmio_mmc doesn't manage external clocks and don't have ->clk_enable|disable() callbacks, but using runtime PM. That made me think that perhaps the clocks were managed through the PM domain. Kind regards Uffe
Hi Ulf, On Tue, May 19, 2020 at 10:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Mon, 18 May 2020 at 23:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, May 15, 2020 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > If the tmio device is attached to a genpd (PM domain), that genpd may have > > > ->start|stop() callback assigned to it. To make sure the device is > > > accessible during ->probe(), genpd's ->start() callback must be invoked, > > > which is currently managed by tmio_mmc_host_probe(). This is very likely to > > > be too late for some cases, as registers may be read and written way before > > > that. > > > > > > To fix this behaviour, let's drop the call to dev_pm_domain_start() from > > > tmio_mmc_host_probe() - and let the tmio clients manage this instead. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > So this moves the call to dev_pm_domain_start(). No new calls are added. > > If I get it right, dev_pm_domain_start() just calls into > > genpd_dev_pm_start() through a function pointer, and starts the device > > through the system-specific PM Domain handler. On R-Car SoCs, that > > is pm_clk_resume(), i.e. enabling the module clock through the clock > > domain. > > Correct. > > > I have two questions there: > > 1. What if the device is already started? > > There seems to be no reference counting involved. > > The device can't be started as runtime PM hasn't been enabled yet for > the device - and this is controlled by the tmio/renesas driver. OK. > > 2. Who stops the device again? > > Beyond this point there are two main cases to consider. > > 1. The driver is probed successfully and thus up and running. Then, > when the device becomes runtime suspended (because of request > inactivity, for example), the device will be "stopped" through genpd. OK. > 2. The driver failed to probe or the ->remove() callback is invoked > for the device. This will trigger the platform bus to call > dev_pm_domain_detach(). In this path, genpd invokes the > genpd->detach_dev() callback for the device, which allows the genpd > provider to deal with the clean up. In this particular case, I assume > pm_clk_destroy() is going to be called for the device. OK. > > I always thought the PM Domain was powered on (if still off), and the > > device started, by calling pm_runtime_get_sync(). > > Correct. > > However, deploying that kind of pattern in a driver can be a bit > messy, while considering that CONFIG_PM may be set or unset and the > driver should work in both configurations. In principle, it leads to > boilerplate code in drivers, especially if the driver has runtime PM > callbacks assigned to it as shown in commit 1b32999e205b ("mmc: tmio: > Avoid boilerplate code in ->runtime_suspend()"). > > Does it make sense? Now it does, thanks! Note that for devices handled by renesas_sdhi, CONFIG_PM is always set (cfr. drivers/soc/renesas/Kconfig), and there will always by a genpd. So the "messy" part matters for TMIO and UniPhier only. Gr{oetje,eeting}s, Geert
Hi Wolfram, On Tue, May 19, 2020 at 10:46 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > Can't we put it before the custom clk_enable()? And then clean up > > > further like this to have the main clock only controlled via RPM? > > > > I understand what you want to achieve, but to allow that to work we > > need to consider the below things first. > > > > 1. If the driver is built with CONFIG_PM unset, then runtime PM > > doesn't work and hence the clock won't be managed by a PM domain. > > 2. If there is a platform configuration where a PM domain (genpd) > > isn't going to be attached, then the clock needs to be managed locally > > in the driver. > > Similar to what Geert responded, for Renesas SDHI both is always true > AFAIU. Geert? For Renesas SDHI, both are always false ;-) I.e. CONFIG_PM is always set, genpd is always attached. Gr{oetje,eeting}s, Geert
On Tue, 19 May 2020 at 10:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Wolfram, > > On Tue, May 19, 2020 at 10:46 AM Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > > > Can't we put it before the custom clk_enable()? And then clean up > > > > further like this to have the main clock only controlled via RPM? > > > > > > I understand what you want to achieve, but to allow that to work we > > > need to consider the below things first. > > > > > > 1. If the driver is built with CONFIG_PM unset, then runtime PM > > > doesn't work and hence the clock won't be managed by a PM domain. > > > 2. If there is a platform configuration where a PM domain (genpd) > > > isn't going to be attached, then the clock needs to be managed locally > > > in the driver. > > > > Similar to what Geert responded, for Renesas SDHI both is always true > > AFAIU. Geert? > > For Renesas SDHI, both are always false ;-) > I.e. CONFIG_PM is always set, genpd is always attached. OK, thanks for clarifying. This means dev_pm_domain_start() is needed only for the SDHI renesas variants. But on the other hand, it doesn't hurt for the others (the uniphier-sd variant doesn't even use runtime PM as confirmed by Yamada-san). I don't have a strong opinion, but it looks like we can either apply $subject patch as is, or modify it to make dev_pm_domain_start() be called only for the SDHI renesas variants. What do you prefer? Kind regards Uffe
> And no matter what solution, can always drop to manage the "main" > clock from renesas_sdhi_clk_enable|disable() as it's managed by the PM > domain. Good. That was what I was aiming for. > > Although, then we need to call dev_pm_domain_start() prior > renesas_sdhi_clk_enable() during ->probe()? Yep, I moved it upwards in my proof-of-concept diff. But it would be better if you would apply this change in your patch already.
> This means dev_pm_domain_start() is needed only for the SDHI renesas > variants. But on the other hand, it doesn't hurt for the others (the > uniphier-sd variant doesn't even use runtime PM as confirmed by > Yamada-san). > > I don't have a strong opinion, but it looks like we can either apply > $subject patch as is, or modify it to make dev_pm_domain_start() be > called only for the SDHI renesas variants. > > What do you prefer? Dealing with PM is confusing often enough. To keep it simple, I'd suggest to avoid unneeded calls. It may take a while later to rediscover if this call is essential or not. So, for the uniphier case, we know it is not needed. If we agree on a best effort basis that it is also not needed for tmio_mmc, then I think we should keep it only for Renesas SDHI.
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index ff72b381a6b3..5d44240d57ec 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -24,6 +24,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/mmc/host.h> #include <linux/mmc/slot-gpio.h> #include <linux/mfd/tmio.h> @@ -909,6 +910,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, if (ret) goto efree; + dev_pm_domain_start(&pdev->dev); + ver = sd_ctrl_read16(host, CTL_VERSION); /* GEN2_SDR104 is first known SDHI to use 32bit block count */ if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c index 93e83ad25976..b8f5687e10be 100644 --- a/drivers/mmc/host/tmio_mmc.c +++ b/drivers/mmc/host/tmio_mmc.c @@ -17,6 +17,7 @@ #include <linux/mmc/host.h> #include <linux/module.h> #include <linux/pagemap.h> +#include <linux/pm_domain.h> #include <linux/scatterlist.h> #include "tmio_mmc.h" @@ -172,6 +173,8 @@ static int tmio_mmc_probe(struct platform_device *pdev) host->mmc->f_max = pdata->hclk; host->mmc->f_min = pdata->hclk / 512; + dev_pm_domain_start(&pdev->dev); + ret = tmio_mmc_host_probe(host); if (ret) goto host_free; diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index ba301fb7656b..d7fde57c78c1 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -39,7 +39,6 @@ #include <linux/module.h> #include <linux/pagemap.h> #include <linux/platform_device.h> -#include <linux/pm_domain.h> #include <linux/pm_qos.h> #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> @@ -1192,7 +1191,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); diff --git a/drivers/mmc/host/uniphier-sd.c b/drivers/mmc/host/uniphier-sd.c index f82baf99fd69..0c41dc1cc96c 100644 --- a/drivers/mmc/host/uniphier-sd.c +++ b/drivers/mmc/host/uniphier-sd.c @@ -15,6 +15,7 @@ #include <linux/of_device.h> #include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/reset.h> #include "tmio_mmc.h" @@ -624,6 +625,8 @@ static int uniphier_sd_probe(struct platform_device *pdev) if (ret) goto free_host; + dev_pm_domain_start(dev); + uniphier_sd_host_init(host); tmio_data->ocr_mask = MMC_VDD_32_33 | MMC_VDD_33_34;
If the tmio device is attached to a genpd (PM domain), that genpd may have ->start|stop() callback assigned to it. To make sure the device is accessible during ->probe(), genpd's ->start() callback must be invoked, which is currently managed by tmio_mmc_host_probe(). This is very likely to be too late for some cases, as registers may be read and written way before that. To fix this behaviour, let's drop the call to dev_pm_domain_start() from tmio_mmc_host_probe() - and let the tmio clients manage this instead. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/host/renesas_sdhi_core.c | 3 +++ drivers/mmc/host/tmio_mmc.c | 3 +++ drivers/mmc/host/tmio_mmc_core.c | 2 -- drivers/mmc/host/uniphier-sd.c | 3 +++ 4 files changed, 9 insertions(+), 2 deletions(-) -- 2.20.1