[1/3] clk: renesas: mstp: Set GENPD_FLAG_ALWAYS_ON for clock domain
diff mbox series

Message ID 20190816125225.16061-2-geert+renesas@glider.be
State Accepted
Delegated to: Geert Uytterhoeven
Headers show
Series
  • clk: renesas: Set GENPD_FLAG_ALWAYS_ON for clock domain
Related show

Commit Message

Geert Uytterhoeven Aug. 16, 2019, 12:52 p.m. UTC
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>
---
 drivers/clk/renesas/clk-mstp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Simon Horman Aug. 16, 2019, 3:38 p.m. UTC | #1
On Fri, Aug 16, 2019 at 02:52:23PM +0200, Geert Uytterhoeven wrote:
> 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>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Stephen Boyd Aug. 16, 2019, 6:01 p.m. UTC | #2
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?
Geert Uytterhoeven Aug. 16, 2019, 7:59 p.m. UTC | #3
Hi Stephen,

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?
Thanks!

Gr{oetje,eeting}s,

                        Geert
Stephen Boyd Aug. 17, 2019, 3:48 a.m. UTC | #4
Quoting Geert Uytterhoeven (2019-08-16 12:59:32)
> Hi Stephen,
> 
> 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?
> Thanks!
> 

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?
Geert Uytterhoeven Aug. 18, 2019, 8:27 a.m. UTC | #5
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
Ulf Hansson Aug. 22, 2019, 2:06 p.m. UTC | #6
[...]

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

Well, wouldn't it be possible that the power is provided through a
master domain, thus not having the ->power_off() callback assigned for
the subdomain is perfectly fine, even without having
GENPD_FLAG_ALWAYS_ON not set.

Kind regards
Uffe
Geert Uytterhoeven Aug. 23, 2019, 8:59 a.m. UTC | #7
Hi Ulf,

On Thu, Aug 22, 2019 at 4:06 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > 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?
>
> Well, wouldn't it be possible that the power is provided through a
> master domain, thus not having the ->power_off() callback assigned for
> the subdomain is perfectly fine, even without having
> GENPD_FLAG_ALWAYS_ON not set.

Thanks, I hadn't considered that the clock domain might be a subdomain
of a power domain.

Gr{oetje,eeting}s,

                        Geert

Patch
diff mbox series

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index dfbef37eed4a82e8..fcf753a6249a0e53 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -335,7 +335,8 @@  void __init cpg_mstp_add_clk_domain(struct device_node *np)
 		return;
 
 	pd->name = np->name;
-	pd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
+	pd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ALWAYS_ON |
+		    GENPD_FLAG_ACTIVE_WAKEUP;
 	pd->attach_dev = cpg_mstp_attach_dev;
 	pd->detach_dev = cpg_mstp_detach_dev;
 	pm_genpd_init(pd, &pm_domain_always_on_gov, false);