[PATCH v2 0/5] Add GPIO and EAVB Pinctrl support

[PATCH v2 0/5] Add GPIO and EAVB Pinctrl support

[Biju Das]

This patch series aims to add GPIO and EAVB Pinctrl support
for RZ/G1C SoC.

V1-->V2
    Add support for gpio-reserved-ranges 

Biju Das (5):
  gpio: rcar: Add GPIO hole support
  dt-bindings: gpio: rcar: Add gpio-reserved-ranges support
  ARM: dts: r8a77470: Add GPIO support
  ARM: dts: iwg23s-sbc: specify EtherAVB PHY IRQ
  ARM: dts: iwg23s-sbc: Add pinctl support for EtherAVB

 .../devicetree/bindings/gpio/renesas,gpio-rcar.txt |  3 +
 arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts          | 10 +++
 arch/arm/boot/dts/r8a77470.dtsi                    | 91 ++++++++++++++++++++++
 drivers/gpio/gpio-rcar.c                           | 30 ++++++-
 4 files changed, 133 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v2 1/5] gpio: rcar: Add GPIO hole support

[Biju Das]

GPIO hole is present in RZ/G1C SoC. Valid GPIO pins on bank3 are in the
range GP3_0 to GP3_16 and GP3_27 to GP3_29. The GPIO pins between GP3_17
to GP3_26 are unused. Add support for handling unused GPIO's.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
V1-->V2
    * Added gpio-reserved-ranges support for handling
      unused gpios.
---
 drivers/gpio/gpio-rcar.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 350390c..378c6e9 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -38,6 +38,7 @@ struct gpio_rcar_bank_info {
 	u32 edglevel;
 	u32 bothedge;
 	u32 intmsk;
+	u32 gpiomsk;
 };
 
 struct gpio_rcar_priv {
@@ -252,6 +253,9 @@ static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
 	struct gpio_rcar_priv *p = gpiochip_get_data(chip);
 	int error;
 
+	if (!gpiochip_line_is_valid(chip, offset))
+		return -EINVAL;
+
 	error = pm_runtime_get_sync(&p->pdev->dev);
 	if (error < 0)
 		return error;
@@ -313,6 +317,9 @@ static void gpio_rcar_set_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long flags;
 	u32 val, bankmask;
 
+	if (chip->valid_mask && (mask[0] & p->bank_info.gpiomsk))
+		return;
+
 	bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);
 	if (!bankmask)
 		return;
@@ -399,7 +406,8 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 	struct device_node *np = p->pdev->dev.of_node;
 	const struct gpio_rcar_info *info;
 	struct of_phandle_args args;
-	int ret;
+	int ret, len, i;
+	u32 start, count;
 
 	info = of_device_get_match_data(&p->pdev->dev);
 
@@ -414,6 +422,22 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 		*npins = RCAR_MAX_GPIO_PER_BANK;
 	}
 
+	p->bank_info.gpiomsk = 0;
+	len = of_property_count_u32_elems(np,  "gpio-reserved-ranges");
+	if (len < 0 || len % 2 != 0)
+		return 0;
+
+	for (i = 0; i < len; i += 2) {
+		of_property_read_u32_index(np, "gpio-reserved-ranges",
+					   i, &start);
+		of_property_read_u32_index(np, "gpio-reserved-ranges",
+					   i + 1, &count);
+		if (start >= *npins || start + count >= *npins)
+			continue;
+
+		p->bank_info.gpiomsk = GENMASK(start + count - 1, start);
+	}
+
 	return 0;
 }
 
@@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 	gpio_chip->owner = THIS_MODULE;
 	gpio_chip->base = -1;
 	gpio_chip->ngpio = npins;
+	gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true : false;
 
 	irq_chip = &p->irq_chip;
 	irq_chip->name = name;
@@ -551,6 +576,9 @@ static int gpio_rcar_resume(struct device *dev)
 
 	for (offset = 0; offset < p->gpio_chip.ngpio; offset++) {
 		mask = BIT(offset);
+		if (p->gpio_chip.valid_mask && (mask & p->bank_info.gpiomsk))
+			continue;
+
 		/* I/O pin */
 		if (!(p->bank_info.iointsel & mask)) {
 			if (p->bank_info.inoutsel & mask)
-- 
2.7.4

[PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges support

[Biju Das]

Update the DT bindings documentation with the optional gpio-reserved-ranges
properties.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
 Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
index 378f132..67968b5 100644
--- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
+++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
@@ -46,6 +46,9 @@ Optional properties:
     mandatory if the hardware implements a controllable functional clock for
     the GPIO instance.
 
+  - gpio-reserved-ranges: Set of tuples to specify the unused GPIOs.This
+    property indicates the start and size of the GPIOs that can't be used.
+
 Please refer to gpio.txt in this directory for details of gpio-ranges property
 and the common GPIO bindings used by client devices.
 
-- 
2.7.4

[PATCH v2 3/5] ARM: dts: r8a77470: Add GPIO support

[Biju Das]

Describe GPIO blocks in the R8A77470 device tree.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
This patch has runtime depency on the gpio driver patch.
gpioblock3 has gpio-reserved-ranges property.
---
 arch/arm/boot/dts/r8a77470.dtsi | 91 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/arch/arm/boot/dts/r8a77470.dtsi b/arch/arm/boot/dts/r8a77470.dtsi
index af65fa0..5af878b 100644
--- a/arch/arm/boot/dts/r8a77470.dtsi
+++ b/arch/arm/boot/dts/r8a77470.dtsi
@@ -61,6 +61,97 @@
 		#size-cells = <2>;
 		ranges;
 
+		gpio0: gpio@e6050000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6050000 0 0x50>;
+			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-ranges = <&pfc 0 0 23>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 912>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 912>;
+		};
+
+		gpio1: gpio@e6051000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6051000 0 0x50>;
+			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-ranges = <&pfc 0 32 23>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 911>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 911>;
+		};
+
+		gpio2: gpio@e6052000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6052000 0 0x50>;
+			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-ranges = <&pfc 0 64 32>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 910>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 910>;
+		};
+
+		gpio3: gpio@e6053000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6053000 0 0x50>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-reserved-ranges = <17 10>;
+			gpio-ranges = <&pfc 0 96 30>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 909>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 909>;
+		};
+
+		gpio4: gpio@e6054000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6054000 0 0x50>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-ranges = <&pfc 0 128 26>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 908>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 908>;
+		};
+
+		gpio5: gpio@e6055000 {
+			compatible = "renesas,gpio-r8a77470",
+				     "renesas,rcar-gen2-gpio";
+			reg = <0 0xe6055000 0 0x50>;
+			interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			gpio-ranges = <&pfc 0 160 32>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			clocks = <&cpg CPG_MOD 907>;
+			power-domains = <&sysc R8A77470_PD_ALWAYS_ON>;
+			resets = <&cpg 907>;
+		};
+
 		pfc: pin-controller@e6060000 {
 			compatible = "renesas,pfc-r8a77470";
 			reg = <0 0xe6060000 0 0x118>;
-- 
2.7.4

[PATCH v2 4/5] ARM: dts: iwg23s-sbc: specify EtherAVB PHY IRQ

[Biju Das]

Specify  EtherAVB PHY IRQ  in the board specific device tree, now that we
have GPIO support.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
V1-->V2
    * No change
    * compile time dependency on gpio dtsi patch
---
 arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
index 56182ee..22da819 100644
--- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
+++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
@@ -35,6 +35,8 @@
 
 	phy3: ethernet-phy@3 {
 		reg = <3>;
+		interrupt-parent = <&gpio5>;
+		interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
 		micrel,led-mode = <1>;
 	};
 };
-- 
2.7.4

[PATCH v2 5/5] ARM: dts: iwg23s-sbc: Add pinctl support for EtherAVB

[Biju Das]

Adding pinctrl support for EtherAVB interface.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
V1-->V2 
     * No change
     Depend onthe below patch
       https://patchwork.kernel.org/patch/10546801/
---
 arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
index 22da819..4ceff9c 100644
--- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
+++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
@@ -28,6 +28,9 @@
 };
 
 &avb {
+	pinctrl-0 = <&avb_pins>;
+	pinctrl-names = "default";
+
 	phy-handle = <&phy3>;
 	phy-mode = "gmii";
 	renesas,no-ether-link;
@@ -46,6 +49,11 @@
 };
 
 &pfc {
+	avb_pins: avb {
+		groups = "avb_mdio", "avb_gmii_tx_rx";
+		function = "avb";
+	};
+
 	scif1_pins: scif1 {
 		groups = "scif1_data_b";
 		function = "scif1";
-- 
2.7.4

Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support

[Geert Uytterhoeven]

Hi Biju,

On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> GPIO hole is present in RZ/G1C SoC. Valid GPIO pins on bank3 are in the
> range GP3_0 to GP3_16 and GP3_27 to GP3_29. The GPIO pins between GP3_17
> to GP3_26 are unused. Add support for handling unused GPIO's.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> V1-->V2
>     * Added gpio-reserved-ranges support for handling
>       unused gpios.

Thanks for the update!

> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -38,6 +38,7 @@ struct gpio_rcar_bank_info {
>         u32 edglevel;
>         u32 bothedge;
>         u32 intmsk;
> +       u32 gpiomsk;

I think you can do without adding this variable (see below).

>  };
>
>  struct gpio_rcar_priv {
> @@ -252,6 +253,9 @@ static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset)
>         struct gpio_rcar_priv *p = gpiochip_get_data(chip);
>         int error;
>
> +       if (!gpiochip_line_is_valid(chip, offset))
> +               return -EINVAL;
> +

Perhaps this check should be added to gpiod_request_commit(), so not all
drivers (currently drivers/pinctrl/qcom/pinctrl-msm.c) have to check it
themselves?

>         error = pm_runtime_get_sync(&p->pdev->dev);
>         if (error < 0)
>                 return error;
> @@ -313,6 +317,9 @@ static void gpio_rcar_set_multiple(struct gpio_chip *chip, unsigned long *mask,
>         unsigned long flags;
>         u32 val, bankmask;
>
> +       if (chip->valid_mask && (mask[0] & p->bank_info.gpiomsk))
> +               return;
> +

You can use chip->valid_mask[0] instead of p->bank_info.gpiomsk.
However,  instead of returning, I would just ignore any invalid bits set.
Bits > chip->ngpio are already ignored below...

>         bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);

... so just add:

    if (chip->valid_mask)
            bankmask &= chip->valid_mask[0];

However, if you force gpiochip->need_valid_mask = true, you can just use

    bankmask = mask[0] & chip->valid_mask[0];

unconditionally.

>         if (!bankmask)
>                 return;
> @@ -399,7 +406,8 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
>         struct device_node *np = p->pdev->dev.of_node;
>         const struct gpio_rcar_info *info;
>         struct of_phandle_args args;
> -       int ret;
> +       int ret, len, i;
> +       u32 start, count;
>
>         info = of_device_get_match_data(&p->pdev->dev);
>
> @@ -414,6 +422,22 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
>                 *npins = RCAR_MAX_GPIO_PER_BANK;
>         }
>
> +       p->bank_info.gpiomsk = 0;
> +       len = of_property_count_u32_elems(np,  "gpio-reserved-ranges");
> +       if (len < 0 || len % 2 != 0)
> +               return 0;
> +
> +       for (i = 0; i < len; i += 2) {
> +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                          i, &start);
> +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                          i + 1, &count);
> +               if (start >= *npins || start + count >= *npins)
> +                       continue;
> +
> +               p->bank_info.gpiomsk = GENMASK(start + count - 1, start);
> +       }

of_gpiochip_init_valid_mask() already does the same, for chip->valid_mask.

> +
>         return 0;
>  }
>
> @@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
>         gpio_chip->owner = THIS_MODULE;
>         gpio_chip->base = -1;
>         gpio_chip->ngpio = npins;
> +       gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true : false;

gpiochip_init_valid_mask() already sets gpio_chip->need_valid_mask to true
if "gpio-reserved-ranges" is detected.
However, setting it to true unconditionally allow to simplify
gpio_rcar_set_multiple()
and gpio_rcar_resume().

>
>         irq_chip = &p->irq_chip;
>         irq_chip->name = name;
> @@ -551,6 +576,9 @@ static int gpio_rcar_resume(struct device *dev)
>
>         for (offset = 0; offset < p->gpio_chip.ngpio; offset++) {
>                 mask = BIT(offset);
> +               if (p->gpio_chip.valid_mask && (mask & p->bank_info.gpiomsk))
> +                       continue;
> +

Just do "if (!gpiochip_line_is_valid(chip, offset)) continue;" at the
top of the loop?

However, if you force gpiochip->need_valid_mask = true, you can just replace
the  hand-coded for loop by for_each_set_bit().

>                 /* I/O pin */
>                 if (!(p->bank_info.iointsel & mask)) {
>                         if (p->bank_info.inoutsel & mask)

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 v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges support

[Geert Uytterhoeven]

On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> Update the DT bindings documentation with the optional gpio-reserved-ranges
> properties.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

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

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 v2 3/5] ARM: dts: r8a77470: Add GPIO support

[Geert Uytterhoeven]

Hi Biju,

On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> Describe GPIO blocks in the R8A77470 device tree.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Thanks for your patch!

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

> --- a/arch/arm/boot/dts/r8a77470.dtsi
> +++ b/arch/arm/boot/dts/r8a77470.dtsi

> +               gpio3: gpio@e6053000 {
> +                       compatible = "renesas,gpio-r8a77470",
> +                                    "renesas,rcar-gen2-gpio";
> +                       reg = <0 0xe6053000 0 0x50>;
> +                       interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +                       #gpio-cells = <2>;
> +                       gpio-controller;
> +                       gpio-reserved-ranges = <17 10>;
> +                       gpio-ranges = <&pfc 0 96 30>;

I would reverse the order of these two properties, but that's purely cosmetic.

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 v2 1/5] gpio: rcar: Add GPIO hole support

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support
> On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > GPIO hole is present in RZ/G1C SoC. Valid GPIO pins on bank3 are in
> > the range GP3_0 to GP3_16 and GP3_27 to GP3_29. The GPIO pins
> between
> > GP3_17 to GP3_26 are unused. Add support for handling unused GPIO's.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >         u32 intmsk;
> > +       u32 gpiomsk;
>
> I think you can do without adding this variable (see below).

 OK.

> >  };
> >
> >  struct gpio_rcar_priv {
> > @@ -252,6 +253,9 @@ static int gpio_rcar_request(struct gpio_chip *chip,
> unsigned offset)
> >         struct gpio_rcar_priv *p = gpiochip_get_data(chip);
> >         int error;
> >
> > +       if (!gpiochip_line_is_valid(chip, offset))
> > +               return -EINVAL;
> > +
>
> Perhaps this check should be added to gpiod_request_commit(), so not all
> drivers (currently drivers/pinctrl/qcom/pinctrl-msm.c) have to check it
> themselves?

Yes I agree.  Linus, what do you think?

> >         error = pm_runtime_get_sync(&p->pdev->dev);
> >         if (error < 0)
> >                 return error;
> > @@ -313,6 +317,9 @@ static void gpio_rcar_set_multiple(struct gpio_chip
> *chip, unsigned long *mask,
> >         unsigned long flags;
> >         u32 val, bankmask;
> >
> > +       if (chip->valid_mask && (mask[0] & p->bank_info.gpiomsk))
> > +               return;
> > +
>
> You can use chip->valid_mask[0] instead of p->bank_info.gpiomsk.
> However,  instead of returning, I would just ignore any invalid bits set.
> Bits > chip->ngpio are already ignored below...
>
> >         bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);
>
> ... so just add:
>
>     if (chip->valid_mask)
>             bankmask &= chip->valid_mask[0];

OK. Will do this.

> However, if you force gpiochip->need_valid_mask = true, you can just use
>
>     bankmask = mask[0] & chip->valid_mask[0];
> unconditionally.
>
> >         if (!bankmask)
> >                 return;
> > @@ -399,7 +406,8 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
> *p, unsigned int *npins)
> >         struct device_node *np = p->pdev->dev.of_node;
> >         const struct gpio_rcar_info *info;
> >         struct of_phandle_args args;
> > -       int ret;
> > +       int ret, len, i;
> > +       u32 start, count;
> >
> >         info = of_device_get_match_data(&p->pdev->dev);
> >
> > @@ -414,6 +422,22 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
> *p, unsigned int *npins)
> >                 *npins = RCAR_MAX_GPIO_PER_BANK;
> >         }
> >
> > +       p->bank_info.gpiomsk = 0;
> > +       len = of_property_count_u32_elems(np,  "gpio-reserved-ranges");
> > +       if (len < 0 || len % 2 != 0)
> > +               return 0;
> > +
> > +       for (i = 0; i < len; i += 2) {
> > +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> > +                                          i, &start);
> > +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> > +                                          i + 1, &count);
> > +               if (start >= *npins || start + count >= *npins)
> > +                       continue;
> > +
> > +               p->bank_info.gpiomsk = GENMASK(start + count - 1, start);
> > +       }
>
> of_gpiochip_init_valid_mask() already does the same, for chip->valid_mask.

Currently we are not setting   "gpio_chip.of_node" variable  in the driver.
So the below  function in gpiochip_init_valid_mask()   will return negative value and won't allocate memory for valid_mask.
size = of_property_count_u32_elems(np,  "gpio-reserved-ranges");

We need to either set  " of_node"  variable  or  parse "gpio-reserved-ranges" and set " need_valid_mask=true ;"
so that  gpiochip_init_valid_mask() will allocate the valid_mask.

> > +
> >         return 0;
> >  }
> >
> > @@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct platform_device
> *pdev)
> >         gpio_chip->owner = THIS_MODULE;
> >         gpio_chip->base = -1;
> >         gpio_chip->ngpio = npins;
> > +       gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true :
> > + false;
>
> gpiochip_init_valid_mask() already sets gpio_chip->need_valid_mask to true
> if "gpio-reserved-ranges" is detected.

The plan is  to set  "of_node"  variable in driver. So that  gpiochip_init_valid_mask() sets gpio_chip->need_valid_mask to true.
What is your opinion on this?

> However, setting it to true unconditionally allow to simplify
> gpio_rcar_set_multiple()
> and gpio_rcar_resume().
>
> >
> >         irq_chip = &p->irq_chip;
> >         irq_chip->name = name;
> > @@ -551,6 +576,9 @@ static int gpio_rcar_resume(struct device *dev)
> >
> >         for (offset = 0; offset < p->gpio_chip.ngpio; offset++) {
> >                 mask = BIT(offset);
> > +               if (p->gpio_chip.valid_mask && (mask & p->bank_info.gpiomsk))
> > +                       continue;
> > +
>
> Just do "if (!gpiochip_line_is_valid(chip, offset)) continue;" at the top of the
> loop?

Ok. Will do this.

> However, if you force gpiochip->need_valid_mask = true, you can just
> replace the  hand-coded for loop by for_each_set_bit().
>
> >                 /* I/O pin */
> >                 if (!(p->bank_info.iointsel & mask)) {
> >                         if (p->bank_info.inoutsel & mask)

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH v2 3/5] ARM: dts: r8a77470: Add GPIO support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: devicetree-owner@vger.kernel.org <devicetree-
> owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven
> Sent: 03 August 2018 10:10
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Simon Horman <horms@verge.net.au>; Magnus
> Damm <magnus.damm@gmail.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <devicetree@vger.kernel.org>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>
> Subject: Re: [PATCH v2 3/5] ARM: dts: r8a77470: Add GPIO support
>
> Hi Biju,
>
> On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > Describe GPIO blocks in the R8A77470 device tree.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/arch/arm/boot/dts/r8a77470.dtsi
> > +++ b/arch/arm/boot/dts/r8a77470.dtsi
>
> > +               gpio3: gpio@e6053000 {
> > +                       compatible = "renesas,gpio-r8a77470",
> > +                                    "renesas,rcar-gen2-gpio";
> > +                       reg = <0 0xe6053000 0 0x50>;
> > +                       interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +                       #gpio-cells = <2>;
> > +                       gpio-controller;
> > +                       gpio-reserved-ranges = <17 10>;
> > +                       gpio-ranges = <&pfc 0 96 30>;
>
> I would reverse the order of these two properties, but that's purely
> cosmetic.

Will send V3 with this change.

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH v2 3/5] ARM: dts: r8a77470: Add GPIO support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: devicetree-owner@vger.kernel.org <devicetree-
> owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven
> Sent: 03 August 2018 10:10
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Simon Horman <horms@verge.net.au>; Magnus
> Damm <magnus.damm@gmail.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <devicetree@vger.kernel.org>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>
> Subject: Re: [PATCH v2 3/5] ARM: dts: r8a77470: Add GPIO support
>
> Hi Biju,
>
> On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > Describe GPIO blocks in the R8A77470 device tree.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/arch/arm/boot/dts/r8a77470.dtsi
> > +++ b/arch/arm/boot/dts/r8a77470.dtsi
>
> > +               gpio3: gpio@e6053000 {
> > +                       compatible = "renesas,gpio-r8a77470",
> > +                                    "renesas,rcar-gen2-gpio";
> > +                       reg = <0 0xe6053000 0 0x50>;
> > +                       interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +                       #gpio-cells = <2>;
> > +                       gpio-controller;
> > +                       gpio-reserved-ranges = <17 10>;
> > +                       gpio-ranges = <&pfc 0 96 30>;
>
> I would reverse the order of these two properties, but that's purely
> cosmetic.

Will send V3 with this change.

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support

[Geert Uytterhoeven]

Hi Biju

On Fri, Aug 3, 2018 at 6:47 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support
> > On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > GPIO hole is present in RZ/G1C SoC. Valid GPIO pins on bank3 are in
> > > the range GP3_0 to GP3_16 and GP3_27 to GP3_29. The GPIO pins
> > between
> > > GP3_17 to GP3_26 are unused. Add support for handling unused GPIO's.
> > >
> > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> > > @@ -414,6 +422,22 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
> > *p, unsigned int *npins)
> > >                 *npins = RCAR_MAX_GPIO_PER_BANK;
> > >         }
> > >
> > > +       p->bank_info.gpiomsk = 0;
> > > +       len = of_property_count_u32_elems(np,  "gpio-reserved-ranges");
> > > +       if (len < 0 || len % 2 != 0)
> > > +               return 0;
> > > +
> > > +       for (i = 0; i < len; i += 2) {
> > > +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> > > +                                          i, &start);
> > > +               of_property_read_u32_index(np, "gpio-reserved-ranges",
> > > +                                          i + 1, &count);
> > > +               if (start >= *npins || start + count >= *npins)
> > > +                       continue;
> > > +
> > > +               p->bank_info.gpiomsk = GENMASK(start + count - 1, start);
> > > +       }
> >
> > of_gpiochip_init_valid_mask() already does the same, for chip->valid_mask.
>
> Currently we are not setting   "gpio_chip.of_node" variable  in the driver.
> So the below  function in gpiochip_init_valid_mask()   will return negative value and won't allocate memory for valid_mask.
> size = of_property_count_u32_elems(np,  "gpio-reserved-ranges");

Nice catch, I had missed that!

> We need to either set  " of_node"  variable  or  parse "gpio-reserved-ranges" and set " need_valid_mask=true ;"
> so that  gpiochip_init_valid_mask() will allocate the valid_mask.

Agreed.

>
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct platform_device
> > *pdev)
> > >         gpio_chip->owner = THIS_MODULE;
> > >         gpio_chip->base = -1;
> > >         gpio_chip->ngpio = npins;
> > > +       gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true :
> > > + false;
> >
> > gpiochip_init_valid_mask() already sets gpio_chip->need_valid_mask to true
> > if "gpio-reserved-ranges" is detected.
>
> The plan is  to set  "of_node"  variable in driver. So that  gpiochip_init_valid_mask() sets gpio_chip->need_valid_mask to true.
> What is your opinion on this?

Sounds fine to me.

You said on IRC that it currently crashes when "gpio-reserved-ranges" is used.
IMHO that's something that should be fixed separately, possibly for v4.19.

Looks like all other Renesas gpio drivers (some combined pinctrl/gpio), except
for gpio-em.c, also fail to set gpio_chip.of_node. Will have a closer look...

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 v2 1/5] gpio: rcar: Add GPIO hole support

[Biju Das]

HI Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 04 August 2018 10:25
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; Simon Horman <horms@verge.net.au>;
> Geert Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>
> Subject: Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support
> > > > @@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct
> > > > platform_device
> > > *pdev)
> > > >         gpio_chip->owner = THIS_MODULE;
> > > >         gpio_chip->base = -1;
> > > >         gpio_chip->ngpio = npins;
> > > > +       gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true :
> > > > + false;
> > >
> > > gpiochip_init_valid_mask() already sets gpio_chip->need_valid_mask
> > > to true if "gpio-reserved-ranges" is detected.
> >
> > The plan is  to set  "of_node"  variable in driver. So that
> gpiochip_init_valid_mask() sets gpio_chip->need_valid_mask to true.
> > What is your opinion on this?
>
> Sounds fine to me.
>
> You said on IRC that it currently crashes when "gpio-reserved-ranges" is
> used.

Yes that is correct. if we add "gpio-reserved-ranges"  on device tree without
setting  "of_node" or  " need_valid_mask"  in driver, gpiochip_init_valid_mask()
won't allocate memory for valid_mask and  bitmap_clear() in of_gpiochip_init_valid_mask()
crashes. May be we need to modify the code like this to fix the crash.

if (chip->valid_mask)
bitmap_clear(chip->valid_mask, start, count);
else
pr_warn("valid_mask is not set ");



> IMHO that's something that should be fixed separately, possibly for v4.19.
>
> Looks like all other Renesas gpio drivers (some combined pinctrl/gpio),
> except for gpio-em.c, also fail to set gpio_chip.of_node. Will have a closer
> look...

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges support

[Linus Walleij]

Hi Biju!

Thanks for the patches and sorry for slow feedback.
Luckily you have Geert to help out and I can see this is already
getting in nice shape.

On Thu, Aug 2, 2018 at 4:17 PM Biju Das <biju.das@bp.renesas.com> wrote:

> Update the DT bindings documentation with the optional gpio-reserved-ranges
> properties.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
(...)
> +  - gpio-reserved-ranges: Set of tuples to specify the unused GPIOs.This
> +    property indicates the start and size of the GPIOs that can't be used.
> +
>  Please refer to gpio.txt in this directory for details of gpio-ranges property
>  and the common GPIO bindings used by client devices.

You see it yourself if you look at the context below: just refer to the existing
documentation in gpio.txt as this is a standard bindings.

The absolutely best (IIUC) is:

gpio-ranges: See gpio.txt
gpio-reserved-ranges: See gpio.txt

Maybe you could add an extra example using the reserved ranges?

Yours,
Linus Walleij

RE: [PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges support

[Biju Das]

Hi Linus,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges
> support

> > Update the DT bindings documentation with the optional
> > gpio-reserved-ranges properties.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> (...)
> > +  - gpio-reserved-ranges: Set of tuples to specify the unused GPIOs.This
> > +    property indicates the start and size of the GPIOs that can't be used.
> > +
> >  Please refer to gpio.txt in this directory for details of gpio-ranges
> > property  and the common GPIO bindings used by client devices.
>
> You see it yourself if you look at the context below: just refer to the existing
> documentation in gpio.txt as this is a standard bindings.
>
> The absolutely best (IIUC) is:
>
> gpio-ranges: See gpio.txt
> gpio-reserved-ranges: See gpio.txt
OK. Will add this.
> Maybe you could add an extra example using the reserved ranges?
OK.  Will send an example with reserved ranges.

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

RE: [PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges support

[Biju Das]

Hi Linus,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/5] dt-bindings: gpio: rcar: Add gpio-reserved-ranges
> support

> > Update the DT bindings documentation with the optional
> > gpio-reserved-ranges properties.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> (...)
> > +  - gpio-reserved-ranges: Set of tuples to specify the unused GPIOs.This
> > +    property indicates the start and size of the GPIOs that can't be used.
> > +
> >  Please refer to gpio.txt in this directory for details of gpio-ranges
> > property  and the common GPIO bindings used by client devices.
>
> You see it yourself if you look at the context below: just refer to the existing
> documentation in gpio.txt as this is a standard bindings.
>
> The absolutely best (IIUC) is:
>
> gpio-ranges: See gpio.txt
> gpio-reserved-ranges: See gpio.txt
OK. Will add this.
> Maybe you could add an extra example using the reserved ranges?
OK.  Will send an example with reserved ranges.

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.