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

[Geert Uytterhoeven]

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...

>
>         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...

> +               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.

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

We can always revisit this later, if the need arises for more variants, so
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