Re: [PATCH 1/8] clk: r9a08g045-cpg: Add clocks and resets for CAN-FD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The P4 core clock is registered using an internal enum ID instead of its Device Tree exported ID, rendering it inaccessible to DT consumers.
--

--- Patch [1]: [PATCH 1/8] clk: r9a08g045-cpg: Add clocks and resets for CAN-FD ---
commit 6c9b264ec67002e41ec321d8af3b04f800484a8a
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

clk: r9a08g045-cpg: Add clocks and resets for CAN-FD
    
Renesas RZ/G3S SoC has a CAN-FD IP. Add clocks and resets for it.

> diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
> index 1232fec913eb5..34927d1ee4639 100644
> --- a/drivers/clk/renesas/r9a08g045-cpg.c
> +++ b/drivers/clk/renesas/r9a08g045-cpg.c
> @@ -88,6 +88,8 @@ enum clk_ids {
>  	CLK_SEL_PLL4,
>  	CLK_P1_DIV2,
>  	CLK_P3_DIV2,
> +	CLK_P4,

[Severity: High]
Should this clock be exported to the device tree instead of being added as an
internal enum?

Because CLK_P4 is defined after LAST_DT_CORE_CLK in clk_ids, it seems device
tree consumers requesting <&cpg CPG_CORE R9A08G045_CLK_P4> would fail to
probe with -ENOENT.

> +	CLK_P4_DIV2,
>  	CLK_SD0_DIV4,

[ ... ]

> @@ -182,6 +184,8 @@ static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
>  		    dtable_1_32, 0, 0, 0, NULL),
>  	DEF_G3S_DIV("P3", R9A08G045_CLK_P3, CLK_PLL3_DIV2_4, DIVPL3C, G3S_DIVPL3C_STS,
>  		    dtable_1_32, 0, 0, 0, NULL),
> +	DEF_FIXED("P4", CLK_P4, CLK_PLL2_DIV2, 1, 5),

[Severity: High]
Looking at the surrounding definitions for P1, P2, P3, and P5 in
r9a08g045_core_clks, they all use their respective R9A08G045_CLK_* macros.

Does P4 need to be registered with the exported R9A08G045_CLK_P4 macro instead
of CLK_P4 to ensure the clock populates correctly at the intended device tree
index?

> +	DEF_FIXED("P4_DIV2", CLK_P4_DIV2, CLK_P4, 1, 2),
>  	DEF_FIXED("P3_DIV2", CLK_P3_DIV2, R9A08G045_CLK_P3, 1, 2),

[ ... ]

> @@ -285,6 +289,10 @@ static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
>  					MSTOP(BUS_MCPU2, BIT(5))),
>  	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5,
>  					MSTOP(BUS_MCPU3, BIT(4))),
> +	DEF_MOD("canfd_pclk",		R9A08G045_CANFD_PCLK, CLK_P4_DIV2, 0x594, 0,
> +					MSTOP(BUS_MCPU2, BIT(9))),
> +	DEF_MOD("canfd_clk_ram",	R9A08G045_CANFD_CLK_RAM, CLK_P4, 0x594, 1,

[Severity: High]
If P4 is exported as R9A08G045_CLK_P4, should this module clock reference
the exported macro as its parent here in r9a08g045_mod_clks?

> +					MSTOP(BUS_MCPU2, BIT(9))),
>  	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707102418.1646159-1-claudiu.beznea+renesas@tuxon.dev?part=1