[PATCH v3 0/3] Add missing port pins for RZ/Five SoC

[PATCH v3 0/3] Add missing port pins for RZ/Five SoC

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series intends to incorporate the absent port pins P19 to P28,
which are exclusively available on the RZ/Five SoC.

Cheers,
Prabhakar

v2->v3:
* Fixed build warnings for m68k as reported by Kernel test robot.

RFC -> v2:
* Fixed review comments pointed by Geert & Biju

RFC: https://lore.kernel.org/lkml/20230630120433.49529-3-prabhakar.mahadev-lad.rj@bp.renesas.com/T/

Lad Prabhakar (3):
  pinctrl: renesas: rzg2l: Include pinmap in RZG2L_GPIO_PORT_PACK()
    macro
  pinctrl: renesas: pinctrl-rzg2l: Add the missing port pins P19 to P28
  riscv: dts: renesas: r9a07g043f: Update gpio-ranges property

 arch/riscv/boot/dts/renesas/r9a07g043f.dtsi |   4 +
 drivers/pinctrl/renesas/pinctrl-rzg2l.c     | 265 ++++++++++++++++++--
 2 files changed, 243 insertions(+), 26 deletions(-)

-- 
2.34.1

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

[Prabhakar]

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(-)

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;
-- 
2.34.1

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

[Geert Uytterhoeven]

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

-- 
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

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

[Lad, Prabhakar]

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

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

[Geert Uytterhoeven]

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

-- 
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

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@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

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

[Geert Uytterhoeven]

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

-- 
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

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

[Lad, Prabhakar]

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

[PATCH v3 2/3] pinctrl: renesas: pinctrl-rzg2l: Add the missing port pins P19 to P28

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add the missing port pins P19 to P28 for RZ/Five SoC. These additional
pins provide expanded capabilities and are exclusive to the RZ/Five SoC.

Couple of port pins have different configuration and is not identical for
the complete port so introduced struct rzg2l_variable_pin_cfg to handle
such cases and introduced PIN_CFG_VARIABLE macro. The actual pin config is
then assigned rzg2l_pinctrl_get_variable_pin_cfg().

Add an additional check in rzg2l_gpio_get_gpioint() to only allow GPIO pins
which support interrupt facility.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 215 +++++++++++++++++++++++-
 1 file changed, 213 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 94d072c8a743..083cc63c2c82 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -57,6 +57,8 @@
 #define PIN_CFG_FILCLKSEL		BIT(12)
 #define PIN_CFG_IOLH_C			BIT(13)
 #define PIN_CFG_SOFT_PS			BIT(14)
+#define PIN_CFG_VARIABLE		BIT(15)
+#define PIN_CFG_NOGPIO_INT		BIT(16)
 
 #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
 					(PIN_CFG_IOLH_##group | \
@@ -82,6 +84,11 @@
  */
 #define RZG2L_GPIO_PORT_PACK(n, a, f)	(((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \
 					 ((a) << 20) | (f))
+/*
+ * m indicates the bitmap of supported pins, a is the register index
+ * and f is pin configuration capabilities supported.
+ */
+#define RZG2L_GPIO_PORT_SPARSE_PACK(m, a, f)	(((u64)(m) << 28) | ((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))))
 
@@ -185,6 +192,18 @@ struct rzg2l_dedicated_configs {
 	u64 config;
 };
 
+/**
+ * struct rzg2l_variable_pin_cfg - pin data cfg
+ * @cfg: port pin configuration
+ * @port: port number
+ * @pin: port pin
+ */
+struct rzg2l_variable_pin_cfg {
+	u32 cfg;
+	u8 port;
+	u8 pin;
+};
+
 struct rzg2l_pinctrl_data {
 	const char * const *port_pins;
 	const u64 *port_pin_configs;
@@ -193,6 +212,8 @@ struct rzg2l_pinctrl_data {
 	unsigned int n_port_pins;
 	unsigned int n_dedicated_pins;
 	const struct rzg2l_hwcfg *hwcfg;
+	const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
+	unsigned int n_variable_pin_cfg;
 };
 
 /**
@@ -228,6 +249,158 @@ struct rzg2l_pinctrl {
 
 static const u16 available_ps[] = { 1800, 2500, 3300 };
 
+#ifdef CONFIG_RISCV
+static u64 rzg2l_pinctrl_get_variable_pin_cfg(struct rzg2l_pinctrl *pctrl,
+					      u64 pincfg,
+					      unsigned int port,
+					      u8 pin)
+{
+	unsigned int i;
+	u8 pincount;
+	u8 pinmap;
+	u32 off;
+
+	if (!pctrl->data->n_variable_pin_cfg)
+		return pincfg;
+
+	for (i = 0; i < pctrl->data->n_variable_pin_cfg; i++) {
+		if (pctrl->data->variable_pin_cfg[i].port == port &&
+		    pctrl->data->variable_pin_cfg[i].pin == pin)
+			break;
+	}
+	if (i == pctrl->data->n_variable_pin_cfg)
+		return pincfg;
+
+	pinmap = RZG2L_GPIO_PORT_GET_PINMAP(pincfg);
+	pincount = RZG2L_GPIO_PORT_GET_PINCNT(pincfg);
+	off = RZG2L_PIN_CFG_TO_PORT_OFFSET(pincfg);
+
+	if (pinmap == pincount)
+		return RZG2L_GPIO_PORT_PACK(pincount, off, pctrl->data->variable_pin_cfg[i].cfg);
+
+	return RZG2L_GPIO_PORT_SPARSE_PACK(pinmap, off, pctrl->data->variable_pin_cfg[i].cfg);
+}
+
+static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] = {
+	{
+		.port = 20,
+		.pin = 0,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 1,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 2,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+		       PIN_CFG_IEN  | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 3,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 4,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 5,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 6,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 20,
+		.pin = 7,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 23,
+		.pin = 1,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT
+	},
+	{
+		.port = 23,
+		.pin = 2,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 23,
+		.pin = 3,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 23,
+		.pin = 4,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 23,
+		.pin = 5,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 0,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 1,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 2,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 3,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 4,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_NOGPIO_INT,
+	},
+	{
+		.port = 24,
+		.pin = 5,
+		.cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+		       PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+		       PIN_CFG_NOGPIO_INT,
+	},
+};
+#endif
+
 static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
 				       u8 pin, u8 off, u8 func)
 {
@@ -1320,6 +1493,27 @@ static const u64 r9a07g043_gpio_configs[] = {
 	RZG2L_GPIO_PORT_PACK(2, 0x20, RZG2L_MPXED_PIN_FUNCS),
 	RZG2L_GPIO_PORT_PACK(4, 0x21, RZG2L_MPXED_PIN_FUNCS),
 	RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS),
+#ifdef CONFIG_RISCV
+	/* Below additional port pins (P19 - P28) are exclusively available on RZ/Five SoC only */
+	RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x06, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+				    PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+				    PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),	/* P19 */
+	RZG2L_GPIO_PORT_PACK(8, 0x07, PIN_CFG_VARIABLE),	/* P20 */
+	RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x08, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+				    PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),	/* P21 */
+	RZG2L_GPIO_PORT_PACK(4, 0x09, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
+			     PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),	/* P22 */
+	RZG2L_GPIO_PORT_SPARSE_PACK(0x3e, 0x0a, PIN_CFG_VARIABLE),	/* P23 */
+	RZG2L_GPIO_PORT_PACK(6, 0x0b, PIN_CFG_VARIABLE),	/* P24 */
+	RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x0c, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_FILONOFF |
+				    PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+				    PIN_CFG_NOGPIO_INT),	/* P25 */
+	0x0,	/* Dummy port P26 */
+	0x0,	/* Dummy port P27 */
+	RZG2L_GPIO_PORT_PACK(6, 0x0f, PIN_CFG_IOLH_A | PIN_CFG_SR | PIN_CFG_PUPD |
+			     PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
+			     PIN_CFG_NOGPIO_INT),	/* P28 */
+#endif
 };
 
 static const u64 r9a08g045_gpio_configs[] = {
@@ -1478,12 +1672,18 @@ static const struct rzg2l_dedicated_configs rzg3s_dedicated_pins[] = {
 						       PIN_CFG_IO_VMC_SD1)) },
 };
 
-static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data)
+static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl)
 {
+	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[virq];
+	const struct rzg2l_pinctrl_data *data = pctrl->data;
+	u64 *pin_data = pin_desc->drv_data;
 	unsigned int gpioint;
 	unsigned int i;
 	u32 port, bit;
 
+	if (*pin_data & PIN_CFG_NOGPIO_INT)
+		return -EINVAL;
+
 	port = virq / 8;
 	bit = virq % 8;
 
@@ -1593,7 +1793,7 @@ static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
 	unsigned long flags;
 	int gpioint, irq;
 
-	gpioint = rzg2l_gpio_get_gpioint(child, pctrl->data);
+	gpioint = rzg2l_gpio_get_gpioint(child, pctrl);
 	if (gpioint < 0)
 		return gpioint;
 
@@ -1778,6 +1978,13 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
 		if (i && !(i % RZG2L_PINS_PER_PORT))
 			j++;
 		pin_data[i] = pctrl->data->port_pin_configs[j];
+#ifdef CONFIG_RISCV
+		if (pin_data[i] & PIN_CFG_VARIABLE)
+			pin_data[i] = rzg2l_pinctrl_get_variable_pin_cfg(pctrl,
+									 pin_data[i],
+									 j,
+									 i % RZG2L_PINS_PER_PORT);
+#endif
 		pins[i].drv_data = &pin_data[i];
 	}
 
@@ -1925,6 +2132,10 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
 	.n_port_pins = ARRAY_SIZE(r9a07g043_gpio_configs) * RZG2L_PINS_PER_PORT,
 	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common),
 	.hwcfg = &rzg2l_hwcfg,
+#ifdef CONFIG_RISCV
+	.variable_pin_cfg = r9a07g043f_variable_pin_cfg,
+	.n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
+#endif
 };
 
 static struct rzg2l_pinctrl_data r9a07g044_data = {
-- 
2.34.1

Re: [PATCH v3 2/3] pinctrl: renesas: pinctrl-rzg2l: Add the missing port pins P19 to P28

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

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>
>
> Add the missing port pins P19 to P28 for RZ/Five SoC. These additional
> pins provide expanded capabilities and are exclusive to the RZ/Five SoC.
>
> Couple of port pins have different configuration and is not identical for

s/is/are/

> the complete port so introduced struct rzg2l_variable_pin_cfg to handle

introduce

> such cases and introduced PIN_CFG_VARIABLE macro. The actual pin config is

introduce the

> then assigned rzg2l_pinctrl_get_variable_pin_cfg().

assigned in

>
> Add an additional check in rzg2l_gpio_get_gpioint() to only allow GPIO pins
> which support interrupt facility.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 215 +++++++++++++++++++++++-
>  1 file changed, 213 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 94d072c8a743..083cc63c2c82 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -57,6 +57,8 @@
>  #define PIN_CFG_FILCLKSEL              BIT(12)
>  #define PIN_CFG_IOLH_C                 BIT(13)
>  #define PIN_CFG_SOFT_PS                        BIT(14)
> +#define PIN_CFG_VARIABLE               BIT(15)
> +#define PIN_CFG_NOGPIO_INT             BIT(16)

Note to self: this conflicts with "[PATCH 08/14] pinctrl: renesas:
rzg2l: Add output enable support", so the numbers need to be adapted.

https://lore.kernel.org/all/20231120070024.4079344-9-claudiu.beznea.uj@bp.renesas.com

>
>  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
>                                         (PIN_CFG_IOLH_##group | \
> @@ -82,6 +84,11 @@
>   */
>  #define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \
>                                          ((a) << 20) | (f))

I'd rather define RZG2L_GPIO_PORT_PACK() using RZG2L_GPIO_PORT_SPARSE_PACK():

 #define RZG2L_GPIO_PORT_PACK(n, a, f) \
        RZG2L_GPIO_PORT_SPARSE_PACK((1U << (n)) -1, (a), (f))


> +/*
> + * m indicates the bitmap of supported pins, a is the register index
> + * and f is pin configuration capabilities supported.
> + */
> +#define RZG2L_GPIO_PORT_SPARSE_PACK(m, a, f)   (((u64)(m) << 28) | ((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))))
>
> @@ -185,6 +192,18 @@ struct rzg2l_dedicated_configs {
>         u64 config;
>  };
>
> +/**
> + * struct rzg2l_variable_pin_cfg - pin data cfg
> + * @cfg: port pin configuration
> + * @port: port number
> + * @pin: port pin
> + */
> +struct rzg2l_variable_pin_cfg {
> +       u32 cfg;
> +       u8 port;
> +       u8 pin;

As cfg only contains the lower bits (PIN_CFG_*), I think you can fit
everything in a u32:

    u32 cfg: 20;
    u32 port: 5;
    u32 pin: 3;

> +};
> +
>  struct rzg2l_pinctrl_data {
>         const char * const *port_pins;
>         const u64 *port_pin_configs;
> @@ -193,6 +212,8 @@ struct rzg2l_pinctrl_data {
>         unsigned int n_port_pins;
>         unsigned int n_dedicated_pins;
>         const struct rzg2l_hwcfg *hwcfg;
> +       const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
> +       unsigned int n_variable_pin_cfg;
>  };
>
>  /**
> @@ -228,6 +249,158 @@ struct rzg2l_pinctrl {
>
>  static const u16 available_ps[] = { 1800, 2500, 3300 };
>
> +#ifdef CONFIG_RISCV
> +static u64 rzg2l_pinctrl_get_variable_pin_cfg(struct rzg2l_pinctrl *pctrl,
> +                                             u64 pincfg,
> +                                             unsigned int port,
> +                                             u8 pin)
> +{
> +       unsigned int i;
> +       u8 pincount;
> +       u8 pinmap;
> +       u32 off;
> +
> +       if (!pctrl->data->n_variable_pin_cfg)
> +               return pincfg;

This cannot happen (but implies a driver table bug).

> +
> +       for (i = 0; i < pctrl->data->n_variable_pin_cfg; i++) {
> +               if (pctrl->data->variable_pin_cfg[i].port == port &&
> +                   pctrl->data->variable_pin_cfg[i].pin == pin)
> +                       break;
> +       }
> +       if (i == pctrl->data->n_variable_pin_cfg)
> +               return pincfg;

My first thought was that this cannot happen either, but this function
is called for non-existent pins on sparse ports?

> +
> +       pinmap = RZG2L_GPIO_PORT_GET_PINMAP(pincfg);
> +       pincount = RZG2L_GPIO_PORT_GET_PINCNT(pincfg);
> +       off = RZG2L_PIN_CFG_TO_PORT_OFFSET(pincfg);
> +
> +       if (pinmap == pincount)

Huh?

> +               return RZG2L_GPIO_PORT_PACK(pincount, off, pctrl->data->variable_pin_cfg[i].cfg);
> +
> +       return RZG2L_GPIO_PORT_SPARSE_PACK(pinmap, off, pctrl->data->variable_pin_cfg[i].cfg);

Can't you just replace the lower bits of pincfg by
pctrl->data->variable_pin_cfg[i].cfg?

    return (pincfg & ~PIN_CFG_...) | pctrl->data->variable_pin_cfg[i].cfg;

And just move this single statement into if-condition in the for-loop
above?

> +}
> +
> +static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] = {
> +       {
> +               .port = 20,
> +               .pin = 0,
> +               .cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> +                      PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> +                      PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,

Why do all new pins have PIN_CFG_NOGPIO_INT set?
P19_1, P20_0-2, P24_5, P25_1, P28_0-4 do have bits defined in an
Interrupt Enable Control Register (ISEL)?

> +       },

> @@ -1320,6 +1493,27 @@ static const u64 r9a07g043_gpio_configs[] = {
>         RZG2L_GPIO_PORT_PACK(2, 0x20, RZG2L_MPXED_PIN_FUNCS),
>         RZG2L_GPIO_PORT_PACK(4, 0x21, RZG2L_MPXED_PIN_FUNCS),
>         RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS),
> +#ifdef CONFIG_RISCV
> +       /* Below additional port pins (P19 - P28) are exclusively available on RZ/Five SoC only */
> +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x06, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> +                                   PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> +                                   PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),  /* P19 */
> +       RZG2L_GPIO_PORT_PACK(8, 0x07, PIN_CFG_VARIABLE),        /* P20 */
> +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x08, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> +                                   PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),  /* P21 */
> +       RZG2L_GPIO_PORT_PACK(4, 0x09, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> +                            PIN_CFG_IEN | PIN_CFG_NOGPIO_INT), /* P22 */
> +       RZG2L_GPIO_PORT_SPARSE_PACK(0x3e, 0x0a, PIN_CFG_VARIABLE),      /* P23 */
> +       RZG2L_GPIO_PORT_PACK(6, 0x0b, PIN_CFG_VARIABLE),        /* P24 */
> +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x0c, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_FILONOFF |
> +                                   PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> +                                   PIN_CFG_NOGPIO_INT),        /* P25 */
> +       0x0,    /* Dummy port P26 */
> +       0x0,    /* Dummy port P27 */
> +       RZG2L_GPIO_PORT_PACK(6, 0x0f, PIN_CFG_IOLH_A | PIN_CFG_SR | PIN_CFG_PUPD |
> +                            PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> +                            PIN_CFG_NOGPIO_INT),       /* P28 */

The P28 config can be simplified to "RZG2L_MPXED_PIN_FUNCS |
PIN_CFG_NOGPIO_INT".

> +#endif
>  };
>
>  static const u64 r9a08g045_gpio_configs[] = {
> @@ -1478,12 +1672,18 @@ static const struct rzg2l_dedicated_configs rzg3s_dedicated_pins[] = {
>                                                        PIN_CFG_IO_VMC_SD1)) },
>  };
>
> -static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data)
> +static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl)
>  {
> +       const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[virq];
> +       const struct rzg2l_pinctrl_data *data = pctrl->data;
> +       u64 *pin_data = pin_desc->drv_data;
>         unsigned int gpioint;
>         unsigned int i;
>         u32 port, bit;
>
> +       if (*pin_data & PIN_CFG_NOGPIO_INT)
> +               return -EINVAL;
> +
>         port = virq / 8;
>         bit = virq % 8;

Out-of-context, you have:

        gpioint = bit;
        for (i = 0; i < port; i++)
                gpioint +=
RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[i]);

        return gpioint;

Shouldn't the for-loop skip pins with PIN_CFG_NOGPIO_INT set?

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

Re: [PATCH v3 2/3] pinctrl: renesas: pinctrl-rzg2l: Add the missing port pins P19 to P28

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Wed, Dec 6, 2023 at 2:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> 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>
> >
> > Add the missing port pins P19 to P28 for RZ/Five SoC. These additional
> > pins provide expanded capabilities and are exclusive to the RZ/Five SoC.
> >
> > Couple of port pins have different configuration and is not identical for
>
> s/is/are/
>
OK.

> > the complete port so introduced struct rzg2l_variable_pin_cfg to handle
>
> introduce
>
OK.

> > such cases and introduced PIN_CFG_VARIABLE macro. The actual pin config is
>
> introduce the
>
OK.

> > then assigned rzg2l_pinctrl_get_variable_pin_cfg().
>
> assigned in
>
OK.

> >
> > Add an additional check in rzg2l_gpio_get_gpioint() to only allow GPIO pins
> > which support interrupt facility.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 215 +++++++++++++++++++++++-
> >  1 file changed, 213 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index 94d072c8a743..083cc63c2c82 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -57,6 +57,8 @@
> >  #define PIN_CFG_FILCLKSEL              BIT(12)
> >  #define PIN_CFG_IOLH_C                 BIT(13)
> >  #define PIN_CFG_SOFT_PS                        BIT(14)
> > +#define PIN_CFG_VARIABLE               BIT(15)
> > +#define PIN_CFG_NOGPIO_INT             BIT(16)
>
> Note to self: this conflicts with "[PATCH 08/14] pinctrl: renesas:
> rzg2l: Add output enable support", so the numbers need to be adapted.
>
> https://lore.kernel.org/all/20231120070024.4079344-9-claudiu.beznea.uj@bp.renesas.com
>
I'll rebase the changes, as the patches have now merged.

> >
> >  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
> >                                         (PIN_CFG_IOLH_##group | \
> > @@ -82,6 +84,11 @@
> >   */
> >  #define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \
> >                                          ((a) << 20) | (f))
>
> I'd rather define RZG2L_GPIO_PORT_PACK() using RZG2L_GPIO_PORT_SPARSE_PACK():
>
>  #define RZG2L_GPIO_PORT_PACK(n, a, f) \
>         RZG2L_GPIO_PORT_SPARSE_PACK((1U << (n)) -1, (a), (f))
>
OK, i'll update it.

>
> > +/*
> > + * m indicates the bitmap of supported pins, a is the register index
> > + * and f is pin configuration capabilities supported.
> > + */
> > +#define RZG2L_GPIO_PORT_SPARSE_PACK(m, a, f)   (((u64)(m) << 28) | ((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))))
> >
> > @@ -185,6 +192,18 @@ struct rzg2l_dedicated_configs {
> >         u64 config;
> >  };
> >
> > +/**
> > + * struct rzg2l_variable_pin_cfg - pin data cfg
> > + * @cfg: port pin configuration
> > + * @port: port number
> > + * @pin: port pin
> > + */
> > +struct rzg2l_variable_pin_cfg {
> > +       u32 cfg;
> > +       u8 port;
> > +       u8 pin;
>
> As cfg only contains the lower bits (PIN_CFG_*), I think you can fit
> everything in a u32:
>
>     u32 cfg: 20;
>     u32 port: 5;
>     u32 pin: 3;
>
Agreed.

> > +};
> > +
> >  struct rzg2l_pinctrl_data {
> >         const char * const *port_pins;
> >         const u64 *port_pin_configs;
> > @@ -193,6 +212,8 @@ struct rzg2l_pinctrl_data {
> >         unsigned int n_port_pins;
> >         unsigned int n_dedicated_pins;
> >         const struct rzg2l_hwcfg *hwcfg;
> > +       const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
> > +       unsigned int n_variable_pin_cfg;
> >  };
> >
> >  /**
> > @@ -228,6 +249,158 @@ struct rzg2l_pinctrl {
> >
> >  static const u16 available_ps[] = { 1800, 2500, 3300 };
> >
> > +#ifdef CONFIG_RISCV
> > +static u64 rzg2l_pinctrl_get_variable_pin_cfg(struct rzg2l_pinctrl *pctrl,
> > +                                             u64 pincfg,
> > +                                             unsigned int port,
> > +                                             u8 pin)
> > +{
> > +       unsigned int i;
> > +       u8 pincount;
> > +       u8 pinmap;
> > +       u32 off;
> > +
> > +       if (!pctrl->data->n_variable_pin_cfg)
> > +               return pincfg;
>
> This cannot happen (but implies a driver table bug).
>
agreed, I will drop it.

> > +
> > +       for (i = 0; i < pctrl->data->n_variable_pin_cfg; i++) {
> > +               if (pctrl->data->variable_pin_cfg[i].port == port &&
> > +                   pctrl->data->variable_pin_cfg[i].pin == pin)
> > +                       break;
> > +       }
> > +       if (i == pctrl->data->n_variable_pin_cfg)
> > +               return pincfg;
>
> My first thought was that this cannot happen either, but this function
> is called for non-existent pins on sparse ports?
>
I will drop that.

> > +
> > +       pinmap = RZG2L_GPIO_PORT_GET_PINMAP(pincfg);
> > +       pincount = RZG2L_GPIO_PORT_GET_PINCNT(pincfg);
> > +       off = RZG2L_PIN_CFG_TO_PORT_OFFSET(pincfg);
> > +
> > +       if (pinmap == pincount)
>
> Huh?
>
Oops.

> > +               return RZG2L_GPIO_PORT_PACK(pincount, off, pctrl->data->variable_pin_cfg[i].cfg);
> > +
> > +       return RZG2L_GPIO_PORT_SPARSE_PACK(pinmap, off, pctrl->data->variable_pin_cfg[i].cfg);
>
> Can't you just replace the lower bits of pincfg by
> pctrl->data->variable_pin_cfg[i].cfg?
>
>     return (pincfg & ~PIN_CFG_...) | pctrl->data->variable_pin_cfg[i].cfg;
>
> And just move this single statement into if-condition in the for-loop
> above?
>
Agreed.

> > +}
> > +
> > +static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] = {
> > +       {
> > +               .port = 20,
> > +               .pin = 0,
> > +               .cfg = PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> > +                      PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> > +                      PIN_CFG_IEN | PIN_CFG_NOGPIO_INT,
>
> Why do all new pins have PIN_CFG_NOGPIO_INT set?
> P19_1, P20_0-2, P24_5, P25_1, P28_0-4 do have bits defined in an
> Interrupt Enable Control Register (ISEL)?
>
I have got clarification from the HW team that the ISEL bits will be
dropped for P19-P28 (and also we dont corresponding GPIOINTx for
P19-P28).

> > +       },
>
> > @@ -1320,6 +1493,27 @@ static const u64 r9a07g043_gpio_configs[] = {
> >         RZG2L_GPIO_PORT_PACK(2, 0x20, RZG2L_MPXED_PIN_FUNCS),
> >         RZG2L_GPIO_PORT_PACK(4, 0x21, RZG2L_MPXED_PIN_FUNCS),
> >         RZG2L_GPIO_PORT_PACK(6, 0x22, RZG2L_MPXED_PIN_FUNCS),
> > +#ifdef CONFIG_RISCV
> > +       /* Below additional port pins (P19 - P28) are exclusively available on RZ/Five SoC only */
> > +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x06, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> > +                                   PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> > +                                   PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),  /* P19 */
> > +       RZG2L_GPIO_PORT_PACK(8, 0x07, PIN_CFG_VARIABLE),        /* P20 */
> > +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x08, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> > +                                   PIN_CFG_IEN | PIN_CFG_NOGPIO_INT),  /* P21 */
> > +       RZG2L_GPIO_PORT_PACK(4, 0x09, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_PUPD |
> > +                            PIN_CFG_IEN | PIN_CFG_NOGPIO_INT), /* P22 */
> > +       RZG2L_GPIO_PORT_SPARSE_PACK(0x3e, 0x0a, PIN_CFG_VARIABLE),      /* P23 */
> > +       RZG2L_GPIO_PORT_PACK(6, 0x0b, PIN_CFG_VARIABLE),        /* P24 */
> > +       RZG2L_GPIO_PORT_SPARSE_PACK(0x2, 0x0c, PIN_CFG_IOLH_B | PIN_CFG_SR | PIN_CFG_FILONOFF |
> > +                                   PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> > +                                   PIN_CFG_NOGPIO_INT),        /* P25 */
> > +       0x0,    /* Dummy port P26 */
> > +       0x0,    /* Dummy port P27 */
> > +       RZG2L_GPIO_PORT_PACK(6, 0x0f, PIN_CFG_IOLH_A | PIN_CFG_SR | PIN_CFG_PUPD |
> > +                            PIN_CFG_FILONOFF | PIN_CFG_FILNUM | PIN_CFG_FILCLKSEL |
> > +                            PIN_CFG_NOGPIO_INT),       /* P28 */
>
> The P28 config can be simplified to "RZG2L_MPXED_PIN_FUNCS |
> PIN_CFG_NOGPIO_INT".
>
Agreed.

> > +#endif
> >  };
> >
> >  static const u64 r9a08g045_gpio_configs[] = {
> > @@ -1478,12 +1672,18 @@ static const struct rzg2l_dedicated_configs rzg3s_dedicated_pins[] = {
> >                                                        PIN_CFG_IO_VMC_SD1)) },
> >  };
> >
> > -static int rzg2l_gpio_get_gpioint(unsigned int virq, const struct rzg2l_pinctrl_data *data)
> > +static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl)
> >  {
> > +       const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[virq];
> > +       const struct rzg2l_pinctrl_data *data = pctrl->data;
> > +       u64 *pin_data = pin_desc->drv_data;
> >         unsigned int gpioint;
> >         unsigned int i;
> >         u32 port, bit;
> >
> > +       if (*pin_data & PIN_CFG_NOGPIO_INT)
> > +               return -EINVAL;
> > +
> >         port = virq / 8;
> >         bit = virq % 8;
>
> Out-of-context, you have:
>
>         gpioint = bit;
>         for (i = 0; i < port; i++)
>                 gpioint +=
> RZG2L_GPIO_PORT_GET_PINCNT(data->port_pin_configs[i]);
>
>         return gpioint;
>
> Shouldn't the for-loop skip pins with PIN_CFG_NOGPIO_INT set?
>
As of now P19-P28 cannot be used as interrupt pins and there is no SoC
where we can test this case (as this has to match GPIOINTx for a given
port pin). So we can ignore it for now.


> 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

[PATCH v3 3/3] riscv: dts: renesas: r9a07g043f: Update gpio-ranges property

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On RZ/Five we have additional pins compared to the RZ/G2UL SoC so update
the gpio-ranges property in RZ/Five SoC DTSI.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
index a92cfcfc021b..09ef10b39f46 100644
--- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
+++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
@@ -46,6 +46,10 @@ cpu0_intc: interrupt-controller {
 	};
 };
 
+&pinctrl {
+	gpio-ranges = <&pinctrl 0 0 232>;
+};
+
 &soc {
 	dma-noncoherent;
 	interrupt-parent = <&plic>;
-- 
2.34.1

Re: [PATCH v3 3/3] riscv: dts: renesas: r9a07g043f: Update gpio-ranges property

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

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>
>
> On RZ/Five we have additional pins compared to the RZ/G2UL SoC so update
> the gpio-ranges property in RZ/Five SoC DTSI.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> +++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> @@ -46,6 +46,10 @@ cpu0_intc: interrupt-controller {
>         };
>  };
>
> +&pinctrl {
> +       gpio-ranges = <&pinctrl 0 0 232>;
> +};
> +
>  &soc {
>         dma-noncoherent;
>         interrupt-parent = <&plic>;

I believe this has a hard dependency on the pinctrl driver changes, due to
the following check in in rzg2l_gpio_register():

    if (of_args.args[0] != 0 || of_args.args[1] != 0 ||
        of_args.args[2] != pctrl->data->n_port_pins) {
            dev_err(pctrl->dev, "gpio-ranges does not match selected SOC\n");
            return -EINVAL;
    }

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