Re: [PATCH 3/9] clk: renesas: r8a779h0: Add display clocks

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Tue, Dec 03, 2024 at 10:01:37AM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> Add display related clocks for DU, DSI, FCPVD, and VSPD.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/clk/renesas/r8a779h0-cpg-mssr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a779h0-cpg-mssr.c b/drivers/clk/renesas/r8a779h0-cpg-mssr.c
> index e20c048bfa9b..dc37e987c0e6 100644
> --- a/drivers/clk/renesas/r8a779h0-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a779h0-cpg-mssr.c
> @@ -179,6 +179,9 @@ static const struct mssr_mod_clk r8a779h0_mod_clks[] __initconst = {
>  	DEF_MOD("canfd0",	328,	R8A779H0_CLK_SASYNCPERD2),
>  	DEF_MOD("csi40",	331,	R8A779H0_CLK_CSI),
>  	DEF_MOD("csi41",	400,	R8A779H0_CLK_CSI),
> +	DEF_MOD("dis0",		411,	R8A779H0_CLK_S0D3),
> +	DEF_MOD("dsitxlink0",	415,	R8A779H0_CLK_DSIREF),
> +	DEF_MOD("fcpvd0",	508,	R8A779H0_CLK_S0D3),
>  	DEF_MOD("hscif0",	514,	R8A779H0_CLK_SASYNCPERD1),
>  	DEF_MOD("hscif1",	515,	R8A779H0_CLK_SASYNCPERD1),
>  	DEF_MOD("hscif2",	516,	R8A779H0_CLK_SASYNCPERD1),
> @@ -227,6 +230,7 @@ static const struct mssr_mod_clk r8a779h0_mod_clks[] __initconst = {
>  	DEF_MOD("vin15",	811,	R8A779H0_CLK_S0D4_VIO),
>  	DEF_MOD("vin16",	812,	R8A779H0_CLK_S0D4_VIO),
>  	DEF_MOD("vin17",	813,	R8A779H0_CLK_S0D4_VIO),
> +	DEF_MOD("vspd0",	830,	R8A779H0_CLK_S0D1_VIO),

The clock names and numbers are fine. The parents are not explicitly
listed in documentation, but the VIODBUSD1 clock description (table
8.1.4a) mentions FCPVD and VSPD as target modules. This is something
that should probably be double-checked. Quite interestingly, VIOBUSD2
also mentions the same target modules, hinting as a more complex clock
tree. A similar issue is perhaps present for "dis0" too, that's a DU
clock and the DU isn't listed as a target module of S0D3.

The way we model the "module stop" bits as clocks is clearly a limiting
factor as it can't represent real clock topologies. I don't however
don't expect it to cause any functional issue here, as the devices
related to the above clocks do not depend on the clock frequency.
There's no strict need to model the real hardware clock tree if it has
no impact on software, so

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I think it could be worth it check with Renesas what parent to use here,
and we can update the clock definitions later if needed.

>  	DEF_MOD("wdt1:wdt0",	907,	R8A779H0_CLK_R),
>  	DEF_MOD("cmt0",		910,	R8A779H0_CLK_R),
>  	DEF_MOD("cmt1",		911,	R8A779H0_CLK_R),
> 

-- 
Regards,

Laurent Pinchart