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.
>
> 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?
> +
> + /* 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.
> +
> + 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?
> +
> + 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;
}
}
> ++ }
> ++ break;
> ++ case BCMA_CORE_CHIPCOMMON:
> ++ case BCMA_CORE_I2S:
Use default here to assign irq 0 to all the other devices.
> ++ 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
