Re: [PATCH] clk: renesas: cpg-mssr: Add module reset support for RZ/T2H

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, 7 Aug 2025 at 18:44, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add support for module reset handling on the RZ/T2H SoC. Unlike earlier
> CPG/MSSR variants, RZ/T2H uses a unified set of Module Reset Control
> Registers (MRCR) where both reset and deassert actions are done via
> read-modify-write (RMW) to the same register.
>
> Introduce a new MRCR offset table (mrcr_for_rzt2h) for RZ/T2H and assign
> it to both reset_regs and reset_clear_regs. For RZ/T2H, set
> rcdev.nr_resets based on the number of MRCR registers rather than the
> number of module clocks.
>
> Update the reset/assert/deassert/status operations to perform RMW when
> handling RZ/T2H-specific layout. This enables proper reset sequencing for
> modules on RZ/T2H without affecting the behavior of other supported SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -137,6 +137,22 @@ static const u16 srcr_for_gen4[] = {
>         0x2C60, 0x2C64, 0x2C68, 0x2C6C, 0x2C70, 0x2C74,
>  };
>
> +static const u16 mrcr_for_rzt2h[] = {
> +       0x240,  /* MRCTLA */
> +       0x244,  /* Reserved */
> +       0x248,  /* Reserved */
> +       0x24C,  /* Reserved */
> +       0x250,  /* MRCTLE */
> +       0x254,  /* Reserved */
> +       0x258,  /* Reserved */
> +       0x25C,  /* Reserved */
> +       0x260,  /* MRCTLI */
> +       0x264,  /* Reserved */
> +       0x268,  /* Reserved */
> +       0x26C,  /* Reserved */
> +       0x270,  /* MRCTLM */
> +};
> +
>  /*
>   * Software Reset Clearing Register offsets
>   */
> @@ -686,12 +702,16 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>
>         dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
>
> +       if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> +               bitmask = readl(priv->pub.base0 + priv->reset_regs[reg]) | bitmask;

bitmask |= ...

>         /* Reset module */
>         writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);

However, you should hold the &priv->pub.rmw_lock spinlock everywhere
you do an RMW operation.

>         /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
>         udelay(35);

Please read "6.5.1 Notes on Module Reset Control Register Operation"...

> +       if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> +               bitmask = readl(priv->pub.base0 + priv->reset_clear_regs[reg]) & ~bitmask;
>         /* Release module from reset state */
>         writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);

Hence I think it would be worthwhile to have your own struct
reset_control_ops mrcr_reset_ops with its own set of callbacks.

> @@ -764,7 +788,16 @@ static int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv)
>         priv->rcdev.of_node = priv->dev->of_node;
>         priv->rcdev.of_reset_n_cells = 1;
>         priv->rcdev.of_xlate = cpg_mssr_reset_xlate;
> -       priv->rcdev.nr_resets = priv->num_mod_clks;
> +
> +       /*
> +        * RZ/T2H (and family) has the Module Reset Control Registers
> +        * which allows control resets of certain modules.
> +        * The number of resets is not equal to the number of module clocks.
> +        */
> +       if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> +               priv->rcdev.nr_resets = ARRAY_SIZE(mrcr_for_rzt2h) * 32;
> +       else
> +               priv->rcdev.nr_resets = priv->num_mod_clks;

I guess this is fine for now, but probably we will have to introduce
a proper nr_resets field when the next RZ/T* SoC is introduced...

>         return devm_reset_controller_register(priv->dev, &priv->rcdev);
>  }
>
> @@ -1166,6 +1199,8 @@ static int __init cpg_mssr_common_init(struct device *dev,
>                 priv->control_regs = stbcr;
>         } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
>                 priv->control_regs = mstpcr_for_rzt2h;
> +               priv->reset_regs = mrcr_for_rzt2h;
> +               priv->reset_clear_regs = mrcr_for_rzt2h;

With its own set of reset callbacks directly accessing mrcr_for_rzt2h,
this could be dropped.  However, if you want to keep on using
cpg_mssr_status(), you do have to keep the assignment to
priv->reset_regs.

>         } else if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4) {
>                 priv->status_regs = mstpsr_for_gen4;
>                 priv->control_regs = mstpcr_for_gen4;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds