| Message ID | 20190816125225.16061-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | clk: renesas: Set GENPD_FLAG_ALWAYS_ON for clock domain | expand |
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>
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?
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
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?
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
[...] > > > > 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
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
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);
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(-)