[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Vaibhav Hiremath]

With addition to TI81XX, AM33XX family of devices, the number
of interrupts supported has increased to 128, compared to 96.
The current implementation for irq handling is hardcoded to use
96 interrupts (with 3 register-sets to handle), this patch cleanups
the code, to increase maximum number of interrupts support
to 128, with dynamic detection of no of registers required for
handling all interrupts.


Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
Ideally, we should use dynamic allocation to allocate memory
for registers/arrays, may be too much cleanup for this patch,
so as of now restricting to minimal changes to fit devices
like, am33xx/ti81xx.

 arch/arm/mach-omap2/irq.c              |   47 +++++++++++++++----------------
 arch/arm/plat-omap/include/plat/irqs.h |    7 +++-
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
index 80f3ced..ea5aed9 100644
--- a/arch/arm/mach-omap2/irq.c
+++ b/arch/arm/mach-omap2/irq.c
@@ -58,10 +58,12 @@
 static struct omap_irq_bank {
 	void __iomem *base_reg;
 	unsigned int nr_irqs;
+	unsigned int nr_regs_req;
 } __attribute__ ((aligned(4))) irq_banks[] = {
 	{
 		/* MPU INTC */
 		.nr_irqs	= 96,
+		.nr_regs_req	= 3,
 	},
 };

@@ -73,8 +75,8 @@ struct omap3_intc_regs {
 	u32 protection;
 	u32 idle;
 	u32 threshold;
-	u32 ilr[INTCPS_NR_IRQS];
-	u32 mir[INTCPS_NR_MIR_REGS];
+	u32 ilr[INTCPS_MAX_NR_IRQS];
+	u32 mir[INTCPS_MAX_NR_REGS_REQ];
 };

 /* INTC bank register get/set */
@@ -182,6 +184,7 @@ static void __init omap_init_irq(u32 base, int nr_irqs,
 		struct omap_irq_bank *bank = irq_banks + i;

 		bank->nr_irqs = nr_irqs;
+		bank->nr_regs_req = 0;

 		/* Static mapping, never released */
 		bank->base_reg = ioremap(base, SZ_4K);
@@ -192,8 +195,10 @@ static void __init omap_init_irq(u32 base, int nr_irqs,

 		omap_irq_bank_init_one(bank);

-		for (j = 0; j < bank->nr_irqs; j += 32)
+		for (j = 0; j < bank->nr_irqs; j += 32) {
 			omap_alloc_gc(bank->base_reg + j, j + irq_base, 32);
+			bank->nr_regs_req++;
+		}

 		nr_of_irqs += bank->nr_irqs;
 		nr_banks++;
@@ -218,25 +223,19 @@ void __init ti81xx_init_irq(void)
 	omap_init_irq(OMAP34XX_IC_BASE, 128, NULL);
 }

-static inline void omap_intc_handle_irq(void __iomem *base_addr, struct pt_regs *regs)
+static inline void omap_intc_handle_irq(void __iomem *base_addr,
+		unsigned int no_regs_req, struct pt_regs *regs)
 {
-	u32 irqnr;
+	u32 irqnr = 0;

 	do {
-		irqnr = readl_relaxed(base_addr + 0x98);
-		if (irqnr)
-			goto out;
-
-		irqnr = readl_relaxed(base_addr + 0xb8);
-		if (irqnr)
-			goto out;
+		int i = 0;

-		irqnr = readl_relaxed(base_addr + 0xd8);
-#ifdef CONFIG_SOC_OMAPTI816X
-		if (irqnr)
-			goto out;
-		irqnr = readl_relaxed(base_addr + 0xf8);
-#endif
+		for (i = 0; i < no_regs_req; i++) {
+			irqnr = readl_relaxed(base_addr + 0x98 + (0x20 * i));
+			if (irqnr)
+				goto out;
+		}

 out:
 		if (!irqnr)
@@ -255,7 +254,7 @@ out:
 asmlinkage void __exception_irq_entry omap2_intc_handle_irq(struct pt_regs *regs)
 {
 	void __iomem *base_addr = OMAP2_IRQ_BASE;
-	omap_intc_handle_irq(base_addr, regs);
+	omap_intc_handle_irq(base_addr, irq_banks[0].nr_regs_req, regs);
 }

 int __init omap_intc_of_init(struct device_node *node,
@@ -296,10 +295,10 @@ void omap_intc_save_context(void)
 			intc_bank_read_reg(bank, INTC_IDLE);
 		intc_context[ind].threshold =
 			intc_bank_read_reg(bank, INTC_THRESHOLD);
-		for (i = 0; i < INTCPS_NR_IRQS; i++)
+		for (i = 0; i < bank->nr_irqs; i++)
 			intc_context[ind].ilr[i] =
 				intc_bank_read_reg(bank, (0x100 + 0x4*i));
-		for (i = 0; i < INTCPS_NR_MIR_REGS; i++)
+		for (i = 0; i < bank->nr_regs_req; i++)
 			intc_context[ind].mir[i] =
 				intc_bank_read_reg(&irq_banks[0], INTC_MIR0 +
 				(0x20 * i));
@@ -322,10 +321,10 @@ void omap_intc_restore_context(void)
 					bank, INTC_IDLE);
 		intc_bank_write_reg(intc_context[ind].threshold,
 					bank, INTC_THRESHOLD);
-		for (i = 0; i < INTCPS_NR_IRQS; i++)
+		for (i = 0; i < bank->nr_irqs; i++)
 			intc_bank_write_reg(intc_context[ind].ilr[i],
 				bank, (0x100 + 0x4*i));
-		for (i = 0; i < INTCPS_NR_MIR_REGS; i++)
+		for (i = 0; i < bank->nr_regs_req; i++)
 			intc_bank_write_reg(intc_context[ind].mir[i],
 				 &irq_banks[0], INTC_MIR0 + (0x20 * i));
 	}
@@ -356,6 +355,6 @@ void omap3_intc_resume_idle(void)
 asmlinkage void __exception_irq_entry omap3_intc_handle_irq(struct pt_regs *regs)
 {
 	void __iomem *base_addr = OMAP3_IRQ_BASE;
-	omap_intc_handle_irq(base_addr, regs);
+	omap_intc_handle_irq(base_addr, irq_banks[0].nr_regs_req, regs);
 }
 #endif /* CONFIG_ARCH_OMAP3 */
diff --git a/arch/arm/plat-omap/include/plat/irqs.h b/arch/arm/plat-omap/include/plat/irqs.h
index 37bbbbb..8e1b6ae 100644
--- a/arch/arm/plat-omap/include/plat/irqs.h
+++ b/arch/arm/plat-omap/include/plat/irqs.h
@@ -441,8 +441,11 @@

 #define OMAP_IRQ_BIT(irq)	(1 << ((irq) % 32))

-#define INTCPS_NR_MIR_REGS	3
-#define INTCPS_NR_IRQS		96
+/*
+ * Max from AM33XX device
+ */
+#define INTCPS_MAX_NR_REGS_REQ	4
+#define INTCPS_MAX_NR_IRQS	128

 #include <mach/hardware.h>

--
1.7.0.4

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Kevin Hilman]

Vaibhav Hiremath <hvaibhav@ti.com> writes:

> With addition to TI81XX, AM33XX family of devices, the number
> of interrupts supported has increased to 128, compared to 96.
> The current implementation for irq handling is hardcoded to use
> 96 interrupts (with 3 register-sets to handle), this patch cleanups
> the code, to increase maximum number of interrupts support
> to 128, with dynamic detection of no of registers required for
> handling all interrupts.
>
>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> ---
> Ideally, we should use dynamic allocation to allocate memory
> for registers/arrays, 

Yes.

> may be too much cleanup for this patch,

There is no such thing as too much cleanup.  ;)  
And the INTC is in need of it, IMO.

> so as of now restricting to minimal changes to fit devices
> like, am33xx/ti81xx.

Then someone else will have to do the cleanup later.  It would be
greatly appreciated if you could do the necessary cleanup in order to
cleanly add support for more SoCs.  Yes, we probably should've insisted
when support for TI81xx was added, but that one slipped in under the
radar.

For starters, the notion of a banks this code is a rather messed up and
needs a cleanup.  A bank is supposed to be a group of 32 interrupts, 
and the INTC is made up of 3 (or 4) banks.   However, the current
code creates a single "bank" of 96 (or 128) interrupts.  

It also confuses what registers are part of the bank and what are global
to the INTC.  This confusion is both in init and in context save/restore.

IMO, to clean this up, first the notion of banks needs to be fixed in
that code there is a distinction between what acts on banks and what
works on the whole INTC.

Then, the init/alloc should be done dynamically based on the number of
interrupts passed to omap_init_irq()

Kevin

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Hiremath, Vaibhav]

On Wed, May 09, 2012 at 00:09:34, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
> > With addition to TI81XX, AM33XX family of devices, the number
> > of interrupts supported has increased to 128, compared to 96.
> > The current implementation for irq handling is hardcoded to use
> > 96 interrupts (with 3 register-sets to handle), this patch cleanups
> > the code, to increase maximum number of interrupts support
> > to 128, with dynamic detection of no of registers required for
> > handling all interrupts.
> >
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Afzal Mohammed <afzal@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Kevin Hilman <khilman@ti.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> > ---
> > Ideally, we should use dynamic allocation to allocate memory
> > for registers/arrays, 
> 
> Yes.
> 

Thanks Kevin, I will put this activity in my TODO list.

> > may be too much cleanup for this patch,
> 
> There is no such thing as too much cleanup.  ;)  
> And the INTC is in need of it, IMO.
> 

Indeed it is in need of cleanup...


> > so as of now restricting to minimal changes to fit devices
> > like, am33xx/ti81xx.
> 
> Then someone else will have to do the cleanup later.  It would be
> greatly appreciated if you could do the necessary cleanup in order to
> cleanly add support for more SoCs.  Yes, we probably should've insisted
> when support for TI81xx was added, but that one slipped in under the
> radar.
> 

Yeah, I understand. As I said I will put this activity in my TODO list.

> For starters, the notion of a banks this code is a rather messed up and
> needs a cleanup.  A bank is supposed to be a group of 32 interrupts, 
> and the INTC is made up of 3 (or 4) banks.   However, the current
> code creates a single "bank" of 96 (or 128) interrupts.  
> 
> It also confuses what registers are part of the bank and what are global
> to the INTC.  This confusion is both in init and in context save/restore.
> 
> IMO, to clean this up, first the notion of banks needs to be fixed in
> that code there is a distinction between what acts on banks and what
> works on the whole INTC.
> 
> Then, the init/alloc should be done dynamically based on the number of
> interrupts passed to omap_init_irq()
> 

Kevin,
Let me finish up with am33xx baseport upstream activity which is currently 
going on at full swing, then next thing I will pick up is this code cleanup.

I still feel, this is still a valid cleanup patch, and can be merged, as it 
is required/used when we do major cleanup.

Thanks,
Vaibhav

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Kevin Hilman]

"Hiremath, Vaibhav" <hvaibhav@ti.com> writes:

> On Wed, May 09, 2012 at 00:09:34, Hilman, Kevin wrote:
>> Vaibhav Hiremath <hvaibhav@ti.com> writes:
>> 
>> > With addition to TI81XX, AM33XX family of devices, the number
>> > of interrupts supported has increased to 128, compared to 96.
>> > The current implementation for irq handling is hardcoded to use
>> > 96 interrupts (with 3 register-sets to handle), this patch cleanups
>> > the code, to increase maximum number of interrupts support
>> > to 128, with dynamic detection of no of registers required for
>> > handling all interrupts.
>> >
>> >
>> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> > Signed-off-by: Afzal Mohammed <afzal@ti.com>
>> > Cc: Tony Lindgren <tony@atomide.com>
>> > Cc: Kevin Hilman <khilman@ti.com>
>> > Cc: Paul Walmsley <paul@pwsan.com>
>> > ---
>> > Ideally, we should use dynamic allocation to allocate memory
>> > for registers/arrays, 
>> 
>> Yes.
>> 
>
> Thanks Kevin, I will put this activity in my TODO list.
>
>> > may be too much cleanup for this patch,
>> 
>> There is no such thing as too much cleanup.  ;)  
>> And the INTC is in need of it, IMO.
>> 
>
> Indeed it is in need of cleanup...
>
>
>> > so as of now restricting to minimal changes to fit devices
>> > like, am33xx/ti81xx.
>> 
>> Then someone else will have to do the cleanup later.  It would be
>> greatly appreciated if you could do the necessary cleanup in order to
>> cleanly add support for more SoCs.  Yes, we probably should've insisted
>> when support for TI81xx was added, but that one slipped in under the
>> radar.
>> 
>
> Yeah, I understand. As I said I will put this activity in my TODO list.
>
>> For starters, the notion of a banks this code is a rather messed up and
>> needs a cleanup.  A bank is supposed to be a group of 32 interrupts, 
>> and the INTC is made up of 3 (or 4) banks.   However, the current
>> code creates a single "bank" of 96 (or 128) interrupts.  
>> 
>> It also confuses what registers are part of the bank and what are global
>> to the INTC.  This confusion is both in init and in context save/restore.
>> 
>> IMO, to clean this up, first the notion of banks needs to be fixed in
>> that code there is a distinction between what acts on banks and what
>> works on the whole INTC.
>> 
>> Then, the init/alloc should be done dynamically based on the number of
>> interrupts passed to omap_init_irq()
>> 
>
> Kevin,
> Let me finish up with am33xx baseport upstream activity which is currently 
> going on at full swing, then next thing I will pick up is this code cleanup.
>
> I still feel, this is still a valid cleanup patch, and can be merged, as it 
> is required/used when we do major cleanup.

Well, Tony can make that decision.

Personally, I think this patch continues to add confusion on top of an
existing mess, and to me provides the proverbial straw that broke the
camel's back.

That being said, the INTC core is an obviously important and sensitive
piece of code so needs to be handled with care.

In case Tony decides to merge this patch and allow a future** cleanup,
I'll provide some comments.

Kevin

** since they rarely happen, we tend to not have too much faith in
   promises of mythical "future" cleanups.  This is not because we don't
   trust you personally, but simply based on based experience.

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Tony Lindgren]

* Kevin Hilman <khilman@ti.com> [120510 14:53]:
> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
> >
> > Let me finish up with am33xx baseport upstream activity which is currently 
> > going on at full swing, then next thing I will pick up is this code cleanup.
> >
> > I still feel, this is still a valid cleanup patch, and can be merged, as it 
> > is required/used when we do major cleanup.
> 
> Well, Tony can make that decision.
> 
> Personally, I think this patch continues to add confusion on top of an
> existing mess, and to me provides the proverbial straw that broke the
> camel's back.
> 
> That being said, the INTC core is an obviously important and sensitive
> piece of code so needs to be handled with care.
> 
> In case Tony decides to merge this patch and allow a future** cleanup,
> I'll provide some comments.

Heh I agree with Kevin here :)

Tony

 
> ** since they rarely happen, we tend to not have too much faith in
>    promises of mythical "future" cleanups.  This is not because we don't
>    trust you personally, but simply based on based experience.

[PATCH] ARM: OMAP2+: irq: Increase no of supported interrupts to 128

[Hiremath, Vaibhav]

On Fri, May 11, 2012 at 03:19:21, Hilman, Kevin wrote:
> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
> 
> > On Wed, May 09, 2012 at 00:09:34, Hilman, Kevin wrote:
> >> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> >> 
> >> > With addition to TI81XX, AM33XX family of devices, the number
> >> > of interrupts supported has increased to 128, compared to 96.
> >> > The current implementation for irq handling is hardcoded to use
> >> > 96 interrupts (with 3 register-sets to handle), this patch cleanups
> >> > the code, to increase maximum number of interrupts support
> >> > to 128, with dynamic detection of no of registers required for
> >> > handling all interrupts.
> >> >
> >> >
> >> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> >> > Signed-off-by: Afzal Mohammed <afzal@ti.com>
> >> > Cc: Tony Lindgren <tony@atomide.com>
> >> > Cc: Kevin Hilman <khilman@ti.com>
> >> > Cc: Paul Walmsley <paul@pwsan.com>
> >> > ---
> >> > Ideally, we should use dynamic allocation to allocate memory
> >> > for registers/arrays, 
> >> 
> >> Yes.
> >> 
> >
> > Thanks Kevin, I will put this activity in my TODO list.
> >
> >> > may be too much cleanup for this patch,
> >> 
> >> There is no such thing as too much cleanup.  ;)  
> >> And the INTC is in need of it, IMO.
> >> 
> >
> > Indeed it is in need of cleanup...
> >
> >
> >> > so as of now restricting to minimal changes to fit devices
> >> > like, am33xx/ti81xx.
> >> 
> >> Then someone else will have to do the cleanup later.  It would be
> >> greatly appreciated if you could do the necessary cleanup in order to
> >> cleanly add support for more SoCs.  Yes, we probably should've insisted
> >> when support for TI81xx was added, but that one slipped in under the
> >> radar.
> >> 
> >
> > Yeah, I understand. As I said I will put this activity in my TODO list.
> >
> >> For starters, the notion of a banks this code is a rather messed up and
> >> needs a cleanup.  A bank is supposed to be a group of 32 interrupts, 
> >> and the INTC is made up of 3 (or 4) banks.   However, the current
> >> code creates a single "bank" of 96 (or 128) interrupts.  
> >> 
> >> It also confuses what registers are part of the bank and what are global
> >> to the INTC.  This confusion is both in init and in context save/restore.
> >> 
> >> IMO, to clean this up, first the notion of banks needs to be fixed in
> >> that code there is a distinction between what acts on banks and what
> >> works on the whole INTC.
> >> 
> >> Then, the init/alloc should be done dynamically based on the number of
> >> interrupts passed to omap_init_irq()
> >> 
> >
> > Kevin,
> > Let me finish up with am33xx baseport upstream activity which is currently 
> > going on at full swing, then next thing I will pick up is this code cleanup.
> >
> > I still feel, this is still a valid cleanup patch, and can be merged, as it 
> > is required/used when we do major cleanup.
> 
> Well, Tony can make that decision.
> 
> Personally, I think this patch continues to add confusion on top of an
> existing mess, and to me provides the proverbial straw that broke the
> camel's back.
> 
> That being said, the INTC core is an obviously important and sensitive
> piece of code so needs to be handled with care.
> 
> In case Tony decides to merge this patch and allow a future** cleanup,
> I'll provide some comments.
> 

I am OK, lets drop this patch as of now, and I will take this when I start 
cleanup activity.


> Kevin
> 
> ** since they rarely happen, we tend to not have too much faith in
>    promises of mythical "future" cleanups.  This is not because we don't
>    trust you personally, but simply based on based experience.
> 

Very nicely said :) :) :)

I agree with you, sometimes priorities changes and things doesn't happen the 
way we wanted to be.

As I said earlier, I am now full time working on upstream (no ifs and buts); else I wouldn't have committed for this activity.


Thanks,
Vaibhav