| Message ID | 20231201131551.201503-2-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add missing port pins for RZ/Five SoC | expand |
Hi Prabhakar, On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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@bp.renesas.com> 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 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. > > /* > - * BIT(31) indicates dedicated pin, p is the register index while > + * BIT(63) indicates dedicated pin, p is the register index while > * referencing to SR/IEN/IOLH/FILxx registers, b is the register bits > * (b * 8) and f is the pin configuration capabilities supported. > */ > -#define RZG2L_SINGLE_PIN BIT(31) > +#define RZG2L_SINGLE_PIN BIT_ULL(63) > #define RZG2L_SINGLE_PIN_PACK(p, b, f) (RZG2L_SINGLE_PIN | \ > ((p) << 24) | ((b) << 20) | (f)) > #define RZG2L_SINGLE_PIN_GET_BIT(x) (((x) & GENMASK(22, 20)) >> 20) Likewise. > @@ -180,12 +182,12 @@ struct rzg2l_hwcfg { > > struct rzg2l_dedicated_configs { > const char *name; > - u32 config; > + u64 config; > }; The rest LGTM. It's a pity we have to switch to 64 bits, but I'm afraid there is not much we can do about that... Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > 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@bp.renesas.com> > > 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? Cheers, Prabhakar > > > > /* > > - * BIT(31) indicates dedicated pin, p is the register index while > > + * BIT(63) indicates dedicated pin, p is the register index while > > * referencing to SR/IEN/IOLH/FILxx registers, b is the register bits > > * (b * 8) and f is the pin configuration capabilities supported. > > */ > > -#define RZG2L_SINGLE_PIN BIT(31) > > +#define RZG2L_SINGLE_PIN BIT_ULL(63) > > #define RZG2L_SINGLE_PIN_PACK(p, b, f) (RZG2L_SINGLE_PIN | \ > > ((p) << 24) | ((b) << 20) | (f)) > > #define RZG2L_SINGLE_PIN_GET_BIT(x) (((x) & GENMASK(22, 20)) >> 20) > > Likewise. > > > @@ -180,12 +182,12 @@ struct rzg2l_hwcfg { > > > > struct rzg2l_dedicated_configs { > > const char *name; > > - u32 config; > > + u64 config; > > }; > > The rest LGTM. It's a pity we have to switch to 64 bits, but I'm > afraid there is not much we can do about that... > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > 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
Hi Prabhakar, On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > 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@bp.renesas.com> > > > > 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 :-( 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. /** * FIELD_PREP_CONST() - prepare a constant bitfield element * @_mask: shifted mask defining the field's length and position * @_val: value to put in the field * * FIELD_PREP_CONST() masks and shifts up the value. The result should * be combined with other fields of the bitfield using logical OR. * * Unlike FIELD_PREP() this is a constant expression and can therefore * be used in initializers. Error checking is less comfortable for this * version, and non-constant masks cannot be used. */ Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Jan 2, 2024 at 10:18 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > 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@bp.renesas.com> > > > > > > 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
Hi Prabhakar, On Thu, Jan 4, 2024 at 4:55 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Tue, Jan 2, 2024 at 10:18 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > 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@bp.renesas.com> > > > > > > > > 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. > 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. The PIN_CFG_IOLH_A..PIN_CFG_OEN macros are single-bit definitions. I mean to #define PIN_CFG_*_MASK macros for all multi-bit fields, currently accessed using open-coded GENMASK(). You already tried: #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) As they actually share the PIN_CFG_* bit space, I'd call them: #define PIN_CFG_PIN_CNT_MASK GENMASK(31, 28) #define PIN_CFG_PIN_REG_MASK GENMASK(27, 20) #define PIN_CFG_MASK GENMASK(19, 0) Also, you already have: #define MUX_PIN_ID_MASK GENMASK(15, 0) #define MUX_FUNC_MASK GENMASK(31, 16) #define MUX_FUNC_OFFS 16 But all of #define MUX_FUNC(pinconf) (((pinconf) & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) pins[i] = value & MUX_PIN_ID_MASK; can use FIELD_GET(), removing the need for MUX_FUNC_OFFS. Also: u8 pincount = RZG2L_GPIO_PORT_GET_PINCNT(cfg); can become u8 pincount = FIELD_GET(PIN_CFG_PIN_CNT_MASK, cfg); Same for all the other macros using GENMASK(). I hope this makes it more clear what I had in mind? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On Thu, Jan 4, 2024 at 4:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Jan 4, 2024 at 4:55 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Tue, Jan 2, 2024 at 10:18 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > 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@bp.renesas.com> > > > > > > > > > > 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. > > > 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. > > The PIN_CFG_IOLH_A..PIN_CFG_OEN macros are single-bit definitions. > I mean to #define PIN_CFG_*_MASK macros for all multi-bit fields, currently > accessed using open-coded GENMASK(). > > You already tried: > > #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) > > As they actually share the PIN_CFG_* bit space, I'd call them: > > #define PIN_CFG_PIN_CNT_MASK GENMASK(31, 28) > #define PIN_CFG_PIN_REG_MASK GENMASK(27, 20) > #define PIN_CFG_MASK GENMASK(19, 0) > > Also, you already have: > > #define MUX_PIN_ID_MASK GENMASK(15, 0) > #define MUX_FUNC_MASK GENMASK(31, 16) > #define MUX_FUNC_OFFS 16 > > But all of > > #define MUX_FUNC(pinconf) (((pinconf) & MUX_FUNC_MASK) >> > MUX_FUNC_OFFS) > > pins[i] = value & MUX_PIN_ID_MASK; > > can use FIELD_GET(), removing the need for MUX_FUNC_OFFS. > > Also: > > u8 pincount = RZG2L_GPIO_PORT_GET_PINCNT(cfg); > > can become > > u8 pincount = FIELD_GET(PIN_CFG_PIN_CNT_MASK, cfg); > > Same for all the other macros using GENMASK(). > > I hope this makes it more clear what I had in mind? > Thanks! > Thanks for the detailed explanation. I'll get that sorted soon. Cheers, Prabhakar
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index aed59c53207c..94d072c8a743 100644 --- 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) | \ + ((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)))) /* - * BIT(31) indicates dedicated pin, p is the register index while + * BIT(63) indicates dedicated pin, p is the register index while * referencing to SR/IEN/IOLH/FILxx registers, b is the register bits * (b * 8) and f is the pin configuration capabilities supported. */ -#define RZG2L_SINGLE_PIN BIT(31) +#define RZG2L_SINGLE_PIN BIT_ULL(63) #define RZG2L_SINGLE_PIN_PACK(p, b, f) (RZG2L_SINGLE_PIN | \ ((p) << 24) | ((b) << 20) | (f)) #define RZG2L_SINGLE_PIN_GET_BIT(x) (((x) & GENMASK(22, 20)) >> 20) @@ -180,12 +182,12 @@ struct rzg2l_hwcfg { struct rzg2l_dedicated_configs { const char *name; - u32 config; + u64 config; }; struct rzg2l_pinctrl_data { const char * const *port_pins; - const u32 *port_pin_configs; + const u64 *port_pin_configs; unsigned int n_ports; const struct rzg2l_dedicated_configs *dedicated_pins; unsigned int n_port_pins; @@ -286,7 +288,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev, pins = group->pins; for (i = 0; i < group->num_pins; i++) { - unsigned int *pin_data = pctrl->desc.pins[pins[i]].drv_data; + u64 *pin_data = pctrl->desc.pins[pins[i]].drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u32 pin = RZG2L_PIN_ID_TO_PIN(pins[i]); @@ -536,13 +538,13 @@ static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev, } static int rzg2l_validate_gpio_pin(struct rzg2l_pinctrl *pctrl, - u32 cfg, u32 port, u8 bit) + u64 cfg, u32 port, u8 bit) { - u8 pincount = RZG2L_GPIO_PORT_GET_PINCNT(cfg); + u8 pinmap = RZG2L_GPIO_PORT_GET_PINMAP(cfg); u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg); - u32 data; + u64 data; - if (bit >= pincount || port >= pctrl->data->n_port_pins) + if (!(pinmap & BIT(bit)) || port >= pctrl->data->n_port_pins) return -EINVAL; data = pctrl->data->port_pin_configs[port]; @@ -743,7 +745,7 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, enum pin_config_param param = pinconf_to_config_param(*config); const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin]; - unsigned int *pin_data = pin->drv_data; + u64 *pin_data = pin->drv_data; unsigned int arg = 0; u32 off, cfg; int ret; @@ -840,7 +842,7 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin]; const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; struct rzg2l_pinctrl_pin_settings settings = pctrl->settings[_pin]; - unsigned int *pin_data = pin->drv_data; + u64 *pin_data = pin->drv_data; enum pin_config_param param; unsigned int i; u32 cfg, off; @@ -1044,7 +1046,7 @@ static int rzg2l_gpio_request(struct gpio_chip *chip, unsigned int offset) { struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset]; - u32 *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u32 port = RZG2L_PIN_ID_TO_PORT(offset); u8 bit = RZG2L_PIN_ID_TO_PIN(offset); @@ -1076,7 +1078,7 @@ static void rzg2l_gpio_set_direction(struct rzg2l_pinctrl *pctrl, u32 offset, bool output) { const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(offset); unsigned long flags; @@ -1097,7 +1099,7 @@ static int rzg2l_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(offset); @@ -1128,7 +1130,7 @@ static void rzg2l_gpio_set(struct gpio_chip *chip, unsigned int offset, { struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(offset); unsigned long flags; @@ -1161,7 +1163,7 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset) { struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(offset); u16 reg16; @@ -1246,7 +1248,7 @@ static const char * const rzg2l_gpio_names[] = { "P48_0", "P48_1", "P48_2", "P48_3", "P48_4", "P48_5", "P48_6", "P48_7", }; -static const u32 r9a07g044_gpio_configs[] = { +static const u64 r9a07g044_gpio_configs[] = { RZG2L_GPIO_PORT_PACK(2, 0x10, RZG2L_MPXED_PIN_FUNCS), RZG2L_GPIO_PORT_PACK(2, 0x11, RZG2L_MPXED_PIN_FUNCS), RZG2L_GPIO_PORT_PACK(2, 0x12, RZG2L_MPXED_PIN_FUNCS), @@ -1298,7 +1300,7 @@ static const u32 r9a07g044_gpio_configs[] = { RZG2L_GPIO_PORT_PACK(5, 0x40, RZG2L_MPXED_PIN_FUNCS), }; -static const u32 r9a07g043_gpio_configs[] = { +static const u64 r9a07g043_gpio_configs[] = { RZG2L_GPIO_PORT_PACK(4, 0x10, RZG2L_MPXED_PIN_FUNCS), RZG2L_GPIO_PORT_PACK(5, 0x11, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IO_VMC_ETH0)), RZG2L_GPIO_PORT_PACK(4, 0x12, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IO_VMC_ETH0)), @@ -1320,7 +1322,7 @@ static const u32 r9a07g043_gpio_configs[] = { RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS), }; -static const u32 r9a08g045_gpio_configs[] = { +static const u64 r9a08g045_gpio_configs[] = { RZG2L_GPIO_PORT_PACK(4, 0x20, RZG3S_MPXED_PIN_FUNCS(A)), /* P0 */ RZG2L_GPIO_PORT_PACK(5, 0x30, RZG2L_MPXED_ETH_PIN_FUNCS(PIN_CFG_IOLH_C | PIN_CFG_IO_VMC_ETH0)), /* P1 */ @@ -1502,7 +1504,7 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d) struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); unsigned int hwirq = irqd_to_hwirq(d); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(hwirq); unsigned long flags; @@ -1529,7 +1531,7 @@ static void rzg2l_gpio_irq_enable(struct irq_data *d) struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); unsigned int hwirq = irqd_to_hwirq(d); const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[hwirq]; - unsigned int *pin_data = pin_desc->drv_data; + u64 *pin_data = pin_desc->drv_data; u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data); u8 bit = RZG2L_PIN_ID_TO_PIN(hwirq); unsigned long flags; @@ -1748,7 +1750,7 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl) const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg; struct pinctrl_pin_desc *pins; unsigned int i, j; - u32 *pin_data; + u64 *pin_data; int ret; pctrl->desc.name = DRV_NAME;