[PATCH] ARM: pxa: fix lubbock interrupts handling

[PATCH] ARM: pxa: fix lubbock interrupts handling

[Robert Jarzmik]

When gpio-pxa was moved to drivers/pxa, it became a driver, and its
initialization and probing happen at postcore initcall. The lubbock code
used to install the chained lubbock interrupt handler at init_irq()
time.

The consequence of the gpio-pxa change is that the installed chained irq
handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
removing :
 - the handler
 - the falling edge detection setting of GPIO0, which revealed the
   interrupt request from the lubbock IO board.

As a fix, move the gpio0 chained handler setup to a place where we have
the guarantee that pxa_gpio_probe() was called before, so that lubbock
handler becomes the true IRQ chained handler of GPIO0, demuxing the
lubbock IO board interrupts.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
For Thomas: as a side note, I'm not very happy with this patch. What
            makes me unhappy is that I don't know how to express the
            dependency between gpio-pxa probe time and
            irq_set_chained_handler(irq, lubbock_irq_handler).

            At the moment I rely on the fact that
            lubbock_irq_device_init() is called as device initcall while
            pxa_gpio_probe() is called as postcore initcall.

            If you have a better idea I'm all ears.
---
 arch/arm/mach-pxa/lubbock.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index d8a1be6..1f138f9 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -172,9 +172,6 @@ static void __init lubbock_init_irq(void)
 					 handle_level_irq);
 		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
 	}
-
-	irq_set_chained_handler(PXA_GPIO_TO_IRQ(0), lubbock_irq_handler);
-	irq_set_irq_type(PXA_GPIO_TO_IRQ(0), IRQ_TYPE_EDGE_FALLING);
 }
 
 #ifdef CONFIG_PM
@@ -190,7 +187,13 @@ static struct syscore_ops lubbock_irq_syscore_ops = {
 
 static int __init lubbock_irq_device_init(void)
 {
+	int irq;
+
 	if (machine_is_lubbock()) {
+		irq = PXA_GPIO_TO_IRQ(0);
+		irq_set_chained_handler(irq, lubbock_irq_handler);
+		irq_set_irq_type(irq, IRQ_TYPE_EDGE_FALLING);
+
 		register_syscore_ops(&lubbock_irq_syscore_ops);
 		return 0;
 	}
-- 
2.1.0

Re: [PATCH] ARM: pxa: fix lubbock interrupts handling

[Dmitry Eremin-Solenikov]

On Thu, 27 Nov 2014 19:42:01 +0100, Robert Jarzmik wrote:
> When gpio-pxa was moved to drivers/pxa, it became a driver, and its
> initialization and probing happen at postcore initcall. The lubbock code
> used to install the chained lubbock interrupt handler at init_irq()
> time.
> 
> The consequence of the gpio-pxa change is that the installed chained irq
> handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
> removing :
>  - the handler - the falling edge detection setting of GPIO0, which
>  revealed the
>    interrupt request from the lubbock IO board.
> 
> As a fix, move the gpio0 chained handler setup to a place where we have
> the guarantee that pxa_gpio_probe() was called before, so that lubbock
> handler becomes the true IRQ chained handler of GPIO0, demuxing the
> lubbock IO board interrupts.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> For Thomas: as a side note, I'm not very happy with this patch. What
>             makes me unhappy is that I don't know how to express the
>             dependency between gpio-pxa probe time and
>             irq_set_chained_handler(irq, lubbock_irq_handler).
> 
>             At the moment I rely on the fact that
>             lubbock_irq_device_init() is called as device initcall while
>             pxa_gpio_probe() is called as postcore initcall.
> 
>             If you have a better idea I'm all ears.

What about just making a lubbock CPLD a special separate device?
Then it will have normal probe callback and a possibility to return
-EPROBE_DEFER? If only syscon (drivers/mfd/syscon.c) could support
irq generation, it would fit ideally.

-- 
With best wishes
Dmitry

Re: [PATCH] ARM: pxa: fix lubbock interrupts handling

[Thomas Gleixner]

On Thu, 27 Nov 2014, Robert Jarzmik wrote:

> When gpio-pxa was moved to drivers/pxa, it became a driver, and its
> initialization and probing happen at postcore initcall. The lubbock code
> used to install the chained lubbock interrupt handler at init_irq()
> time.
> 
> The consequence of the gpio-pxa change is that the installed chained irq
> handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(),
> removing :
>  - the handler
>  - the falling edge detection setting of GPIO0, which revealed the
>    interrupt request from the lubbock IO board.
>
> As a fix, move the gpio0 chained handler setup to a place where we have
> the guarantee that pxa_gpio_probe() was called before, so that lubbock
> handler becomes the true IRQ chained handler of GPIO0, demuxing the
> lubbock IO board interrupts.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
> For Thomas: as a side note, I'm not very happy with this patch. What
>             makes me unhappy is that I don't know how to express the
>             dependency between gpio-pxa probe time and
>             irq_set_chained_handler(irq, lubbock_irq_handler).
> 
>             At the moment I rely on the fact that
>             lubbock_irq_device_init() is called as device initcall while
>             pxa_gpio_probe() is called as postcore initcall.

Admittedly I'm confused.

So what is the relationship between installing that chained handler
and that gpio-pxa probe stuff?

And why is the GPIO0 interrupt handled from arch code rather than from
a regular driver setup, which depends on the availablity of the GPIO
driver?

Thanks,

	tglx

Re: [PATCH] ARM: pxa: fix lubbock interrupts handling

[Robert Jarzmik]

Thomas Gleixner <tglx@linutronix.de> writes:

> So what is the relationship between installing that chained handler
> and that gpio-pxa probe stuff?
The relation is in gpio-pxa probe, look at the extract of pxa_gpio_probe() :
pxa_gpio_probe()
		irq = gpio_to_irq(0);
		irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
					 handle_edge_irq);
		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
		irq_set_chained_handler(IRQ_GPIO0, pxa_gpio_demux_handler);

Now look at the extract from the former lubbock_init_irq() :
lubbock_init_irq()
		irq = PXA_GPIO_TO_IRQ(0);
		irq_set_chained_handler(irq, lubbock_irq_handler);
		irq_set_irq_type(irq, IRQ_TYPE_EDGE_FALLING);

Given that gpio_to_irq(0) = PXA_GPIO_TO_IRQ(0), see how these 2 are fighting to
install the handler, and how the resulting installed handler depends on the
order of execution of pxa_gpio_to_irq() wrt lubbock_init_irq().

> And why is the GPIO0 interrupt handled from arch code rather than from
> a regular driver setup, which depends on the availablity of the GPIO
> driver?
Ah that's a good question. Maybe the answer is that there is no driver in this
case.
When I say "no driver", it's because this interrupt is a consequence of the
IO-Board (or motherboard) wiring topology.

I think I need to add a bit of context, so pardon my crude ascii-art style, and
see in the lubbock case, we have this wiring (list of IPs not exhaustive, and
gates to mask each XXX irq not added) :

IPs on Motherboard          Gates on motherboard                       SoC

+-------------+              +-------+
|  SMC Lan    | --lan irq--- | Latch | -
+-------------+              |       |  \                      +------PXA-----+
                             |       |   \                     |              |
+-------------+              |       |                         |+----------+  |
|   UDC Vbus  | --vbus irq-- | Latch | -- NOR gate -- GPIO0 -- ||GPIO block|  |
+-------------+              |       |                line     |+----------+  |
                             |       |   /                     |              |
+-------------+              |       |  /                      +--------------+
|   SA1111    | --sa11x irq--| Latch | -
+-------------+              +-------+

The "gates on motherboard" is what lubbock.c is describing, ie. the
interconnection on the motherboard. I don't see the device/driver model fitting
to describe these gates, do you ?

Cheers.

-- 
Robert

Re: [PATCH] ARM: pxa: fix lubbock interrupts handling

[Haojian Zhuang]

On Fri, Nov 28, 2014 at 9:30 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>
>> So what is the relationship between installing that chained handler
>> and that gpio-pxa probe stuff?
> The relation is in gpio-pxa probe, look at the extract of pxa_gpio_probe() :
> pxa_gpio_probe()
>                 irq = gpio_to_irq(0);
>                 irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
>                                          handle_edge_irq);
>                 set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>                 irq_set_chained_handler(IRQ_GPIO0, pxa_gpio_demux_handler);
>
> Now look at the extract from the former lubbock_init_irq() :
> lubbock_init_irq()
>                 irq = PXA_GPIO_TO_IRQ(0);
>                 irq_set_chained_handler(irq, lubbock_irq_handler);
>                 irq_set_irq_type(irq, IRQ_TYPE_EDGE_FALLING);
>
> Given that gpio_to_irq(0) = PXA_GPIO_TO_IRQ(0), see how these 2 are fighting to
> install the handler, and how the resulting installed handler depends on the
> order of execution of pxa_gpio_to_irq() wrt lubbock_init_irq().
>
>> And why is the GPIO0 interrupt handled from arch code rather than from
>> a regular driver setup, which depends on the availablity of the GPIO
>> driver?
> Ah that's a good question. Maybe the answer is that there is no driver in this
> case.
> When I say "no driver", it's because this interrupt is a consequence of the
> IO-Board (or motherboard) wiring topology.
>
> I think I need to add a bit of context, so pardon my crude ascii-art style, and
> see in the lubbock case, we have this wiring (list of IPs not exhaustive, and
> gates to mask each XXX irq not added) :
>
> IPs on Motherboard          Gates on motherboard                       SoC
>
> +-------------+              +-------+
> |  SMC Lan    | --lan irq--- | Latch | -
> +-------------+              |       |  \                      +------PXA-----+
>                              |       |   \                     |              |
> +-------------+              |       |                         |+----------+  |
> |   UDC Vbus  | --vbus irq-- | Latch | -- NOR gate -- GPIO0 -- ||GPIO block|  |
> +-------------+              |       |                line     |+----------+  |
>                              |       |   /                     |              |
> +-------------+              |       |  /                      +--------------+
> |   SA1111    | --sa11x irq--| Latch | -
> +-------------+              +-------+
>
> The "gates on motherboard" is what lubbock.c is describing, ie. the
> interconnection on the motherboard. I don't see the device/driver model fitting
> to describe these gates, do you ?
>

I think that it's a kind of irq muxing, just like lots of PMIC (power
management IC).
We should move the lubbock board irqs to a mfd driver, and register them as
threaded irqs.

Best Regards
Haojian

Re: [PATCH] ARM: pxa: fix lubbock interrupts handling

[Robert Jarzmik]

Haojian Zhuang <haojian.zhuang@gmail.com> writes:

>
> I think that it's a kind of irq muxing, just like lots of PMIC (power
> management IC).
> We should move the lubbock board irqs to a mfd driver, and register them as
> threaded irqs.
I will have a look.

I didn't consider mfd for motherboard gates, I'll think of it, thanks for the
idea.

Cheers.

-- 
Robert