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

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+V-a8tvd1uWNWqLSt4TQ-rD4Odfy0SiFX-4tWA2wsqae1wTeA@mail.gmail.com>
Date: Tue, 14 Oct 2025 08:29:50 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: 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 v4] clk: renesas: cpg-mssr: Add module reset support for RZ/T2H

Hi Geert,

On Tue, Oct 14, 2025 at 7:46 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, 13 Oct 2025 at 18:45, Lad, Prabhakar <prabhakar.csengg@...il.com> wrote:
> > On Mon, Oct 13, 2025 at 4:46 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > On Mon, 29 Sept 2025 at 13:23, Prabhakar <prabhakar.csengg@...il.com> 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>
> > > > ---
> > > > 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 the reset operation no, for this I would like to keep this as is.
> > But for the MSTP registers I will be adding a delay. Or did I
> > misunderstand something?
>
> How to make sure both "#define RZT2H_RESET_REG_READ_COUNT 7" and
> "seven" are updated together?
>
>     /*
>      * For secure processing after release from a module reset, one must
>      * perform multiple dummy reads of the same register.
>      */
>
Ahh got you now, thanks for the clarification.

> >
> > > > +        */
> > > > +       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?
> > >
> > Ok, I will update it to the above in v5.
>
> No need to resend yet, I could make these changes while applying
> (when Philipp is happy).
>
Ok, thank you.


Cheers,
Prabhakar