Re: [PATCH] pinctrl: renesas: rzg2l: Use configurable PFC mask

["Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>]

Hi Geert,

Thank you for the review.

On Fri, Jan 10, 2025 at 11:30 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Tue, Jan 7, 2025 at 11:59 AM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Make the PFC mask for the PFC_mx register configurable to address
> > differences between SoCs in the RZ/G2L family and RZ/V2H.
>
> "and the RZ/V2h family", as RZ/G3E is also affected.
>
> > On RZ/G2L family SoCs, the PFC_mx mask is `0x7` (3 bits), while on RZ/V2H
> > it is `0xf` (4 bits). The previous implementation hardcoded the PFC mask
> > as `0x7`, which caused incorrect behavior when configuring PFC registers
> > on RZ/V2H.
> >
> > To resolve this, introduce a `pfcmask` field in the `rzg2l_hwcfg` structure
> > to make the mask value configurable based on the SoC. Update PFC-related
> > logic to use `hwcfg->pfcmask` instead of a hardcoded value. Additionally,
> > update hardware configuration definitions to include the appropriate mask
> > values (`0x7` for RZ/G2L family and `0xf` for RZ/V2H).
>
> i understand this means that SD1_CD has always been broken:
>
Yea, but since the TF-A configured it we didnt see an issue.

>     arch/arm64/boot/dts/renesas/r9a09g057h44-rzv2h-evk.dts:
>         pinmux = <RZV2H_PORT_PINMUX(9, 4, 14)>; /* SD1_CD */
>
> and this should be queued as a fix, and backported to v6.12?
>
Agreed.

> > Fixes: 9bd95ac86e70 ("pinctrl: renesas: rzg2l: Add support for RZ/V2H SoC")
> > Reported-by: Hien Huynh <hien.huynh.px@xxxxxxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -159,7 +159,6 @@
> >  #define PWPR_REGWE_B           BIT(5)  /* OEN Register Write Enable, known only in RZ/V2H(P) */
> >
> >  #define PM_MASK                        0x03
> > -#define PFC_MASK               0x07
>
> What about just changing this to 0x0f instead? The bitfields are
> 4-bit wide anyway, and reserved bits on RZ/G2L are documented to be
> read as zero, and ignored when written.  Sole impact would be that
> specifying invalid functions 8-15 in an RZ/G2L DTS would no longer be
> masked by software.
>
agreed let's make PFC_MASK to 0x0f and keep the changes simple.

> >  #define IEN_MASK               0x01
> >  #define IOLH_MASK              0x03
> >  #define SR_MASK                        0x01
>
> > @@ -505,7 +507,7 @@ static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> >
> >         /* Set pin to 'Non-use (Hi-Z input protection)'  */
> >         reg = readw(pctrl->base + PM(off));
> > -       reg &= ~(PM_MASK << (pin * 2));
> > +       reg &= ~(hwcfg->pfcmask << (pin * 2));
>
> Oops, this should not be replaced?
>
Arghh!

Cheers,
Prabhakar