Hi Geert,
Thank you for the review.
On Fri, Aug 8, 2025 at 11:14 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Thu, 7 Aug 2025 at 18:44, 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 both reset_regs and reset_clear_regs. For RZ/T2H, set
> > rcdev.nr_resets based on the number of MRCR registers rather than the
> > number of module clocks.
> >
> > Update the reset/assert/deassert/status operations to perform RMW when
> > handling RZ/T2H-specific layout. This enables proper reset sequencing for
> > modules on RZ/T2H without affecting the behavior of other supported SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- 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 */
> > +};
> > +
> > /*
> > * Software Reset Clearing Register offsets
> > */
> > @@ -686,12 +702,16 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> >
> > dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
> >
> > + if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> > + bitmask = readl(priv->pub.base0 + priv->reset_regs[reg]) | bitmask;
>
> bitmask |= ...
>
> > /* Reset module */
> > writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>
> However, you should hold the &priv->pub.rmw_lock spinlock everywhere
> you do an RMW operation.
>
Ok.
> > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> > udelay(35);
>
> Please read "6.5.1 Notes on Module Reset Control Register Operation"...
>
I had missed that...
> > + if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> > + bitmask = readl(priv->pub.base0 + priv->reset_clear_regs[reg]) & ~bitmask;
> > /* Release module from reset state */
> > writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
>
> Hence I think it would be worthwhile to have your own struct
> reset_control_ops mrcr_reset_ops with its own set of callbacks.
>
Ok, I will add a new mrcr_reset_ops for T2H which will adhere to
"6.5.1 Notes on Module Reset Control Register Operation" restrictions
with spinlock held.
> > @@ -764,7 +788,16 @@ static int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv)
> > priv->rcdev.of_node = priv->dev->of_node;
> > priv->rcdev.of_reset_n_cells = 1;
> > priv->rcdev.of_xlate = cpg_mssr_reset_xlate;
> > - priv->rcdev.nr_resets = priv->num_mod_clks;
> > +
> > + /*
> > + * RZ/T2H (and family) has the Module Reset Control Registers
> > + * which allows control resets of certain modules.
> > + * The number of resets is not equal to the number of module clocks.
> > + */
> > + if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> > + priv->rcdev.nr_resets = ARRAY_SIZE(mrcr_for_rzt2h) * 32;
> > + else
> > + priv->rcdev.nr_resets = priv->num_mod_clks;
>
> I guess this is fine for now, but probably we will have to introduce
> a proper nr_resets field when the next RZ/T* SoC is introduced...
>
Thanks.
> > return devm_reset_controller_register(priv->dev, &priv->rcdev);
> > }
> >
> > @@ -1166,6 +1199,8 @@ static int __init cpg_mssr_common_init(struct device *dev,
> > priv->control_regs = stbcr;
> > } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> > priv->control_regs = mstpcr_for_rzt2h;
> > + priv->reset_regs = mrcr_for_rzt2h;
> > + priv->reset_clear_regs = mrcr_for_rzt2h;
>
> With its own set of reset callbacks directly accessing mrcr_for_rzt2h,
> this could be dropped. However, if you want to keep on using
> cpg_mssr_status(), you do have to keep the assignment to
> priv->reset_regs.
>
To reuse cpg_mssr_status() I'll keep ` priv->reset_regs =
mrcr_for_rzt2h;` for now and drop the later.
Cheers,
Prabhakar
