RE: [PATCH v4 3/7] pinctrl: renesas: rzg2l: Add support for selecting power source for {WDT,AWO,ISO}

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 06 May 2026 16:39
> Subject: Re: [PATCH v4 3/7] pinctrl: renesas: rzg2l: Add support for selecting power source for
> {WDT,AWO,ISO}
> 
> Hi Biju,
> 
> On Thu, 30 Apr 2026 at 11:34, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > The RZ/G3L SoC has support for setting power source that are not
> > controlled by the following voltage control registers:
> >   - SD_CH{0,1,2}_POC, XSPI_POC, ETH{0,1}_POC, I3C_SET.POC
> >
> > Add support for selecting voltages using OTHER_POC register for
> > setting I/O domain voltage for WDT, ISO and AWO by extending
> > rzg2l_caps_to_pwr_reg() with a mask output parameter so that callers
> > callers can identify which bit(s) within OTHER_POC correspond to the
> > requested domain. Update rzg2l_get_power_source() to extract the
> > relevant bit field via field_get() when reading OTHER_POC, and update
> > rzg2l_set_power_source() to perform a read-modify-write under the
> > spinlock when writing to OTHER_POC, since multiple domains share the
> > same register.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v3->v4:
> >  * Updated commit description.
> >  * Updated rzg2l_caps_to_pwr_reg() to return mask in addition to register
> >    offset.
> >  * Dropped ffs(), using field_get() instead to get PoC offset in
> >    rzg2l_get_power_source().
> >  * Simplified rzg2l_set_power_source() by using mask from
> >    rzg2l_caps_to_pwr_reg().
> >  * Added scoped_guard() for RMW operation in rzg2l_set_power_source().
> 
> Thanks for the update!
> 
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> 
> > @@ -900,7 +909,8 @@ static void rzg2l_rmw_pin_config(struct rzg2l_pinctrl *pctrl, u32 offset,
> >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);  }
> >
> > -static int rzg2l_caps_to_pwr_reg(const struct rzg2l_register_offsets
> > *regs, u32 caps)
> > +static int rzg2l_caps_to_pwr_reg(const struct rzg2l_register_offsets *regs,
> > +                                u32 caps, u8 *mask)
> >  {
> >         if (caps & PIN_CFG_IO_VMC_SD0)
> >                 return SD_CH(regs->sd_ch, 0); @@ -912,6 +922,16 @@
> > static int rzg2l_caps_to_pwr_reg(const struct rzg2l_register_offsets *regs, u32
> >                 return ETH_POC(regs->eth_poc, 1);
> >         if (caps & PIN_CFG_IO_VMC_QSPI)
> >                 return QSPI;
> > +       if (caps & PIN_CFG_OTHER_POC_MASK) {
> > +               if (caps & PIN_CFG_PVDD1833_OTH_AWO_POC)
> > +                       *mask = BIT(0);
> > +               else if (caps & PIN_CFG_PVDD1833_OTH_ISO_POC)
> > +                       *mask = BIT(1);
> > +               else
> > +                       *mask = BIT(2);
> > +
> > +               return OTHER_POC;
> > +       }
> 
> You could always return a proper value in *mask...

OK.

> 
> >
> >         return -EINVAL;
> >  }
> > @@ -920,17 +940,20 @@ static int rzg2l_get_power_source(struct
> > rzg2l_pinctrl *pctrl, u32 pin, u32 caps  {
> >         const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> >         const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> > +       u8 val, mask;
> >         int pwr_reg;
> > -       u8 val;
> >
> >         if (caps & PIN_CFG_SOFT_PS)
> >                 return pctrl->settings[pin].power_source;
> >
> > -       pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps);
> > +       pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps, &mask);
> >         if (pwr_reg < 0)
> >                 return pwr_reg;
> >
> >         val = readb(pctrl->base + pwr_reg);
> > +       if (pwr_reg == OTHER_POC)
> 
> ... so you could drop this check...

OK.

> 
> > +               val = field_get(mask, val);
> > +
> >         switch (val) {
> >         case PVDD_1800:
> >                 return 1800;
> 
> > @@ -958,25 +981,37 @@ static int rzg2l_set_power_source(struct
> > rzg2l_pinctrl *pctrl, u32 pin, u32 caps
> >
> >         switch (ps) {
> >         case 1800:
> > -               val = PVDD_1800;
> > +               poc_val = PVDD_1800;
> >                 break;
> >         case 2500:
> >                 if (!(caps & (PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1)))
> >                         return -EINVAL;
> > -               val = PVDD_2500;
> > +               poc_val = PVDD_2500;
> >                 break;
> >         case 3300:
> > -               val = PVDD_3300;
> > +               poc_val = PVDD_3300;
> >                 break;
> >         default:
> >                 return -EINVAL;
> >         }
> >
> > -       pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps);
> > +       pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps, &mask);
> >         if (pwr_reg < 0)
> >                 return pwr_reg;
> >
> > -       writeb(val, pctrl->base + pwr_reg);
> > +       if (pwr_reg == OTHER_POC) {
> > +               scoped_guard(raw_spinlock, &pctrl->lock) {
> > +                       val = readb(pctrl->base + pwr_reg);
> > +                       if (poc_val)
> > +                               val |= mask;
> > +                       else
> > +                               val &= ~mask;
> > +                       writeb(val, pctrl->base + pwr_reg);
> > +               }
> > +       } else {
> > +               writeb(poc_val, pctrl->base + pwr_reg);
> > +       }
> 
> ... but that would mean you would always do an RMW-cycle, while other SoCs have just one field, and
> don't need RMW.


OK.

> 
> > +
> >         pctrl->settings[pin].power_source = ps;
> >
> >         return 0;
> 
> We can always revisit this later, if the need arises for more variants, so

Agreed. Will revisit this later.

Cheers,
Biju

> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> i.e. will queue in renesas-pinctrl for v7.2.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But when I'm talking to
> journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds