[PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

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(). However, it's likely
that may be too late for some cases, as registers may be read and written
way before that point.

To fix the behaviour, let's move the call to dev_pm_domain_start() from
tmio_mmc_host_probe() into those clients that needs it. From discussions at
linux-mmc mailing list, it turned out that it should be sufficient to do
this for the SDHI renesas variants, hence the call is move to
renesas_sdhi_probe().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/renesas_sdhi_core.c | 3 +++
 drivers/mmc/host/tmio_mmc_core.c     | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index ff72b381a6b3..dcba9ad35dd1 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>
@@ -905,6 +906,8 @@ 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;
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);
-- 
2.20.1

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

On Tue, May 19, 2020 at 05:24:45PM +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(). However, it's likely
> that may be too late for some cases, as registers may be read and written
> way before that point.
> 
> To fix the behaviour, let's move the call to dev_pm_domain_start() from
> tmio_mmc_host_probe() into those clients that needs it. From discussions at
> linux-mmc mailing list, it turned out that it should be sufficient to do
> this for the SDHI renesas variants, hence the call is move to
> renesas_sdhi_probe().
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Rebind works on my Salvator-XS board (M3-N) and my Lager board (H2). No
other issues found when booting and accessing SD cards or eMMC.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks, Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Tue, 19 May 2020 at 18:38, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Tue, May 19, 2020 at 05:24:45PM +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(). However, it's likely
> > that may be too late for some cases, as registers may be read and written
> > way before that point.
> >
> > To fix the behaviour, let's move the call to dev_pm_domain_start() from
> > tmio_mmc_host_probe() into those clients that needs it. From discussions at
> > linux-mmc mailing list, it turned out that it should be sufficient to do
> > this for the SDHI renesas variants, hence the call is move to
> > renesas_sdhi_probe().
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Rebind works on my Salvator-XS board (M3-N) and my Lager board (H2). No
> other issues found when booting and accessing SD cards or eMMC.
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Thanks, Ulf!
>

Applied for next!

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Geert Uytterhoeven]

Hi Ulf,

On Tue, May 19, 2020 at 5:24 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(). However, it's likely
> that may be too late for some cases, as registers may be read and written
> way before that point.
>
> To fix the behaviour, let's move the call to dev_pm_domain_start() from
> tmio_mmc_host_probe() into those clients that needs it. From discussions at
> linux-mmc mailing list, it turned out that it should be sufficient to do
> this for the SDHI renesas variants, hence the call is move to
> renesas_sdhi_probe().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 3 +++
>  drivers/mmc/host/tmio_mmc_core.c     | 2 --
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index ff72b381a6b3..dcba9ad35dd1 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>
> @@ -905,6 +906,8 @@ 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);
> +


I have debug prints at the top of genpd_stop_dev():

    pr_info("==== %s/%s: stop\n", genpd->name, dev_name(dev));

and genpd_start_dev():

    pr_info("==== %s/%s: start\n", genpd->name, dev_name(dev));

On Koelsch (R-Car M2-W), the three SDHI devices are started twice:

    PM: ==== always-on/ee100000.sd: start
    PM: ==== always-on/ee140000.sd: start
    PM: ==== always-on/ee160000.sd: start
    PM: ==== always-on/ee100000.sd: start
    PM: ==== always-on/ee140000.sd: start
    PM: ==== always-on/ee160000.sd: start

The first time, the probe is deferred, because the regulator needed in
tmio_mmc_init_ocr() hasn't been instantiated yet. The SDHI device is
detached from the PM domain, but not stopped.
Interestingly, I see no clock imbalances afterwards.

On R-Car Gen3, R-Mobile A1, and RZ/A systems, they are started once,
as expected.

On R-Mobile APE6 and SH-Mobile AG5, one device is stopped and started
again:

    PM: ==== a3sp/ee100000.sd: start
    PM: ==== a3sp/ee120000.sd: start
    PM: ==== a3sp/ee120000.sd: stop
    PM: ==== a3sp/ee120000.sd: start

But here no probe deferral is involved.
Just Runtime PM kicking in, I guess.

>         ret = renesas_sdhi_clk_enable(host);
>         if (ret)
>                 goto efree;
> 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);

Before, the issue on probe deferral didn't happen, as the device was only
started after the regulator was found.

>         pm_runtime_get_noresume(&pdev->dev);
>         pm_runtime_set_active(&pdev->dev);
>         pm_runtime_set_autosuspend_delay(&pdev->dev, 50);

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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Wed, 20 May 2020 at 17:57, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Tue, May 19, 2020 at 5:24 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(). However, it's likely
> > that may be too late for some cases, as registers may be read and written
> > way before that point.
> >
> > To fix the behaviour, let's move the call to dev_pm_domain_start() from
> > tmio_mmc_host_probe() into those clients that needs it. From discussions at
> > linux-mmc mailing list, it turned out that it should be sufficient to do
> > this for the SDHI renesas variants, hence the call is move to
> > renesas_sdhi_probe().
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/host/renesas_sdhi_core.c | 3 +++
> >  drivers/mmc/host/tmio_mmc_core.c     | 2 --
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > index ff72b381a6b3..dcba9ad35dd1 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>
> > @@ -905,6 +906,8 @@ 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);
> > +
>
>
> I have debug prints at the top of genpd_stop_dev():
>
>     pr_info("==== %s/%s: stop\n", genpd->name, dev_name(dev));
>
> and genpd_start_dev():
>
>     pr_info("==== %s/%s: start\n", genpd->name, dev_name(dev));
>
> On Koelsch (R-Car M2-W), the three SDHI devices are started twice:
>
>     PM: ==== always-on/ee100000.sd: start
>     PM: ==== always-on/ee140000.sd: start
>     PM: ==== always-on/ee160000.sd: start
>     PM: ==== always-on/ee100000.sd: start
>     PM: ==== always-on/ee140000.sd: start
>     PM: ==== always-on/ee160000.sd: start
>
> The first time, the probe is deferred, because the regulator needed in
> tmio_mmc_init_ocr() hasn't been instantiated yet. The SDHI device is
> detached from the PM domain, but not stopped.
> Interestingly, I see no clock imbalances afterwards.

That's because genpd's->detach_dev() callback is invoked at the
"deferred probe" case. In your case this ends up calling
pm_clk_destroy(). Thus the clock gets disabled and unprepared
correctly.

>
> On R-Car Gen3, R-Mobile A1, and RZ/A systems, they are started once,
> as expected.
>
> On R-Mobile APE6 and SH-Mobile AG5, one device is stopped and started
> again:
>
>     PM: ==== a3sp/ee100000.sd: start
>     PM: ==== a3sp/ee120000.sd: start
>     PM: ==== a3sp/ee120000.sd: stop
>     PM: ==== a3sp/ee120000.sd: start
>
> But here no probe deferral is involved.
> Just Runtime PM kicking in, I guess.

Yep, and that's okay, right?

>
> >         ret = renesas_sdhi_clk_enable(host);
> >         if (ret)
> >                 goto efree;
> > 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);
>
> Before, the issue on probe deferral didn't happen, as the device was only
> started after the regulator was found.

I am not sure there is an issue or did I miss something?

>
> >         pm_runtime_get_noresume(&pdev->dev);
> >         pm_runtime_set_active(&pdev->dev);
> >         pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>
> Gr{oetje,eeting}s,
>
>                         Geert

Thanks a lot for testing and sharing results! Very much appreciated!

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Geert Uytterhoeven]

Hi Ulf,

On Wed, May 20, 2020 at 6:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Wed, 20 May 2020 at 17:57, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, May 19, 2020 at 5:24 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(). However, it's likely
> > > that may be too late for some cases, as registers may be read and written
> > > way before that point.
> > >
> > > To fix the behaviour, let's move the call to dev_pm_domain_start() from
> > > tmio_mmc_host_probe() into those clients that needs it. From discussions at
> > > linux-mmc mailing list, it turned out that it should be sufficient to do
> > > this for the SDHI renesas variants, hence the call is move to
> > > renesas_sdhi_probe().
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/host/renesas_sdhi_core.c | 3 +++
> > >  drivers/mmc/host/tmio_mmc_core.c     | 2 --
> > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > > index ff72b381a6b3..dcba9ad35dd1 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>
> > > @@ -905,6 +906,8 @@ 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);
> > > +
> >
> >
> > I have debug prints at the top of genpd_stop_dev():
> >
> >     pr_info("==== %s/%s: stop\n", genpd->name, dev_name(dev));
> >
> > and genpd_start_dev():
> >
> >     pr_info("==== %s/%s: start\n", genpd->name, dev_name(dev));
> >
> > On Koelsch (R-Car M2-W), the three SDHI devices are started twice:
> >
> >     PM: ==== always-on/ee100000.sd: start
> >     PM: ==== always-on/ee140000.sd: start
> >     PM: ==== always-on/ee160000.sd: start
> >     PM: ==== always-on/ee100000.sd: start
> >     PM: ==== always-on/ee140000.sd: start
> >     PM: ==== always-on/ee160000.sd: start
> >
> > The first time, the probe is deferred, because the regulator needed in
> > tmio_mmc_init_ocr() hasn't been instantiated yet. The SDHI device is
> > detached from the PM domain, but not stopped.
> > Interestingly, I see no clock imbalances afterwards.
>
> That's because genpd's->detach_dev() callback is invoked at the
> "deferred probe" case. In your case this ends up calling
> pm_clk_destroy(). Thus the clock gets disabled and unprepared
> correctly.

Indeed, I had just arrived at the same conclusion.
While genpd doesn't have reference counting on start/stop, pm_clk does
have pce_status to track state, so when needed, __pm_clk_remove()
disables the clock before its destruction.

> > On R-Car Gen3, R-Mobile A1, and RZ/A systems, they are started once,
> > as expected.
> >
> > On R-Mobile APE6 and SH-Mobile AG5, one device is stopped and started
> > again:
> >
> >     PM: ==== a3sp/ee100000.sd: start
> >     PM: ==== a3sp/ee120000.sd: start
> >     PM: ==== a3sp/ee120000.sd: stop
> >     PM: ==== a3sp/ee120000.sd: start
> >
> > But here no probe deferral is involved.
> > Just Runtime PM kicking in, I guess.
>
> Yep, and that's okay, right?

Correct.

> > > @@ -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);
> >
> > Before, the issue on probe deferral didn't happen, as the device was only
> > started after the regulator was found.
>
> I am not sure there is an issue or did I miss something?

No, it's just a bit strange to see an imbalance in start/stop.

Note that this does mean that all PM domain providers that do not rely
on pm_clk, but have their own start/stop methods, need to be aware of
this quirk, and should take care of reference counting themselves.
Fortunately there seems to be only one:
drivers/soc/ti/ti_sci_pm_domains.c.
Unfortunately it doesn't do reference counting, so if that PM domain
driver is ever used with a driver that calls dev_pm_domain_start(),
mysterious things may happen...

> Thanks a lot for testing and sharing results! Very much appreciated!

So I guess I can provide my
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
for this one, too, finally.

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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Wed, 20 May 2020 at 19:42, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Wed, May 20, 2020 at 6:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Wed, 20 May 2020 at 17:57, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, May 19, 2020 at 5:24 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(). However, it's likely
> > > > that may be too late for some cases, as registers may be read and written
> > > > way before that point.
> > > >
> > > > To fix the behaviour, let's move the call to dev_pm_domain_start() from
> > > > tmio_mmc_host_probe() into those clients that needs it. From discussions at
> > > > linux-mmc mailing list, it turned out that it should be sufficient to do
> > > > this for the SDHI renesas variants, hence the call is move to
> > > > renesas_sdhi_probe().
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/mmc/host/renesas_sdhi_core.c | 3 +++
> > > >  drivers/mmc/host/tmio_mmc_core.c     | 2 --
> > > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > > > index ff72b381a6b3..dcba9ad35dd1 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>
> > > > @@ -905,6 +906,8 @@ 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);
> > > > +
> > >
> > >
> > > I have debug prints at the top of genpd_stop_dev():
> > >
> > >     pr_info("==== %s/%s: stop\n", genpd->name, dev_name(dev));
> > >
> > > and genpd_start_dev():
> > >
> > >     pr_info("==== %s/%s: start\n", genpd->name, dev_name(dev));
> > >
> > > On Koelsch (R-Car M2-W), the three SDHI devices are started twice:
> > >
> > >     PM: ==== always-on/ee100000.sd: start
> > >     PM: ==== always-on/ee140000.sd: start
> > >     PM: ==== always-on/ee160000.sd: start
> > >     PM: ==== always-on/ee100000.sd: start
> > >     PM: ==== always-on/ee140000.sd: start
> > >     PM: ==== always-on/ee160000.sd: start
> > >
> > > The first time, the probe is deferred, because the regulator needed in
> > > tmio_mmc_init_ocr() hasn't been instantiated yet. The SDHI device is
> > > detached from the PM domain, but not stopped.
> > > Interestingly, I see no clock imbalances afterwards.
> >
> > That's because genpd's->detach_dev() callback is invoked at the
> > "deferred probe" case. In your case this ends up calling
> > pm_clk_destroy(). Thus the clock gets disabled and unprepared
> > correctly.
>
> Indeed, I had just arrived at the same conclusion.
> While genpd doesn't have reference counting on start/stop, pm_clk does
> have pce_status to track state, so when needed, __pm_clk_remove()
> disables the clock before its destruction.
>
> > > On R-Car Gen3, R-Mobile A1, and RZ/A systems, they are started once,
> > > as expected.
> > >
> > > On R-Mobile APE6 and SH-Mobile AG5, one device is stopped and started
> > > again:
> > >
> > >     PM: ==== a3sp/ee100000.sd: start
> > >     PM: ==== a3sp/ee120000.sd: start
> > >     PM: ==== a3sp/ee120000.sd: stop
> > >     PM: ==== a3sp/ee120000.sd: start
> > >
> > > But here no probe deferral is involved.
> > > Just Runtime PM kicking in, I guess.
> >
> > Yep, and that's okay, right?
>
> Correct.
>
> > > > @@ -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);
> > >
> > > Before, the issue on probe deferral didn't happen, as the device was only
> > > started after the regulator was found.
> >
> > I am not sure there is an issue or did I miss something?
>
> No, it's just a bit strange to see an imbalance in start/stop.
>
> Note that this does mean that all PM domain providers that do not rely
> on pm_clk, but have their own start/stop methods, need to be aware of
> this quirk, and should take care of reference counting themselves.
> Fortunately there seems to be only one:
> drivers/soc/ti/ti_sci_pm_domains.c.
> Unfortunately it doesn't do reference counting, so if that PM domain
> driver is ever used with a driver that calls dev_pm_domain_start(),
> mysterious things may happen...

Good point. Perhaps we should document this somewhere.

>
> > Thanks a lot for testing and sharing results! Very much appreciated!
>
> So I guess I can provide my
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> for this one, too, finally.

Great, thanks! I have added your tag for the other renesas sdhi patch as well.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]


> > Note that this does mean that all PM domain providers that do not rely
> > on pm_clk, but have their own start/stop methods, need to be aware of
> > this quirk, and should take care of reference counting themselves.
> > Fortunately there seems to be only one:
> > drivers/soc/ti/ti_sci_pm_domains.c.
> > Unfortunately it doesn't do reference counting, so if that PM domain
> > driver is ever used with a driver that calls dev_pm_domain_start(),
> > mysterious things may happen...
> 
> Good point. Perhaps we should document this somewhere.

I haven't understood all of the details, but Geert's description sounds
like we definately should document this. Anyone up for it? Otherwise
I'll dig more into it...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

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(-)

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;
-- 
2.20.1

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

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.

> @@ -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?

--- 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.

Thanks, Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]


> > 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On Tue, May 19, 2020 at 10:53:28AM +0200, Geert Uytterhoeven 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.

I hope it was obvious that this is what I meant :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Tue, 19 May 2020 at 11:15, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> 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?

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.

Although, then we need to call dev_pm_domain_start() prior
renesas_sdhi_clk_enable() during ->probe()?

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Tue, 19 May 2020 at 13:35, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > 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.

Alright, I re-spin the series taking into account what has been said.

Then I wait for your tested/reviewed-by tags before applying.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Ulf Hansson]

On Mon, 18 May 2020 at 23:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> 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.

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.

>   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.

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.

>
> 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?

[...]

Kind regards
Uffe

Re: [PATCH 2/2] mmc: tmio: Make sure the PM domain is 'started' while probing

[Geert Uytterhoeven]

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

-- 
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