Re: [PATCH v2 1/4] dt-bindings: pinctrl: renesas: Document RZ/G3E SoC

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Biju,

On Thu, Dec 12, 2024 at 6:15 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Sent: 12 December 2024 16:27
> > Subject: Re: [PATCH v2 1/4] dt-bindings: pinctrl: renesas: Document RZ/G3E SoC
> >
> > On Fri, Dec 6, 2024 at 11:23 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > Add documentation for the pin controller found on the Renesas RZ/G3E
> > > (R9A09G047) SoC. The RZ/G3E PFC is similar to the RZ/V2H SoC but has
> > > more pins(P00-PS3). The port number is alpha-numeric compared to the
> > > number on the other SoCs. So add macros for alpha-numeric to number conversion.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

> > > --- a/include/dt-bindings/pinctrl/rzg2l-pinctrl.h
> > > +++ b/include/dt-bindings/pinctrl/rzg2l-pinctrl.h
> > > @@ -11,13 +11,38 @@
> > >
> > >  #define RZG2L_PINS_PER_PORT    8
> > >
> > > +#define RZG3E_P0               0
> > > +#define RZG3E_P1               1
> > > +#define RZG3E_P2               2
> > > +#define RZG3E_P3               3
> > > +#define RZG3E_P4               4
> > > +#define RZG3E_P5               5
> > > +#define RZG3E_P6               6
> > > +#define RZG3E_P7               7
> > > +#define RZG3E_P8               8
> > > +#define RZG3E_PA               9
> > > +#define RZG3E_PB               10
> > > +#define RZG3E_PC               11
> > > +#define RZG3E_PD               12
> > > +#define RZG3E_PE               13
> > > +#define RZG3E_PF               14
> > > +#define RZG3E_PG               15
> > > +#define RZG3E_PH               16
> > > +#define RZG3E_PJ               17
> > > +#define RZG3E_PK               18
> > > +#define RZG3E_PL               19
> > > +#define RZG3E_PM               20
> > > +#define RZG3E_PS               21
> >
> > This maps the discontiguous alpha-numerical port name range to a contiguous numerical range.
> > As there are corresponding holes in the register layout, I am not sure such a mapping is a good idea.
>
> If I make contiguous alpha-numerical port name range to a contiguous numerical range.
> GPIO ranges increases from 172->232. that is the reason for making exactly ports defined
> in hardware manual to contiguous numerical range.

True. We do have (smaller) gaps already, as not all ports have 8 GPIOs.

> > What if a future variant (or a future documentation update) exposes the ports in between?
>
> If a future variant or to accommodate RZ/V2H, contiguous alpha-numerical port name range
> to a contiguous numerical range will be better, if we plan to support ports as alpha
> numeric as mentioned in the hardware manual.
>
> Other option is just using numbers.
>
> Please let me know your preference
>
> 1) discontinuous alpha-numerical port name range to a contiguous numerical range.
> 2) contiguous alpha-numerical port name range to a contiguous numerical range.
> 3) Just use numbers like the one used in RZ/V2H
> Or
> 4)Any other smart way of handling this.

At the lowest level, 2 and 3 are the same solution.
I think using the numbers from the hardware manual (which match
the hardware registers indices) is the safest solution.
And the RZG3E_{PORT_PINMUX,GPIO}() macros below improve the user
experience, by retaining the actual alpha-numerical names.

BTW, have you checked the non-documented registers in the gaps, i.e.
do their values look like they are backed by hardware blocks?
I wouldn't be surprised if they do exist, and are reserved for use by
the CM33, NPU, or some other non-disclosed processing core.
Or perhaps there is a non-public variant in a package with more pins?

BTW, the sentence about IRQ0 pinmuxing on page 312 refers to P90,
which does not exist. In fact none of the referred pins can be muxed
to IRQ0 on RZ/G3E.

> > > +
> > >  /*
> > >   * Create the pin index from its bank and position numbers and store in
> > >   * the upper 16 bits the alternate function identifier
> > >   */
> > >  #define RZG2L_PORT_PINMUX(b, p, f)     ((b) * RZG2L_PINS_PER_PORT + (p) | ((f) << 16))
> > > +#define RZG3E_PORT_PINMUX(b, p, f)     RZG2L_PORT_PINMUX(RZG3E_P##b, p, f)
> > >
> > >  /* Convert a port and pin label to its global pin index */  #define
> > > RZG2L_GPIO(port, pin)  ((port) * RZG2L_PINS_PER_PORT + (pin))
> > > +#define RZG3E_GPIO(port, pin)  RZG2L_GPIO(RZG3E_P##port, pin)
> > >
> > >  #endif /* __DT_BINDINGS_RZG2L_PINCTRL_H */
> >
> > Note that I do like the clever scheme to handle alpha-numerical port names. Perhaps this should be
> > implemented for RZ/V2H, too?
> > RZG2L_GPIO(10, 2) and RZG2L_GPIO(10, 3) in r9a09g057h44-rzv2h-evk.dts do refer to PA2 and PA3.
>
> I agree, if we are taking alpha-numeric ports route, then we need to fix RZ/V2H as well.

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