diff mbox

[RFC,BUG] arm/dts: OMAP3: set #interrupt-cells to two

Message ID 1364631689.3767.7.camel@mars (mailing list archive)
State New, archived
Headers show

Commit Message

Christoph Fritz March 30, 2013, 8:21 a.m. UTC
This patch sets gpio #interrupt-cells from a falsely acquired '1' to '2'
referring to Documentation/devicetree/bindings/gpio/gpio-omap.txt:

      The first cell is the GPIO number.
      The second cell is used to specify flags:
        bits[3:0] trigger type and level flags:
          1 = low-to-high edge triggered.
          2 = high-to-low edge triggered.
          4 = active high level-sensitive.
          8 = active low level-sensitive.

But using this trigger cell in a board specific devicetree leads to a
non-starting kernel. This is due to not yet enabled gpio-clocks while
kernel/irq/irqdomain.c tries to set this trigger-flag (from the second
interrupt-cell) to gpio-irq-controller.

 Any ideas?
---
 arch/arm/boot/dts/omap3.dtsi |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Javier Martinez Canillas March 30, 2013, 1:18 p.m. UTC | #1
On Sat, Mar 30, 2013 at 9:21 AM, Christoph Fritz
<chf.fritz@googlemail.com> wrote:
> This patch sets gpio #interrupt-cells from a falsely acquired '1' to '2'
> referring to Documentation/devicetree/bindings/gpio/gpio-omap.txt:
>
>       The first cell is the GPIO number.
>       The second cell is used to specify flags:
>         bits[3:0] trigger type and level flags:
>           1 = low-to-high edge triggered.
>           2 = high-to-low edge triggered.
>           4 = active high level-sensitive.
>           8 = active low level-sensitive.
>
> But using this trigger cell in a board specific devicetree leads to a
> non-starting kernel. This is due to not yet enabled gpio-clocks while
> kernel/irq/irqdomain.c tries to set this trigger-flag (from the second
> interrupt-cell) to gpio-irq-controller.
>
>  Any ideas?

Hi Christoph,

A call to gpio_request() to enable the GPIO bank is needed before
using a GPIO as an IRQ source, otherwise accesses to the GPIO bank
registers fails making the kernel to hang. Jon's (added as cc)
"gpio/omap: warn if bank is not enabled on setting irq type" patch [1]
fixes the issue by warning and returning -EINVAL.

This patch will make the kernel to boot but the call to request_irq()
will fail of course. For now, the only solution is to call
gpio_request() before request_irq() in your platform code or device
driver. There is an on going discussion about what's the better way to
address this but we still haven't found a good solution to this
problem, you can see the last email for this discussion here [2]

Also, even when calling gpio_request() before request_irq() this won't
work. When specifying the trigger/level flags on the second cell for
an GPIO-IRQ, this is not set on the IORESOURCE_IRQ struct resource.
The IRQ flag is set on of_irq_to_resource() but it just does:

r->flags = IORESOURCE_IRQ;

and then the call stack is irq_to_parse_and_map() ->
irq_set_irq_type() ->  __irq_set_trigger() -> chip->irq_set_type() ->
(drivers/gpio/gpio-omap.c) gpio_irq_type().

So, even when gpio_irq_type() receive the correct flags, this are not
returned neither stored on the flags member of the IORESOURCE_IRQ
struct resource that passed to the drivers in their struct
platform_device.

I have on my TODO to better investigate if this behavior is
intentional or is a bug in the interrupt core but didn't have time to
work on this yet. A relevant discussion about this is here [3].

> ---
>  arch/arm/boot/dts/omap3.dtsi |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> index 1997b41..e8e6b8f 100644
> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -99,7 +99,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 gpio2: gpio@49050000 {
> @@ -108,7 +108,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 gpio3: gpio@49052000 {
> @@ -117,7 +117,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 gpio4: gpio@49054000 {
> @@ -126,7 +126,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 gpio5: gpio@49056000 {
> @@ -135,7 +135,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 gpio6: gpio@49058000 {
> @@ -144,7 +144,7 @@
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
> -                       #interrupt-cells = <1>;
> +                       #interrupt-cells = <2>;
>                 };
>
>                 uart1: serial@4806a000 {
> --
> 1.7.10.4
>
>
>

By the way, Jon has already sent a "ARM: dts: OMAP3+: Correct gpio
#interrupts-cells property" patch [4] that changes #interrupt-cells to
<2> for all OMAP platforms.

Best regards,
Javier

[1]: https://patchwork.kernel.org/patch/2202511/
[2]: http://www.spinics.net/lists/linux-omap/msg89247.html
[3]: https://patchwork.kernel.org/patch/2194911/
[4]: https://patchwork.kernel.org/patch/2278081/
Christoph Fritz April 1, 2013, 4:41 p.m. UTC | #2
Hi Javier

On Sat, 2013-03-30 at 14:18 +0100, Javier Martinez Canillas wrote:
> A call to gpio_request() to enable the GPIO bank is needed before
> using a GPIO as an IRQ source, otherwise accesses to the GPIO bank
> registers fails making the kernel to hang.

Yes, that is exactly my problem here. I'm using the GPIO as an IRQ
source.

> Jon's (added as cc)"gpio/omap: warn if bank is not enabled on setting
> irq type" patch [1] fixes the issue by warning and returning -EINVAL.
> 
> This patch will make the kernel to boot but the call to request_irq()
> will fail of course. For now, the only solution is to call
> gpio_request() before request_irq() in your platform code or device
> driver. There is an on going discussion about what's the better way to
> address this but we still haven't found a good solution to this
> problem, you can see the last email for this discussion here [2]
> 
> Also, even when calling gpio_request() before request_irq() this won't
> work. When specifying the trigger/level flags on the second cell for
> an GPIO-IRQ, this is not set on the IORESOURCE_IRQ struct resource.
> The IRQ flag is set on of_irq_to_resource() but it just does:
> 
> r->flags = IORESOURCE_IRQ;
> 
> and then the call stack is irq_to_parse_and_map() ->
> irq_set_irq_type() ->  __irq_set_trigger() -> chip->irq_set_type() ->
> (drivers/gpio/gpio-omap.c) gpio_irq_type().
> 
> So, even when gpio_irq_type() receive the correct flags, this are not
> returned neither stored on the flags member of the IORESOURCE_IRQ
> struct resource that passed to the drivers in their struct
> platform_device.

As a quick-fix (hack) I wrote directly to the registers in gpio_probe()
to enable GPIO banks. I now geht this:

> > [    0.214630] omap_gpio_probe, 1133, CM_CLKSEL_PER 0x48005040: 0x000000ff
> > [    0.214660] omap_gpio_probe, 1136, CM_ICLKEN_PER 0x48005010: 0x0007ffff
> > [    0.214660] omap_gpio_probe, 1139, CM_FCLKEN_PER 0x48005000: 0x0007ffff

And it works for me. _But_ when I do enable regulator twl4030
(CONFIG_REGULATOR_TWL4030=y) in my config these registers get reset:

[    2.935455] smsc911x_open, 1537, CM_CLKSEL_PER 0x48005040: 0x000000ff
[    2.942291] smsc911x_open, 1540, CM_ICLKEN_PER 0x48005010: 0x00040fff
[    2.949066] smsc911x_open, 1543, CM_FCLKEN_PER 0x48005000: 0x00000000

And the IRQ source for the network chip (smsc911x) is disabled :-(

Do you have any idea how to ("quick") fix this?

 Thanks
  -- Christoph
diff mbox

Patch

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 1997b41..e8e6b8f 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -99,7 +99,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		gpio2: gpio@49050000 {
@@ -108,7 +108,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		gpio3: gpio@49052000 {
@@ -117,7 +117,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		gpio4: gpio@49054000 {
@@ -126,7 +126,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		gpio5: gpio@49056000 {
@@ -135,7 +135,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		gpio6: gpio@49058000 {
@@ -144,7 +144,7 @@ 
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
-			#interrupt-cells = <1>;
+			#interrupt-cells = <2>;
 		};
 
 		uart1: serial@4806a000 {