Messages in this thread | | | Subject | Re: [PATCH 3/3] clk: meson: clk-pll: always enable a critical PLL when setting the rate | From | Neil Armstrong <> | Date | Fri, 20 Sep 2019 10:06:58 +0200 |
| |
Hi Stephen,
On 19/09/2019 19:06, Stephen Boyd wrote: > Quoting Jerome Brunet (2019-09-19 06:01:28) >> On Thu 19 Sep 2019 at 11:38, Neil Armstrong <narmstrong@baylibre.com> wrote: >> >>> Make sure we always enable a PLL on a set_rate() when the PLL is >>> flagged as critical. >>> >>> This fixes the case when the Amlogic G12A SYS_PLL gets disabled by the >>> PSCI firmware when resuming from suspend-to-memory, in the case >>> where the CPU was not clocked by the SYS_PLL, but by the fixed PLL >>> fixed divisors. >>> In this particular case, when changing the PLL rate, CCF doesn't handle >>> the fact the PLL could have been disabled in the meantime and set_rate() >>> only changes the rate and never enables it again. >>> >>> Fixes: d6e81845b7d9 ("clk: meson: clk-pll: check if the clock is already enabled') >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/clk/meson/clk-pll.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c >>> index ddb1e5634739..8c5adccb7959 100644 >>> --- a/drivers/clk/meson/clk-pll.c >>> +++ b/drivers/clk/meson/clk-pll.c >>> @@ -379,7 +379,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate, >>> } >>> >>> /* If the pll is stopped, bail out now */ >>> - if (!enabled) >>> + if (!(hw->init->flags & CLK_IS_CRITICAL) && !enabled) >> >> This is surely a work around to the issue at hand but: >> >> * Enabling the clock, critical or not, should not be done but the >> set_rate() callback. This is not the purpose of this callback. >> >> * Enabling the clock in such way does not walk the tree. So, if there is >> ever another PSCI Fw which disable we would get into the same issue >> again. IOW, This is not specific to the PLL driver so it should not have >> to deal with this. > > Exactly. > >> >> Since this clock can change out of CCF maybe it should be marked with >> CLK_GET_RATE_NOCACHE ? > > Yes, or figure out a way to make the clk state match what PSCI leaves it > in on resume from suspend. > > >> >> When CCF hits a clock with CLK_GET_RATE_NOCACHE while walking the tree, >> in addition to to calling get_rate(), CCF could also call is_enabled() >> if the clock has CLK_IS_CRITICAL and possibly .enable() ? > > This logic should go under a new flag. The CLK_GET_RATE_NOCACHE flag > specifically means get rate shouldn't be a cached operation. It doesn't > relate to the enable state. I hope that you can implement some sort of > resume hook that synchronizes the state though so that you don't need to > rely on clk_set_rate() or clk_get_rate() to trigger a sync. >
It's exactly the goal of [1] where I resync a clock tree after a resume.
But I don't check the enable state, would you mean that: if core->ops->enable && core->enable_count > 0 && !clk_core_is_enabled(core) core->ops->enable(core->hw)
along the parent/rate resync ?
Isn't that dangerous ?
[1] https://patchwork.kernel.org/patch/11152101/
Neil
| |