Messages in this thread | | | From | Stephen Boyd <> | Subject | Re: [PATCH 3/3] clk: meson: clk-pll: always enable a critical PLL when setting the rate | Date | Thu, 19 Sep 2019 10:06:09 -0700 |
| |
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.
| |