irq-bcm2836: Avoid "Invalid trigger warning"

RFC PATCH New Stefan Wahren 3 participants 7 messages Nov 16, 2017 – Nov 17, 2017

Subsystems (3)

irqchip drivers Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture

lore

Share & tools

lore

Stefan Wahren

author

2017-11-16 06:53 UTC #0 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

From: Phil Elwell <[email protected]>

Initialise the level for each IRQ to avoid a warning from the arm arch timer code:

arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).

Signed-off-by Phil Elwell <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-bcm2836.c +1 -1

index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
 static void

Signature · 1 line

2.7.4

Marc Zyngier

2017-11-16 08:57 UTC #1 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Quoted 20 lines · Stefan Wahren

From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +0 -0
index 667b9e1..abc9b40 100644

drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110

Quoted 8 lines

drivers/irqchip/irq-bcm2836.c +1 -1

@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
 static void

Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...

Thanks,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Stefan Wahren

author

2017-11-16 17:34 UTC #2 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Hi Phil,

Quoted 31 lines

Marc Zyngier <[email protected]> hat am 16. November 2017 um 09:57 geschrieben:
Quoted 27 lines · Stefan Wahren
From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +1 -1
index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 

Quoted 4 lines

drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110
Quoted 1 line
drivers/irqchip/irq-bcm2836.c +0 -0
 static void
Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...

i don't know. Could you please answer?

I'm only interested to get the rid of this ugly warning ...

and the right fix ;-)

Stefan

Quoted 11 lines

Thanks,

M.

Signature · 6 lines

Jazz is not dead, it just smell funny.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Phil Elwell

2017-11-16 17:53 UTC #3 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Hi Stefan,

Quoted 38 lines · Stefan Wahren

Hi Phil,
Quoted 31 lines
Marc Zyngier <[email protected]> hat am 16. November 2017 um 09:57 geschrieben:
Quoted 27 lines · Stefan Wahren
From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +1 -1
index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
Quoted 4 lines
drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110
Quoted 1 line
drivers/irqchip/irq-bcm2836.c +0 -0
 static void
Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...

Quoted 5 lines

i don't know. Could you please answer?
I'm only interested to get the rid of this ugly warning ...
and the right fix ;-)

That was my motivation too, with a preference for the smallest change that had the desired effect.

The warning message comes from the arch_timer code, which only cares about the interrupts it has been configured to use. Since these are all per-cpu interrupts, the state of the global interrupts is irrelevant.

Phil

Marc Zyngier

2017-11-16 18:16 UTC #4 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Quoted 45 lines · Phil Elwell

Hi Stefan,
Quoted 42 lines · Stefan Wahren
Hi Phil,
Quoted 31 lines
Marc Zyngier <[email protected]> hat am 16. November 2017 um 09:57 geschrieben:
Quoted 27 lines · Stefan Wahren
From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +1 -1
index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
Quoted 4 lines
drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110
Quoted 1 line
drivers/irqchip/irq-bcm2836.c +0 -0
 static void
Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...
i don't know. Could you please answer?
I'm only interested to get the rid of this ugly warning ...

Quoted 8 lines

Quoted 1 line
and the right fix ;-)
That was my motivation too, with a preference for the smallest change that had the desired effect.
The warning message comes from the arch_timer code, which only cares about the interrupts it has been configured to use. Since these are all per-cpu interrupts, the state of the global interrupts is irrelevant.

The fact that no driver screams about it doesn't make it right. You're being bitten by the lack of interrupt polarity description in your DT. That is fine as long as you only support one type, but you need to tell the core code what you're doing.

Your whole interrupt registration thing is already quite a departure from the normal flow, where the interrupt should be created via the DT parsing code, and not eagerly like it is done here. I'd rather you address it by using the expected flow for a DT platform, with a xlate method that returns the actual trigger type (I assume all interrupts on this system are level...).

The same issue is present in the bcm2835 interrupt controller, which seems to be the one implementing global interrupts.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Stefan Wahren

author

2017-11-16 18:42 UTC #5 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Quoted 71 lines

Marc Zyngier <[email protected]> hat am 16. November 2017 um 19:16 geschrieben:
Quoted 45 lines · Phil Elwell
Hi Stefan,
Quoted 42 lines · Stefan Wahren
Hi Phil,
Quoted 31 lines
Marc Zyngier <[email protected]> hat am 16. November 2017 um 09:57 geschrieben:
Quoted 27 lines · Stefan Wahren
From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +1 -1
index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
Quoted 4 lines
drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110
Quoted 1 line
drivers/irqchip/irq-bcm2836.c +0 -0
 static void
Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...
i don't know. Could you please answer?
I'm only interested to get the rid of this ugly warning ...
Quoted 8 lines
Quoted 1 line
and the right fix ;-)
That was my motivation too, with a preference for the smallest change that had the desired effect.
The warning message comes from the arch_timer code, which only cares about the interrupts it has been configured to use. Since these are all per-cpu interrupts, the state of the global interrupts is irrelevant.
The fact that no driver screams about it doesn't make it right. You're being bitten by the lack of interrupt polarity description in your DT. That is fine as long as you only support one type, but you need to tell the core code what you're doing.
Your whole interrupt registration thing is already quite a departure from the normal flow, where the interrupt should be created via the DT parsing code, and not eagerly like it is done here. I'd rather you address it by using the expected flow for a DT platform, with a xlate method that returns the actual trigger type (I assume all interrupts on this system are level...).

Quoted 2 lines

The same issue is present in the bcm2835 interrupt controller, which seems to be the one implementing global interrupts.

Okay, thanks for the explanation.

Can you please point me to a good reference implementation?

Quoted 6 lines

Thanks,
	M.
Signature · 1 line
Jazz is not dead. It just smells funny...

Marc Zyngier

2017-11-17 09:35 UTC #6 lore raw

RFC PATCH 1/1 irq-bcm2836: Avoid "Invalid trigger warning"

Quoted 70 lines · Stefan Wahren

Quoted 70 lines
Marc Zyngier <[email protected]> hat am 16. November 2017 um 19:16 geschrieben:
Quoted 45 lines · Phil Elwell
Hi Stefan,
Quoted 42 lines · Stefan Wahren
Hi Phil,
Quoted 31 lines
Marc Zyngier <[email protected]> hat am 16. November 2017 um 09:57 geschrieben:
Quoted 27 lines · Stefan Wahren
From: Phil Elwell <[email protected]>
Initialise the level for each IRQ to avoid a warning from the arm arch timer code:
arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
arch_timer: WARNING: Please fix your firmware
arch_timer: cp15 timer(s) running at 19.20MHz (virt).
Signed-off-by Phil Elwell <[email protected]>
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-bcm2836.c +1 -1
index 667b9e1..abc9b40 100644
@@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
 
 	irq_set_percpu_devid(irq);
 	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
-	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
 }
 
Quoted 4 lines
drivers/irqchip/irq-bcm2836.c static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip) · line 110
Quoted 1 line
drivers/irqchip/irq-bcm2836.c +0 -0
 static void
Why is this only done for the per-cpu interrupts? I can't see what guarantees the same thing for global interrupts...
i don't know. Could you please answer?
I'm only interested to get the rid of this ugly warning ...
Quoted 8 lines
Quoted 1 line
and the right fix ;-)
That was my motivation too, with a preference for the smallest change that had the desired effect.
The warning message comes from the arch_timer code, which only cares about the interrupts it has been configured to use. Since these are all per-cpu interrupts, the state of the global interrupts is irrelevant.
The fact that no driver screams about it doesn't make it right. You're being bitten by the lack of interrupt polarity description in your DT. That is fine as long as you only support one type, but you need to tell the core code what you're doing.
Your whole interrupt registration thing is already quite a departure from the normal flow, where the interrupt should be created via the DT parsing code, and not eagerly like it is done here. I'd rather you address it by using the expected flow for a DT platform, with a xlate method that returns the actual trigger type (I assume all interrupts on this system are level...).

Quoted 6 lines

Quoted 2 lines
The same issue is present in the bcm2835 interrupt controller, which seems to be the one implementing global interrupts.
Okay, thanks for the explanation.
Can you please point me to a good reference implementation?

Any interrupt controller that has both an xlate and a map function should be good enough. The result should look something like the hack below (untested). Fixing the bcm2835 driver should be even simpler, though the lack of DT trigger information may unearth some horrible things in drivers. We'll see.

Thanks,

M.

drivers/irqchip/irq-bcm2836.c +34 ~~-17~~

index dc8c1e3eafe7..5d5ff2a19e61 100644
@@ -151,13 +151,33 @@ static struct irq_chip bcm2836_arm_irqchip_gpu = {
 	.irq_unmask	= bcm2836_arm_irqchip_unmask_gpu_irq,
 };
 
-static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
+static int bcm2836_map(struct irq_domain *d, unsigned int irq,
+		       irq_hw_number_t hw)
 {
-	int irq = irq_create_mapping(intc.domain, hwirq);
+	struct irq_chip *chip;
+
+	switch (hw) {
+	case LOCAL_IRQ_CNTPSIRQ:
+	case LOCAL_IRQ_CNTPNSIRQ:
+	case LOCAL_IRQ_CNTHPIRQ:
+	case LOCAL_IRQ_CNTVIRQ:
+		chip = &bcm2836_arm_irqchip_timer;
+		break;
+	case LOCAL_IRQ_GPU_FAST:
+		chip = &bcm2836_arm_irqchip_gpu;
+		break;
+	case LOCAL_IRQ_PMU_FAST:
+		chip = &bcm2836_arm_irqchip_pmu;
+		break;
+	default:
+		BUG();
+	}
 
 	irq_set_percpu_devid(irq);
-	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
+	irq_domain_set_info(d, irq, hw, chip, d->host_data,
+			    handle_percpu_devid_irq, NULL, NULL);
 	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	return 0;
 }
 
 static void

@@ -235,8 +255,18 @@ static const struct smp_operations bcm2836_smp_ops __initconst = {
 #endif
 #endif
 
+static int bcm2836_xlate(struct irq_domain *d, struct device_node *ctrlr,
+			 const u32 *intspec, unsigned int intsize,
+			 unsigned long *out_hwirq, unsigned int *out_type)
+{
+	*out_hwirq = intspec[0];
+	*out_type = IRQ_TYPE_LEVEL_LOW;
+	return 0;
+}
+
 static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
-	.xlate = irq_domain_xlate_onecell
+	.xlate = bcm2836_xlate,
+	.map = bcm2836_map,
 };
 
 static void

@@ -293,19 +323,6 @@ static int __init bcm2836_arm_irqchip_l1_intc_of_init(struct device_node *node,
 	if (!intc.domain)
 		panic("%pOF: unable to create IRQ domain\n", node);
 
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_CNTPSIRQ,
-					 &bcm2836_arm_irqchip_timer);
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_CNTPNSIRQ,
-					 &bcm2836_arm_irqchip_timer);
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_CNTHPIRQ,
-					 &bcm2836_arm_irqchip_timer);
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_CNTVIRQ,
-					 &bcm2836_arm_irqchip_timer);
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_GPU_FAST,
-					 &bcm2836_arm_irqchip_gpu);
-	bcm2836_arm_irqchip_register_irq(LOCAL_IRQ_PMU_FAST,
-					 &bcm2836_arm_irqchip_pmu);
-
 	bcm2836_arm_irqchip_smp_init();
 
 	set_handle_irq(bcm2836_arm_irqchip_handle_irq);

Signature · 1 line

Jazz is not dead. It just smells funny.