Re: [PATCH 4/9] OMAP: Add new function to check wether there is irq pending

[Tony Lindgren <tony@atomide.com>]

* Högander Jouni <jouni.hogander@nokia.com> [080519 23:27]:
> "ext Tony Lindgren" <tony@atomide.com> writes:
> 
> > * Högander Jouni <jouni.hogander@nokia.com> [080519 01:05]:
> >> "ext Tony Lindgren" <tony@atomide.com> writes:
> >> 
> >> > Hi,
> >> >
> >> > * Jouni Hogander <jouni.hogander@nokia.com> [080516 03:57]:
> >> >> Add common omap2/3 function to check wether there is irq pending.
> >> >> Switch to use it in omap2 pm code instead of its own.
> >> >> 
> >> >> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
> >> >> ---
> >> >>  arch/arm/mach-omap2/irq.c        |   29 +++++++++++++++++++++++------
> >> >>  arch/arm/mach-omap2/pm24xx.c     |   19 +++----------------
> >> >>  include/asm-arm/arch-omap/irqs.h |    1 +
> >> >>  3 files changed, 27 insertions(+), 22 deletions(-)
> >> >> 
> >> >> diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
> >> >> index ac062ee..f1e1e2e 100644
> >> >> --- a/arch/arm/mach-omap2/irq.c
> >> >> +++ b/arch/arm/mach-omap2/irq.c
> >> >> @@ -20,12 +20,13 @@
> >> >>  
> >> >>  /* selected INTC register offsets */
> >> >>  
> >> >> -#define INTC_REVISION	0x0000
> >> >> -#define INTC_SYSCONFIG	0x0010
> >> >> -#define INTC_SYSSTATUS	0x0014
> >> >> -#define INTC_CONTROL	0x0048
> >> >> -#define INTC_MIR_CLEAR0	0x0088
> >> >> -#define INTC_MIR_SET0	0x008c
> >> >> +#define INTC_REVISION		0x0000
> >> >> +#define INTC_SYSCONFIG		0x0010
> >> >> +#define INTC_SYSSTATUS		0x0014
> >> >> +#define INTC_CONTROL		0x0048
> >> >> +#define INTC_MIR_CLEAR0		0x0088
> >> >> +#define INTC_MIR_SET0		0x008c
> >> >> +#define INTC_PENDING_IRQ0	0x0098
> >> >>  
> >> >>  /*
> >> >>   * OMAP2 has a number of different interrupt controllers, each interrupt
> >> >> @@ -122,6 +123,22 @@ static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
> >> >>  	intc_bank_write_reg(1 << 0, bank, INTC_SYSCONFIG);
> >> >>  }
> >> >>  
> >> >> +int omap_irq_pending(void)
> >> >> +{
> >> >> +	int i;
> >> >> +
> >> >> +	for (i = 0; i < ARRAY_SIZE(irq_banks); i++) {
> >> >> +		struct omap_irq_bank *bank = irq_banks + i;
> >> >> +		int irq;
> >> >> +
> >> >> +		for (irq = 0; irq < bank->nr_irqs; irq += 32)
> >> >> +			if (intc_bank_read_reg(bank, INTC_PENDING_IRQ0 +
> >> >> +					       ((irq >> 5) << 5)))
> >> >> +				return 1;
> >> >> +	}
> >> >> +	return 0;
> >> >> +}
> >> >> +
> >> >
> >> > In this case it should be enough to know if anything is set in the
> >> > bank registers, so you should be able to leave out the second for loop.
> >> > At most you need to read only the four bank registers. No need to check
> >> > for individual interrupts.
> >> 
> >> Current code is presenting each interrupt controller as a bank. I
> >> think in TRM they are talking about banks in each controller. So there
> >> is three banks in mpu intc 0..2. This is in arch/arm/mach-omap2/irq.c:
> >> 
> >>  * OMAP2 has a number of different interrupt controllers, each interrupt
> >>  * controller is identified as its own "bank". Register definitions are
> >>  * fairly consistent for each bank, but not all registers are implemented
> >>  * for each bank.. when in doubt, consult the TRM.
> >>  */
> >> 
> >> So the first loop is going through controllers which is only mpu
> >> intc. Second loop is going through bank registers. I'm using irq only
> >> for finding out right offset for bank register. This is another
> >> implementation. Which does exactly same thing:
> >> 
> >> int omap_irq_pending(void)
> >> {
> >> 	int i, j;
> >> 
> >> 	for (i = 0; i < ARRAY_SIZE(irq_banks); i++) {
> >> 		struct omap_irq_bank *bank = irq_banks + i;
> >> 
> >> 		for (j = 0; j < 3; j++)
> >> 			if (intc_bank_read_reg(bank, INTC_PENDING_IRQ0 +
> >> 					       0x20 * j))
> >> 				return 1;
> >> 	}
> >> 	return 0;
> >> }
> >> 
> >> If you think this is better I can change it. In first version I was
> >> just trying to follow practices used in irq.c for finding out offsets.
> >
> > Hmm, AFAIK, something like below should be enough to see if any interrupts
> > are pending (patch untested). See also
> > include/asm-arm/arch-omap/entry-macro.S> and what pm.c was doing.
> 
> No it doesn't work. It would work if irq_banks were presenting banks
> instead of controllers. See my comments below.
> 
> >
> > Regards,
> >
> > Tony
> >
> >
> >
> > diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
> > index ac062ee..aef28ab 100644
> > --- a/arch/arm/mach-omap2/irq.c
> > +++ b/arch/arm/mach-omap2/irq.c
> > @@ -26,6 +26,7 @@
> >  #define INTC_CONTROL	0x0048
> >  #define INTC_MIR_CLEAR0	0x0088
> >  #define INTC_MIR_SET0	0x008c
> > +#define INTC_PENDING	0x0098
> >  
> >  /*
> >   * OMAP2 has a number of different interrupt controllers, each interrupt
> > @@ -122,6 +123,20 @@ static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
> >  	intc_bank_write_reg(1 << 0, bank, INTC_SYSCONFIG);
> >  }
> >  
> > +int omap_irq_pending(void)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(irq_banks); i++) {
> 
> ARRAY_SIZE(irq_banks) == 1, because there is only mpu intc in
> irq_banks. irq_banks contains interrupt controllers, so we need to
> loop through them and loop each pending (bank) register inside these
> controllers.

Ah, OK. Yes you're right there is only one bank in irq_banks. And that's
why you still need to calculate the register offset with (i >> 5) << 5.
And you're increasing the for loop by 32, not by 1, I guess that's
where I originally misread your patch and got confused.

Let's just use your original patch then.

Tony


> 
> > +		struct omap_irq_bank *bank = irq_banks + i;
> > +
> > +		if (intc_bank_read_reg(bank, INTC_PENDING))
> > +			return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Your function reads only first INTC_PENDING register. There are three
> of them in mpu intc.
> 
> > +
> >  void __init omap_init_irq(void)
> >  {
> >  	unsigned long nr_irqs = 0;
> >
> 
> -- 
> Jouni Högander
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html