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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Register configuration overlap between `CLK_RTC_32K` and `CLK_I2C1` multiplexers.
- [High] Missing reset controller implementation.
--

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

clk: rockchip: Add clock controller for the RV1106

Add the clock and reset controller driver for the Rockchip RV1106,
based on the vendor kernel from the Luckfox Pico SDK.

> diff --git a/drivers/clk/rockchip/clk-rv1106.c b/drivers/clk/rockchip/clk-rv1106.c
> new file mode 100644
> index 0000000000000..3f26573e6cb70
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rv1106.c
> @@ -0,0 +1,1099 @@
[ ... ]
> +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);
[ ... ]
> +	GATE(PCLK_I2C1, "pclk_i2c1", "pclk_pmu_root", 0,
> +			RV1106_PMUCLKGATE_CON(0), 3, GFLAGS),
> +	COMPOSITE_NODIV(CLK_I2C1, "clk_i2c1", mux_200m_100m_24m_32k_p, 0,
> +			RV1106_PMUCLKSEL_CON(0), 6, 2, MFLAGS,
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code introduce a regression by assigning the same configuration
bits for both the RTC_32K and I2C1 clocks?

The multiplexer for clk_rtc32k_pmu_fracmux is defined at
RV1106_PMUCLKSEL_CON(0) shift 6 width 2. Here in rv1106_clk_branches, CLK_I2C1
is defined at the exact same register, shift, and width.

Will reparenting or setting the rate for one of these clocks inadvertently
overwrite the configuration of the other?

[ ... ]
> +	/*
> +	 * The MMC drive and sample phase clocks have their control
> +	 * registers in the GRF region, so they can only be provided when
> +	 * the rockchip,grf property points at a valid syscon.
> +	 */
> +	if (!IS_ERR(ctx->grf)) {
> +		ret = rockchip_clk_add_grf(ctx, ctx->grf, grf_type_sys);
> +		if (ret)
> +			dev_err(dev, "failed to add grf: %d\n", ret);
> +		else
> +			rockchip_clk_register_branches(ctx,
> +					rv1106_grf_clk_branches,
> +					ARRAY_SIZE(rv1106_grf_clk_branches));
> +	}
> +
> +	rockchip_register_restart_notifier(ctx, RV1106_GLB_SRST_FST, NULL);
> +
> +	rockchip_clk_of_add_provider(np, ctx);
> +
> +	return 0;
> +}

[Severity: High]
Does this driver omit the reset controller registration?

The commit message states that this patch adds the clock and reset controller
driver, but rockchip_register_softrst() is not called in clk_rv1106_probe().
Will this regression cause peripheral drivers on this SoC to fail to probe
with -ENOENT or -EPROBE_DEFER if they request a reset from this CRU node in
their devicetree?

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