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

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

-- 
2.20.1

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

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

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