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

[Philipp Zabel <p.zabel@pengutronix.de>]

On Mi, 2025-08-20 at 21:48 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> 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@bp.renesas.com>
> ---
> v1->v2:
> - Added cpg_mrcr_reset_ops for RZ/T2H specific handling
> - Updated commit message
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 139 ++++++++++++++++++++++++-
>  1 file changed, 135 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 5ff6ee1f7d4b..77a298b50c9c 100644
> --- 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 */
> +};

Does each of these registers contain 32 reset controls?

Why are reserved registers in this list? It looks like the driver
allows poking around in reserved registers.

> +
>  /*
>   * Software Reset Clearing Register offsets
>   */
> @@ -736,6 +752,102 @@ static int cpg_mssr_status(struct reset_controller_dev *rcdev,
>  	return !!(readl(priv->pub.base0 + priv->reset_regs[reg]) & bitmask);
>  }
>  
> +static int cpg_mrcr_reset(struct reset_controller_dev *rcdev,
> +			  unsigned long id)
> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;
> +	u32 bitmask = BIT(bit);
> +	unsigned long flags;
> +	unsigned int i;
> +
> +	dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
> +
> +	spin_lock_irqsave(&priv->pub.rmw_lock, flags);
> +	/* Reset module */
> +	bitmask |= readl(priv->pub.base0 + priv->reset_regs[reg]);
> +	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> +
> +	/* Ensure module reset control register is set */
> +	if (!(bitmask & readl(priv->pub.base0 + priv->reset_regs[reg]))) {

Can this actually happen on this hardware? Under which circumstances?

> +		dev_err(priv->dev, "Reset register %u%02u is not set\n",
> +			readl(priv->pub.base0 + priv->reset_regs[reg]), bit);

Why read the register again? Could it have changed in the meantime?
Maybe it would be better to store the value that was actually checked
and print the variable here.

> +		spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> +		return -EIO;
> +	}
> +
> +	/* Release module from reset state */
> +	bitmask = readl(priv->pub.base0 + priv->reset_regs[reg]) & ~bitmask;
> +	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> +
> +	/*
> +	 * To secure processing after release from a module reset, dummy read
> +	 * the same register at least seven times.

Why 7? Is this documented in a reference manual?

> +	 */
> +	for (i = 0; i < 7; i++)
> +		readl(priv->pub.base0 + priv->reset_regs[reg]);

It would be better to define a macro and use it in both places instead
of the magic number.

regards
Philipp