Re: [PATCH 3/3] clk: ti: dpll: apply the spread spectrum configuration right away

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: ti: dpll: broken bypass check in omap3_noncore_dpll_set_spread_spectrum()
--

commit 64256b22bd32636a50ecb98dab75abb69bbaf964
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>

clk: ti: dpll: apply the spread spectrum configuration right away

This commit updates omap3_noncore_dpll_set_spread_spectrum() to reprogram
the DPLL at its current rate immediately when configuring spread spectrum
clocking, unless the DPLL is in bypass mode or the configuration is
unchanged.

> diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
> index 634fe0a1f9fda..c181bba2792db 100644
> --- a/drivers/clk/ti/dpll3xxx.c
> +++ b/drivers/clk/ti/dpll3xxx.c
> @@ -690,17 +690,26 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,

[ ... ]

> @@ -718,9 +727,37 @@ int omap3_noncore_dpll_set_spread_spectrum(struct clk_hw *hw,
>  		return -EINVAL;
>  	}
>  
> +	if (!memcmp(ss_conf, &dd->ssc_conf, sizeof(*ss_conf)))
> +		return 0;
> +
> +	old_conf = dd->ssc_conf;
>  	dd->ssc_conf = *ss_conf;
>  
> -	return 0;
> +	/* In bypass: SSC is programmed along with the next rate change */
> +	if (clk_hw_get_parent(hw) != dd->clk_ref)
> +		return 0;
> +
> +	/* Disabling SSC does not need to reprogram the DPLL */
> +	if (ss_conf->method == CLK_SPREAD_NO) {
> +		omap3_noncore_dpll_ssc_program(clk);
> +		return 0;
> +	}
> +
> +	rate = clk_hw_get_rate(hw);
> +	clk_hw_init_rate_request(hw, &req, rate);
> +	ret = omap2_dpll_determine_rate(hw, &req);
> +	if (!ret && req.rate != rate)
> +		ret = -EINVAL;
> +	if (!ret)
> +		ret = omap3_noncore_dpll_set_rate(hw, rate,
> +						  clk_hw_get_rate(dd->clk_ref));
> +	if (ret) {
> +		pr_warn("clock: %s: rate %lu can't be kept with SSC enabled\n",
> +			clk_hw_get_name(hw), rate);
> +		dd->ssc_conf = old_conf;
> +	}

[Severity: High]
Does comparing clk_hw_get_parent(hw) against dd->clk_ref in
omap3_noncore_dpll_set_spread_spectrum() in drivers/clk/ti/dpll3xxx.c
actually detect when an AM33xx or AM43xx DPLL is in bypass mode?

In drivers/clk/ti/dpll.c, dd->ssc_enable_mask is only set for
ti,am3-dpll-no-gate-clock and ti,am3-dpll-clock. In
arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi and
arch/arm/boot/dts/ti/omap/am43xx-clocks.dtsi, all DPLLs using these
compatibles (such as dpll_disp_ck and dpll_extdev_ck) specify the same clock
for both the reference clock (index 0) and the bypass clock (index 1):

arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi:
	dpll_disp_ck: clock@498 {
		#clock-cells = <0>;
		compatible = "ti,am3-dpll-no-gate-clock";
		clock-output-names = "dpll_disp_ck";
		clocks = <&sys_clkin_ck>, <&sys_clkin_ck>;
		reg = <0x0498>, <0x0448>, <0x0454>, <0x044c>, <0x0450>;
	};

When _register_dpll() in drivers/clk/ti/dpll.c initializes the DPLL:

	dd->clk_ref = __clk_get_hw(clk);

	clk = of_clk_get(node, 1);
	...
	dd->clk_bypass = __clk_get_hw(clk);

both dd->clk_ref and dd->clk_bypass point to the same struct clk_hw
(sys_clkin_ck). Even when omap2_init_dpll_parent() in
drivers/clk/ti/clkt_dpll.c returns parent index 1 because the hardware is in
bypass mode, clk_hw_get_parent(hw) still returns sys_clkin_ck, so
clk_hw_get_parent(hw) != dd->clk_ref is always false.

When of_clk_set_defaults() runs on a bypassed DPLL with assigned-clock-sscs,
 the call chain is:

of_clk_set_defaults()
  __set_clk_spread_spectrum()
    clk_hw_set_spread_spectrum()
      omap3_noncore_dpll_set_spread_spectrum()

Because __set_clk_spread_spectrum() runs before __set_clk_rates(),
clk_hw_get_rate(hw) returns the bypass reference clock rate. Depending on
dd->min_divider, won't this either prematurely lock the bypassed DPLL at
ref_rate with M = N (where last_rounded_m < 20 triggers the out-of-range SSC
multiplier warning in omap3_noncore_dpll_ssc_program()), or fail in
omap2_dpll_determine_rate() and revert dd->ssc_conf to old_conf so that SSC
is lost when __set_clk_rates() runs next?

Should this check the hardware bypass state via dd->control_reg and
_omap2_dpll_is_in_bypass() (similar to omap2_init_dpll_parent() in
drivers/clk/ti/clkt_dpll.c) instead of comparing parent pointers?

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