Re: [PATCH v2 2/6] clk: rockchip: Add clock controller for the RV1106

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `RV1106_CPUCLK_RATE` macro unconditionally overwrites the dynamically calculated safe intermediate CPU divider during frequency transitions, causing CPU crashes.
- [High] The exported `PLL_APLL` clock is silently overwritten by anonymous clock branches due to a dummy ID collision.
- [High] Register collision between `CLK_RTC_32K` and `CLK_I2C1` multiplexers.
- [Medium] CLK_VICAP_M1_FRAC is missing the CLK_SET_RATE_PARENT flag.
--

--- Patch [2]: [PATCH v2 2/6] clk: rockchip: Add clock controller for the RV1106 ---
commit f23e8ffe0002c99e81093bf229093ffef3a65c00
Author: Simon Glass <sjg@chromium.org>

clk: rockchip: Add clock controller for the RV1106

The patch adds the clock and reset controller driver for the Rockchip
RV1106, based on the vendor kernel adapted to the upstream clock framework.
It also provides the MMC drive and sample phase clocks from the CRU via
an auxiliary GRF regmap.

Link: https://github.com/LuckfoxTECH/luckfox-pico

> diff --git a/drivers/clk/rockchip/clk-rv1106.c b/drivers/clk/rockchip/clk-rv1106.c
> new file mode 100644
> index 0000000000000..cb21f32c8a26b
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rv1106.c
> @@ -0,0 +1,1100 @@

[ ... ]

> +#define RV1106_CPUCLK_RATE(_prate, _aclk_core, _pclk_dbg, _is_pvtpll)	\
> +{									\
> +	.prate = _prate,						\
> +	.divs = {							\
> +		RV1106_CLKSEL0(_aclk_core),				\
> +		RV1106_CLKSEL1(_pclk_dbg),				\
> +	},								\
> +	.pre_muxs = {							\
> +		RV1106_CLKSEL3(1),					\
> +		RV1106_CLKSEL2(2),					\
> +	},								\

[Severity: High]
Does this hardcoded array overwrite the safe intermediate divider calculated
by the generic CPU clock framework during frequency scaling?

The core clock framework calculates a safe intermediate divider and writes it
to RV1106_CORECLKSEL_CON(0) bits 4:0, as configured in rv1106_cpuclk_data:

	.core_reg[0] = RV1106_CORECLKSEL_CON(0),
	.div_core_shift[0] = 0,
	.div_core_mask[0] = 0x1f,

By unconditionally applying RV1106_CLKSEL3(1) in the pre_muxs array, could it
overwrite this dynamically calculated divider and potentially cause instability
during CPU frequency transitions?

[ ... ]

> +static struct rockchip_clk_branch rv1106_rtc32k_pmu_fracmux __initdata =
> +	MUX(CLK_RTC_32K, "clk_rtc_32k", clk_rtc32k_pmu_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
> +			RV1106_PMUCLKSEL_CON(0), 6, 2, MFLAGS);

[Severity: High]
Are clk_rtc_32k and clk_i2c1 sharing the same register bitfield?

This multiplexer is defined using bits 6:7 of RV1106_PMUCLKSEL_CON(0).
However, later in rv1106_clk_branches, CLK_I2C1 is defined using the exact
same bits:

	COMPOSITE_NODIV(CLK_I2C1, "clk_i2c1", mux_200m_100m_24m_32k_p, 0,
			RV1106_PMUCLKSEL_CON(0), 6, 2, MFLAGS,

Could modifying the parent of one clock inadvertently corrupt the
configuration of the other?

[ ... ]

> +static struct rockchip_clk_branch rv1106_clk_branches[] __initdata = {
> +
> +	FACTOR(0, "xin_osc0_half", "xin24m", 0, 1, 2),
> +
> +	/* PD_CORE */
> +	GATE(CLK_PVTM_CORE, "clk_pvtm_core", "xin24m", 0,

[Severity: High]
Will these anonymous branches using ID 0 silently overwrite the PLL_APLL
clock in the provider's clock array?

Since PLL_APLL corresponds to ID 0 in the DT bindings, it gets registered
first at ctx->clk_data.clks[0]. Since rockchip_clk_set_lookup() doesn't
ignore ID 0, do these dummy IDs cause device tree consumers of the APLL
clock to receive an incorrect internal anonymous clock instead?

[ ... ]

> +	COMPOSITE(CLK_VICAP_M1_SRC, "clk_vicap_m1_src", mux_gpll_cpll_p, 0,
> +			RV1106_CLKSEL_CON(31), 7, 1, MFLAGS, 2, 5, DFLAGS,
> +			RV1106_CLKGATE_CON(3), 13, GFLAGS),
> +	COMPOSITE_FRACMUX(CLK_VICAP_M1_FRAC, "clk_vicap_m1_frac", "clk_vicap_m1_src", 0,
> +			RV1106_CLKSEL_CON(32), 0,
> +			RV1106_CLKGATE_CON(3), 14, GFLAGS,
> +			&rv1106_clk_vicap_m1_fracmux),

[Severity: Medium]
Should CLK_VICAP_M1_FRAC pass the CLK_SET_RATE_PARENT flag instead of 0?

Its sibling CLK_VICAP_M0_FRAC uses CLK_SET_RATE_PARENT to allow rate changes
to propagate up to the integer divider. Without this flag, will the clock
framework fail to adjust the parent divider when a rate change is requested?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714193656.2196447-1-sjg@chromium.org?part=2