On Thu, 2012-12-13 at 19:10 +0100, Hauke Mehrtens wrote:
> On 12/11/2012 05:32 AM, Nathan Hintz wrote:
> > Enable use of IRQ6 for PCIE core on 4716 (Linksys E3000), correct
> > resetting of previous IRQ (oldirq), and insure that
> > bcma_core_set_mips_irq is called for PCIE, Chipcommon and I2S cores.
>
> Could you split this into more than one patch each addressing one issue?
>
> It makes the patch hard to understand that it tries to fix more than one
> issue. You should also use -1 (or better -Esomething) for error cases
> and use 0 as the shared irq number and not INVALID_IRQ_FLAG.
>
I did split it up some from the last version of the patch, but sounds
like it was not enough. Part of it is OBE anyway, as the second issue
is now corrected in the 3.6 generic patches (025-bcma_backport.patch).
I think I could split it up some more so it will be easier to review and
submit upstream if it get's to a point where it is acceptable.
> >
> > Signed-off-by: Nathan Hintz <nlhi...@hotmail.com>
> >
> > --- /dev/null
> > +++ target/linux/brcm47xx/patches-3.6/236-bcma-fix-irq-assignment.patch
> > @@ -0,0 +1,132 @@
> > +--- a/drivers/bcma/driver_mips.c
> > ++++ b/drivers/bcma/driver_mips.c
> > +@@ -65,6 +65,10 @@ static const u32 ipsflag_irq_shift[] = {
> > + BCMA_MIPS_IPSFLAG_IRQ4_SHIFT,
> > + };
> > +
> > ++#define IRQ_FLAG_MASK 0x1F
> > ++#define INVALID_IRQ_FLAG 0x3F
> > ++#define IRQ_NOT_ASSIGNED 5
> > ++#define IRQ_NOT_REQUIRED 6
> > + static u32 bcma_core_mips_irqflag(struct bcma_device *dev)
> > + {
> > + u32 flag;
> > +@@ -75,11 +79,14 @@ static u32 bcma_core_mips_irqflag(struct
> > + return dev->core_index;
> > + flag = bcma_aread32(dev, BCMA_MIPS_OOBSELOUTA30);
> > +
> > +- return flag & 0x1F;
> > ++ if (flag)
> > ++ return flag & IRQ_FLAG_MASK;
> > ++ else
> > ++ return INVALID_IRQ_FLAG;
> > + }
>
> flags == 0 is IRQ2, the shared irq, why do you return 0x3F in that case?
This only returns "INVALID_IRQ_FLAG" if the entire value read from
"BCMA_MIPS_OOBSELOUTA30" is "0" (not just the lowest 5 bits). From what
I could tell, the lowest 5 bits of the value read from
"BCMA_MIPS_OOBSELOUTA30" only contains a valid IRQ # if bit 7 is set as
well (e.g., 0x8z where z=0..7). Is there documentation on what the
meaning of the higher order bits really are? It might be a little more
obvious if it was changed to be something like this:
/* check for validity */
#define IRQ_FLAG_VALIDITY_MASK 0x80
if (flag & IRQ_FLAG_VALIDITY_MASK)
return flag & IRQ_FLAG_MASK;
else
return INVALID_IRQ_FLAG;
>
> > +
> > + /* Get the MIPS IRQ assignment for a specified device.
> > +- * If unassigned, 0 is returned.
> > ++ * If unassigned, 5 is returned; if no IRQ is required, 6 is returned
> > + */
> > + static unsigned int bcma_core_mips_irq(struct bcma_device *dev)
> > + {
> > +@@ -88,22 +95,25 @@ static unsigned int bcma_core_mips_irq(s
> > + unsigned int irq;
> > +
> > + irqflag = bcma_core_mips_irqflag(dev);
> > ++ if (irqflag == INVALID_IRQ_FLAG)
> > ++ return IRQ_NOT_REQUIRED;
> > +
> > +- for (irq = 1; irq <= 4; irq++)
> > ++ for (irq = 0; irq <= 4; irq++)
> > + if (bcma_read32(mdev, BCMA_MIPS_MIPS74K_INTMASK(irq)) &
> > + (1 << irqflag))
> > + return irq;
> > +
> > +- return 0;
> > ++ return IRQ_NOT_ASSIGNED;
> > + }
>
> Why not return -Esomething if the dev does not have an irq assigned, 0
> is not a good value as this is the shared irq.
>
I'm not sure I understand this, as it is not returning "0" in the
patched form; IRQ_NOT_ASSIGNED has a value of "5". I can return
"-Esomething" if I change the signature of bcma_core_mips_irq to return
a signed value.
> > +
> > + unsigned int bcma_core_irq(struct bcma_device *dev)
> > + {
> > +- return bcma_core_mips_irq(dev) + 2;
> > ++ unsigned int irq = bcma_core_mips_irq(dev);
> > ++ return (irq > 4) ? 0 : (irq + 2);
> > + }
> > + EXPORT_SYMBOL(bcma_core_irq);
> > +
> > +-static void bcma_core_mips_set_irq(struct bcma_device *dev, unsigned int
> > irq)
> > ++static bool bcma_core_mips_set_irq(struct bcma_device *dev, unsigned int
> > irq)
> > + {
> > + unsigned int oldirq = bcma_core_mips_irq(dev);
> > + struct bcma_bus *bus = dev->bus;
> > +@@ -111,7 +121,8 @@ static void bcma_core_mips_set_irq(struc
> > + u32 irqflag;
> > +
> > + irqflag = bcma_core_mips_irqflag(dev);
> > +- BUG_ON(oldirq == 6);
> > ++ if (irqflag == INVALID_IRQ_FLAG)
> > ++ return false;
>
> Why is it now impossible to set some core with irq 0 (the shared one) to
> some own irq number?
>
I don't think this is the case (a core with an INVALID_IRQ_FLAG
shouldn't be assigned an IRQ at all).
> > +
> > + dev->irq = irq + 2;
> > +
> > +@@ -120,8 +131,8 @@ static void bcma_core_mips_set_irq(struc
> > + bcma_write32(mdev, BCMA_MIPS_MIPS74K_INTMASK(0),
> > + bcma_read32(mdev, BCMA_MIPS_MIPS74K_INTMASK(0)) &
> > + ~(1 << irqflag));
> > +- else
> > +- bcma_write32(mdev, BCMA_MIPS_MIPS74K_INTMASK(irq), 0);
> > ++ else if (oldirq <= 4)
> > ++ bcma_write32(mdev, BCMA_MIPS_MIPS74K_INTMASK(oldirq), 0);
> > +
> > + /* assign the new one */
> > + if (irq == 0) {
> > +@@ -150,7 +161,9 @@ static void bcma_core_mips_set_irq(struc
> > + }
> > +
> > + bcma_info(bus, "set_irq: core 0x%04x, irq %d => %d\n",
> > +- dev->id.id, oldirq + 2, irq + 2);
> > ++ dev->id.id, (oldirq > 4) ? 0 : (oldirq + 2), irq + 2);
> > ++
> > ++ return true;
> > + }
> > +
> > + static void bcma_core_mips_print_irq(struct bcma_device *dev, unsigned
> > int irq)
> > +@@ -266,7 +279,7 @@ void bcma_core_mips_init(struct bcma_drv
> > +
> > + /* Assign IRQs to all cores on the bus */
> > + list_for_each_entry(core, &bus->cores, list) {
> > +- int mips_irq;
> > ++ unsigned int mips_irq;
> > + if (core->irq)
> > + continue;
> > +
> > +@@ -275,8 +288,7 @@ void bcma_core_mips_init(struct bcma_drv
> > + core->irq = 0;
> > + else
> > + core->irq = mips_irq + 2;
> > +- if (core->irq > 5)
> > +- continue;
> > ++
> > + switch (core->id.id) {
> > + case BCMA_CORE_PCI:
> > + case BCMA_CORE_PCIE:
> > +@@ -288,9 +300,17 @@ void bcma_core_mips_init(struct bcma_drv
> > + /* These devices get their own IRQ line if available,
> > + * the rest goes on IRQ0
> > + */
> > +- if (mcore->assigned_irqs <= 4)
> > +- bcma_core_mips_set_irq(core,
> > +- mcore->assigned_irqs++);
> > ++ if (mcore->assigned_irqs <= 4) {
> > ++ if (bcma_core_mips_set_irq(core,
> > ++
> > mcore->assigned_irqs))
> > ++ mcore->assigned_irqs++;
> > ++ } else {
> > ++ bcma_core_mips_set_irq(core, 0);
> Please change this to a fall through, but I do not know why you do not
> what to assign an irq to a core which did not had one before.
>
> if (mcore->assigned_irqs <= 4) {
> if (bcma_core_mips_set_irq(core,
> mcore->assigned_irqs)) {
> mcore->assigned_irqs++;
> break;
> }
> }
The only reason "bcma_core_mips_set_irq" would fail is if the core had
an INVALID_IRQ_FLAG; meaning the core shouldn't have an IRQ assigned at
all. I think falling through in that case would not be the right thing
(although I guess it would just fail again when it tries to set it to
IRQ 0 on the fall through).
>
> > ++ }
> > ++ break;
> > ++ case BCMA_CORE_CHIPCOMMON:
> > ++ case BCMA_CORE_I2S:
>
> Use default here to assign irq 0 to all the other devices.
I think a default would be okay here if the core has a valid IRQ FLAG.
Perhaps it should first verify the core has a valid IRQ FLAG at the very
start of the loop, then all of the checking along the way wouldn't be
required.
>
> > ++ bcma_core_mips_set_irq(core, 0);
> > + break;
> > + }
> > + }
>
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
