Hi Nathan,
On 04/25/2012 06:12 AM, Nathan Hintz wrote:
> Hi Hauke:
>
> On Wed, 2012-04-25 at 00:15 +0200, Hauke Mehrtens wrote:
>> Hi Nathan,
>>
>> It would make reviewing the patches a loot easier if you just but one
>> thing into one patch. This patch changes a loot of different things that
>> do not have many thing to do with each other. some part like
>> bcma_core_irq() I understand and would like to apply and some other
>> parts I do not understand.
> I can split out the code related to "bcma_fix_i2s_irqflag" into a
> separate patch, and maybe the calls to ""bcma_core_mips_set_irq(core,
> 0)" for the CC and I2S cores; but the remaining seems relevant to me.
> If it's more than that, can you provide additional feedback? Do you
> want the patch files posted independently to the mailing list, even
> though there may be order dependencies?
Making two patches out of this would be enough. Sending them in separate
mails makes it easier for me.
>>
>> On 04/10/2012 05:56 AM, Nathan Hintz wrote:
>>> Changes since v3: Fixed two references to bcma_core_mips_irq in
>>> driver_pci_host.c
>>>
>>> Signed-off-by: Nathan Hintz <nlhi...@hotmail.com>
>>>
>>> --- /dev/null 2012-04-07 22:03:23.120698218 -0700
>>> +++ target/linux/brcm47xx/patches-3.2/235-bcma-dont-expose-mips-irq.patch
>>> 2012-04-09 20:18:16.988877496 -0700
>>> @@ -0,0 +1,95 @@
>>> +--- a/include/linux/bcma/bcma.h
>>> ++++ b/include/linux/bcma/bcma.h
>>> +@@ -295,6 +295,7 @@ extern struct bcma_device *bcma_find_cor
>>> + extern bool bcma_core_is_enabled(struct bcma_device *core);
>>> + extern void bcma_core_disable(struct bcma_device *core, u32 flags);
>>> + extern int bcma_core_enable(struct bcma_device *core, u32 flags);
>>> ++extern unsigned int bcma_core_irq(struct bcma_device *core);
>>> + extern void bcma_core_set_clockmode(struct bcma_device *core,
>>> + enum bcma_clkmode clkmode);
>>> + extern void bcma_core_pll_ctl(struct bcma_device *core, u32 req, u32
>>> status,
>>> +--- a/include/linux/bcma/bcma_driver_mips.h
>>> ++++ b/include/linux/bcma/bcma_driver_mips.h
>>> +@@ -46,6 +46,4 @@ static inline void bcma_core_mips_init(s
>>> +
>>> + extern u32 bcma_cpu_clock(struct bcma_drv_mips *mcore);
>>> +
>>> +-extern unsigned int bcma_core_mips_irq(struct bcma_device *dev);
>>> +-
>>> + #endif /* LINUX_BCMA_DRIVER_MIPS_H_ */
>>> +--- a/arch/mips/bcm47xx/gpio.c
>>> ++++ b/arch/mips/bcm47xx/gpio.c
>>> +@@ -94,7 +94,7 @@ int gpio_to_irq(unsigned gpio)
>>> + #endif
>>> + #ifdef CONFIG_BCM47XX_BCMA
>>> + case BCM47XX_BUS_TYPE_BCMA:
>>> +- return bcma_core_mips_irq(bcm47xx_bus.bcma.bus.drv_cc.core) + 2;
>>> ++ return bcma_core_irq(bcm47xx_bus.bcma.bus.drv_cc.core);
>>> + #endif
>>> + }
>>> + return -EINVAL;
>>> +--- a/arch/mips/bcm47xx/serial.c
>>> ++++ b/arch/mips/bcm47xx/serial.c
>>> +@@ -62,7 +62,7 @@ static int __init uart8250_init_bcma(voi
>>> +
>>> + p->mapbase = (unsigned int) bcma_port->regs;
>>> + p->membase = (void *) bcma_port->regs;
>>> +- p->irq = bcma_port->irq + 2;
>>> ++ p->irq = bcma_port->irq;
>>> + p->uartclk = bcma_port->baud_base;
>>> + p->regshift = bcma_port->reg_shift;
>>> + p->iotype = UPIO_MEM;
>>> +--- a/drivers/bcma/driver_chipcommon.c
>>> ++++ b/drivers/bcma/driver_chipcommon.c
>>> +@@ -147,7 +147,7 @@ void bcma_chipco_serial_init(struct bcma
>>> + return;
>>> + }
>>> +
>>> +- irq = bcma_core_mips_irq(cc->core);
>>> ++ irq = bcma_core_irq(cc->core);
>>> +
>>> + /* Determine the registers of the UARTs */
>>> + cc->nr_serial_ports = (cc->capabilities & BCMA_CC_CAP_NRUART);
>>> +--- a/drivers/bcma/driver_mips.c
>>> ++++ b/drivers/bcma/driver_mips.c
>>> +@@ -81,7 +81,7 @@ static u32 bcma_core_mips_irqflag(struct
>>> + /* Get the MIPS IRQ assignment for a specified device.
>>> + * If unassigned, 0 is returned.
>>> + */
>>> +-unsigned int bcma_core_mips_irq(struct bcma_device *dev)
>>> ++static unsigned int bcma_core_mips_irq(struct bcma_device *dev)
>>> + {
>>> + struct bcma_device *mdev = dev->bus->drv_mips.core;
>>> + u32 irqflag;
>>> +@@ -96,7 +96,11 @@ unsigned int bcma_core_mips_irq(struct b
>>> +
>>> + return 0;
>>> + }
>>> +-EXPORT_SYMBOL(bcma_core_mips_irq);
>>> ++
>>> ++unsigned int bcma_core_irq(struct bcma_device *dev) {
>>> ++ return bcma_core_mips_irq(dev) + 2;
>>> ++}
>>> ++EXPORT_SYMBOL(bcma_core_irq);
>>> +
>>> + static void bcma_core_mips_set_irq(struct bcma_device *dev, unsigned int
>>> irq)
>>> + {
>>> +--- a/drivers/bcma/driver_pci_host.c
>>> ++++ b/drivers/bcma/driver_pci_host.c
>>> +@@ -565,7 +565,7 @@
>>> + pr_info("PCI: Fixing up device %s\n", pci_name(dev));
>>> +
>>> + /* Fix up interrupt lines */
>>> +- dev->irq = bcma_core_mips_irq(pc_host->pdev->core) + 2;
>>> ++ dev->irq = bcma_core_irq(pc_host->pdev->core);
>>> + pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
>>> +
>>> + return 0;
>>> +@@ -584,6 +584,6 @@
>>> +
>>> + pc_host = container_of(dev->bus->ops, struct bcma_drv_pci_host,
>>> + pci_ops);
>>> +- return bcma_core_mips_irq(pc_host->pdev->core) + 2;
>>> ++ return bcma_core_irq(pc_host->pdev->core);
>>> + }
>>> + EXPORT_SYMBOL(bcma_core_pci_pcibios_map_irq);
>>> --- /dev/null 2012-03-21 21:16:41.055325718 -0700
>>> +++ target/linux/brcm47xx/patches-3.2/237-bcma-fix-irq-assignment.patch
>>> 2012-03-29 21:05:49.189660716 -0700
>>> @@ -0,0 +1,173 @@
>>> +--- a/include/linux/bcma/bcma_driver_mips.h
>>> ++++ b/include/linux/bcma/bcma_driver_mips.h
>>> +@@ -28,6 +28,7 @@
>>> + #define BCMA_MIPS_MIPS74K_GPIOEN 0x0048
>>> + #define BCMA_MIPS_MIPS74K_CLKCTLST 0x01E0
>>> +
>>> ++#define BCMA_MIPS_OOBSELINA74 0x004
>>> + #define BCMA_MIPS_OOBSELOUTA30 0x100
>>> +
>>> + struct bcma_device;
>>> +--- 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,11 @@ static u32 bcma_core_mips_irqflag(struct
>>> + return dev->core_index;
>>> + flag = bcma_aread32(dev, BCMA_MIPS_OOBSELOUTA30);
>>> +
>>> +- return flag & 0x1F;
>>> ++ return ((flag && ((flag & IRQ_FLAG_MASK) <= 7)) ? (flag &
>>> IRQ_FLAG_MASK) : INVALID_IRQ_FLAG);
>> For what core do you get INVALID_IRQ_FLAG?
> A value of INVALID_IRQ_FLAG is not expected to be read from any core,
> valid IRQ Flags should always be <= 31. INVALID_IRQ_FLAG is just an
> arbitrary (internal) value to indicate a valid IRQ Flag was not read for
> the core.
Ah ok, the code looks strange to me. Why not something like this:
if (flag && (flag & IRQ_FLAG_MASK) <= 7)
return flag & IRQ_FLAG_MASK;
else
return INVALID_IRQ_FLAG;
It should do the same thing and I think it is much more readable.
>>> + }
>>> +
>>> + /* 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,21 +92,24 @@ 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;
>>> + }
>>> +
>>> + 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;
>>> +@@ -110,7 +117,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;
>>> +
>>> + dev->irq = irq + 2;
>>> +
>>> +@@ -119,8 +127,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) {
>>> +@@ -149,7 +157,9 @@ static void bcma_core_mips_set_irq(struc
>>> + }
>>> +
>>> + pr_info("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)
>>> +@@ -226,6 +236,27 @@ static void bcma_core_mips_flash_detect(
>>> + }
>>> + }
>>> +
>>> ++static void bcma_fix_i2s_irqflag(struct bcma_bus *bus) {
>>> ++ struct bcma_device *cpu, *pcie, *i2s;
>>> ++
>>> ++ /* IRQ flags >= 8 are not honored in the IRQ masks (2010 Broadcom SDK)
>>> */
>>> ++ if (bus->chipinfo.id != 0x4716 &&
>>> ++ bus->chipinfo.id != 0x4748)
>>> ++ return;
>>> ++
>>> ++ if ((cpu = bcma_find_core(bus, BCMA_CORE_MIPS_74K)) &&
>>> ++ (pcie = bcma_find_core(bus, BCMA_CORE_PCIE)) &&
>>> ++ (i2s = bcma_find_core(bus, BCMA_CORE_I2S))) {
>>> ++ if (bcma_aread32(cpu, BCMA_MIPS_OOBSELINA74) == 0x08060504 &&
>>> ++ bcma_aread32(pcie, BCMA_MIPS_OOBSELINA74) == 0x08060504 &&
>>> ++ bcma_aread32(i2s, BCMA_MIPS_OOBSELOUTA30) == 0x88) {
>>> ++ bcma_awrite32(cpu, BCMA_MIPS_OOBSELINA74, 0x07060504);
>>> ++ bcma_awrite32(pcie, BCMA_MIPS_OOBSELINA74, 0x07060504);
>>> ++ bcma_awrite32(i2s, BCMA_MIPS_OOBSELOUTA30, 0x87);
>>> ++ }
>>> ++ }
>>> ++}
>>> ++
>> In the Broadcom SDK this code is guarded by #ifdef _CFE_ as I understand
>> it this means that it will just be compiled into the cfe. Do you need
>> this code for your device? Don't you have a CFE boot loader on your device?
> The testing I have done has demonstrated that IRQ Flags > 7 are not
> supported. Any attempt to set an IRQ Mask using an IRQ Flag value > 7
> is ignored by the device; that is, the IRQ Mask remains un-set for that
> IRQ Flag. I scratched my head for quite some time when I found that the
> call to "bcma_core_mips_set_irq(core, 0)" for "BCMA_CORE_I2S" was not
> working. I finally found the code in the Broadcom SDK that adjusts the
> IRQ Flag value from 8 to 7 (in the CFE). Unfortunately, my device has
> an older CFE version, so the code to make the adjustment is not present.
> That's why I added it here. As to how important it is to set the IRQ
> Mask properly for the I2S Core, I don't have an answer for that, but I
> haven't noticed any bad behavior. Would this improve performance in
> some way?
Ok then we should add that code.
>>
>>> + void bcma_core_mips_init(struct bcma_drv_mips *mcore)
>>> + {
>>> + struct bcma_bus *bus;
>>> +@@ -234,12 +265,14 @@ void bcma_core_mips_init(struct bcma_drv
>>> +
>>> + pr_info("Initializing MIPS core...\n");
>>> +
>>> ++ bcma_fix_i2s_irqflag(bus);
>>> ++
>>> + if (!mcore->setup_done)
>>> + mcore->assigned_irqs = 1;
>>> +
>>> + /* Assign IRQs to all cores on the bus */
>>> + list_for_each_entry_reverse(core, &bus->cores, list) {
>>> +- int mips_irq;
>>> ++ unsigned int mips_irq;
>>> + if (core->irq)
>>> + continue;
>>> +
>>> +@@ -248,8 +281,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:
>>> +@@ -261,9 +293,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);
>>> ++ break;
>>> ++ case BCMA_CORE_CHIPCOMMON:
>>> ++ case BCMA_CORE_I2S:
>>> ++ bcma_core_mips_set_irq(core, 0);
>> This looks good to me, expect for the formating.
> Please elaborate on the formatting, as I'm oblivious to the problem. I
> will fix if you will point me to the problem area(s).
This code should formated acordingly to the Linux kernel formating guide
see Documentation/CodingStyle
See:
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
>>> + break;
>>> + }
>>> + }
>>>
>>> _______________________________________________
>>> openwrt-devel mailing list
>>> openwrt-devel@lists.openwrt.org
>>> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
>>
>>
> Thanks for reviewing,
>
> Nathan
>
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
