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

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, 29 Sept 2025 at 13:23, 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 reset_regs. For this SoC, the number of resets is based on the
> number of MRCR registers rather than the number of module clocks. Also
> add cpg_mrcr_reset_ops to implement reset, assert, and deassert using RMW
> while holding the spinlock. This follows the RZ/T2H requirements, where
> processing after releasing a module reset must be secured by performing
> seven dummy reads of the same register, and where a module that is reset
> and released again must ensure the target bit in the Module Reset Control
> Register is set to 1.
>
> Update the reset controller registration to select cpg_mrcr_reset_ops for
> RZ/T2H, while keeping the existing cpg_mssr_reset_ops for other SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v3->v4:
> - Renamed cpg_mrcr_set_bit() to cpg_mrcr_set_reset_state() for clarity.
> - Updated the parameters in cpg_mrcr_set_reset_state().

Thanks for the update!

> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -736,6 +754,72 @@ static int cpg_mssr_status(struct reset_controller_dev *rcdev,
>         return !!(readl(priv->pub.base0 + priv->reset_regs[reg]) & bitmask);
>  }
>
> +static int cpg_mrcr_set_reset_state(struct reset_controller_dev *rcdev,
> +                                   unsigned long id, bool set)
> +{
> +       struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +       unsigned int reg = id / 32;
> +       unsigned int bit = id % 32;
> +       u32 bitmask = BIT(bit);
> +       void __iomem *reg_addr;
> +       unsigned long flags;
> +       unsigned int i;
> +       u32 val;
> +
> +       dev_dbg(priv->dev, "%s %u%02u\n", set ? "assert" : "deassert", reg, bit);
> +
> +       spin_lock_irqsave(&priv->pub.rmw_lock, flags);
> +
> +       reg_addr = priv->pub.base0 + priv->reset_regs[reg];
> +       /* Read current value and modify */
> +       val = readl(reg_addr);
> +       if (set)
> +               val |= bitmask;
> +       else
> +               val &= ~bitmask;
> +       writel(val, reg_addr);
> +
> +       /*
> +        * For secure processing after release from a module reset, dummy read
> +        * the same register at least seven times.

This comment is waiting to become out-of-sync with the actual value...

> +        */
> +       for (i = 0; !set && i < RZT2H_RESET_REG_READ_COUNT; i++)
> +               readl(reg_addr);
> +
> +       /* Verify the operation */
> +       val = readl(reg_addr);
> +       if ((set && !(bitmask & val)) || (!set && (bitmask & val))) {

Perhaps just "set == !(bitmask & val)"? Or is that too obscure?

> +               dev_err(priv->dev, "Reset register %u%02u operation failed\n", reg, bit);
> +               spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> +               return -EIO;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> +
> +       return 0;
> +}

Regardless:
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

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