Re: [PATCH 1/3] clk: renesas: mstp: Set GENPD_FLAG_ALWAYS_ON for clock domain

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

Hi Stephen,

On Sat, Aug 17, 2019 at 5:48 AM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Geert Uytterhoeven (2019-08-16 12:59:32)
> > On Fri, Aug 16, 2019 at 8:01 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > Quoting Geert Uytterhoeven (2019-08-16 05:52:23)
> > > > The CPG/MSTP Clock Domain driver does not implement the
> > > > generic_pm_domain.power_{on,off}() callbacks, as the domain itself
> > > > cannot be powered down.  Hence the domain should be marked as always-on
> > > > by setting the GENPD_FLAG_ALWAYS_ON flag.
> > > >
> > > > This gets rid of the following boot warning on RZ/A1:
> > > >
> > > >     sh_mtu2 fcff0000.timer: PM domain cpg_clocks will not be powered off
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > >
> > > Are you going to add a Fixes tag?
> >
> > I didn't add a Fixes tag, as there's no clear point in history where the
> > problem appeared: the Clock Domain code in this driver predates the
> > introduction of the GENPD_FLAG_ALWAYS_ON flag by ca. 18 months.
> >
> > Candidates are:
> > d716f4798ff8c65a ("PM / Domains: Support IRQ safe PM domains")
> > ffaa42e8a40b7f10 ("PM / Domains: Enable users of genpd to specify
> > always on PM domains")
> > 075c37d59ecd4a8b ("PM / Domains: Don't warn about IRQ safe device for
> > an always on PM domain")
> >
> > Do you think it's worth adding one or more of the above?
>
> Well is it actually a problem to not specify the flag? I guess it's just
> a potential problem if the genpd is ever powered off, but given that the
> governor decides to leave it always enabled it doesn't actually matter?
> So it's not really fixing anything besides silencing a harmless warning?

The warning is indeed harmless.

The "interesting" case is the case where no warning is printed, as no
IRQ-safe device is present.  In that case, the absence of the
GENPD_FLAG_ALWAYS_ON flag means that the core PM Domain code will
consider the domain for power-off, and will loop over all devices part
of it, which is suboptimal.  Setting the flag avoids that.

Thanks for your continued questions, it made me realize I need to add more
meat to the description to these "simple" patches!

For the PM people: would it make sense to add a
WARN(!genpd->power_off && !genpd_is_always_on(genpd), "...") check to
pm_genpd_init()?
Or set GENPD_FLAG_ALWAYS_ON automatically if !genpd->power_off?

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