Re: [PATCH 1/2] clk: spacemit: re-enable PLLs after init

[Troy Mitchell]

> I don't think this patch makes sense.
>
> If this PLL is essential for some components of the systems to work,
> there's no chance to re-program it safely after the kernel gets started,
> since all the PLLs in K1 and K3 are specified as CLK_SET_RATE_GATE.
>
> If the essential clocks could be re-parented glitchlessly, registering
> a notifier and re-parenting it before the PLL disables might be a
> solution. In this case, we need to delay the operation in
> ccu_pll*_init() until clock registration fully completes.
>
> Otherwise, re-enabling the PLL seems totally extra here, they could be
> enabled on the fly when it's going to be used.

You are right. In both hunks, ccu_pll*_disable() has already gated the
PLL before the newly added ccu_pll*_enable() can run:

> @@ -148,7 +148,7 @@ static int ccu_pll_init(struct clk_hw *hw)
> 	ccu_pll_disable(hw);
> 	ccu_pll_update_param(pll, &pll->config.rate_tbl[0]);
>
> -	return 0;
> +	return ccu_pll_enable(hw);
>  }

> [...]
>
> @@ -252,7 +252,7 @@ static int ccu_plla_init(struct clk_hw *hw)
> 	ccu_plla_disable(hw);
> 	ccu_plla_update_param(pll, &pll->config.rate_tbl[0]);
>
> -	return 0;
> +	return ccu_plla_enable(hw);
>  }

Re-enabling the PLL afterwards therefore does not make reprogramming
safe for a live consumer. I will drop this patch, "[PATCH 1/2]
clk: spacemit: re-enable PLLs after init", from v2.

To avoid delaying the 1.8 GHz rate-table update while this broader issue
is investigated, I will resend the current "[PATCH 2/2] clk: spacemit:
k3: add 1.8 GHz PLL5 and PLL8 rates" as a standalone patch. It only adds
the firmware-selected 1.8 GHz settings to the rate tables.

The existing problem of reprogramming an unmatched PLL from
ccu_pll*_init() needs a broader solution and is independent of the rate
table update. I will address this issue in a separate patch series.

                                            - Troy

Attachment: signature.asc
Description: PGP signature