Patchwork [RESEND,v2,17/33] ARM: davinci: aintc: move timer-specific irq_set_handler() out of irq.c

login
register
mail settings
Submitter Bartosz Golaszewski
Date Feb. 11, 2019, 12:25 p.m.
Message ID <20190211122606.8662-18-brgl@bgdev.pl>
Download mbox | patch
Permalink /patch/722779/
State New
Headers show

Comments

Bartosz Golaszewski - Feb. 11, 2019, 12:25 p.m.
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I've been unable to figure out exactly why, but it seems that the
IRQ_TINT1_TINT34 interrupt for timer 1 needs to be handled as a
level irq, not edge like all others.

Let's move the handler setup out of the aintc driver where it's lived
since the beginning and into the dm* SoC-specific files where it
belongs.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/dm355.c  | 8 ++++++++
 arch/arm/mach-davinci/dm365.c  | 8 ++++++++
 arch/arm/mach-davinci/dm644x.c | 8 ++++++++
 arch/arm/mach-davinci/dm646x.c | 8 ++++++++
 arch/arm/mach-davinci/irq.c    | 3 ---
 5 files changed, 32 insertions(+), 3 deletions(-)
Marc Zyngier - Feb. 11, 2019, 1 p.m.
On 11/02/2019 12:25, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> I've been unable to figure out exactly why, but it seems that the
> IRQ_TINT1_TINT34 interrupt for timer 1 needs to be handled as a
> level irq, not edge like all others.

That's probably because its comparator maintains the interrupt asserted
as long as its value is less than the counter.

Level-trigger timers are the most common thing on this side of the known
universe.

> 
> Let's move the handler setup out of the aintc driver where it's lived
> since the beginning and into the dm* SoC-specific files where it
> belongs.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/mach-davinci/dm355.c  | 8 ++++++++
>  arch/arm/mach-davinci/dm365.c  | 8 ++++++++
>  arch/arm/mach-davinci/dm644x.c | 8 ++++++++
>  arch/arm/mach-davinci/dm646x.c | 8 ++++++++
>  arch/arm/mach-davinci/irq.c    | 3 ---
>  5 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
> index c7cd765114af..a732f2ea1d9a 100644
> --- a/arch/arm/mach-davinci/dm355.c
> +++ b/arch/arm/mach-davinci/dm355.c
> @@ -15,6 +15,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/init.h>
> +#include <linux/irq.h>
>  #include <linux/irqchip/irq-davinci-aintc.h>
>  #include <linux/platform_data/edma.h>
>  #include <linux/platform_data/gpio-davinci.h>
> @@ -744,6 +745,13 @@ void __init dm355_init_time(void)
>  	psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K);
>  	dm355_psc_init(NULL, psc);
>  
> +	/*
> +	 * Nobody knows why anymore, but this interrupt has been handled as
> +	 * a level irq from the very beginning of davinci support in mainline
> +	 * linux.
> +	 */
> +	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
> +

Now, the real question is: why isn't that set as part of the
set_irq_type() callback, instead of hardcoding it in the platform?

This is exactly the kind of information that should come as part of the
DT or from the driver as one of the request_irq() flags.

It would save quite a bit of boilerplate code.

Thanks,

	M.
Sekhar Nori - Feb. 12, 2019, 5:51 a.m.
On 11/02/19 6:30 PM, Marc Zyngier wrote:
> On 11/02/2019 12:25, Bartosz Golaszewski wrote:

>> +	/*
>> +	 * Nobody knows why anymore, but this interrupt has been handled as
>> +	 * a level irq from the very beginning of davinci support in mainline
>> +	 * linux.
>> +	 */
>> +	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
>> +
> 
> Now, the real question is: why isn't that set as part of the
> set_irq_type() callback, instead of hardcoding it in the platform?
> 
> This is exactly the kind of information that should come as part of the
> DT or from the driver as one of the request_irq() flags.
> 
> It would save quite a bit of boilerplate code.

True, but I would keep this series feature/bug compatible with existing
code. Without that, tracking regression reports will be a nightmare. Its
a pretty major change already.

We can (and should) fix this, but I prefer thats done as follow-up patch
after this series is merged. That way a revert is easy.

BTW, I don't quite see which in-kernel module uses this interrupt. It
might be some out-of-tree code, or some code that since got removed from
kernel.

Thanks,
Sekhar
Marc Zyngier - Feb. 14, 2019, 10:55 a.m.
On Tue, 12 Feb 2019 05:51:11 +0000,
Sekhar Nori <nsekhar@ti.com> wrote:
> 
> On 11/02/19 6:30 PM, Marc Zyngier wrote:
> > On 11/02/2019 12:25, Bartosz Golaszewski wrote:
> 
> >> +	/*
> >> +	 * Nobody knows why anymore, but this interrupt has been handled as
> >> +	 * a level irq from the very beginning of davinci support in mainline
> >> +	 * linux.
> >> +	 */
> >> +	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
> >> +
> > 
> > Now, the real question is: why isn't that set as part of the
> > set_irq_type() callback, instead of hardcoding it in the platform?
> > 
> > This is exactly the kind of information that should come as part of the
> > DT or from the driver as one of the request_irq() flags.
> > 
> > It would save quite a bit of boilerplate code.
> 
> True, but I would keep this series feature/bug compatible with existing
> code. Without that, tracking regression reports will be a nightmare. Its
> a pretty major change already.
> 
> We can (and should) fix this, but I prefer thats done as follow-up patch
> after this series is merged. That way a revert is easy.

And I'd rather see it done already, as a preliminary to this
series. If I'm going to take 35 patches that are mostly churn, surely
we can another couple to fix something that is a real bug.

> BTW, I don't quite see which in-kernel module uses this interrupt. It
> might be some out-of-tree code, or some code that since got removed from
> kernel.

Yet another reason to fix it now.

Thanks,

	M.

Patch

diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index c7cd765114af..a732f2ea1d9a 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -15,6 +15,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/init.h>
+#include <linux/irq.h>
 #include <linux/irqchip/irq-davinci-aintc.h>
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/gpio-davinci.h>
@@ -744,6 +745,13 @@  void __init dm355_init_time(void)
 	psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K);
 	dm355_psc_init(NULL, psc);
 
+	/*
+	 * Nobody knows why anymore, but this interrupt has been handled as
+	 * a level irq from the very beginning of davinci support in mainline
+	 * linux.
+	 */
+	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
+
 	clk = clk_get(NULL, "timer0");
 
 	davinci_timer_init(clk);
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index bde3c3b94cc9..79afde34cfbb 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -19,6 +19,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/init.h>
+#include <linux/irq.h>
 #include <linux/irqchip/irq-davinci-aintc.h>
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/gpio-davinci.h>
@@ -785,6 +786,13 @@  void __init dm365_init_time(void)
 	psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K);
 	dm365_psc_init(NULL, psc);
 
+	/*
+	 * Nobody knows why anymore, but this interrupt has been handled as
+	 * a level irq from the very beginning of davinci support in mainline
+	 * linux.
+	 */
+	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
+
 	clk = clk_get(NULL, "timer0");
 
 	davinci_timer_init(clk);
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index 6d3498058283..007d979d2d64 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -14,6 +14,7 @@ 
 #include <linux/clkdev.h>
 #include <linux/dmaengine.h>
 #include <linux/init.h>
+#include <linux/irq.h>
 #include <linux/irqchip/irq-davinci-aintc.h>
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/gpio-davinci.h>
@@ -680,6 +681,13 @@  void __init dm644x_init_time(void)
 	psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K);
 	dm644x_psc_init(NULL, psc);
 
+	/*
+	 * Nobody knows why anymore, but this interrupt has been handled as
+	 * a level irq from the very beginning of davinci support in mainline
+	 * linux.
+	 */
+	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
+
 	clk = clk_get(NULL, "timer0");
 
 	davinci_timer_init(clk);
diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index a0a8b336c1a4..a643d78ad644 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -15,6 +15,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/init.h>
+#include <linux/irq.h>
 #include <linux/irqchip/irq-davinci-aintc.h>
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/gpio-davinci.h>
@@ -664,6 +665,13 @@  void __init dm646x_init_time(unsigned long ref_clk_rate,
 	psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K);
 	dm646x_psc_init(NULL, psc);
 
+	/*
+	 * Nobody knows why anymore, but this interrupt has been handled as
+	 * a level irq from the very beginning of davinci support in mainline
+	 * linux.
+	 */
+	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
+
 	clk = clk_get(NULL, "timer0");
 
 	davinci_timer_init(clk);
diff --git a/arch/arm/mach-davinci/irq.c b/arch/arm/mach-davinci/irq.c
index f5578abfc0aa..0f469c59acfb 100644
--- a/arch/arm/mach-davinci/irq.c
+++ b/arch/arm/mach-davinci/irq.c
@@ -18,8 +18,6 @@ 
 #include <asm/mach/irq.h>
 #include <asm/exception.h>
 
-#include "irqs.h"
-
 #define DAVINCI_AINTC_FIQ_REG0		0x00
 #define DAVINCI_AINTC_FIQ_REG1		0x04
 #define DAVINCI_AINTC_IRQ_REG0		0x08
@@ -146,6 +144,5 @@  void __init davinci_aintc_init(const struct davinci_aintc_config *config)
 		davinci_aintc_setup_gc(davinci_aintc_base + reg_off,
 				       irq_base + irq_off, 32);
 
-	irq_set_handler(DAVINCI_INTC_IRQ(IRQ_TINT1_TINT34), handle_level_irq);
 	set_handle_irq(davinci_aintc_handle_irq);
 }