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

[Ulf Hansson <ulf.hansson@linaro.org>]

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