Re: [PATCH 3/3] clk: meson: clk-pll: always enable a critical PLL when setting the rate

[Jerome Brunet]

On Thu 19 Sep 2019 at 11:38, Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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.

Since this clock can change out of CCF maybe it should be marked with
CLK_GET_RATE_NOCACHE ?

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

Stephen, what do you think ?

>  		return 0;
>  
>  	if (meson_clk_pll_enable(hw)) {
> -- 
> 2.22.0