| Message ID | 20190909104649.4960-1-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: tmio: Fixup runtime PM management during probe and remove | expand |
On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > During probe, tmio variant drivers calls pm_runtime_enable() before they > call tmio_mmc_host_probe(). This doesn't work as expected, because > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > To avoid the device from being runtime suspended during the probe phase, > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > Consequentially, each tmio variant driver can decide themselves when to > call pm_runtime_put(), to allow the device to become runtime suspended. > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > during probe, it's is expected that it also calls pm_runtime_get_sync() to > restore the usage count, before it calls tmio_mmc_host_remove(). > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> So I decided to apply this for my fixes branch, as to get it tested for a few days. If you have any objections, please tell. Kind regards Uffe > --- > drivers/mmc/host/renesas_sdhi_core.c | 10 +++------- > drivers/mmc/host/tmio_mmc.c | 6 ++---- > drivers/mmc/host/tmio_mmc_core.c | 8 +++++--- > drivers/mmc/host/uniphier-sd.c | 6 ++---- > 4 files changed, 12 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 4c9774dbcfc1..6846c6b688a2 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -777,8 +777,6 @@ 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; > > - pm_runtime_enable(&pdev->dev); > - > ret = renesas_sdhi_clk_enable(host); > if (ret) > goto efree; > @@ -845,6 +843,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, > goto eirq; > } > > + pm_runtime_put(&pdev->dev); > + > dev_info(&pdev->dev, "%s base at 0x%08lx max clock rate %u MHz\n", > mmc_hostname(host->mmc), (unsigned long) > (platform_get_resource(pdev, IORESOURCE_MEM, 0)->start), > @@ -858,9 +858,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, > renesas_sdhi_clk_disable(host); > efree: > tmio_mmc_host_free(host); > - > - pm_runtime_disable(&pdev->dev); > - > return ret; > } > EXPORT_SYMBOL_GPL(renesas_sdhi_probe); > @@ -869,11 +866,10 @@ int renesas_sdhi_remove(struct platform_device *pdev) > { > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > + pm_runtime_get_sync(&pdev->dev); > tmio_mmc_host_remove(host); > renesas_sdhi_clk_disable(host); > > - pm_runtime_disable(&pdev->dev); > - > return 0; > } > EXPORT_SYMBOL_GPL(renesas_sdhi_remove); > diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c > index 8539e10784b4..219029a1c420 100644 > --- a/drivers/mmc/host/tmio_mmc.c > +++ b/drivers/mmc/host/tmio_mmc.c > @@ -172,7 +172,6 @@ static int tmio_mmc_probe(struct platform_device *pdev) > host->mmc->f_max = pdata->hclk; > host->mmc->f_min = pdata->hclk / 512; > > - pm_runtime_enable(&pdev->dev); > > ret = tmio_mmc_host_probe(host); > if (ret) > @@ -184,6 +183,7 @@ static int tmio_mmc_probe(struct platform_device *pdev) > if (ret) > goto host_remove; > > + pm_runtime_put(&pdev->dev); > pr_info("%s at 0x%08lx irq %d\n", mmc_hostname(host->mmc), > (unsigned long)host->ctl, irq); > > @@ -193,7 +193,6 @@ static int tmio_mmc_probe(struct platform_device *pdev) > tmio_mmc_host_remove(host); > host_free: > tmio_mmc_host_free(host); > - pm_runtime_disable(&pdev->dev); > cell_disable: > if (cell->disable) > cell->disable(pdev); > @@ -206,12 +205,11 @@ static int tmio_mmc_remove(struct platform_device *pdev) > const struct mfd_cell *cell = mfd_get_cell(pdev); > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > + pm_runtime_get_sync(&pdev->dev); > tmio_mmc_host_remove(host); > if (cell->disable) > cell->disable(pdev); > > - pm_runtime_disable(&pdev->dev); > - > return 0; > } > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 2cb3f951c3e2..ad8f3e902daa 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1257,9 +1257,11 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > /* See if we also get DMA */ > tmio_mmc_request_dma(_host, pdata); > > + 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) > @@ -1283,9 +1285,6 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) > if (host->pdata->flags & TMIO_MMC_SDIO_IRQ) > sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000); > > - if (!host->native_hotplug) > - pm_runtime_get_sync(&pdev->dev); > - > dev_pm_qos_hide_latency_limit(&pdev->dev); > > mmc_remove_host(mmc); > @@ -1294,7 +1293,10 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) > tmio_mmc_release_dma(host); > > pm_runtime_dont_use_autosuspend(&pdev->dev); > + if (host->native_hotplug) > + pm_runtime_put_noidle(&pdev->dev); > pm_runtime_put_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > } > EXPORT_SYMBOL_GPL(tmio_mmc_host_remove); > > diff --git a/drivers/mmc/host/uniphier-sd.c b/drivers/mmc/host/uniphier-sd.c > index e09336f9166d..8f7bf138408b 100644 > --- a/drivers/mmc/host/uniphier-sd.c > +++ b/drivers/mmc/host/uniphier-sd.c > @@ -629,7 +629,6 @@ static int uniphier_sd_probe(struct platform_device *pdev) > host->clk_disable = uniphier_sd_clk_disable; > host->set_clock = uniphier_sd_set_clock; > > - pm_runtime_enable(&pdev->dev); > ret = uniphier_sd_clk_enable(host); > if (ret) > goto free_host; > @@ -647,12 +646,11 @@ static int uniphier_sd_probe(struct platform_device *pdev) > if (ret) > goto free_host; > > + pm_runtime_put(&pdev->dev); > return 0; > > free_host: > tmio_mmc_host_free(host); > - pm_runtime_disable(&pdev->dev); > - > return ret; > } > > @@ -660,9 +658,9 @@ static int uniphier_sd_remove(struct platform_device *pdev) > { > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > + pm_runtime_get_sync(&pdev->dev); > tmio_mmc_host_remove(host); > uniphier_sd_clk_disable(host); > - pm_runtime_disable(&pdev->dev); > > return 0; > } > -- > 2.17.1 >
On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > To avoid the device from being runtime suspended during the probe phase, > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > Consequentially, each tmio variant driver can decide themselves when to > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > So I decided to apply this for my fixes branch, as to get it tested > for a few days. > > If you have any objections, please tell. Sadly, I can't test until next week because I am still on the road. Yet, I recall Niklas said at LPC that the patch looks good to him, at least.
On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > To avoid the device from being runtime suspended during the probe phase, > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > Consequentially, each tmio variant driver can decide themselves when to > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > So I decided to apply this for my fixes branch, as to get it tested > > for a few days. > > > > If you have any objections, please tell. > > Sadly, I can't test until next week because I am still on the road. Yet, > I recall Niklas said at LPC that the patch looks good to him, at least. > Yes I think it looks good and was planing to test it. Unfortunately I'm also on the road until the end of next week ;-( -- Regards, Niklas Söderlund
Hi all, On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund <niklas.soderlund@ragnatech.se> wrote: > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > To avoid the device from being runtime suspended during the probe phase, > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > for a few days. > > > > > > If you have any objections, please tell. > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > also on the road until the end of next week ;-( So I decided to give it a try on my boards. Note that apart from eMMC, I do not have any SD cards inserted. The first thing I noticed, on R-Car Gen2/Gen3: sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup of_get_named_gpiod_flags: parsed 'cd-gpios' property of node '/soc/sd@ee100000[0]' - status (0) gpio-812 (cd): gpiod_request: status -16 -PM: ==== always-on/ee100000.sd: start gpio_rcar e6055400.gpio: sense irq = 6, type = 3 sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 195 MHz sh_mobile_sdhi ee140000.sd: adding to PM domain always-on @@ -964,7 +961,6 @@ sh_mobile_sdhi ee140000.sd: using device tree for GPIO lookup of_get_named_gpiod_flags: parsed 'cd-gpios' property of node '/soc/sd@ee140000[0]' - status (0) gpio-820 (cd): gpiod_request: status -16 -PM: ==== always-on/ee140000.sd: start gpio_rcar e6055400.gpio: sense irq = 14, type = 3 sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 max clock rate 97 MHz sh_mobile_sdhi ee160000.sd: adding to PM domain always-on @@ -984,7 +980,6 @@ sh_mobile_sdhi ee160000.sd: using device tree for GPIO lookup of_get_named_gpiod_flags: parsed 'cd-gpios' property of node '/soc/sd@ee160000[0]' - status (0) gpio-828 (cd): gpiod_request: status -16 -PM: ==== always-on/ee160000.sd: start gpio_rcar e6055400.gpio: sense irq = 22, type = 3 sh_mobile_sdhi ee160000.sd: mmc2 base at 0xee160000 max clock rate 97 MHz The "PM: ==== always-on/ee100000.sd: start" are from debug prints in genpd_start_dev(). Likewise, I have a similar print in genpd_stop_dev(). So the device is no longer started? Or does it starts in started state? On SH/R-Mobile, the same happens: sh_mobile_sdhi ee100000.sd: adding to PM domain a3sp PM: genpd_add_device: Add ee100000.sd to a3sp -PM: ==== a3sp/ee100000.sd: start sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 88 MHz sh_mobile_sdhi ee120000.sd: adding to PM domain a3sp PM: genpd_add_device: Add ee120000.sd to a3sp -PM: ==== a3sp/ee120000.sd: start sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 max clock rate 12 MHz But later: +PM: ==== a3sp/ee120000.sd: stop ... +PM: ==== a3sp/ee120000.sd: start So it is stopped without being started first, and restarted later. This fuels the theory that the device starts in started state. Is that expected? On R-Car Gen2/Gen3, and some instances on SH/R-Mobile, clk_summary has changed, compared to before this patch: -clk_summary: sdhi0 3 3 0 97500000 0 0 50000 +clk_summary: sdhi0 1 2 0 97500000 0 0 50000 On other instances on SH/R-Mobile: -clk_summary: sdhi1 3 3 0 12500000 0 0 50000 +clk_summary: sdhi1 2 2 0 12500000 0 0 50000 After s2ram, clk_summary has changed, like: -clk_summary: sdhi0 1 2 0 97500000 0 0 50000 +clk_summary: sdhi0 2 2 0 97500000 0 0 50000 Recently I started tracking regulator_summary changes, too. S2ram also impacts regulator usecounts, but that may be a pre-existing issue, as I didn't compare them before: -regulator_summary: fixed-1.8V 2 1 0 unknown 1800mV 0mA 1800mV 1800mV -regulator_summary: ee140000.sd 1 0mA 1800mV 1950mV -regulator_summary: fixed-3.3V 2 1 0 unknown 3300mV 0mA 3300mV 3300mV -regulator_summary: ee140000.sd 1 0mA 3300mV 3400mV +regulator_summary: fixed-1.8V 1 1 0 unknown 1800mV 0mA 1800mV 1800mV +regulator_summary: ee140000.sd 0 0mA 1800mV 1950mV +regulator_summary: fixed-3.3V 1 1 0 unknown 3300mV 0mA 3300mV 3300mV +regulator_summary: ee140000.sd 0 0mA 3300mV 3400mV Regardless, the eMMC on Salvator-XS is still working. Does this makes sense? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi all, > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > <niklas.soderlund@ragnatech.se> wrote: > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > for a few days. > > > > > > > > If you have any objections, please tell. > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > also on the road until the end of next week ;-( > > So I decided to give it a try on my boards. Note that apart from eMMC, > I do not have any SD cards inserted. Thanks for testing! > > The first thing I noticed, on R-Car Gen2/Gen3: > > sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup > of_get_named_gpiod_flags: parsed 'cd-gpios' property of node > '/soc/sd@ee100000[0]' - status (0) > gpio-812 (cd): gpiod_request: status -16 > -PM: ==== always-on/ee100000.sd: start > gpio_rcar e6055400.gpio: sense irq = 6, type = 3 > sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 195 MHz > sh_mobile_sdhi ee140000.sd: adding to PM domain always-on > @@ -964,7 +961,6 @@ > sh_mobile_sdhi ee140000.sd: using device tree for GPIO lookup > of_get_named_gpiod_flags: parsed 'cd-gpios' property of node > '/soc/sd@ee140000[0]' - status (0) > gpio-820 (cd): gpiod_request: status -16 > -PM: ==== always-on/ee140000.sd: start > gpio_rcar e6055400.gpio: sense irq = 14, type = 3 > sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 max clock rate 97 MHz > sh_mobile_sdhi ee160000.sd: adding to PM domain always-on > @@ -984,7 +980,6 @@ > sh_mobile_sdhi ee160000.sd: using device tree for GPIO lookup > of_get_named_gpiod_flags: parsed 'cd-gpios' property of node > '/soc/sd@ee160000[0]' - status (0) > gpio-828 (cd): gpiod_request: status -16 > -PM: ==== always-on/ee160000.sd: start > gpio_rcar e6055400.gpio: sense irq = 22, type = 3 > sh_mobile_sdhi ee160000.sd: mmc2 base at 0xee160000 max clock rate 97 MHz > > The "PM: ==== always-on/ee100000.sd: start" are from debug prints > in genpd_start_dev(). Likewise, I have a similar print in genpd_stop_dev(). > > So the device is no longer started? Or does it starts in started state? Right. That is the behaviour I expect at this point. In other words, more fixes on top is needed. I assume the above problem is what Niklas tried to fix in commit 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations"). However, that commit introduced even more problems. For example, in case the device was not having genpd attached, there was no problem, but after commit 7ff213193310, there is always a clock imbalance problem. Hmm, maybe we should simply revert Niklas patch and make a proper fix on top instead, as that would mean we can tag that fix for stable more easily. > > On SH/R-Mobile, the same happens: > > sh_mobile_sdhi ee100000.sd: adding to PM domain a3sp > PM: genpd_add_device: Add ee100000.sd to a3sp > -PM: ==== a3sp/ee100000.sd: start > sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 88 MHz > sh_mobile_sdhi ee120000.sd: adding to PM domain a3sp > PM: genpd_add_device: Add ee120000.sd to a3sp > -PM: ==== a3sp/ee120000.sd: start > sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 max clock rate 12 MHz > > But later: > > +PM: ==== a3sp/ee120000.sd: stop > ... > +PM: ==== a3sp/ee120000.sd: start > > So it is stopped without being started first, and restarted later. > This fuels the theory that the device starts in started state. > Is that expected? See above. > > On R-Car Gen2/Gen3, and some instances on SH/R-Mobile, clk_summary > has changed, compared to before this patch: > -clk_summary: sdhi0 3 3 0 > 97500000 0 0 50000 > +clk_summary: sdhi0 1 2 0 > 97500000 0 0 50000 > > On other instances on SH/R-Mobile: > -clk_summary: sdhi1 3 3 0 > 12500000 0 0 50000 > +clk_summary: sdhi1 2 2 0 > 12500000 0 0 50000 > > After s2ram, clk_summary has changed, like: > > -clk_summary: sdhi0 1 2 0 > 97500000 0 0 50000 > +clk_summary: sdhi0 2 2 0 > 97500000 0 0 50000 > > Recently I started tracking regulator_summary changes, too. > S2ram also impacts regulator usecounts, but that may be a pre-existing issue, > as I didn't compare them before: > > -regulator_summary: fixed-1.8V 2 1 0 > unknown 1800mV 0mA 1800mV 1800mV > -regulator_summary: ee140000.sd 1 > 0mA 1800mV 1950mV > -regulator_summary: fixed-3.3V 2 1 0 > unknown 3300mV 0mA 3300mV 3300mV > -regulator_summary: ee140000.sd 1 > 0mA 3300mV 3400mV > +regulator_summary: fixed-1.8V 1 1 0 > unknown 1800mV 0mA 1800mV 1800mV > +regulator_summary: ee140000.sd 0 > 0mA 1800mV 1950mV > +regulator_summary: fixed-3.3V 1 1 0 > unknown 3300mV 0mA 3300mV 3300mV > +regulator_summary: ee140000.sd 0 > 0mA 3300mV 3400mV > > Regardless, the eMMC on Salvator-XS is still working. > > Does this makes sense? Thanks for providing the details. In regards to regulators, I would expect all of them to become disabled when the system suspends, then re-enabled during system resume. Let's have a look at that in the next steps though and fix the probe problems first. I can post a patch or two in an hour or so, have you the possibility to test this today? Kind regards Uffe
Hi Ulf, On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > <niklas.soderlund@ragnatech.se> wrote: > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > for a few days. > > > > > > > > > > If you have any objections, please tell. > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > also on the road until the end of next week ;-( > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > I do not have any SD cards inserted. > > Thanks for testing! [...] > Let's have a look at that in the next steps though and fix the probe > problems first. I can post a patch or two in an hour or so, have you > the possibility to test this today? Probably I can. Else it'll happen on Monday. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, 13 Sep 2019 at 09:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > > <niklas.soderlund@ragnatech.se> wrote: > > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > > for a few days. > > > > > > > > > > > > If you have any objections, please tell. > > > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > > also on the road until the end of next week ;-( > > > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > > I do not have any SD cards inserted. > > > > Thanks for testing! > > [...] > > > Let's have a look at that in the next steps though and fix the probe > > problems first. I can post a patch or two in an hour or so, have you > > the possibility to test this today? > > Probably I can. Else it'll happen on Monday. Currently compile testing, posting three patch in few minutes. If we can't make it today, we will probably don't make it for 5.3. On the other hand, the problem has been there for a while anyway. Kind regards Uffe
Hi Ulf, On Fri, Sep 13, 2019 at 11:32 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Fri, 13 Sep 2019 at 09:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > > > <niklas.soderlund@ragnatech.se> wrote: > > > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > > > for a few days. > > > > > > > > > > > > > > If you have any objections, please tell. > > > > > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > > > also on the road until the end of next week ;-( > > > > > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > > > I do not have any SD cards inserted. > > > > > > Thanks for testing! > > > > [...] > > > > > Let's have a look at that in the next steps though and fix the probe > > > problems first. I can post a patch or two in an hour or so, have you > > > the possibility to test this today? > > > > Probably I can. Else it'll happen on Monday. > > Currently compile testing, posting three patch in few minutes. > > If we can't make it today, we will probably don't make it for 5.3. On > the other hand, the problem has been there for a while anyway. Oh, I thought you were aiming for v5.4... Including it in v5.3 may be a stretch, as Wolfram and Niklas can't test it before the release of v5.3. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, 13 Sep 2019 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Sep 13, 2019 at 11:32 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 13 Sep 2019 at 09:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > > > > <niklas.soderlund@ragnatech.se> wrote: > > > > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > > > > for a few days. > > > > > > > > > > > > > > > > If you have any objections, please tell. > > > > > > > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > > > > also on the road until the end of next week ;-( > > > > > > > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > > > > I do not have any SD cards inserted. > > > > > > > > Thanks for testing! > > > > > > [...] > > > > > > > Let's have a look at that in the next steps though and fix the probe > > > > problems first. I can post a patch or two in an hour or so, have you > > > > the possibility to test this today? > > > > > > Probably I can. Else it'll happen on Monday. > > > > Currently compile testing, posting three patch in few minutes. > > > > If we can't make it today, we will probably don't make it for 5.3. On > > the other hand, the problem has been there for a while anyway. > > Oh, I thought you were aiming for v5.4... > > Including it in v5.3 may be a stretch, as Wolfram and Niklas can't test it > before the release of v5.3. The point is, v5.3 is broken anyway and older kernels as well. Anyway, the patches is out, let's see what you find out. Kind regards Uffe
Hi Ulf, On Fri, Sep 13, 2019 at 11:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Fri, 13 Sep 2019 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Sep 13, 2019 at 11:32 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Fri, 13 Sep 2019 at 09:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > > > > > <niklas.soderlund@ragnatech.se> wrote: > > > > > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > > > > > for a few days. > > > > > > > > > > > > > > > > > > If you have any objections, please tell. > > > > > > > > > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > > > > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > > > > > also on the road until the end of next week ;-( > > > > > > > > > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > > > > > I do not have any SD cards inserted. > > > > > > > > > > Thanks for testing! > > > > > > > > [...] > > > > > > > > > Let's have a look at that in the next steps though and fix the probe > > > > > problems first. I can post a patch or two in an hour or so, have you > > > > > the possibility to test this today? > > > > > > > > Probably I can. Else it'll happen on Monday. > > > > > > Currently compile testing, posting three patch in few minutes. > > > > > > If we can't make it today, we will probably don't make it for 5.3. On > > > the other hand, the problem has been there for a while anyway. > > > > Oh, I thought you were aiming for v5.4... > > > > Including it in v5.3 may be a stretch, as Wolfram and Niklas can't test it > > before the release of v5.3. > > The point is, v5.3 is broken anyway and older kernels as well. > > Anyway, the patches is out, let's see what you find out. Thanks! I've reverted this one, and applied: [PATCH 1/3] Revert "mmc: tmio: move runtime PM enablement to the [PATCH 2/3] mmc: tmio: Fixup runtime PM management during probe [PATCH 3/3] mmc: tmio: Fixup runtime PM management during remove All SDHI devices are properly started again. After boot, clock use counts are now consistently at 2, for all boards, i.e.: -clk_summary: sdhi0 3 3 0 97500000 0 0 50000 +clk_summary: sdhi0 2 2 0 97500000 0 0 50000 After s2ram, clock use counts are the same as before s2ram, so no imbalance detected. Good. S2ram still cause regulator imbalances, but that's not genpd related. -regulator_summary: fixed-1.8V 2 1 0 unknown 1800mV 0mA 1800mV 1800mV -regulator_summary: ee140000.sd 1 0mA 1800mV 1950mV -regulator_summary: fixed-3.3V 2 1 0 unknown 3300mV 0mA 3300mV 3300mV -regulator_summary: ee140000.sd 1 0mA 3300mV 3400mV +regulator_summary: fixed-1.8V 1 1 0 unknown 1800mV 0mA 1800mV 1800mV +regulator_summary: ee140000.sd 0 0mA 1800mV 1950mV +regulator_summary: fixed-3.3V 1 1 0 unknown 3300mV 0mA 3300mV 3300mV +regulator_summary: ee140000.sd 0 0mA 3300mV 3400mV No change in use counts after subsequent s2ram cycles. And eMMC on Salvator-XS is still working. A good state to enter the weekend ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, 13 Sep 2019 at 12:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Sep 13, 2019 at 11:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 13 Sep 2019 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Sep 13, 2019 at 11:32 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 13 Sep 2019 at 09:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Fri, Sep 13, 2019 at 9:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund > > > > > > > <niklas.soderlund@ragnatech.se> wrote: > > > > > > > > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote: > > > > > > > > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote: > > > > > > > > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they > > > > > > > > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because > > > > > > > > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the > > > > > > > > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM. > > > > > > > > > > > > > > > > > > > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. > > > > > > > > > > > To avoid the device from being runtime suspended during the probe phase, > > > > > > > > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe(). > > > > > > > > > > > Consequentially, each tmio variant driver can decide themselves when to > > > > > > > > > > > call pm_runtime_put(), to allow the device to become runtime suspended. > > > > > > > > > > > > > > > > > > > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put() > > > > > > > > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to > > > > > > > > > > > restore the usage count, before it calls tmio_mmc_host_remove(). > > > > > > > > > > > > > > > > > > > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") > > > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > > > > > > > > > So I decided to apply this for my fixes branch, as to get it tested > > > > > > > > > > for a few days. > > > > > > > > > > > > > > > > > > > > If you have any objections, please tell. > > > > > > > > > > > > > > > > > > Sadly, I can't test until next week because I am still on the road. Yet, > > > > > > > > > I recall Niklas said at LPC that the patch looks good to him, at least. > > > > > > > > > > > > > > > > > > > > > > > > > Yes I think it looks good and was planing to test it. Unfortunately I'm > > > > > > > > also on the road until the end of next week ;-( > > > > > > > > > > > > > > So I decided to give it a try on my boards. Note that apart from eMMC, > > > > > > > I do not have any SD cards inserted. > > > > > > > > > > > > Thanks for testing! > > > > > > > > > > [...] > > > > > > > > > > > Let's have a look at that in the next steps though and fix the probe > > > > > > problems first. I can post a patch or two in an hour or so, have you > > > > > > the possibility to test this today? > > > > > > > > > > Probably I can. Else it'll happen on Monday. > > > > > > > > Currently compile testing, posting three patch in few minutes. > > > > > > > > If we can't make it today, we will probably don't make it for 5.3. On > > > > the other hand, the problem has been there for a while anyway. > > > > > > Oh, I thought you were aiming for v5.4... > > > > > > Including it in v5.3 may be a stretch, as Wolfram and Niklas can't test it > > > before the release of v5.3. > > > > The point is, v5.3 is broken anyway and older kernels as well. > > > > Anyway, the patches is out, let's see what you find out. > > Thanks! > > I've reverted this one, and applied: > > [PATCH 1/3] Revert "mmc: tmio: move runtime PM enablement to the > [PATCH 2/3] mmc: tmio: Fixup runtime PM management during probe > [PATCH 3/3] mmc: tmio: Fixup runtime PM management during remove > > All SDHI devices are properly started again. > > After boot, clock use counts are now consistently at 2, for all boards, > i.e.: > > -clk_summary: sdhi0 3 3 0 > 97500000 0 0 50000 > +clk_summary: sdhi0 2 2 0 > 97500000 0 0 50000 > > After s2ram, clock use counts are the same as before s2ram, so no > imbalance detected. Good. Great, thanks for testing! > > S2ram still cause regulator imbalances, but that's not genpd related. > > -regulator_summary: fixed-1.8V 2 1 0 > unknown 1800mV 0mA 1800mV 1800mV > -regulator_summary: ee140000.sd 1 > 0mA 1800mV 1950mV > -regulator_summary: fixed-3.3V 2 1 0 > unknown 3300mV 0mA 3300mV 3300mV > -regulator_summary: ee140000.sd 1 > 0mA 3300mV 3400mV > +regulator_summary: fixed-1.8V 1 1 0 > unknown 1800mV 0mA 1800mV 1800mV > +regulator_summary: ee140000.sd 0 > 0mA 1800mV 1950mV > +regulator_summary: fixed-3.3V 1 1 0 > unknown 3300mV 0mA 3300mV 3300mV > +regulator_summary: ee140000.sd 0 > 0mA 3300mV 3400mV > > No change in use counts after subsequent s2ram cycles. > > And eMMC on Salvator-XS is still working. > > A good state to enter the weekend ;-) Perfect! I am queuing up the three patches for 5.3. Yes, it's a bit risky, but I am quite confident that we have improved things. Perhaps there are additional changes needed, at least we need to look into the regulator imbalances at some point. Thanks and have a nice weekend! Kind regards Uffe
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 4c9774dbcfc1..6846c6b688a2 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -777,8 +777,6 @@ 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; - pm_runtime_enable(&pdev->dev); - ret = renesas_sdhi_clk_enable(host); if (ret) goto efree; @@ -845,6 +843,8 @@ int renesas_sdhi_probe(struct platform_device *pdev, goto eirq; } + pm_runtime_put(&pdev->dev); + dev_info(&pdev->dev, "%s base at 0x%08lx max clock rate %u MHz\n", mmc_hostname(host->mmc), (unsigned long) (platform_get_resource(pdev, IORESOURCE_MEM, 0)->start), @@ -858,9 +858,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, renesas_sdhi_clk_disable(host); efree: tmio_mmc_host_free(host); - - pm_runtime_disable(&pdev->dev); - return ret; } EXPORT_SYMBOL_GPL(renesas_sdhi_probe); @@ -869,11 +866,10 @@ int renesas_sdhi_remove(struct platform_device *pdev) { struct tmio_mmc_host *host = platform_get_drvdata(pdev); + pm_runtime_get_sync(&pdev->dev); tmio_mmc_host_remove(host); renesas_sdhi_clk_disable(host); - pm_runtime_disable(&pdev->dev); - return 0; } EXPORT_SYMBOL_GPL(renesas_sdhi_remove); diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c index 8539e10784b4..219029a1c420 100644 --- a/drivers/mmc/host/tmio_mmc.c +++ b/drivers/mmc/host/tmio_mmc.c @@ -172,7 +172,6 @@ static int tmio_mmc_probe(struct platform_device *pdev) host->mmc->f_max = pdata->hclk; host->mmc->f_min = pdata->hclk / 512; - pm_runtime_enable(&pdev->dev); ret = tmio_mmc_host_probe(host); if (ret) @@ -184,6 +183,7 @@ static int tmio_mmc_probe(struct platform_device *pdev) if (ret) goto host_remove; + pm_runtime_put(&pdev->dev); pr_info("%s at 0x%08lx irq %d\n", mmc_hostname(host->mmc), (unsigned long)host->ctl, irq); @@ -193,7 +193,6 @@ static int tmio_mmc_probe(struct platform_device *pdev) tmio_mmc_host_remove(host); host_free: tmio_mmc_host_free(host); - pm_runtime_disable(&pdev->dev); cell_disable: if (cell->disable) cell->disable(pdev); @@ -206,12 +205,11 @@ static int tmio_mmc_remove(struct platform_device *pdev) const struct mfd_cell *cell = mfd_get_cell(pdev); struct tmio_mmc_host *host = platform_get_drvdata(pdev); + pm_runtime_get_sync(&pdev->dev); tmio_mmc_host_remove(host); if (cell->disable) cell->disable(pdev); - pm_runtime_disable(&pdev->dev); - return 0; } diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 2cb3f951c3e2..ad8f3e902daa 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1257,9 +1257,11 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) /* See if we also get DMA */ tmio_mmc_request_dma(_host, pdata); + 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) @@ -1283,9 +1285,6 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) if (host->pdata->flags & TMIO_MMC_SDIO_IRQ) sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000); - if (!host->native_hotplug) - pm_runtime_get_sync(&pdev->dev); - dev_pm_qos_hide_latency_limit(&pdev->dev); mmc_remove_host(mmc); @@ -1294,7 +1293,10 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) tmio_mmc_release_dma(host); pm_runtime_dont_use_autosuspend(&pdev->dev); + if (host->native_hotplug) + pm_runtime_put_noidle(&pdev->dev); pm_runtime_put_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); } EXPORT_SYMBOL_GPL(tmio_mmc_host_remove); diff --git a/drivers/mmc/host/uniphier-sd.c b/drivers/mmc/host/uniphier-sd.c index e09336f9166d..8f7bf138408b 100644 --- a/drivers/mmc/host/uniphier-sd.c +++ b/drivers/mmc/host/uniphier-sd.c @@ -629,7 +629,6 @@ static int uniphier_sd_probe(struct platform_device *pdev) host->clk_disable = uniphier_sd_clk_disable; host->set_clock = uniphier_sd_set_clock; - pm_runtime_enable(&pdev->dev); ret = uniphier_sd_clk_enable(host); if (ret) goto free_host; @@ -647,12 +646,11 @@ static int uniphier_sd_probe(struct platform_device *pdev) if (ret) goto free_host; + pm_runtime_put(&pdev->dev); return 0; free_host: tmio_mmc_host_free(host); - pm_runtime_disable(&pdev->dev); - return ret; } @@ -660,9 +658,9 @@ static int uniphier_sd_remove(struct platform_device *pdev) { struct tmio_mmc_host *host = platform_get_drvdata(pdev); + pm_runtime_get_sync(&pdev->dev); tmio_mmc_host_remove(host); uniphier_sd_clk_disable(host); - pm_runtime_disable(&pdev->dev); return 0; }
During probe, tmio variant drivers calls pm_runtime_enable() before they call tmio_mmc_host_probe(). This doesn't work as expected, because tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the status to RPM_ACTIVE for the device, when its been enabled for runtime PM. Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead. To avoid the device from being runtime suspended during the probe phase, let's also increase the runtime PM usage count in tmio_mmc_host_probe(). Consequentially, each tmio variant driver can decide themselves when to call pm_runtime_put(), to allow the device to become runtime suspended. Additionally, if the tmio variant driver decided to call pm_runtime_put() during probe, it's is expected that it also calls pm_runtime_get_sync() to restore the usage count, before it calls tmio_mmc_host_remove(). Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations") Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/host/renesas_sdhi_core.c | 10 +++------- drivers/mmc/host/tmio_mmc.c | 6 ++---- drivers/mmc/host/tmio_mmc_core.c | 8 +++++--- drivers/mmc/host/uniphier-sd.c | 6 ++---- 4 files changed, 12 insertions(+), 18 deletions(-) -- 2.17.1