Re: [PATCH 1/2] mmc: tmio: leave clock handling to PM if enabled

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

Hi Ulf,

On Thu, Jul 25, 2019 at 3:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Tue, 16 Jul 2019 at 17:01, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> > This fixes a clock imbalance that occurs because the SD clock is handled
> > by both PM and the hardware driver.
> > See https://www.spinics.net/lists/linux-mmc/msg44431.html for details.
>
> This is a generic problem, when a device are being attached to a genpd
> and when the genpd has got the ->stop|start() callbacks assigned, as
> to manage device clocks.
>
> Can you try to describe this problem a little bit more in detail, as I
> think that's important to carry in the change log.
>
> >
> > This patch removes the clock handling from the driver's PM callbacks and
>
> runtime PM callbacks and/or system PM callbacks?
>
> > turns the clock off after probing.
> >
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > ---
> >  drivers/mmc/host/tmio_mmc_core.c | 24 ++++++------------------
> >  1 file changed, 6 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> > index 31ffcc3..26dcbba 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -1260,9 +1260,14 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
> >         /* See if we also get DMA */
> >         tmio_mmc_request_dma(_host, pdata);
> >
> > -       pm_runtime_set_active(&pdev->dev);
> > +#ifdef CONFIG_PM
> > +       /* PM handles the clock; disable it so it won't be enabled twice. */
> > +       if (_host->clk_disable)
> > +               _host->clk_disable(_host);
>
> The clock managed here, is that the same clock as being managed by
> genpd's ->stop|start() callbacks?
>
> > +       pm_runtime_get_sync(&pdev->dev);
> >         pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >         pm_runtime_use_autosuspend(&pdev->dev);
> > +#endif
>
> So what happens if you have CONFIG_PM set, but the device doesn't have
> a genpd attached?

That's OK for all SoCs served by renesas_sdhi_internal_dmac.c and
renesas_sdhi_sys_dmac.c, as they all have their clock domain described
in DT...

> I am guessing the driver should handle the clock in such scenario, right?

... but it's not for (non-Renesas) systems served by tmio_mmc.c.

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