Hi David,
Before submitting irq_ffs() cleanup patchset, it seems I/we need to sort
out PowerTV's IRQ code properly. Please find my comments below.
On 07/02/2010 11:13 PM, Shinya Kuribayashi wrote:
> On 07/01/2010 07:01 AM, David VomLehn wrote:
>> Thanks! You are correct in your analysis and make a good point that
>> clz should be used in interrupt handling. I think, though, that it's
>> better to go ahead and supply a full-blown cpu-features-override.h
>> rather than focusing on this one case. This way fls() will be optimized
>> to use clz everywhere and any other optimizations that depend on constant
>> cpu_has_* values will also be used.
>
> Your choice, either one will be fine :-)
Double-checking the generated code, current PowerTV IRQ code is slightly
different from what I expected:
1)
PowerTV doesn't use mips_cpu_irq_init(), so IRQ #0-7 are not allocated
for MIPS CPU IRQs, but used for its ASIC interrupts. All irq_desc[0..
126] (NR_IRQS=127) are meant for ASIC interrupts, a bit surprising ;-)
Presumably it intentionally skips primary CP0.Status decoding. Just
check CP0.Status, and if it's flagged, then jump into ASIC dispatcher
directly.
2) PowerTV's irq_ffs() behaves differently from Malta or MISPSim one.
Without CLZ optimization, current PowerTV's irq_ffs() returns:
PowerTV
-------
status=0x 100 => 9
status=0x 300 => 10
status=0x 700 => 11
status=0x f00 => 12
status=0x1f00 => 13
status=0x3f00 => 14
status=0x7f00 => 15
status=0xff00 => 16
while Malta and MIPSSim would return:
Malta, MIPSSim
-------
status=0x 100 => 0
status=0x 300 => 1
status=0x 700 => 2
status=0x f00 => 3
status=0x1f00 => 4
status=0x3f00 => 5
status=0x7f00 => 6
status=0xff00 => 7
3)
In addition to 2), the most questionable part is (irq == CAUSEF_IP3):
> asmlinkage void plat_irq_dispatch(void)
> {
> unsigned int pending = read_c0_cause() & read_c0_status() & ST0_IM;
> int irq;
>
> irq = irq_ffs(pending);
>
> if (irq == CAUSEF_IP3)
> asic_irqdispatch();
> else if (irq >= 0)
> do_IRQ(irq);
> else
> spurious_interrupt();
> }
CAUSEF_IP3 is 0x0800, while irq_ffs() returns (9..16). This implies
that asic_irqdispatch() is not used here, and all interrupts are
forwarded to 'else-if (irq >= 0) do_IRQ(irq);' path.
Remember that all irq_desc[0..126] are meant for ASIC interrupts, and
irq_ffs() returns (9..16). How do you handle rest of interrupts? I'm
lost here.
Taking a closer look, PowerTV has the code registering VI- or EIC-
handlers. Asic_irqdispatch() might be directly strapped via VI- or
EIC-mode, and plat_irq_dispatch() is not used completely, hmm.
---
For example, the patch like this still works for PowerTV?
I'd like to make sure whether PowerTV still require irq_ffs() or not,
as it prevents irq_ffs() consolidation patch from being submitted.
But no need to hurry, I can hold the patch for weeks, for months.
Shinya
diff --git a/arch/mips/powertv/asic/asic_int.c b/arch/mips/powertv/asic/asic_int.c
index 529c44a..2a8fd99 100644
--- a/arch/mips/powertv/asic/asic_int.c
+++ b/arch/mips/powertv/asic/asic_int.c
@@ -33,7 +33,6 @@
#include <asm/irq_cpu.h>
#include <linux/io.h>
-#include <asm/irq_regs.h>
#include <asm/mips-boards/generic.h>
#include <asm/mach-powertv/asic_regs.h>
@@ -68,40 +67,15 @@ static void asic_irqdispatch(void)
do_IRQ(irq);
}
-static inline int clz(unsigned long x)
-{
- __asm__(
- " .set push \n"
- " .set mips32 \n"
- " clz %0, %1 \n"
- " .set pop \n"
- : "=r" (x)
- : "r" (x));
-
- return x;
-}
-
-/*
- * Version of ffs that only looks at bits 12..15.
- */
-static inline unsigned int irq_ffs(unsigned int pending)
-{
- return fls(pending) - 1 + CAUSEB_IP;
-}
-
/*
* TODO: check how it works under EIC mode.
*/
asmlinkage void plat_irq_dispatch(void)
{
- unsigned int pending = read_c0_cause() & read_c0_status() & ST0_IM;
int irq;
- irq = irq_ffs(pending);
-
- if (irq == CAUSEF_IP3)
- asic_irqdispatch();
- else if (irq >= 0)
+ irq = get_int();
+ if (irq >= 0)
do_IRQ(irq);
else
spurious_interrupt();
