ARM: VIC: use irq_domain_add_simple()

PATCH New Linus Walleij 6 participants 10 messages Oct 16, 2012 – Jan 11, 2013

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm/versatile express platform Arm integrator, versatile and realview support

lore

Share & tools

lore

Linus Walleij

author

2012-10-16 13:06 UTC #0 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

From: Linus Walleij <[email protected]>

Instead of allocating descriptors on-the-fly for the device tree initialization case, use irq_domain_add_simple() which will take care of this if you pass negative as the first_irq.

Alter the signature of __vic_init() to pass the first_irq as signed so this works as expected.

Switching the VIC to use irq_domain_add_simple() also has the upside of displaying the same WARNING when you boot with pre-allocated descriptors on systems using SPARSE_IRQ but yet not using device tree.

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
 arch/arm/common/vic.c               | 18 ++++++------------
 arch/arm/include/asm/hardware/vic.h |  2 +-
 2 files changed, 7 insertions(+), 13 deletions(-)

arch/arm/common/vic.c +6 ~~-12~~

index e0d5388..4fd5d98 100644
@@ -218,7 +218,7 @@ static void __init vic_register(void __iomem *base, unsigned int irq,
 	v->resume_sources = resume_sources;
 	v->irq = irq;
 	vic_id++;
-	v->domain = irq_domain_add_legacy(node, fls(valid_sources), irq, 0,
+	v->domain = irq_domain_add_simple(node, fls(valid_sources), irq,
 					  &vic_irqdomain_ops, v);
 }

@@ -350,7 +350,7 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start,
 	vic_register(base, irq_start, vic_sources, 0, node);
 }
 
-void __init __vic_init(void __iomem *base, unsigned int irq_start,
+void __init __vic_init(void __iomem *base, int irq_start,
 			      u32 vic_sources, u32 resume_sources,
 			      struct device_node *node)
 {

@@ -416,18 +416,12 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
 	if (WARN_ON(!regs))
 		return -EIO;
 
-	irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
-	if (WARN_ON(irq_base < 0))
-		goto out_unmap;
-
-	__vic_init(regs, irq_base, ~0, ~0, node);
+	/*
+	 * Passing -1 as first IRQ makes the simple domain allocate descriptors
+	 */
+	__vic_init(regs, -1, ~0, ~0, node);
 
 	return 0;
-
- out_unmap:
-	iounmap(regs);
-
-	return -EIO;
 }
 #endif /* CONFIG OF */

arch/arm/include/asm/hardware/vic.h +1 -1

index e14af1a..2bebad3 100644
@@ -47,7 +47,7 @@
 struct device_node;
 struct pt_regs;
 
-void __vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources,
+void __vic_init(void __iomem *base, int irq_start, u32 vic_sources,
 		u32 resume_sources, struct device_node *node);
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
 int vic_of_init(struct device_node *node, struct device_node *parent);

Signature · 1 line

1.7.11.3

Rob Herring

2012-10-16 13:32 UTC #1 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

Quoted 9 lines · Linus Walleij

From: Linus Walleij <[email protected]>
Instead of allocating descriptors on-the-fly for the device tree initialization case, use irq_domain_add_simple() which will take care of this if you pass negative as the first_irq.
Alter the signature of __vic_init() to pass the first_irq as signed so this works as expected.

Quoted 8 lines

Switching the VIC to use irq_domain_add_simple() also has the upside of displaying the same WARNING when you boot with pre-allocated descriptors on systems using SPARSE_IRQ but yet not using device tree.
Cc Rob Herring <[email protected]>
Cc Grant Likely <[email protected]>
Signed-off-by Linus Walleij <[email protected]>

Acked-by Rob Herring <[email protected]>

Rob

Quoted 64 lines

---
 arch/arm/common/vic.c               | 18 ++++++------------
 arch/arm/include/asm/hardware/vic.h |  2 +-
 2 files changed, 7 insertions(+), 13 deletions(-)

arch/arm/common/vic.c +6 ~~-12~~

index e0d5388..4fd5d98 100644
@@ -218,7 +218,7 @@ static void __init vic_register(void __iomem *base, unsigned int irq,
 	v->resume_sources = resume_sources;
 	v->irq = irq;
 	vic_id++;
-	v->domain = irq_domain_add_legacy(node, fls(valid_sources), irq, 0,
+	v->domain = irq_domain_add_simple(node, fls(valid_sources), irq,
 					  &vic_irqdomain_ops, v);
 }

@@ -350,7 +350,7 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start,
 	vic_register(base, irq_start, vic_sources, 0, node);
 }
 
-void __init __vic_init(void __iomem *base, unsigned int irq_start,
+void __init __vic_init(void __iomem *base, int irq_start,
 			      u32 vic_sources, u32 resume_sources,
 			      struct device_node *node)
 {

@@ -416,18 +416,12 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
 	if (WARN_ON(!regs))
 		return -EIO;
 
-	irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
-	if (WARN_ON(irq_base < 0))
-		goto out_unmap;
-
-	__vic_init(regs, irq_base, ~0, ~0, node);
+	/*
+	 * Passing -1 as first IRQ makes the simple domain allocate descriptors
+	 */
+	__vic_init(regs, -1, ~0, ~0, node);
 
 	return 0;
-
- out_unmap:
-	iounmap(regs);
-
-	return -EIO;
 }
 #endif /* CONFIG OF */

arch/arm/include/asm/hardware/vic.h +1 -1

index e14af1a..2bebad3 100644
@@ -47,7 +47,7 @@
 struct device_node;
 struct pt_regs;
 
-void __vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources,
+void __vic_init(void __iomem *base, int irq_start, u32 vic_sources,
 		u32 resume_sources, struct device_node *node);
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
 int vic_of_init(struct device_node *node, struct device_node *parent);

Grant Likely

2012-12-18 23:34 UTC #2 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

Russell,

It looks to me like the below patch breaks versatile because it makes it try to register a linear irq_domain instead of the legacy one that it needs. Here are the relevant commits:

07c9249f1: "ARM: 7554/1: VIC: use irq_domain_add_simple()" 946c59a08: "ARM: vic: fix build warning caused by previous commit"

I'm working on getting it properly sorted out (and more importantly *simplified*), but in the mean time I think the above two commits need to be reverted. Reverting them on my tree fixes booting for me.

Quoted 81 lines · Rob Herring

Quoted 9 lines · Linus Walleij

From: Linus Walleij <[email protected]>
Instead of allocating descriptors on-the-fly for the device tree initialization case, use irq_domain_add_simple() which will take care of this if you pass negative as the first_irq.
Alter the signature of __vic_init() to pass the first_irq as signed so this works as expected.

Quoted 8 lines

Switching the VIC to use irq_domain_add_simple() also has the upside of displaying the same WARNING when you boot with pre-allocated descriptors on systems using SPARSE_IRQ but yet not using device tree.
Cc Rob Herring <[email protected]>
Cc Grant Likely <[email protected]>
Signed-off-by Linus Walleij <[email protected]>

Acked-by Rob Herring <[email protected]>

Rob

Quoted 58 lines

---
 arch/arm/common/vic.c               | 18 ++++++------------
 arch/arm/include/asm/hardware/vic.h |  2 +-
 2 files changed, 7 insertions(+), 13 deletions(-)

arch/arm/common/vic.c +6 ~~-12~~

index e0d5388..4fd5d98 100644
@@ -218,7 +218,7 @@ static void __init vic_register(void __iomem *base, unsigned int irq,
      v->resume_sources = resume_sources;
      v->irq = irq;
      vic_id++;
-     v->domain = irq_domain_add_legacy(node, fls(valid_sources), irq, 0,
+     v->domain = irq_domain_add_simple(node, fls(valid_sources), irq,
                                        &vic_irqdomain_ops, v);
 }

@@ -350,7 +350,7 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start,
      vic_register(base, irq_start, vic_sources, 0, node);
 }
 
-void __init __vic_init(void __iomem *base, unsigned int irq_start,
+void __init __vic_init(void __iomem *base, int irq_start,
                            u32 vic_sources, u32 resume_sources,
                            struct device_node *node)
 {

@@ -416,18 +416,12 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
      if (WARN_ON(!regs))
              return -EIO;
 
-     irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
-     if (WARN_ON(irq_base < 0))
-             goto out_unmap;
-
-     __vic_init(regs, irq_base, ~0, ~0, node);
+     /*
+      * Passing -1 as first IRQ makes the simple domain allocate descriptors
+      */
+     __vic_init(regs, -1, ~0, ~0, node);
 
      return 0;
-
- out_unmap:
-     iounmap(regs);
-
-     return -EIO;
 }
 #endif /* CONFIG OF */

arch/arm/include/asm/hardware/vic.h +0 -0

index e14af1a..2bebad3 100644
@@ -47,7 +47,7 @@
 struct device_node;
 struct pt_regs;

Quoted 7 lines

arch/arm/include/asm/hardware/vic.h line 53

Quoted 6 lines

arch/arm/include/asm/hardware/vic.h +1 -1

-void __vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources,
+void __vic_init(void __iomem *base, int irq_start, u32 vic_sources,
              u32 resume_sources, struct device_node *node);
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
 int vic_of_init(struct device_node *node, struct device_node *parent);

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Linus Walleij

2012-12-20 18:45 UTC #3 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

Quoted 10 lines · Grant Likely

It looks to me like the below patch breaks versatile because it makes it try to register a linear irq_domain instead of the legacy one that it needs. Here are the relevant commits:
07c9249f1: "ARM: 7554/1: VIC: use irq_domain_add_simple()" 946c59a08: "ARM: vic: fix build warning caused by previous commit"
I'm working on getting it properly sorted out (and more importantly *simplified*), but in the mean time I think the above two commits need to be reverted. Reverting them on my tree fixes booting for me.

Isn't that a bit violent, can't we just fix the real bug?

Does the below patch work for you, it does two things:

1) Bump all Versatile IRQs to offset at 32, because it is using IRQ 0 which is NO_IRQ and illegal anyway so it's anyway a bug that should be fixed.

2) Make sure we call irq_create_mapping() if the start IRQ is anyway 0, as in the device tree case, and make sure to actually pass zero in that case.

The initial patch set used negative IRQ as an indicator that the linear domain should be used and not descriptors allocated, but as that require changing a lot of singatures to signed int and since IRQ 0 is now illegal anyway, I let zero signify linear. Sorry for leftover mistakes like this...

I'll mail them out as separate patches too, and submit to Russell's tracker if they fix the problem.

arch/arm/common/vic.c +0 -0

index e4df17c..8f324b9 100644
@@ -206,6 +206,7 @@ static void __init vic_register(void __iomem

*base, unsigned int irq,
struct device_node *node)
{
struct vic_device *v;
+ int i;

if (vic_id >= ARRAY_SIZE(vic_devices)) {
printk(KERN_ERR "%s: too few VICs, increase CONFIG_ARM_VIC_NR\n", __func__);
@@ -220,6 +221,10 @@ static void __init vic_register(void __iomem
*base, unsigned int irq,
vic_id++;
v->domain = irq_domain_add_simple(node, fls(valid_sources), irq,
&vic_irqdomain_ops, v);
+ /* create an IRQ mapping for each valid IRQ */
+ for (i = 0; i < fls(valid_sources); i++)
+ if (valid_sources & (1 << i))
+ irq_create_mapping(v->domain, i);
}

static void vic_ack_irq(struct irq_data *d)
@@ -416,9 +421,9 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
return -EIO;

/*
- * Passing -1 as first IRQ makes the simple domain allocate descriptors
+ * Passing 0 as first IRQ makes the simple domain allocate descriptors
*/
- __vic_init(regs, -1, ~0, ~0, node);
+ __vic_init(regs, 0, ~0, ~0, node);

return 0;
}

(quoted diff) +0 -0

b/arch/arm/mach-versatile/include/mach/irqs.h
index bf44c61..0fd771c 100644

arch/arm/mach-versatile/include/mach/irqs.h +3 -3

@@ -25,7 +25,7 @@
  *  IRQ interrupts definitions are the same as the INT definitions
  *  held within platform.h
  */
-#define IRQ_VIC_START		0
+#define IRQ_VIC_START		32
 #define IRQ_WDOGINT		(IRQ_VIC_START + INT_WDOGINT)
 #define IRQ_SOFTINT		(IRQ_VIC_START + INT_SOFTINT)
 #define IRQ_COMMRx		(IRQ_VIC_START + INT_COMMRx)

@@ -100,7 +100,7 @@
 /*
  * Secondary interrupt controller
  */
-#define IRQ_SIC_START		32
+#define IRQ_SIC_START		64
 #define IRQ_SIC_MMCI0B 		(IRQ_SIC_START + SIC_INT_MMCI0B)
 #define IRQ_SIC_MMCI1B 		(IRQ_SIC_START + SIC_INT_MMCI1B)
 #define IRQ_SIC_KMI0		(IRQ_SIC_START + SIC_INT_KMI0)

@@ -120,7 +120,7 @@
 #define IRQ_SIC_PCI1		(IRQ_SIC_START + SIC_INT_PCI1)
 #define IRQ_SIC_PCI2		(IRQ_SIC_START + SIC_INT_PCI2)
 #define IRQ_SIC_PCI3		(IRQ_SIC_START + SIC_INT_PCI3)
-#define IRQ_SIC_END		63
+#define IRQ_SIC_END		95
 
 #define IRQ_GPIO0_START		(IRQ_SIC_END + 1)
 #define IRQ_GPIO0_END		(IRQ_GPIO0_START + 31)

Yours,
Linus Walleij

Linus Walleij

2012-12-26 00:43 UTC #4 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

Quoted 10 lines · Linus Walleij

Quoted 7 lines · Grant Likely
It looks to me like the below patch breaks versatile because it makes it try to register a linear irq_domain instead of the legacy one that it needs. Here are the relevant commits:
07c9249f1: "ARM: 7554/1: VIC: use irq_domain_add_simple()" 946c59a08: "ARM: vic: fix build warning caused by previous commit"

Quoted 7 lines

Quoted 3 lines
I'm working on getting it properly sorted out (and more importantly *simplified*), but in the mean time I think the above two commits need to be reverted. Reverting them on my tree fixes booting for me.
Isn't that a bit violent, can't we just fix the real bug?
Does the below patch work for you, it does two things:

I've submitted the fixes to Russell's patch tracker now ... would appreciate some testing but I'm pretty sure they will fix the issue.

Yours,
Linus Walleij

Grant Likely

2013-01-11 18:57 UTC #5 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

In reply to Linus Walleij

Quoted 9 lines · Linus Walleij

Quoted 6 lines · Grant Likely
It looks to me like the below patch breaks versatile because it makes it try to register a linear irq_domain instead of the legacy one that it needs. Here are the relevant commits:
07c9249f1: "ARM: 7554/1: VIC: use irq_domain_add_simple()" 946c59a08: "ARM: vic: fix build warning caused by previous commit"

(sorry for the really late reply on this. I took a break at Christmas and jetlag has been beating up my concentration since I got back. I actually started writing this reply before Christmas, but the investigation sidetracked me enough that I never finished it.)

Quoted 5 lines

Quoted 3 lines
I'm working on getting it properly sorted out (and more importantly *simplified*), but in the mean time I think the above two commits need to be reverted. Reverting them on my tree fixes booting for me.
Isn't that a bit violent, can't we just fix the real bug?

I did try to fix the bug first, but it became more involved that I would like for a stablization change (and I went down a lot of rabbit trails in the process). I preferred reverting because this was all contained to a single driver which can be backed out easily to have another cycle for a correct fix. I did try your change though...

Quoted 9 lines

Does the below patch work for you, it does two things:
1) Bump all Versatile IRQs to offset at 32, because it is using IRQ 0 which is NO_IRQ and illegal anyway so it's anyway a bug that should be fixed.
2) Make sure we call irq_create_mapping() if the start IRQ is anyway 0, as in the device tree case, and make sure to actually pass zero in that case.

In actual fact, only changing the offset to 32 is required to get Versatile to boot with DT. That platform doesn't yet use vic_of_init(). It currently depends on pre-allocated irq_descs. Calling irq_create_mapping() doesn't resolve this. I now have patches to rework the versatile OF support to use irq_of_init(), but they aren't ready for posting yet. Merging the bump to offset 32 would be fine for now.

Quoted 15 lines

arch/arm/mach-versatile/include/mach/irqs.h +1 -1

index bf44c61..0fd771c 100644
@@ -25,7 +25,7 @@
  *  IRQ interrupts definitions are the same as the INT definitions
  *  held within platform.h
  */
-#define IRQ_VIC_START		0
+#define IRQ_VIC_START		32
 #define IRQ_WDOGINT		(IRQ_VIC_START + INT_WDOGINT)
 #define IRQ_SOFTINT		(IRQ_VIC_START + INT_SOFTINT)
 #define IRQ_COMMRx		(IRQ_VIC_START + INT_COMMRx)

arch/arm/mach-versatile/include/mach/irqs.h line 103

Quoted 5 lines

arch/arm/mach-versatile/include/mach/irqs.h +1 -1

@@ -100,7 +100,7 @@
 /*
  * Secondary interrupt controller
  */
-#define IRQ_SIC_START		32
+#define IRQ_SIC_START		64

Since you're touching this line anyway, please change to:

#define IRQ_SIC_START (IRQ_VIC_START + 32)

arch/arm/mach-versatile/include/mach/irqs.h line 123

Quoted 9 lines

arch/arm/mach-versatile/include/mach/irqs.h +1 -1

 #define IRQ_SIC_MMCI0B 		(IRQ_SIC_START + SIC_INT_MMCI0B)
 #define IRQ_SIC_MMCI1B 		(IRQ_SIC_START + SIC_INT_MMCI1B)
 #define IRQ_SIC_KMI0		(IRQ_SIC_START + SIC_INT_KMI0)

@@ -120,7 +120,7 @@
 #define IRQ_SIC_PCI1		(IRQ_SIC_START + SIC_INT_PCI1)
 #define IRQ_SIC_PCI2		(IRQ_SIC_START + SIC_INT_PCI2)
 #define IRQ_SIC_PCI3		(IRQ_SIC_START + SIC_INT_PCI3)
-#define IRQ_SIC_END		63
+#define IRQ_SIC_END		95

Similarly here: #define IRQ_SIC_END (IRQ_SIC_START + 31)

And then you can add my acked by for the irq number changes.

Acked-by Grant Likely <[email protected]>

Linus Walleij

2013-01-11 20:38 UTC #6 lore raw

PATCH 1/1 ARM: VIC: use irq_domain_add_simple()

Quoted 6 lines · Grant Likely

Quoted 6 lines
Does the below patch work for you, it does two things:
1) Bump all Versatile IRQs to offset at 32, because it is using IRQ 0 which is NO_IRQ and illegal anyway so it's anyway a bug that should be fixed.

Quoted 7 lines

Quoted 3 lines
2) Make sure we call irq_create_mapping() if the start IRQ is anyway 0, as in the device tree case, and make sure to actually pass zero in that case.
In actual fact, only changing the offset to 32 is required to get Versatile to boot with DT. That platform doesn't yet use vic_of_init(). It currently depends on pre-allocated irq_descs.

OK both are already upstream, because there was someone else verifying they solved the problem.

And the second patch was needed on the Integrator which does use SPARSE_IRQ + vic_init_of().
Probably the DT-based SPEArs need it too.

Yours,
Linus Walleij

Arnd Bergmann

2012-11-08 13:40 UTC #7 lore raw

PATCH 1/1 ARM: VIC: remove unused irq_base variable

In reply to Linus Walleij

"ARM: VIC: use irq_domain_add_simple()" causes a new warning:

arch/arm/common/vic.c: In function 'vic_of_init':
arch/arm/common/vic.c:410:6: warning: unused variable 'irq_base' [-Wunused-variable]

Since the irq_base variable is not used any more, it should be removed now

Signed-off-by Arnd Bergmann <[email protected]>

---

Quoted 9 lines · Linus Walleij

From: Linus Walleij <[email protected]>
Instead of allocating descriptors on-the-fly for the device tree initialization case, use irq_domain_add_simple() which will take care of this if you pass negative as the first_irq.
Alter the signature of __vic_init() to pass the first_irq as signed so this works as expected.

Quoted 8 lines

Switching the VIC to use irq_domain_add_simple() also has the upside of displaying the same WARNING when you boot with pre-allocated descriptors on systems using SPARSE_IRQ but yet not using device tree.
Cc Rob Herring <[email protected]>
Cc Grant Likely <[email protected]>
Signed-off-by Linus Walleij <[email protected]>

arch/arm/common/vic.c +0 -1

index 4fd5d98..e4df17c 100644
@@ -407,7 +407,6 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
 int __init vic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *regs;
-	int irq_base;
 
 	if (WARN(parent, "non-root VICs are not supported"))
 		return -EINVAL;

Russell King - ARM Linux

2012-11-08 13:55 UTC #8 lore raw

PATCH 1/1 ARM: VIC: remove unused irq_base variable

Quoted 7 lines · Arnd Bergmann

"ARM: VIC: use irq_domain_add_simple()" causes a new warning:
arch/arm/common/vic.c: In function 'vic_of_init':
arch/arm/common/vic.c:410:6: warning: unused variable 'irq_base' [-Wunused-variable]
Since the irq_base variable is not used any more, it should be removed now

I've just committed exactly the same patch to my tree...

Linus Walleij

2012-11-08 14:00 UTC #9 lore raw

PATCH 1/1 ARM: VIC: remove unused irq_base variable

Quoted 10 lines · Russell King - ARM Linux

Quoted 7 lines · Arnd Bergmann
"ARM: VIC: use irq_domain_add_simple()" causes a new warning:
arch/arm/common/vic.c: In function 'vic_of_init':
arch/arm/common/vic.c:410:6: warning: unused variable 'irq_base' [-Wunused-variable]
Since the irq_base variable is not used any more, it should be removed now
I've just committed exactly the same patch to my tree...

Thanks guys...
Acked-by, FWIW

Yours,
Linus Walleij