All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: VIC: use irq_domain_add_simple()
@ 2012-10-16 13:06 Linus Walleij
  2012-10-16 13:32 ` Rob Herring
  2012-11-08 13:40 ` [PATCH] ARM: VIC: remove unused irq_base variable Arnd Bergmann
  0 siblings, 2 replies; 10+ messages in thread
From: Linus Walleij @ 2012-10-16 13:06 UTC (permalink / raw)
  To: linux-arm-kernel

From: Linus Walleij <linus.walleij@linaro.org>

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 <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/common/vic.c               | 18 ++++++------------
 arch/arm/include/asm/hardware/vic.h |  2 +-
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index e0d5388..4fd5d98 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -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 */
 
diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h
index e14af1a..2bebad3 100644
--- a/arch/arm/include/asm/hardware/vic.h
+++ b/arch/arm/include/asm/hardware/vic.h
@@ -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);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2012-10-16 13:06 [PATCH] ARM: VIC: use irq_domain_add_simple() Linus Walleij
@ 2012-10-16 13:32 ` Rob Herring
  2012-12-18 23:34   ` Grant Likely
  2012-11-08 13:40 ` [PATCH] ARM: VIC: remove unused irq_base variable Arnd Bergmann
  1 sibling, 1 reply; 10+ messages in thread
From: Rob Herring @ 2012-10-16 13:32 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/16/2012 08:06 AM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> 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 <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Rob Herring <rob.herring@calxeda.com>

Rob

> ---
>  arch/arm/common/vic.c               | 18 ++++++------------
>  arch/arm/include/asm/hardware/vic.h |  2 +-
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index e0d5388..4fd5d98 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -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 */
>  
> diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h
> index e14af1a..2bebad3 100644
> --- a/arch/arm/include/asm/hardware/vic.h
> +++ b/arch/arm/include/asm/hardware/vic.h
> @@ -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);
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: remove unused irq_base variable
  2012-10-16 13:06 [PATCH] ARM: VIC: use irq_domain_add_simple() Linus Walleij
  2012-10-16 13:32 ` Rob Herring
@ 2012-11-08 13:40 ` Arnd Bergmann
  2012-11-08 13:55   ` Russell King - ARM Linux
  1 sibling, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2012-11-08 13:40 UTC (permalink / raw)
  To: linux-arm-kernel

"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 <arnd@arndb.de>

---
On Tuesday 16 October 2012, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> 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 <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index 4fd5d98..e4df17c 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -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;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: remove unused irq_base variable
  2012-11-08 13:40 ` [PATCH] ARM: VIC: remove unused irq_base variable Arnd Bergmann
@ 2012-11-08 13:55   ` Russell King - ARM Linux
  2012-11-08 14:00     ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux @ 2012-11-08 13:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Nov 08, 2012 at 01:40:54PM +0000, Arnd Bergmann wrote:
> "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...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: remove unused irq_base variable
  2012-11-08 13:55   ` Russell King - ARM Linux
@ 2012-11-08 14:00     ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2012-11-08 14:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Nov 8, 2012 at 2:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Nov 08, 2012 at 01:40:54PM +0000, Arnd Bergmann wrote:
>> "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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2012-10-16 13:32 ` Rob Herring
@ 2012-12-18 23:34   ` Grant Likely
  2012-12-20 18:45     ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Grant Likely @ 2012-12-18 23:34 UTC (permalink / raw)
  To: linux-arm-kernel

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.

g.

On Tue, Oct 16, 2012 at 2:32 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 10/16/2012 08:06 AM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> 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 <rob.herring@calxeda.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
>
> Rob
>
>> ---
>>  arch/arm/common/vic.c               | 18 ++++++------------
>>  arch/arm/include/asm/hardware/vic.h |  2 +-
>>  2 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
>> index e0d5388..4fd5d98 100644
>> --- a/arch/arm/common/vic.c
>> +++ b/arch/arm/common/vic.c
>> @@ -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 */
>>
>> diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h
>> index e14af1a..2bebad3 100644
>> --- a/arch/arm/include/asm/hardware/vic.h
>> +++ b/arch/arm/include/asm/hardware/vic.h
>> @@ -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, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2012-12-18 23:34   ` Grant Likely
@ 2012-12-20 18:45     ` Linus Walleij
  2012-12-26  0:43       ` Linus Walleij
  2013-01-11 18:57       ` Grant Likely
  0 siblings, 2 replies; 10+ messages in thread
From: Linus Walleij @ 2012-12-20 18:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Dec 19, 2012 at 12:34 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:

> 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.

diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index e4df17c..8f324b9 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -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;
 }
diff --git a/arch/arm/mach-versatile/include/mach/irqs.h
b/arch/arm/mach-versatile/include/mach/irqs.h
index bf44c61..0fd771c 100644
--- a/arch/arm/mach-versatile/include/mach/irqs.h
+++ b/arch/arm/mach-versatile/include/mach/irqs.h
@@ -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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2012-12-20 18:45     ` Linus Walleij
@ 2012-12-26  0:43       ` Linus Walleij
  2013-01-11 18:57       ` Grant Likely
  1 sibling, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2012-12-26  0:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 20, 2012 at 7:45 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Dec 19, 2012 at 12:34 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>
>> 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:

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2012-12-20 18:45     ` Linus Walleij
  2012-12-26  0:43       ` Linus Walleij
@ 2013-01-11 18:57       ` Grant Likely
  2013-01-11 20:38         ` Linus Walleij
  1 sibling, 1 reply; 10+ messages in thread
From: Grant Likely @ 2013-01-11 18:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 20 Dec 2012 19:45:24 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Dec 19, 2012 at 12:34 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> 
> > 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.)

> > 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...

> 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.

> diff --git a/arch/arm/mach-versatile/include/mach/irqs.h
> b/arch/arm/mach-versatile/include/mach/irqs.h
> index bf44c61..0fd771c 100644
> --- a/arch/arm/mach-versatile/include/mach/irqs.h
> +++ b/arch/arm/mach-versatile/include/mach/irqs.h
> @@ -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

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

#define IRQ_SIC_START			(IRQ_VIC_START + 32)

>  #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 <grant.likely@secretlab.ca>

g.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ARM: VIC: use irq_domain_add_simple()
  2013-01-11 18:57       ` Grant Likely
@ 2013-01-11 20:38         ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2013-01-11 20:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jan 11, 2013 at 7:57 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

>> 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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-01-11 20:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-16 13:06 [PATCH] ARM: VIC: use irq_domain_add_simple() Linus Walleij
2012-10-16 13:32 ` Rob Herring
2012-12-18 23:34   ` Grant Likely
2012-12-20 18:45     ` Linus Walleij
2012-12-26  0:43       ` Linus Walleij
2013-01-11 18:57       ` Grant Likely
2013-01-11 20:38         ` Linus Walleij
2012-11-08 13:40 ` [PATCH] ARM: VIC: remove unused irq_base variable Arnd Bergmann
2012-11-08 13:55   ` Russell King - ARM Linux
2012-11-08 14:00     ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.