Hi Philipp,
Thank you for the review.
On Fri, Aug 29, 2025 at 10:46 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
>
> On Mi, 2025-08-20 at 21:48 +0100, Prabhakar 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>
> > ---
> > 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?
>
On RZ/T2H SoC for registers MRCTLA-MRCTLM none of them have 32 rest
controls in them; For the bits which does not have rest controls they
are marked as reserved.
> Why are reserved registers in this list? It looks like the driver
> allows poking around in reserved registers.
>
The HW manual currently lists registers MRCTLA/E/I/M, for the rest of
the registers I have marked them as reserved to avoid breakage i.e. in
future if MRCTLC register is exposed we won't have any breakage.
MRCTLA - 0
MRCTLE - 400
MRCTLI - 800
MRCTLM - 1200
For example, MRCTLE[0] controls GMAC0 PCLKH reset; this is represented
as 400 in DT.
This driver uses `4` to get the offset from mrcr_for_rzt2h array and
`00` is the bit to be updated. So suppose in future if MRCTLD is
exposed we just use that in DT.
> > +
> > /*
> > * 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?
>
This check is based on the HW manual,
"When module is reset once and released again, make sure that the
target bit of module reset control register is set to 1 by reading the
register before releasing from a module reset. Then release from a
module reset."
> > + 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.
>
OK.
> > + 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?
>
This is again based on the HW manual,
"To secure processing after release from a module reset, dummy read
the same register at least seven times after writing to initiate
release from the module reset, and only then proceed with the
subsequent processing."
> > + */
> > + 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.
>
OK.
Cheers,
Prabhakar
