two patches for the synposys gpio controller

two patches for the synposys gpio controller

[Sebastian Andrzej Siewior]

Hi,

I didn't manage to fully test this yet therefore they go out as RFC. The
list corruption I refer to in irq_setup_generic_chip() real, this is gone.
What I am not sure yet is if the interrupt functionally is still there…

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

two patches for the synposys gpio controller

[Sebastian Andrzej Siewior]

Hi,

I didn't manage to fully test this yet therefore they go out as RFC. The
list corruption I refer to in irq_setup_generic_chip() real, this is gone.
What I am not sure yet is if the interrupt functionally is still there…

Sebastian

[PATCH 1/3] gpio: dwapb: drop irq_setup_generic_chip()

[Sebastian Andrzej Siewior]

This looks kinda wrong I didn't manage to fully test it.
The driver calls irq_alloc_domain_generic_chips() which creates a gc and
adds it to gc_list. The driver later then calls irq_setup_generic_chip()
which also initializes the gc and adds it to the gc_list() and this
corrupts the list.
I can't find a single chip in tree which uses both functions so I think
that irq_setup_generic_chip() can be dropped.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ed5711f..4d25a06b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -260,9 +260,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	ct->regs.ack = GPIO_PORTA_EOI;
 	ct->regs.mask = GPIO_INTMASK;
 
-	irq_setup_generic_chip(irq_gc, IRQ_MSK(port->bgc.gc.ngpio),
-			IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
-
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
 
-- 
1.9.1

[PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[Sebastian Andrzej Siewior]

After the irq chip got registered the driver creates a mapping for every
gpio line. This mapping be useless if the gpio in question never is
beeing used a irq line. The mapping should be created by the core on
request.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4d25a06b..0db0b94 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -211,7 +211,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct gpio_chip *gc = &port->bgc.gc;
 	struct device_node *node =  gc->of_node;
 	struct irq_chip_generic	*irq_gc;
-	unsigned int hwirq, ngpio = gc->ngpio;
+	unsigned int ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
 	int err, irq;
 
@@ -263,9 +263,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
 
-	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
-		irq_create_mapping(gpio->domain, hwirq);
-
 	port->bgc.gc.to_irq = dwapb_gpio_to_irq;
 }
 
-- 
1.9.1

[PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

The cycloneV has three gpio controllers, each one with 29 gpios. This patch
adds the three controller with the gpio driver which is now sitting the
gpio tree.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 arch/arm/boot/dts/socfpga.dtsi | 60 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 537f1a5..c3a97e1 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -463,6 +463,66 @@
 			status = "disabled";
 		};
 
+		gpio@ff708000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff708000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio0: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 164 4>;
+			};
+		};
+
+		gpio@ff709000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff709000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio1: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 165 4>;
+			};
+		};
+
+		gpio@ff70a000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff70a000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio2: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 166 4>;
+			};
+		};
+
 		L2: l2-cache@fffef000 {
 			compatible = "arm,pl310-cache";
 			reg = <0xfffef000 0x1000>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

The cycloneV has three gpio controllers, each one with 29 gpios. This patch
adds the three controller with the gpio driver which is now sitting the
gpio tree.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/socfpga.dtsi | 60 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 537f1a5..c3a97e1 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -463,6 +463,66 @@
 			status = "disabled";
 		};
 
+		gpio@ff708000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff708000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio0: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 164 4>;
+			};
+		};
+
+		gpio@ff709000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff709000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio1: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 165 4>;
+			};
+		};
+
+		gpio@ff70a000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-gpio";
+			reg = <0xff70a000 0x1000>;
+			clocks = <&per_base_clk>;
+			status = "disabled";
+
+			gpio2: gpio-controller@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				gpio-controller;
+				#gpio-cells = <1>;
+				snps,nr-gpios = <29>;
+				reg = <0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <0 166 4>;
+			};
+		};
+
 		L2: l2-cache@fffef000 {
 			compatible = "arm,pl310-cache";
 			reg = <0xfffef000 0x1000>;
-- 
1.9.1

Re: [PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[delicious quinoa]

On Thu, Mar 20, 2014 at 2:55 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> After the irq chip got registered the driver creates a mapping for every
> gpio line. This mapping be useless if the gpio in question never is
> beeing used a irq line. The mapping should be created by the core on
> request.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpio/gpio-dwapb.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 4d25a06b..0db0b94 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -211,7 +211,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         struct gpio_chip *gc = &port->bgc.gc;
>         struct device_node *node =  gc->of_node;
>         struct irq_chip_generic *irq_gc;
> -       unsigned int hwirq, ngpio = gc->ngpio;
> +       unsigned int ngpio = gc->ngpio;
>         struct irq_chip_type *ct;
>         int err, irq;
>
> @@ -263,9 +263,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         irq_set_chained_handler(irq, dwapb_irq_handler);
>         irq_set_handler_data(irq, gpio);
>
> -       for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
> -               irq_create_mapping(gpio->domain, hwirq);
> -
>         port->bgc.gc.to_irq = dwapb_gpio_to_irq;
>  }


Hi Sebastian,

I think this functionality has changed at some point or maybe
everybody copied the same bad code over and over.  I see a lot of
legacy gpio drivers calling irq_create_mapping in their to_irq()
functions.  Linus Walleij asked me to create the mappings at probe
time.  Please see https://lkml.org/lkml/2014/2/10/154 and the
subsequent discussion in https://lkml.org/lkml/2014/2/24/232

Alan Tull
aka
delicious quinoa

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Gerhard Sittig]

On Thu, Mar 20, 2014 at 20:55 +0100, Sebastian Andrzej Siewior wrote:
> 
> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
> adds the three controller with the gpio driver which is now sitting the
> gpio tree.

The third bank should have 27 pins, only the first two have 29.

The commit message probably should not discuss which git tree a
driver currently resides in.  This is just a temporary detail
which no longer applies to the mainline source base where your
patch will end up in.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[delicious quinoa]

On Thu, Mar 20, 2014 at 2:55 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
> adds the three controller with the gpio driver which is now sitting the
> gpio tree.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/arm/boot/dts/socfpga.dtsi | 60 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 537f1a5..c3a97e1 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -463,6 +463,66 @@
>                         status = "disabled";
>                 };
>
> +               gpio@ff708000 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       compatible = "snps,dw-apb-gpio";
> +                       reg = <0xff708000 0x1000>;
> +                       clocks = <&per_base_clk>;
> +                       status = "disabled";
> +
> +                       gpio0: gpio-controller@0 {
> +                               compatible = "snps,dw-apb-gpio-port";
> +                               gpio-controller;
> +                               #gpio-cells = <1>;

#gpio-cells = <2>;

I applied this patch, fixed the gpio-cells, tested on a cyclone5 soc
devkit, and see that it breaks the interrupts.  Did you test this?

Alan

> +                               snps,nr-gpios = <29>;
> +                               reg = <0>;
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +                               interrupts = <0 164 4>;
> +                       };
> +               };
> +
> +               gpio@ff709000 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       compatible = "snps,dw-apb-gpio";
> +                       reg = <0xff709000 0x1000>;
> +                       clocks = <&per_base_clk>;
> +                       status = "disabled";
> +
> +                       gpio1: gpio-controller@0 {
> +                               compatible = "snps,dw-apb-gpio-port";
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                               snps,nr-gpios = <29>;
> +                               reg = <0>;
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +                               interrupts = <0 165 4>;
> +                       };
> +               };
> +
> +               gpio@ff70a000 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       compatible = "snps,dw-apb-gpio";
> +                       reg = <0xff70a000 0x1000>;
> +                       clocks = <&per_base_clk>;
> +                       status = "disabled";
> +
> +                       gpio2: gpio-controller@0 {
> +                               compatible = "snps,dw-apb-gpio-port";
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                               snps,nr-gpios = <29>;
> +                               reg = <0>;
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +                               interrupts = <0 166 4>;
> +                       };
> +               };
> +
>                 L2: l2-cache@fffef000 {
>                         compatible = "arm,pl310-cache";
>                         reg = <0xfffef000 0x1000>;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

On 03/21/2014 07:14 PM, delicious quinoa wrote:
>> +               gpio@ff708000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff708000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio0: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
> 
> #gpio-cells = <2>;
> 
> I applied this patch, fixed the gpio-cells, tested on a cyclone5 soc
> devkit, and see that it breaks the interrupts.  Did you test this?

No, as I said. in 0/3. Will take a look on that at somepoint next week.

> 
> Alan

Sebastian

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

On 03/21/2014 06:24 PM, Gerhard Sittig wrote:
> On Thu, Mar 20, 2014 at 20:55 +0100, Sebastian Andrzej Siewior wrote:
>>
>> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
>> adds the three controller with the gpio driver which is now sitting the
>> gpio tree.
> 
> The third bank should have 27 pins, only the first two have 29.

It is not a bank it is a complete controller. Why 27 pins? The
reference manual says that there register 0…28.

> The commit message probably should not discuss which git tree a
> driver currently resides in.  This is just a temporary detail
> which no longer applies to the mainline source base where your
> patch will end up in.

This patch adds a binding which is not in kernel. It is worth to
mention.

> 
> 
> virtually yours
> Gerhard Sittig
> 
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

On 03/21/2014 06:24 PM, Gerhard Sittig wrote:
> On Thu, Mar 20, 2014 at 20:55 +0100, Sebastian Andrzej Siewior wrote:
>>
>> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
>> adds the three controller with the gpio driver which is now sitting the
>> gpio tree.
> 
> The third bank should have 27 pins, only the first two have 29.

It is not a bank it is a complete controller. Why 27 pins? The
reference manual says that there register 0…28.

> The commit message probably should not discuss which git tree a
> driver currently resides in.  This is just a temporary detail
> which no longer applies to the mainline source base where your
> patch will end up in.

This patch adds a binding which is not in kernel. It is worth to
mention.

> 
> 
> virtually yours
> Gerhard Sittig
> 
Sebastian

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[delicious quinoa]

On Fri, Mar 21, 2014 at 2:10 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 03/21/2014 06:24 PM, Gerhard Sittig wrote:
>> On Thu, Mar 20, 2014 at 20:55 +0100, Sebastian Andrzej Siewior wrote:
>>>
>>> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
>>> adds the three controller with the gpio driver which is now sitting the
>>> gpio tree.
>>
>> The third bank should have 27 pins, only the first two have 29.
>
> It is not a bank it is a complete controller. Why 27 pins? The
> reference manual says that there register 0...28.

Hi Sebastian,

The ref manual is wrong.  It is 27.  The last two aren't pinned out.

Alan Tull
aka
delicious quinoa

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[delicious quinoa]

On Fri, Mar 21, 2014 at 1:14 PM, delicious quinoa
<delicious.quinoa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Mar 20, 2014 at 2:55 PM, Sebastian Andrzej Siewior
> <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
>> adds the three controller with the gpio driver which is now sitting the
>> gpio tree.
>>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/socfpga.dtsi | 60 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 537f1a5..c3a97e1 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -463,6 +463,66 @@
>>                         status = "disabled";
>>                 };
>>
>> +               gpio@ff708000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff708000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio0: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>
> #gpio-cells = <2>;
>
> I applied this patch, fixed the gpio-cells, tested on a cyclone5 soc
> devkit, and see that it breaks the interrupts.  Did you test this?
>
> Alan
>
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 164 4>;
>> +                       };
>> +               };
>> +
>> +               gpio@ff709000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff709000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio1: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 165 4>;
>> +                       };
>> +               };
>> +
>> +               gpio@ff70a000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff70a000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio2: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>> +                               snps,nr-gpios = <29>;

snps,nr-gpios = <27>;

As noted on other thread, gpio2 is 27 wide, despite what the
documentation says.  When I made that change and remove your other two
patches the gpios worked for me on a cyclone5 devkit board.

So if you fix the "#gpio-cells = <2>;" for all 3 gpios and fix
"snps,nr-gpios = <27>;" for gpio2, then this one patch looks good to
me.  With these changes,

Acked-by: Alan Tull <atull-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>

Alan Tull
aka
delicous quinoa


>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 166 4>;
>> +                       };
>> +               };
>> +
>>                 L2: l2-cache@fffef000 {
>>                         compatible = "arm,pl310-cache";
>>                         reg = <0xfffef000 0x1000>;
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[delicious quinoa]

On Fri, Mar 21, 2014 at 1:14 PM, delicious quinoa
<delicious.quinoa@gmail.com> wrote:
> On Thu, Mar 20, 2014 at 2:55 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>> The cycloneV has three gpio controllers, each one with 29 gpios. This patch
>> adds the three controller with the gpio driver which is now sitting the
>> gpio tree.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>  arch/arm/boot/dts/socfpga.dtsi | 60 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 537f1a5..c3a97e1 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -463,6 +463,66 @@
>>                         status = "disabled";
>>                 };
>>
>> +               gpio@ff708000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff708000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio0: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>
> #gpio-cells = <2>;
>
> I applied this patch, fixed the gpio-cells, tested on a cyclone5 soc
> devkit, and see that it breaks the interrupts.  Did you test this?
>
> Alan
>
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 164 4>;
>> +                       };
>> +               };
>> +
>> +               gpio@ff709000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff709000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio1: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>> +                               snps,nr-gpios = <29>;
>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 165 4>;
>> +                       };
>> +               };
>> +
>> +               gpio@ff70a000 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "snps,dw-apb-gpio";
>> +                       reg = <0xff70a000 0x1000>;
>> +                       clocks = <&per_base_clk>;
>> +                       status = "disabled";
>> +
>> +                       gpio2: gpio-controller@0 {
>> +                               compatible = "snps,dw-apb-gpio-port";
>> +                               gpio-controller;
>> +                               #gpio-cells = <1>;
>> +                               snps,nr-gpios = <29>;

snps,nr-gpios = <27>;

As noted on other thread, gpio2 is 27 wide, despite what the
documentation says.  When I made that change and remove your other two
patches the gpios worked for me on a cyclone5 devkit board.

So if you fix the "#gpio-cells = <2>;" for all 3 gpios and fix
"snps,nr-gpios = <27>;" for gpio2, then this one patch looks good to
me.  With these changes,

Acked-by: Alan Tull <atull@altera.com>

Alan Tull
aka
delicous quinoa


>> +                               reg = <0>;
>> +                               interrupt-controller;
>> +                               #interrupt-cells = <2>;
>> +                               interrupts = <0 166 4>;
>> +                       };
>> +               };
>> +
>>                 L2: l2-cache@fffef000 {
>>                         compatible = "arm,pl310-cache";
>>                         reg = <0xfffef000 0x1000>;
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Sebastian Andrzej Siewior]

On 03/21/2014 11:15 PM, delicious quinoa wrote:
> snps,nr-gpios = <27>;
> 
> As noted on other thread, gpio2 is 27 wide, despite what the
> documentation says.  When I made that change and remove your other two
> patches the gpios worked for me on a cyclone5 devkit board.
> 
> So if you fix the "#gpio-cells = <2>;" for all 3 gpios and fix
> "snps,nr-gpios = <27>;" for gpio2, then this one patch looks good to
> me.  With these changes,

Okay will do. And I also add a note that  the reference manual is wrong
here.

> 
> Acked-by: Alan Tull <atull@altera.com>
> 
> Alan Tull
> aka
> delicous quinoa

Sebastian

Re: [PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[Sebastian Andrzej Siewior]

On 03/20/2014 09:20 PM, delicious quinoa wrote:
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 4d25a06b..0db0b94 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -263,9 +263,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>>         irq_set_chained_handler(irq, dwapb_irq_handler);
>>         irq_set_handler_data(irq, gpio);
>>
>> -       for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
>> -               irq_create_mapping(gpio->domain, hwirq);
>> -
>>         port->bgc.gc.to_irq = dwapb_gpio_to_irq;
>>  }
> 
> 
> Hi Sebastian,
> 
> I think this functionality has changed at some point or maybe
> everybody copied the same bad code over and over.  I see a lot of
> legacy gpio drivers calling irq_create_mapping in their to_irq()
> functions.  Linus Walleij asked me to create the mappings at probe
> time.  Please see https://lkml.org/lkml/2014/2/10/154 and the
> subsequent discussion in https://lkml.org/lkml/2014/2/24/232

Linus, I don't understand why you need the mapping upfront. I looked at
those two links and you quote gpio_to_irq() which is not required.
The mapping is created once irq_of_parse_and_map() is invoked or a
platform_device is created from the DT entry. The latter creates the
warning on my 3.13 tree:

WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171
of_device_alloc+0x164/0x168()
(of_device_alloc+0x164/0x168) from [<8029fc9c>]
(of_platform_device_create_pdata+0x30/0x9c)
(of_platform_device_create_pdata+0x30/0x9c) from [<8029fde0>]
(of_platform_bus_create+0xd8/0x294)
(of_platform_bus_create+0xd8/0x294) from [<8029fe2c>]
(of_platform_bus_create+0x124/0x294)
(of_platform_bus_create+0x124/0x294) from [<8029fff8>]
(of_platform_populate+0x5c/0x9c)
(of_platform_populate+0x5c/0x9c) from [<8048d048>]
(socfpga_cyclone5_init+0x30/0x158)
(socfpga_cyclone5_init+0x30/0x158) from [<80488820>]
(customize_machine+0x20/0x40)

because at the time the device is created (struct resources is
populated) the gpio controller isn't probed yet.

With or without the upfront mapping, the struct irq is not present in
struct resource.

> 
> Alan Tull
> aka
> delicious quinoa
> 
Sebastian

Re: [PATCH 1/3] gpio: dwapb: drop irq_setup_generic_chip()

[Linus Walleij]

On Thu, Mar 20, 2014 at 8:55 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> This looks kinda wrong I didn't manage to fully test it.
> The driver calls irq_alloc_domain_generic_chips() which creates a gc and
> adds it to gc_list. The driver later then calls irq_setup_generic_chip()
> which also initializes the gc and adds it to the gc_list() and this
> corrupts the list.
> I can't find a single chip in tree which uses both functions so I think
> that irq_setup_generic_chip() can be dropped.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Jamie: comments?

Can you instead of this try to use my new generic gpiolib
irqchip helpers that I just merged to the GPIO tree?

Yours,
Linus Walleij

Re: [PATCH 1/3] gpio: dwapb: drop irq_setup_generic_chip()

[Linus Walleij]

Aha Jamie not even on the original thread. Here.

On Tue, Mar 25, 2014 at 9:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 20, 2014 at 8:55 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>
>> This looks kinda wrong I didn't manage to fully test it.
>> The driver calls irq_alloc_domain_generic_chips() which creates a gc and
>> adds it to gc_list. The driver later then calls irq_setup_generic_chip()
>> which also initializes the gc and adds it to the gc_list() and this
>> corrupts the list.
>> I can't find a single chip in tree which uses both functions so I think
>> that irq_setup_generic_chip() can be dropped.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> Jamie: comments?
>
> Can you instead of this try to use my new generic gpiolib
> irqchip helpers that I just merged to the GPIO tree?
>
> Yours,
> Linus Walleij

Re: [PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[Linus Walleij]

On Sat, Mar 22, 2014 at 1:37 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Linus, I don't understand why you need the mapping upfront.

This is because irqchips and gpiochips need to be orthogonal
APIs.

> I looked at
> those two links and you quote gpio_to_irq() which is not required.

In a *lot* of drivers it is implicitly required that gpio_to_irq()
is called first because they only call irq_create_mapping()
there. (And not in subsequent interrupt handlers etc.)

No matter what, I would worry less about that and spend
some time on using my new gpiolib helpers for
gpiochip_irqchip_add() and
gpiochip_set_chained_irqchip().

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[Sebastian Andrzej Siewior]

* Linus Walleij | 2014-03-25 21:43:49 [+0100]:

>> I looked at
>> those two links and you quote gpio_to_irq() which is not required.
>
>In a *lot* of drivers it is implicitly required that gpio_to_irq()
>is called first because they only call irq_create_mapping()
>there. (And not in subsequent interrupt handlers etc.)

so

--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -68,7 +68,7 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 						    dwapb_gpio_port, bgc);
 	struct dwapb_gpio *gpio = port->gpio;
 
-	return irq_find_mapping(gpio->domain, offset);
+	return irq_create_mapping(gpio->domain, offset);
 }
 
 static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)

would fix the problem then. That means you get the gpio number passed as
a value and you plan to use it as an irq via gpio_to_irq() and the
latter will create the interrupt mapping then.

>No matter what, I would worry less about that and spend
>some time on using my new gpiolib helpers for
>gpiochip_irqchip_add() and
>gpiochip_set_chained_irqchip().
>
>Yours,
>Linus Walleij

Sebastian

Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces

[Dinh Nguyen]

On 03/22/2014 06:28 AM, Sebastian Andrzej Siewior wrote:
> On 03/21/2014 11:15 PM, delicious quinoa wrote:
>> snps,nr-gpios = <27>;
>>
>> As noted on other thread, gpio2 is 27 wide, despite what the
>> documentation says.  When I made that change and remove your other two
>> patches the gpios worked for me on a cyclone5 devkit board.
>>
>> So if you fix the "#gpio-cells = <2>;" for all 3 gpios and fix
>> "snps,nr-gpios = <27>;" for gpio2, then this one patch looks good to
>> me.  With these changes,
>
> Okay will do. And I also add a note that  the reference manual is wrong
> here.
>

I've applied this patch to rocketboards/linux-socfpga-next/next-dt. I 
will take through the arm-soc tree for 3.16.

Dinh
>>
>> Acked-by: Alan Tull <atull@altera.com>
>>
>> Alan Tull
>> aka
>> delicous quinoa
>
> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/3] gpio: dwapb: drop irq_setup_generic_chip()

[Jamie Iles]

On Tue, Mar 25, 2014 at 09:37:50PM +0100, Linus Walleij wrote:
> Aha Jamie not even on the original thread. Here.
> 
> On Tue, Mar 25, 2014 at 9:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Mar 20, 2014 at 8:55 PM, Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> wrote:
> >
> >> This looks kinda wrong I didn't manage to fully test it.
> >> The driver calls irq_alloc_domain_generic_chips() which creates a gc and
> >> adds it to gc_list. The driver later then calls irq_setup_generic_chip()
> >> which also initializes the gc and adds it to the gc_list() and this
> >> corrupts the list.
> >> I can't find a single chip in tree which uses both functions so I think
> >> that irq_setup_generic_chip() can be dropped.
> >>
> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >
> > Jamie: comments?

I don't have anything to add to either yours or Alan's comments.  I'll 
be more than happy to review the versions with the generic helpers if 
I'm copied onto the review.

Thanks,

Jamie

Re: [PATCH 2/3] gpio: dwapb: do not create the irq mapping upfront.

[Linus Walleij]

On Tue, Mar 25, 2014 at 10:17 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> * Linus Walleij | 2014-03-25 21:43:49 [+0100]:
>
>>> I looked at
>>> those two links and you quote gpio_to_irq() which is not required.
>>
>>In a *lot* of drivers it is implicitly required that gpio_to_irq()
>>is called first because they only call irq_create_mapping()
>>there. (And not in subsequent interrupt handlers etc.)
>
> so
>
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -68,7 +68,7 @@ static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>                                                     dwapb_gpio_port, bgc);
>         struct dwapb_gpio *gpio = port->gpio;
>
> -       return irq_find_mapping(gpio->domain, offset);
> +       return irq_create_mapping(gpio->domain, offset);
>  }
>
>  static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
>
> would fix the problem then.

No.

This only gets called if some GPIO consumer decides to call
gpio_to_irq() to find out the interrupt number before using the
interrupt.

There is no requirement that consumers do that!

It is perfectly legal for a GPIO irq comsumer to just use
some hard-coded IRQ number from a board file or similar,
or to just use an IRQ from a DT or ACPI entity without any
such translation.

So the mappings must be created in probe().

Yours,
Linus Walleij