On 12/14/2012 06:58 AM, Nathan Hintz wrote:
> 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 <[email protected]>
>>>
>>> --- /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;
Yes this would make it clear what you want to do here, but I do not have
the documentation for the chip and can not say if it is correct. Have
you ever seen the validity mask not being set? Does this fix and problem
you have seen?
>>
>>> +
>>> + /* 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 meant changing this from returning 0 to something else is a good idea.
> I can return
> "-Esomething" if I change the signature of bcma_core_mips_irq to return
> a signed value.
Yes using a signed int and returning some negative value as an error
code is better.
>>> +
>>> + 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).
Have you ever seen a core with an invalid irq flag?
>>> +
>>> + 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).
Yes you are right then just do a break when bcma_core_mips_set_irq() failed.
>>
>>> ++ }
>>> ++ 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.
Yes that sounds good.
>>
>>> ++ bcma_core_mips_set_irq(core, 0);
>>> + break;
>>> + }
>>> + }
>>
>>
>
>
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
