lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+V-a8t46mQ29Qa4EqBDkiRhCEB-fFzWQNRqMvQhNdwix8=ePQ@mail.gmail.com>
Date: Mon, 8 Sep 2025 13:56:01 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Philipp Zabel <p.zabel@...gutronix.de>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, linux-renesas-soc@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3] clk: renesas: cpg-mssr: Add module reset support for RZ/T2H

Hi Philipp,

Thank you for the review.

On Fri, Sep 5, 2025 at 3:46 PM Philipp Zabel <p.zabel@...gutronix.de> wrote:
>
> On Fr, 2025-09-05 at 12:45 +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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@...renesas.com>
> > ---
> > v2->v3:
> > - Simplifed the code by adding a common function cpg_mrcr_set_bit() to handle
> >   set/clear of bits with options for verify and dummy reads.
> > - Added a macro for the number of dummy reads required.
> >
> > v1->v2:
> > - Added cpg_mrcr_reset_ops for RZ/T2H specific handling
> > - Updated commit message
> > ---
> >  drivers/clk/renesas/renesas-cpg-mssr.c | 112 ++++++++++++++++++++++++-
> >  1 file changed, 108 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> > index 5ff6ee1f7d4b..81206db2b873 100644
> > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > @@ -40,6 +40,8 @@
> >  #define WARN_DEBUG(x)        do { } while (0)
> >  #endif
> >
> > +#define RZT2H_RESET_REG_READ_COUNT   7
> > +
> >  /*
> >   * Module Standby and Software Reset register offets.
> >   *
> > @@ -137,6 +139,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
> >   */
> > @@ -736,6 +754,73 @@ 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_bit(struct reset_controller_dev *rcdev, unsigned long id,
> > +                         bool set, bool verify, bool dummy_reads, const char *op_name)
>
> This function is inappropriately named, it does more than just set a
> bit.
>
Ok, I will rename it to `cpg_mrcr_set_reset_state`.

static int cpg_mrcr_set_reset_state(struct reset_controller_dev
*rcdev, unsigned long id, bool assert)

> Why are there three boolean parameters if there are only ever two
> combinations of them used? Just have a single bool assert.
>
> Drop the op_name parameter.
>
OK.

> > +{
> > +     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", op_name, reg, bit);
>
> Replace op_name with set ? "assert" : "deassert".
>
OK.

> You could later add a str_assert_deassert() helper in string_choices.h.
>
Ok.

> > +
> > +     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);
> > +
> > +     /* Verify the operation if requested */
> > +     if (verify) {
> > +             val = readl(reg_addr);
> > +             if ((set && !(bitmask & val)) || (!set && (bitmask & val))) {
> > +                     dev_err(priv->dev, "Reset register %u%02u operation failed\n", reg, bit);
> > +                     spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> > +                     return -EIO;
> > +             }
> > +     }
> > +
> > +     /* Perform dummy reads if required */
> > +     for (i = 0; dummy_reads && i < RZT2H_RESET_REG_READ_COUNT; i++)
> > +             readl(reg_addr);
>
> Both verify and dummy reads could just live in a single if (!assert) {}
> block.
>
I'll move the bit checking after dummy reads.

val = readl(reg_addr);
if ((assert && !(bitmask & val)) || (!assert && (bitmask & val))) {
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;
> > +}
> > +
> > +static int cpg_mrcr_reset(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +     int ret;
> > +
> > +     /* Assert reset */
> > +     ret = cpg_mrcr_set_bit(rcdev, id, true, true, false, "reset");
> > +     if (ret)
> > +             return ret;
>
> No delay necessary for any of the modules that can be reset?
>
Currently the HW manual doesn't mention any delays.

> > +     /* Deassert reset with dummy reads */
> > +     return cpg_mrcr_set_bit(rcdev, id, false, false, true, "reset");
>
> Copy & paste error in op_name, but that is fixed if you drop it.
>
Ok.

Cheers,
Prabhakar

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ