Re: [PATCH v2 06/13] clk: renesas: Add support for R9A09G077 SoC

[Paul Barker <paul.barker.ct@xxxxxxxxxxxxxx>]

Hi Thierry,

On 17/02/2025 10:52, Thierry Bultel wrote:
> RZ/T2H has 2 registers blocks at different addresses.
> 
> The clock tree has configurable dividers and mux selectors.
> Add these new clock types, new register layout type, and
> registration code for mux and div in registration callback.
> 
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/renesas/Kconfig              |   5 +
>  drivers/clk/renesas/Makefile             |   1 +
>  drivers/clk/renesas/r9a09g077-cpg-mssr.c | 237 +++++++++++++++++++++++
>  drivers/clk/renesas/renesas-cpg-mssr.c   |  40 ++++
>  drivers/clk/renesas/renesas-cpg-mssr.h   |  23 +++
>  5 files changed, 306 insertions(+)
>  create mode 100644 drivers/clk/renesas/r9a09g077-cpg-mssr.c
> 
> diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
> index ff01f5f0ed20..58ea50b0e5b8 100644
> --- a/drivers/clk/renesas/Kconfig
> +++ b/drivers/clk/renesas/Kconfig
> @@ -41,6 +41,7 @@ config CLK_RENESAS
>  	select CLK_R9A08G045 if ARCH_R9A08G045
>  	select CLK_R9A09G011 if ARCH_R9A09G011
>  	select CLK_R9A09G057 if ARCH_R9A09G057
> +	select CLK_R9A09G077 if ARCH_R9A09G077
>  	select CLK_SH73A0 if ARCH_SH73A0
>  
>  if CLK_RENESAS
> @@ -198,6 +199,10 @@ config CLK_R9A09G057
>         bool "RZ/V2H(P) clock support" if COMPILE_TEST
>         select CLK_RZV2H
>  
> +config CLK_R9A09G077
> +	bool "RZ/T2H clock support" if COMPILE_TEST
> +	select CLK_RZT2H

CLK_RZT2H isn't needed anymore. Instead we probably need to depend on
CLK_RENESAS_CPG_MSSR.

> +
>  config CLK_SH73A0
>  	bool "SH-Mobile AG5 clock support" if COMPILE_TEST
>  	select CLK_RENESAS_CPG_MSTP

[snip, now in drivers/clk/renesas/r9a09g077-cpg-mssr.c]

> +static struct clk * __init
> +	r9a09g077_cpg_clk_register(struct device *dev,

Looks like an extra tab crept in at the start of this line

[snip]

> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 7d5fba3aef19..2f5aa796b0c6 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -79,6 +79,15 @@ static const u16 mstpcr_for_gen4[] = {
>  	0x2D60, 0x2D64, 0x2D68, 0x2D6C, 0x2D70, 0x2D74,
>  };
>  
> +/* Module Stop Control Register (RZ/T2H)
> + * RZ/T2H has 2 registers blocks. To differentiate them,
> + * 0x1000 is added to offsets of block 2
> + */
> +static const u16 mstpcr_for_rzt2h[] = {
> +	0x0300, 0x0304, 0x0308, 0x030C, 0x0310, 0x1318, 0x1320, 0x0324,
> +	0x0328, 0x032C, 0x0303, 0x1334,
> +};

Should the second to last entry be 0x0330 not 0x0303? I'm suspicious as
it doesn't match the alignment of the other entries.

We need some macros for this instead of just using 0x1000 in the
constants and the calculations below.

Perhaps,

#define RZT2H_MSTPCR_BLOCK_SHIFT	12
#define RZT2H_MSTPCR_OFFSET_MASK	GENMASK(11, 0)
#define RZT2H_MSTPCR(block, offset)	(((block) << RZT2H_MSTPCR_BLOCK_SHIFT) & ((offset) & RZT2H_MSTPCR_OFFSET_MASK))

#define RZT2H_MSTPCR_BLOCK(x)		((x) >> RZT2H_MSTPCR_BLOCK_SHIFT)
#define RZT2H_MSTPCR_OFFSET(x)		((x) & RZT2H_MSTPCR_OFFSET_MASK)

static const u16 mstpcr_for_rzt2h[] = {
	RZT2H_MSTPCR(0, 0x300), RZT2H_MSTPCR(0, 0x304), RZT2H_MSTPCR(0, 0x308), RZT2H_MSTPCR(0, 0x30c),
	RZT2H_MSTPCR(0, 0x310), RZT2H_MSTPCR(1, 0x318), RZT2H_MSTPCR(1, 0x320), RZT2H_MSTPCR(0, 0x324), 
	RZT2H_MSTPCR(0, 0x328), RZT2H_MSTPCR(0, 0x32c), RZT2H_MSTPCR(0, 0x330), RZT2H_MSTPCR(1, 0x334), 
};

> +
>  /*
>   * Standby Control Register offsets (RZ/A)
>   * Base address is FRQCR register
> @@ -189,6 +198,14 @@ struct mstp_clock {
>  
>  #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
>  
> +static void *cpg_rzt2h_addr_from_offset(struct clk_hw *hw, u16 offset)
> +{
> +	struct mstp_clock *clock = to_mstp_clock(hw);
> +	struct cpg_mssr_priv *priv = clock->priv;
> +
> +	return offset + (offset > 0x1000 ? priv->pub.base1 - 0x1000 : priv->pub.base0);

Perhaps,

void *base = RZT2H_MSTPCR_BLOCK(offset) ? priv->pub.base1 : priv->pub.base0;
return base + RZT2H_MSTPCR_OFFSET(offset);

Thanks,

-- 
Paul Barker

Attachment: OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature