RE: [PATCH v3 4/8] 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: 28 April 2026 14:19
> Subject: Re: [PATCH v3 4/8] pinctrl: renesas: rzg2l: Add support for selecting power source for
> {WDT,AWO,ISO}
> 
> Hi Biju,
> 
> On Tue, 17 Mar 2026 at 11:16, 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.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -63,10 +63,18 @@
> >  #define PIN_CFG_SMT                    BIT(16) /* Schmitt-trigger input control */
> >  #define PIN_CFG_ELC                    BIT(17)
> >  #define PIN_CFG_IOLH_RZV2H             BIT(18)
> > +#define PIN_CFG_PVDD1833_OTH_AWO_POC   BIT(19) /* known on RZ/G3L only */
> > +#define PIN_CFG_PVDD1833_OTH_ISO_POC   BIT(20) /* known on RZ/G3L only */
> > +#define PIN_CFG_WDTOVF_N_POC           BIT(21) /* known on RZ/G3L only */
> >
> >  #define RZG2L_SINGLE_PIN               BIT_ULL(63)     /* Dedicated pin */
> >  #define RZG2L_VARIABLE_CFG             BIT_ULL(62)     /* Variable cfg for port pins */
> >
> > +#define PIN_CFG_OTHER_POC_MASK \
> > +                                       (PIN_CFG_PVDD1833_OTH_AWO_POC | \
> > +                                        PIN_CFG_PVDD1833_OTH_ISO_POC | \
> > +                                        PIN_CFG_WDTOVF_N_POC)
> > +
> >  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
> >                                         (PIN_CFG_IOLH_##group | \
> >                                          PIN_CFG_PUPD | \ @@ -146,6
> > +154,7 @@
> >  #define SD_CH(off, ch)         ((off) + (ch) * 4)
> >  #define ETH_POC(off, ch)       ((off) + (ch) * 4)
> >  #define QSPI                   (0x3008)
> > +#define OTHER_POC              (0x3028) /* known on RZ/G3L only */
> >
> >  #define PVDD_2500              2       /* I/O domain voltage 2.5V */
> >  #define PVDD_1800              1       /* I/O domain voltage <= 1.8V */
> > @@ -906,6 +915,12 @@ 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_PVDD1833_OTH_AWO_POC)
> > +               return OTHER_POC;
> > +       if (caps & PIN_CFG_PVDD1833_OTH_ISO_POC)
> > +               return OTHER_POC;
> > +       if (caps & PIN_CFG_WDTOVF_N_POC)
> > +               return OTHER_POC;
> 
> "if (caps & PIN_CFG_OTHER_POC_MASK)" would cover all cases at once.

OK, Will add the code like below, returning mask as mentioned in below comment.

	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;
	}

> 
> >
> >         return -EINVAL;
> >  }
> > @@ -925,6 +940,13 @@ static int rzg2l_get_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps
> >                 return pwr_reg;
> >
> >         val = readb(pctrl->base + pwr_reg);
> > +       if (pwr_reg == OTHER_POC) {
> > +               u32 poc = FIELD_GET(PIN_CFG_OTHER_POC_MASK, caps);
> > +               u8 offs = ffs(poc) - 1;
> 
> Yesterday I thought "Why not store the bit number itself?", but after a night of sleep, I had my aha-
> moment...

Hope, it did not disturb your sleep.

> While clever, it is rather fragile to rely on the three PIN_CFG_* values to match the order of the bits
> in the OTHER_POC registers.
> So please check which PIN_CFG_* flag is set instead.
> 
> Perhaps rzg2l_caps_to_pwr_reg() should return both a register offset and a bitmask, to be used with
> field_prep() and field_get()?

OK. It will be just

	if (pwr_reg == OTHER_POC)
		val = field_get(mask, val);



> 
> > +
> > +               val = (val >> offs) & 0x1;
> > +       }
> > +
> >         switch (val) {
> >         case PVDD_1800:
> >                 return 1800;
> > @@ -943,6 +965,7 @@ static int rzg2l_set_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;
> >         int pwr_reg;
> > +       u8 poc_val;
> >         u8 val;
> 
> u8 poc_val, val;

Ok,

> 
> >
> >         if (caps & PIN_CFG_SOFT_PS) {
> 
> > @@ -970,6 +993,17 @@ static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps
> >         if (pwr_reg < 0)
> >                 return pwr_reg;
> >

Since it is read modify write, I plan to use guard here
by moving power_source. Hope it is fine for you.


	pctrl->settings[pin].power_source = ps;

	guard(raw_spinlock)(&pctrl->lock);

> > +       if (pwr_reg == OTHER_POC) {
> > +               u32 poc = FIELD_GET(PIN_CFG_OTHER_POC_MASK, caps);
> > +               u8 offs = ffs(poc) - 1;
> > +
> > +               val = readb(pctrl->base + pwr_reg);
> 
> if (poc_val)
>     val += BIT(offs);

Assume '+'  is a typo, it should be '|'

> else
>     val &= ~BIT(offs);


Thanks, now the code is much simpler.

Cheers,
Biju