Re: [PATCH v3 1/3] pinctrl: renesas: rzg2l: Include pinmap in RZG2L_GPIO_PORT_PACK() macro

[Lad, Prabhakar]

Hi Geert,

On Tue, Jan 2, 2024 at 10:18 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar
> <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > Currently we assume all the port pins are sequential ie always PX_0 to
> > > > PX_n (n=1..7) exist, but on RZ/Five SoC we have additional pins P19_1 to
> > > > P28_5 which have holes in them, for example only one pin on port19 is
> > > > available and that is P19_1 and not P19_0. So to handle such cases
> > > > include pinmap for each port which would indicate the pin availability
> > > > on each port. As the pincount can be calculated based on pinmap drop this
> > > > from RZG2L_GPIO_PORT_PACK() macro and update RZG2L_GPIO_PORT_GET_PINCNT()
> > > > macro.
> > > >
> > > > Previously we had a max of 7 pins on each port but on RZ/Five Port-20
> > > > has 8 pins, so move the single pin configuration to BIT(63).
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > @@ -80,15 +80,17 @@
> > > >   * n indicates number of pins in the port, a is the register index
> > > >   * and f is pin configuration capabilities supported.
> > > >   */
> > > > -#define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) << 28) | ((a) << 20) | (f))
> > > > -#define RZG2L_GPIO_PORT_GET_PINCNT(x)  (((x) & GENMASK(30, 28)) >> 28)
> > > > +#define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \
> > >
> > > The mask creation can be simplified to
> > >
> > >     ((1ULL << (n)) - 1) << 28
> > >
> > OK.
> >
> > > but see below...
> > >
> > > > +                                        ((a) << 20) | (f))
> > > > +#define RZG2L_GPIO_PORT_GET_PINMAP(x)  (((x) & GENMASK_ULL(35, 28)) >> 28)
> > > > +#define RZG2L_GPIO_PORT_GET_PINCNT(x)  (hweight8(RZG2L_GPIO_PORT_GET_PINMAP((x))))
> > >
> > > I think we've reached the point where it would be easier for the
> > > casual reviewer to #define PIN_CFG_*_MASK for all fields, and use
> > > FIELD_{PREP,GET}() to pack resp. extract values.  That would also
> > > make it more obvious which bits are in use, and how many bits are
> > > still available for future use.
> > >
> > If I use the FIELD_PREP() macro like below I get build issues as below:
> >
> > #define RZG2L_GPIO_PORT_PIN_CNT_MASK    GENMASK(31, 28)
> > #define RZG2L_GPIO_PORT_PIN_REG_MASK    GENMASK(27, 20)
> > #define RZG2L_GPIO_PORT_PIN_CFG_MASK    GENMASK(19, 0)
> > #define RZG2L_GPIO_PORT_PACK(n, a, f)
> > FIELD_PREP(RZG2L_GPIO_PORT_PIN_CNT_MASK, n) | \
> >                     FIELD_PREP(RZG2L_GPIO_PORT_PIN_REG_MASK, a) | \
> >                     FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f)
> >
> >
> > drivers/pinctrl/renesas/pinctrl-rzg2l.c:91:41: note: in expansion of
> > macro 'FIELD_PREP'
> >    91 |
> > FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f)
> >       |                                         ^~~~~~~~~~
> > drivers/pinctrl/renesas/pinctrl-rzg2l.c:1486:9: note: in expansion of
> > macro 'RZG2L_GPIO_PORT_PACK'
> >  1486 |         RZG2L_GPIO_PORT_PACK(6, 0x2a,
> > RZG3S_MPXED_PIN_FUNCS(A)),                        /* P18 */
> >       |         ^~~~~~~~~~~~~~~~~~~~
> >
> > Do you have any pointers?
>
> You left out the actual error :-(
>
Oops sorry.

> include/linux/bitfield.h:113:9: error: braced-group within expression
> allowed only inside a function
>   113 |         ({
>          \
>       |         ^
> drivers/pinctrl/renesas/pinctrl-rzg2l.c:93:39: note: in expansion of
> macro ‘FIELD_PREP’
>    93 | #define RZG2L_GPIO_PORT_PACK(n, a, f)
> FIELD_PREP(RZG2L_GPIO_PORT_PIN_CNT_MASK, n) | \
>       |                                       ^~~~~~~~~~
> drivers/pinctrl/renesas/pinctrl-rzg2l.c:1555:9: note: in expansion of
> macro ‘RZG2L_GPIO_PORT_PACK’
>  1555 |         RZG2L_GPIO_PORT_PACK(2, 0x10, RZG2L_MPXED_PIN_FUNCS),
>       |         ^~~~~~~~~~~~~~~~~~~~
>
> Using FIELD_PREP_CONST() instead makes it build.
>
Thanks for the pointer, that did the trick.

> I think we've reached the point where it would be easier for the
> casual reviewer to #define PIN_CFG_*_MASK for all fields, and use
> FIELD_{PREP,GET}() to pack resp.
To clarify, you mean to define PIN_CFG_*_MASK for all
PIN_CFG_IOLH_A..PIN_CFG_OEN macros? I ask because we dont extract the
respective CFG flags in the code.

Cheers,
Prabhakar