Re: [PATCH] MIPS: PowerTV: Use fls() carefully where static optimization is required

[Shinya Kuribayashi <skuribay@pobox.com>]

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();