Re: [PATCH v7 08/10] clk: realtek: Add RTD1625-CRT clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware race condition due to missing freq_ready_valid initialization in PLL structures.
- [Medium] Missing MODULE_DEVICE_TABLE for the clock controller driver.
- [Medium] rtk_clk_probe writes per-device regmap state into globally shared static clock structures.
- [Low] Kconfig dependency propagation violation for CLK_RTD1625.
- [Low] Typo in MODULE_DESCRIPTION of the reset driver.
--

commit 84d35dbda57cf6e9762915011a556ab1a6e99c67
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add RTD1625-CRT clock controller driver

Add support for the CRT (Clock, Reset, and Test) domain clock controller
on the Realtek RTD1625 SoC. This driver provides essential clock sources
(including PLLs), gating, and multiplexing functionalities for the
platform's peripherals.

> diff --git a/drivers/clk/realtek/Kconfig b/drivers/clk/realtek/Kconfig
> index 9efd70094bd27..2eaaa827ae3ac 100644
> --- a/drivers/clk/realtek/Kconfig
> +++ b/drivers/clk/realtek/Kconfig
> @@ -28,4 +28,19 @@ config RTK_CLK_COMMON
>  config RTK_CLK_PLL_MMC
>  	tristate
>  
> +config CLK_RTD1625
> +	tristate "RTD1625 Clock Controller"
> +	depends on RESET_CONTROLLER
> +	select RESET_RTD1625

Should this config also depend on ARCH_REALTEK || COMPILE_TEST?

RESET_RTD1625 has this dependency, and selecting it without inheriting
the dependencies can cause unmet direct dependency warnings or build
failures on unsupported architectures.

> +	select RTK_CLK_COMMON

[ ... ]

> diff --git a/drivers/clk/realtek/clk-rtd1625-crt.c b/drivers/clk/realtek/clk-rtd1625-crt.c
> new file mode 100644
> index 0000000000000..ec9d8f936199c
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-rtd1625-crt.c

[ ... ]

> +static struct clk_pll pll_acpu = {
> +	.clkr.hw.init = CLK_HW_INIT("pll_acpu", "osc27m", &rtk_clk_pll_ops, CLK_GET_RATE_NOCACHE),
> +	.seq_power_on          = pll_acpu_seq_power_on,
> +	.num_seq_power_on      = ARRAY_SIZE(pll_acpu_seq_power_on),
> +	.seq_power_off         = pll_acpu_seq_power_off,
> +	.num_seq_power_off     = ARRAY_SIZE(pll_acpu_seq_power_off),
> +	.seq_pre_set_freq      = pll_acpu_seq_pre_set_freq,
> +	.num_seq_pre_set_freq  = ARRAY_SIZE(pll_acpu_seq_pre_set_freq),
> +	.seq_post_set_freq     = pll_acpu_seq_post_set_freq,
> +	.num_seq_post_set_freq = ARRAY_SIZE(pll_acpu_seq_post_set_freq),
> +	.freq_reg              = RTD1625_REG_PLL_SSC_DIG_ACPU1,
> +	.freq_tbl              = acpu_tbl,
> +	.freq_mask             = FREQ_NF_MASK,
> +	.freq_ready_reg        = RTD1625_REG_PLL_SSC_DIG_ACPU_DBG2,
> +	.freq_ready_mask       = BIT(20),
> +	.freq_ready_val        = BIT(20),
> +	.power_reg             = RTD1625_REG_PLL_ACPU2,
> +	.power_mask            = 0x7,
> +	.power_val_on          = 0x3,
> +	.lock                  = __SPIN_LOCK_UNLOCKED(pll_acpu.lock),
> +};

Will the clock framework assume this PLL is stable prematurely?

Because this structure is zero-initialized and .freq_ready_valid is not set
to 1, wait_freq_ready() may return 0 immediately without polling the
hardware PLL lock status. This could potentially lead to hardware lockups
or data corruption.

[ ... ]

> +static struct clk_regmap * const rtd1625_crt_regmap_clks[] = {
> +	&clk_en_misc.clkr,
> +	&clk_en_pcie0.clkr,
> +	&clk_en_gspi.clkr,

[ ... ]

> +	&clk_gpu.clkr,

Does this array pass statically allocated global clock objects to
rtk_clk_probe()?

Looking at rtk_clk_probe(), it iterates over desc->clks and writes the
per-device regmap state:

    for (i = 0; i < desc->num_clks; i++)
        desc->clks[i]->regmap = regmap;

If the device is unbound, could this leave a dangling pointer in the global
clock structures?

[ ... ]

> +static const struct of_device_id rtd1625_crt_match[] = {
> +	{.compatible = "realtek,rtd1625-crt-clk", .data = &rtd1625_crt_desc,},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver rtd1625_crt_driver = {

Could this driver use a MODULE_DEVICE_TABLE() for rtd1625_crt_match?

Because CLK_RTD1625 is a tristate config, this can be built as a loadable
module. Without the macro, module auto-loading may fail when the device
tree node is encountered.

[ ... ]

> diff --git a/drivers/reset/realtek/reset-rtd1625-crt.c b/drivers/reset/realtek/reset-rtd1625-crt.c
> new file mode 100644
> index 0000000000000..13553e3224cd3
> --- /dev/null
> +++ b/drivers/reset/realtek/reset-rtd1625-crt.c

[ ... ]

> +MODULE_DESCRIPTION("Reatek RTD1625 CRT Reset Controller Driver");

This isn't a bug, but there's a typo in "Reatek" which should be "Realtek".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508111641.3192177-1-eleanor.lin@realtek.com?part=8