[v3,1/3] pinctrl: renesas: rzg2l: Include pinmap in RZG2L_GPIO_PORT_PACK() macro
Commit Message
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>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 50 +++++++++++++------------
1 file changed, 26 insertions(+), 24 deletions(-)
Comments
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-m68korg> 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
@@ -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;