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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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