Re: [PATCH v3 2/8] clk: renesas: r9a09g077: Register SYSC regmap

Hi Geert,

Thank you for the review.

On Mon, Aug 10, 2026 at 5:00 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Thu, 16 Jul 2026 at 14:34, Prabhakar <[email protected]> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Register a syscon regmap for the System Controller (SYSC) integrated into
> > the RZ/T2H and RZ/N2H CPG block.
> >
> > Unlike traditional Renesas CPG/MSSR implementations, the RZ/T2H and RZ/N2H
> > CPG block also integrates the SYSC, which provides low-power management,
> > clock monitoring, write protection and peripheral configuration registers
> > shared by multiple drivers.
> >
> > Implement the RZ/T2H-specific .post_init() callback to create and register
> > a syscon regmap covering the SYSC register space using the CPG device node.
> > For backward compatibility, return without registering the regmap when the
> > mapped resources correspond to older Device Trees that expose only the
> > legacy 64 KiB CPG register window.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > v2->v3:
> > - Dropped dangerous registers from writeable_readable list.
> > - Renamed sysc_init to post_init and updated signature to take
> >   cpg_mssr_pub struct.
> > - Created a single regmap covering both SYSC register regions
> >   instead of two separate regmaps.
> > - Updated commit message
>
> Thanks for the update!
>
> > --- a/drivers/clk/renesas/r9a09g077-cpg.c
> > +++ b/drivers/clk/renesas/r9a09g077-cpg.c
>
> > @@ -87,6 +90,19 @@ MODULE_IMPORT_NS("RZV2H_CPG");
> >  #define CPG_PLL_MON(x)         ((x) - 0x10)
> >  #define CPG_PLL_MON_LOCK       BIT(0)
> >
> > +#define RZT2H_SYSC_SIZE                        0x20000
>
> This is the size of the two SYSC regions combined (more below).
>
> > +#define RZT2H_SYSC_OFFSET              0x10000
> > +#define RZT2H_SYSC_BLOCK_MASK          BIT(16)
> > +#define RZT2H_SYSC_OFFSET_MASK         GENMASK(15, 0)
> > +#define RZT2H_SYSC_BLOCK(x)            FIELD_GET(RZT2H_SYSC_BLOCK_MASK, x)
> > +#define RZT2H_SYSC_REG_OFFSET(x)       FIELD_GET(RZT2H_SYSC_OFFSET_MASK, x)
> > +#define RZT2H_SYSC_BASE(b0, b1, x)     (RZT2H_SYSC_BLOCK(x) ? (b1) : (b0))
>
> Passing the two bases looks a bit ugly to me...
>
> > +
> > +struct r9a09g077_sysc_reg {
> > +       void __iomem *base0;
> > +       void __iomem *base1;
>
> What about combining them into an array?
>
>     void __iomem base[2];
>
> The you can do
>
>     void __iomem *base = sysc->base[RZT2H_SYSC_BLOCK(reg)];
>
> below?
>
That would be elegant, thanks.

> > +};
> > +
> >  enum rzt2h_clk_types {
> >         CLK_TYPE_RZT2H_DIV = CLK_TYPE_CUSTOM,   /* Clock with divider */
> >         CLK_TYPE_RZT2H_MUX,                     /* Clock with clock source selector */
> > @@ -875,6 +891,118 @@ r9a09g077_cpg_clk_register(struct device *dev, const struct cpg_core_clk *core,
> >         }
> >  }
> >
> > +static int r9a09g077_regmap_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > +       struct r9a09g077_sysc_reg *sysc = context;
> > +       void __iomem *base = RZT2H_SYSC_BASE(sysc->base0, sysc->base1, reg);
> > +
> > +       *val = readl(base + RZT2H_SYSC_REG_OFFSET(reg));
> > +
> > +       return 0;
> > +}
>
> > +static bool r9a09g077_writeable_readable_sysc0(struct device *dev, unsigned int reg)
> > +{
> > +       switch (reg) {
> > +       /* ELOPA/B and GTIOCSEL */
> > +       case 0x0000 ... 0x0008:
> > +       /* Encoder config */
> > +       case 0x1000 ... 0x1164:
> > +       /* PCIe config */
> > +       case 0x2000 ... 0x2024:
> > +       case 0x2030 ... 0x2054:
> > +       case 0x2060:
> > +       /* xSPI config */
> > +       case 0x3000 ... 0x300C:
> > +       case 0x3100 ... 0x310C:
> > +       /* MD_MON */
> > +       case 0x4100:
> > +       /* PRCRN */
> > +       case 0x4200:
> > +               return true;
> > +
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > +static bool r9a09g077_writeable_readable_sysc1(struct device *dev, unsigned int reg)
> > +{
> > +       switch (reg) {
> > +       /* WDTDCRm */
> > +       case 0x5100 ... 0x5114:
> > +       /* PRCRS */
> > +       case 0x6000:
> > +               return true;
> > +
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > +static bool r9a09g077_writeable_readable_sysc(struct device *dev, unsigned int reg)
> > +{
> > +       if (RZT2H_SYSC_BLOCK(reg))
> > +               return r9a09g077_writeable_readable_sysc1(dev, RZT2H_SYSC_REG_OFFSET(reg));
> > +
> > +       return r9a09g077_writeable_readable_sysc0(dev, RZT2H_SYSC_REG_OFFSET(reg));
>
> I like symmetry, though ;-)
>
> IMO the separate sysc0 and sysc1 functions don't add much value.
> What about:
>
>     offset = RZT2H_SYSC_REG_OFFSET(reg);
>     switch (RZT2H_SYSC_BLOCK(reg)) {
>     case 0:
>             switch (offset) {
>                     ...
>             }
>
>     case 1:
>             switch (offset) {
>                     ...
>             }
>     }
>
>     return false;
>
Ok, I will move it in a single function.

> > +}
> > +
> > +static int r9a09g077_post_init(struct device *dev, struct cpg_mssr_pub *pub)
> > +{
> > +       struct regmap_config *regmap_cfg __free(kfree) = kzalloc_obj(*regmap_cfg);
> > +       struct r9a09g077_sysc_reg *sysc_reg;
> > +       struct regmap *regmap;
> > +
> > +       /*
> > +        * Return early if the SYSC sizes are not as expected for backwards
> > +        * compatibility with older device trees.
> > +        */
> > +       if (pub->size0 != RZT2H_SYSC_SIZE || pub->size1 != RZT2H_SYSC_SIZE)
>
> While RZT2H_SYSC_SIZE is the correct value to use here, it is not
> correct from a semantic point of view.
>
Agreed, I will introduce a new macro for it.

/* Size of each mapped CPG/MSSR register region (base0, base1) */
#define RZT2H_CPG_REG_SIZE 0x20000


> > +               return 0;
> > +
> > +       if (!regmap_cfg)
> > +               return -ENOMEM;
> > +
> > +       sysc_reg = devm_kzalloc(dev, sizeof(*sysc_reg), GFP_KERNEL);
> > +       if (!sysc_reg)
> > +               return -ENOMEM;
> > +
> > +       /* Only allow access in the SYSC regions */
> > +       sysc_reg->base0 = pub->base0 + RZT2H_SYSC_OFFSET;
> > +       sysc_reg->base1 = pub->base1 + RZT2H_SYSC_OFFSET;
> > +
> > +       regmap_cfg->name = "rzt2h_sysc";
> > +       regmap_cfg->reg_bits = 32;
> > +       regmap_cfg->reg_stride = 4;
> > +       regmap_cfg->val_bits = 32;
> > +       regmap_cfg->fast_io = true;
> > +       regmap_cfg->max_register = RZT2H_SYSC_SIZE - 1;
>
> This is the size of the two blocks combined.
>
I will add this comment for the macro,

/* This is the size of two SYSC regions combined */
#define RZT2H_SYSC_SIZE 0x20000

> > +       regmap_cfg->readable_reg = r9a09g077_writeable_readable_sysc;
> > +       regmap_cfg->writeable_reg = r9a09g077_writeable_readable_sysc;
> > +
> > +       regmap = devm_regmap_init(dev, &r9a09g077_sys_regmap_bus, sysc_reg, regmap_cfg);
> > +       if (IS_ERR(regmap))
> > +               return PTR_ERR(regmap);
> > +
> > +       return of_syscon_register_regmap(dev->of_node, regmap);
> > +}
> > +
> >  const struct cpg_mssr_info r9a09g077_cpg_mssr_info = {
> >         /* Core Clocks */
> >         .core_clks = r9a09g077_core_clks,
> > @@ -889,4 +1017,6 @@ const struct cpg_mssr_info r9a09g077_cpg_mssr_info = {
> >
> >         .reg_layout = CLK_REG_LAYOUT_RZ_T2H,
> >         .cpg_clk_register = r9a09g077_cpg_clk_register,
> > +
> > +       .post_init = r9a09g077_post_init,
> >  };
> > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> > index 80f4403ea2ba..2987c34f2ba2 100644
> > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > @@ -1267,6 +1267,7 @@ static int __init cpg_mssr_common_init(struct device *dev,
> >  {
> >         struct cpg_mssr_priv *priv;
> >         unsigned int nclks, i;
> > +       struct resource res;
> >         int error;
> >
> >         if (info->init) {
> > @@ -1285,13 +1286,23 @@ static int __init cpg_mssr_common_init(struct device *dev,
> >         priv->dev = dev;
> >         spin_lock_init(&priv->pub.rmw_lock);
> >
> > -       priv->pub.base0 = of_iomap(np, 0);
> > +       error = of_address_to_resource(np, 0, &res);
> > +       if (error)
> > +               return error;
> > +
> > +       priv->pub.size0 = resource_size(&res);
> > +       priv->pub.base0 = ioremap(res.start, priv->pub.size0);
> >         if (!priv->pub.base0) {
> >                 error = -ENOMEM;
> >                 goto out_err;
> >         }
> >         if (info->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> > -               priv->pub.base1 = of_iomap(np, 1);
> > +               error = of_address_to_resource(np, 1, &res);
> > +               if (error)
> > +                       goto out_err;
> > +
> > +               priv->pub.size1 = resource_size(&res);
> > +               priv->pub.base1 = ioremap(res.start, priv->pub.size1);
>
> Instead of complicating the code here, r9a09g077_post_init() might as
> well call of_address_to_resource(dev->of_node) itself.
>
Ok, I will move this to post_init.

> >                 if (!priv->pub.base1) {
> >                         error = -ENOMEM;
> >                         goto out_err;
> > @@ -1414,6 +1425,9 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
> >
> >         error = cpg_mssr_reset_controller_register(priv);
> >
> > +       if (!error && info->post_init)
> > +               error = info->post_init(priv->dev, &priv->pub);
>
> No cleanup in case of error, so the regmap is considered optional, right?
>
regmap is registered with devm_regmap_init() which on failure will
call regmap_exit. For of_syscon_register_regmap() there is no cleanup
code and if cpg_mssr_probe() fails the system wouldnt boot.

Cheers,
Prabhakar