Re: [PATCH v13 08/13] clk: zte: Add zx PLL support infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: zte: unsafe instant PLL rate changes permitted due to missing CLK_SET_RATE_GATE
--

commit 81b8ceca645bb1aa1a8c8fd73b9fdfcf93600c0d
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Add zx PLL support infrastructure

This patch adds code to decode and program PLL settings on ZTE's zx297520v3
SoC, parsing bootloader-programmed values to fill the clock tree with
appropriately clocked parent clocks instead of hardcoded fixed rates.

> diff --git a/drivers/clk/zte/pll-zx.c b/drivers/clk/zte/pll-zx.c
> index fc76c6524a163..d3687495d06ce 100644
> --- a/drivers/clk/zte/pll-zx.c
> +++ b/drivers/clk/zte/pll-zx.c
[ ... ]
> +static int zx29_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			     unsigned long parent_rate)
> +{
> +	struct zx29_clk_pll *pll = to_zx29_clk_pll(hw);
> +	u32 setting;
> +
> +	/*
> +	 * TODO: Implement gradual PLL rate change. PLLs can be changed while they are running and
> +	 * downstream hardware is generally fine with that. The exception is DRAM, which reads
> +	 * incorrect values if changed too fast.
> +	 *
> +	 * Changing the mpll rate is potentially useful for over/underclocking. Gating mpll is
> +	 * unrealistic because too many devices depend on it.
> +	 */
> +	setting = zx29_pll_calc_values(pll, parent_rate, rate, rate, rate);
> +	if (!setting)
> +		return -EINVAL;
> +
> +	dev_dbg(pll->dev, "%s: Setting new configuration: 0x%08x\n", clk_hw_get_name(hw), setting);
> +
> +	return regmap_update_bits(pll->map, pll->reg, 0x00ffffff, setting);

[Severity: Medium]
Will writing the new divider values instantaneously here cause a clock glitch
for large rate changes?

The TODO notes that gradual rate changes are not yet implemented, which could
lead to temporary incorrect clock output or DRAM corruption if a rate change
is requested.

[ ... ]
> +struct clk_hw *zx_clk_register_pll(struct device *dev, struct regmap *regmap,
> +				   const struct zx_pll_desc *desc, struct clk_hw * const *clocks)
> +{
[ ... ]
> +	init.name = desc->name;
> +	init.parent_data = parents;
> +	init.num_parents = desc->num_parents;
> +
> +	/*
> +	 * Disallow live reparent, but allow rate changes. PLLs can handle gradual changes fine, but
> +	 * large ones cause a temporary incorrect output, which can break downstream devices.
> +	 * See zx29_pll_set_rate for considerations on that.
> +	 */
> +	init.flags = CLK_SET_PARENT_GATE;

[Severity: Medium]
Since gradual rate changes are not actually implemented yet, does omitting the
CLK_SET_RATE_GATE flag misinform the clock framework that it is safe to apply
live rate changes to this PLL?

While gating mpll might be unrealistic as noted earlier, leaving out
CLK_SET_RATE_GATE before gradual stepping is implemented might permit unsafe
live jumps if a rate change is ever requested on an active PLL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-zx29clk-v13-0-70d8f673386c@gmail.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy