Re: [PATCH v11 2/2] clk: imx: pll14xx: support spread spectrum clock generation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] SSC settings fail to apply independently of rate changes, and bootloader settings are not cleared.
--

commit 144aa55ef6f15acf393fd40e2f5e663ff37e0de8
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date:   Tue Sep 1 09:05:18 2026 +0000

    clk: imx: pll14xx: support spread spectrum clock generation
    
    Add support for spread spectrum clock (SSC) generation to the pll14xx
    driver.
    
    Tested on the video PLL of i.MX8MN and i.MX8MP based boards.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b6f1cc9f57005..0c2b812ba20c7 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c

[ ... ]

> @@ -390,6 +453,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
>  			       pll->base + DIV_CTL1);
>  
> +		__clk_pll1443x_set_spread_spectrum(hw, prate, rate.pdiv,
> +						   rate.mdiv);
> +
>  		spin_unlock_irqrestore(&pll->lock, flags);
>  
>  		return 0;

[Severity: High]
Is deferring the hardware application of the SSC settings to the rate change
callback sufficient?

If the requested clock rate matches the current rate, CCF optimizes out the
call to clk_pll1443x_set_rate(). This means the hardware is never updated
with the new SSC settings if the rate remains unchanged.

[ ... ]

> @@ -437,6 +505,16 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	return 0;
>  }
>  
> +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +					    const struct clk_spread_spectrum *ss_conf)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +
> +	memcpy(&pll->ss_conf, ss_conf, sizeof(pll->ss_conf));
> +
> +	return 0;
> +}

[Severity: High]
Does this callback need to program the actual hardware instead of just
caching the settings?

When CCF configures SSC via device tree (assigned-clock-sscs), it calls
clk_pll1443x_set_spread_spectrum(). By only caching the configuration in
pll->ss_conf without writing to hardware, the SSC application becomes
entirely dependent on unrelated rate changes.

[ ... ]

> @@ -509,6 +587,7 @@ static const struct clk_ops clk_pll1443x_ops = {
>  	.recalc_rate	= clk_pll14xx_recalc_rate,
>  	.determine_rate = clk_pll1443x_determine_rate,
>  	.set_rate	= clk_pll1443x_set_rate,
> +	.set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
>  };
>  
>  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,

[Severity: High]
Should imx_dev_clk_hw_pll14xx() explicitly clear the SSCG_CTRL register
during initialization?

Since the driver currently relies on clk_pll1443x_set_rate() to disable any
bootloader SSC setups, it will fail to clear them if no spread method is
configured and the clock rate is unchanged. This can leave an active
bootloader configuration running, leading to incorrect clock behavior and
potential EMI issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901090912.585681-1-dario.binacchi@amarulasolutions.com?part=2