| Message ID | 1416796846-28149-7-git-send-email-cernekee@gmail.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Mon, Nov 24, 2014 at 3:40 AM, Kevin Cernekee <cernekee@gmail.com> wrote: > To date, all supported controllers have had the IRQEN register at offset > 0x00 and the IRQSTAT register at 0x04. So in DT we would typically see > something like: > > reg = <0xf0406800 0x8>; > > We still want to support this format, but we also need to support cases > where IRQEN and IRQSTAT aren't adjacent. So, we will amend the driver to > accept an alternate format that looks like this: > > reg = <0xf0406800 0x4 0xf0406804 0x4>; > > i.e. if the first resource_size() == 4, assume that the first resource > points to IRQEN and that the next resource points to IRQSTAT. If the > first resource_size() == 8, retain the current behavior. > > Signed-off-by: Kevin Cernekee <cernekee@gmail.com> Hmm ... the more I think about this, the less I like it. Using the amount and size of the reg-properties to infer a certain layout seems rather hackish and dirty to me. Maybe we should just use different compatible match ids for that? E.g. brcm,bm7120-l2-intc for the 32-bit en/stat pairs, and e.g. brcm,bcm6368-l2-intc for the 64-bit wide one. Or maybe make the bcm63xx one a separate driver and let it share code with the bcm7120-l2-intc driver. This would avoid having to specify a lot of regs (let's assume we also add support for affinity), and cause a lot of io(re)map calls - the bcm63268 one would currently look like: reg = <0x1000002c 0x4 0x1000003c 0x4>, /* irq 0..31 -> mips irq 2 */ <0x10000028 0x4 0x10000038 0x4>, /* irq 32..63 -> mips irq 2 */ <0x10000024 0x4 0x10000034 0x4>, /* irq 64 .. 95 -> mips irq 2 */ <0x10000020 0x4 0x10000030 0x4>, /* irq 96 .. 127 -> mips irq 2 */ <0x1000004c 0x4 0x1000005c 0x4>, /* irq 0.. 31 -> mips irq 3 */ <0x10000048 0x4 0x10000058 0x4>, /* irq 32 .. 63 -> mips irq 3 */ <0x10000044 0x4 0x10000054 0x4>, /* irq 64 ... 95 -> mips irq 3 */ <0x10000040 0x4 0x10000050 0x4>; /* irq 96 ... 127 -> mips irq 3 */ where as with a different match id, we could rather allow something like reg = <0x10000020 0x20>, /* irq 0..127 -> mips irq 2 */ <0x10000040 0x20>; /* irq 0..127 -> mips irq 3 */ This would make the dts(i) files quite a bit more readable IMHO, and make it more likely that newer dts(i) files work with older kernels, e.g. where the mips irq3 routed registers were added - in the current style, the kernel would interpret these as additional irq banks. Not that I think this is expected/required to work, but it wouldn't hurt having at least a bit of backward compatibility for bisecting on a device that provides a newer dtb through the bootloader. Jonas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 6:31 AM, Jonas Gorski <jogo@openwrt.org> wrote: > On Mon, Nov 24, 2014 at 3:40 AM, Kevin Cernekee <cernekee@gmail.com> wrote: >> To date, all supported controllers have had the IRQEN register at offset >> 0x00 and the IRQSTAT register at 0x04. So in DT we would typically see >> something like: >> >> reg = <0xf0406800 0x8>; >> >> We still want to support this format, but we also need to support cases >> where IRQEN and IRQSTAT aren't adjacent. So, we will amend the driver to >> accept an alternate format that looks like this: >> >> reg = <0xf0406800 0x4 0xf0406804 0x4>; >> >> i.e. if the first resource_size() == 4, assume that the first resource >> points to IRQEN and that the next resource points to IRQSTAT. If the >> first resource_size() == 8, retain the current behavior. >> >> Signed-off-by: Kevin Cernekee <cernekee@gmail.com> > > Hmm ... the more I think about this, the less I like it. > > Using the amount and size of the reg-properties to infer a certain > layout seems rather hackish and dirty to me. Maybe we should just use > different compatible match ids for that? E.g. brcm,bm7120-l2-intc for > the 32-bit en/stat pairs, and e.g. brcm,bcm6368-l2-intc for the 64-bit > wide one. Or maybe make the bcm63xx one a separate driver and let it > share code with the bcm7120-l2-intc driver. In my current proposal, it is easy to specify an arbitrary number of enable/status pairs located in any order and spread across different parts of the register space. Even register indices (0,2,4,...) are enable registers, and odd register indices are status registers. If I'm interpreting your post correctly, you don't agree that we need this level of flexibility. But looking over the register listings, here are the cases we need to cover: 6828,6318: 4x mask(exthi,extlo,hi,lo),status(exthi,extlo,hi,lo) TP0ExtIrqMaskHi TP0ExtIrqMaskLo TP0IrqMaskHi TP0IrqMaskLo TP0ExtIrqStatusHi TP0ExtIrqStatusLo TP0IrqStatusHi TP0IrqStatusLo TP1ExtIrqMaskHi TP1ExtIrqMaskLo ... 6816,6362,6328: 2x extmask,mask,extstatus,status ExtraChipIrqMask ChipIrqMask ExtraChipIrqStatus ChipIrqStatus ExtraChipIrqMask1 [1=TP1] ChipIrqMask1 ExtraChipIrqStatus1 ChipIrqStatus1 6368: similar to above, but with yet another naming convention: IRQMASK_MIPS0_Hi IRQMASK_MIPS0_Lo IRQSTATUS_MIPS0_Hi IRQSTATUS_MIPS0_Lo IRQMASK_MIPS1_Hi IRQMASK_MIPS1_Lo IRQSTATUS_MIPS1_Hi IRQSTATUS_MIPS1_Lo 6838,3384: interleaved "mystery meat" mask/status (same IRQ source names, with the output of each bcm7120-l2 routed to several different processors/threads): PeriphIRQMASK0 PeriphIRQSTATUS0 PeriphIRQMASK1 <- mine, if running on Zephyr PeriphIRQSTATUS1 <- mine, if running on Zephyr PeriphIRQMASK2 PeriphIRQSTATUS2 PeriphIRQMASK3 <- mine, if running on Viper PeriphIRQSTATUS3 <- mine, if running on Viper But wait, there's more! There wasn't enough space in this block for >32 IRQ bits, so the upper bits spilled over into a separate "INT_EXT_PER" block that lives elsewhere in the register space: PeriphIRQMASK0_2 PeriphIRQSTATUS0_2 PeriphIRQMASK1_2 <- mine, if running on Zephyr PeriphIRQSTATUS1_2 <- mine, if running on Zephyr PeriphIRQMASK2_2 PeriphIRQSTATUS2_2 PeriphIRQMASK3_2 <- mine, if running on Viper PeriphIRQSTATUS3_2 <- mine, if running on Viper The "INT_PER" and "INT_EXT_PER" outputs are ORed into the same MIPS IRQ lines, so we need to treat them as two sides of a single controller. AFAICT, unlike a shared device IRQ, there is no way to share a MIPS IRQ line between two controller instances. Additionally, we have a few other random MASK/STATUS pairs scattered around (ZMIPS, CMIPS blocks), and then we also have DQM IRQs with multiple mask registers + single status register: CMIPS_NOT_EMPTY_IRQ_MSK CMIPS1_NOT_EMPTY_IRQ_MSK <- mine, if running on Viper ZMIPS_NOT_EMPTY_IRQ_MSK <- mine, if running on Zephyr PMC_NOT_EMPTY_IRQ_MSK DFAP_NOT_EMPTY_IRQ_MSK NOT_EMPTY_IRQ_STS I suppose another alternative is to ioremap() a range large enough to cover enable + status, and then specify the offset of each one in another property. This does run the risk of overlapping mappings. > This would avoid having to specify a lot of regs (let's assume we also > add support for affinity) I concede that I have no idea how affinity should be handled here. AFAICT it is completely off limits on BCM3384 because we just don't have enough L2 outputs to offer proper masking for all of the threads/CPUs in the system. Perhaps we could write a simple, SMP-capable driver for the saner/newer SoCs, and use the flexible-but-non-SMP version for the complicated ones. > and cause a lot of io(re)map calls Is the ioremap() call really that big of a deal? On MIPS it's basically computing CKSEG1ADDR(phys_addr). On ARM, the top level (with 64 to 128+ IRQs) goes through the GIC. On both, ioremap() is a one-time cost at startup. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 6:18 AM, Kevin Cernekee <cernekee@gmail.com> wrote: > On Mon, Nov 24, 2014 at 6:31 AM, Jonas Gorski <jogo@openwrt.org> wrote: >> On Mon, Nov 24, 2014 at 3:40 AM, Kevin Cernekee <cernekee@gmail.com> wrote: >>> To date, all supported controllers have had the IRQEN register at offset >>> 0x00 and the IRQSTAT register at 0x04. So in DT we would typically see >>> something like: >>> >>> reg = <0xf0406800 0x8>; >>> >>> We still want to support this format, but we also need to support cases >>> where IRQEN and IRQSTAT aren't adjacent. So, we will amend the driver to >>> accept an alternate format that looks like this: >>> >>> reg = <0xf0406800 0x4 0xf0406804 0x4>; >>> >>> i.e. if the first resource_size() == 4, assume that the first resource >>> points to IRQEN and that the next resource points to IRQSTAT. If the >>> first resource_size() == 8, retain the current behavior. >>> >>> Signed-off-by: Kevin Cernekee <cernekee@gmail.com> >> >> Hmm ... the more I think about this, the less I like it. >> >> Using the amount and size of the reg-properties to infer a certain >> layout seems rather hackish and dirty to me. Maybe we should just use >> different compatible match ids for that? E.g. brcm,bm7120-l2-intc for >> the 32-bit en/stat pairs, and e.g. brcm,bcm6368-l2-intc for the 64-bit >> wide one. Or maybe make the bcm63xx one a separate driver and let it >> share code with the bcm7120-l2-intc driver. > > In my current proposal, it is easy to specify an arbitrary number of > enable/status pairs located in any order and spread across different > parts of the register space. Even register indices (0,2,4,...) are > enable registers, and odd register indices are status registers. > > If I'm interpreting your post correctly, you don't agree that we need > this level of flexibility. But looking over the register listings, > here are the cases we need to cover: > > > 6828,6318: 4x mask(exthi,extlo,hi,lo),status(exthi,extlo,hi,lo) > > TP0ExtIrqMaskHi > TP0ExtIrqMaskLo > TP0IrqMaskHi > TP0IrqMaskLo > TP0ExtIrqStatusHi > TP0ExtIrqStatusLo > TP0IrqStatusHi > TP0IrqStatusLo > > TP1ExtIrqMaskHi > TP1ExtIrqMaskLo > ... > > > 6816,6362,6328: 2x extmask,mask,extstatus,status > > ExtraChipIrqMask > ChipIrqMask > ExtraChipIrqStatus > ChipIrqStatus > ExtraChipIrqMask1 [1=TP1] > ChipIrqMask1 > ExtraChipIrqStatus1 > ChipIrqStatus1 > > > 6368: similar to above, but with yet another naming convention: > > IRQMASK_MIPS0_Hi > IRQMASK_MIPS0_Lo > IRQSTATUS_MIPS0_Hi > IRQSTATUS_MIPS0_Lo > IRQMASK_MIPS1_Hi > IRQMASK_MIPS1_Lo > IRQSTATUS_MIPS1_Hi > IRQSTATUS_MIPS1_Lo > > 6838,3384: interleaved "mystery meat" mask/status (same IRQ source > names, with the output of each bcm7120-l2 routed to several different > processors/threads): > > PeriphIRQMASK0 > PeriphIRQSTATUS0 > PeriphIRQMASK1 <- mine, if running on Zephyr > PeriphIRQSTATUS1 <- mine, if running on Zephyr > PeriphIRQMASK2 > PeriphIRQSTATUS2 > PeriphIRQMASK3 <- mine, if running on Viper > PeriphIRQSTATUS3 <- mine, if running on Viper > > But wait, there's more! There wasn't enough space in this block for >>32 IRQ bits, so the upper bits spilled over into a separate > "INT_EXT_PER" block that lives elsewhere in the register space: > > PeriphIRQMASK0_2 > PeriphIRQSTATUS0_2 > PeriphIRQMASK1_2 <- mine, if running on Zephyr > PeriphIRQSTATUS1_2 <- mine, if running on Zephyr > PeriphIRQMASK2_2 > PeriphIRQSTATUS2_2 > PeriphIRQMASK3_2 <- mine, if running on Viper > PeriphIRQSTATUS3_2 <- mine, if running on Viper > > The "INT_PER" and "INT_EXT_PER" outputs are ORed into the same MIPS > IRQ lines, so we need to treat them as two sides of a single > controller. AFAICT, unlike a shared device IRQ, there is no way to > share a MIPS IRQ line between two controller instances. > > Additionally, we have a few other random MASK/STATUS pairs scattered > around (ZMIPS, CMIPS blocks), and then we also have DQM IRQs with > multiple mask registers + single status register: > > CMIPS_NOT_EMPTY_IRQ_MSK > CMIPS1_NOT_EMPTY_IRQ_MSK <- mine, if running on Viper > ZMIPS_NOT_EMPTY_IRQ_MSK <- mine, if running on Zephyr > PMC_NOT_EMPTY_IRQ_MSK > DFAP_NOT_EMPTY_IRQ_MSK > NOT_EMPTY_IRQ_STS There are also 63268 and 6828, which follow the 6318/6328 layout; 6818, which follows the 6368 layout, as well as 6358 (Yes I know, very unlikely to ever get SMP support due to its stupid shared TLB design): IrqMask IrqStatus (several unrelated registers) IrqMask1 IrqStatus1 and 63381: IrqStatusHi IrqStatusLo ExtIrqStatusHi ExtIrqStatusLo IrqMask0Hi IrqMask0Lo ExtIrqMask0Hi ExtIrqMask0Lo IrqMask1Hi IrqMask1Lo ExtIrqMask1Hi ExtIrqMask1Lo I see also a 60333, which has three 32bit Mask/Status pairs per thread, also none of the higher irqs seem to be used according to _intr.h). > I suppose another alternative is to ioremap() a range large enough to > cover enable + status, and then specify the offset of each one in > another property. This does run the risk of overlapping mappings. > >> This would avoid having to specify a lot of regs (let's assume we also >> add support for affinity) > > I concede that I have no idea how affinity should be handled here. > AFAICT it is completely off limits on BCM3384 because we just don't > have enough L2 outputs to offer proper masking for all of the > threads/CPUs in the system. Affinity support is "easy"; expect one set of registers for each output irq specified, and if there is only one, then we don't support it / fill the affinity pointers. > Perhaps we could write a simple, SMP-capable driver for the > saner/newer SoCs, and use the flexible-but-non-SMP version for the > complicated ones. As far as I can see, we have three distinct layouts here: a) An arbitrary number of 32-bit Mask/Status-pairs (3384/6838). No per thread support (well, not sure about 60333). b) An arbitrary length (32 to 128 bit) Mask register, followed by a same length Status register (63xx except 63381, 6818, 6828); repeated for each thread. c) A single arbitrary length (currently only 128 bit) Status register, followed by per thread same length Mask registers (63381). On a first glance this could translate to three distinct drivers/compatible properties, where each expects different reg = <>; contents. For a), it should be enough to expand the current 7120-l2 driver to accept/use more than one 0x8 length register, which should simplify the register map setup. For b) we could add a a new compatible name (maybe bcm6358-l2, because that was AFAICT the first one with blocks) that will use the 8 to 32 byte length regs (one for each block). For now we could ignore the SMP capability of it and make it a variant of the 7120-l2 driver, and when we add SMP support, split it into a second different driver if we want to avoid having all the spinlock for register accesses even for a). We could then even easily document/add the extra block registers / interrrupts in documentation / the dtsi files before actually supporting them, because we only have a fixed amount of regs/irqs to expect in the !SMP case and can easily ignore the extra registers/interrupts. For c) we could add a a third compatible name (bcm63381-l2), also with its own setup routine. I would guess it doesn't matter if both thread's irqstatus register pointers point to the same region. >> and cause a lot of io(re)map calls > > Is the ioremap() call really that big of a deal? > > On MIPS it's basically computing CKSEG1ADDR(phys_addr). On ARM, the > top level (with 64 to 128+ IRQs) goes through the GIC. On both, > ioremap() is a one-time cost at startup. For a single driver it probably doesn't hurt that much, but AFAIK these are also stored in a table, so if all drivers did this, the table easily becomes huge. This isn't a very strong argument, just more a smaller nitpick. Jonas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski <jogo@openwrt.org> wrote: > As far as I can see, we have three distinct layouts here: > > a) An arbitrary number of 32-bit Mask/Status-pairs (3384/6838). No per > thread support (well, not sure about 60333). > > b) An arbitrary length (32 to 128 bit) Mask register, followed by a > same length Status register (63xx except 63381, 6818, 6828); repeated > for each thread. > > c) A single arbitrary length (currently only 128 bit) Status register, > followed by per thread same length Mask registers (63381). > > On a first glance this could translate to three distinct > drivers/compatible properties, where each expects different reg = <>; > contents. > > For a), it should be enough to expand the current 7120-l2 driver to > accept/use more than one 0x8 length register, which should simplify > the register map setup. > > For b) we could add a a new compatible name (maybe bcm6358-l2, because > that was AFAICT the first one with blocks) that will use the 8 to 32 > byte length regs (one for each block). For now we could ignore the SMP > capability of it and make it a variant of the 7120-l2 driver, and when > we add SMP support, split it into a second different driver if we want > to avoid having all the spinlock for register accesses even for a). > > We could then even easily document/add the extra block registers / > interrrupts in documentation / the dtsi files before actually > supporting them, because we only have a fixed amount of regs/irqs to > expect in the !SMP case and can easily ignore the extra > registers/interrupts. > > For c) we could add a a third compatible name (bcm63381-l2), also with > its own setup routine. I would guess it doesn't matter if both > thread's irqstatus register pointers point to the same region. This split-up is especially tempting to me after I had a closer look at the current 7120-l2 driver, which already accepts more than one interrupt, but uses it in a different way. So even if we try to make it very flexible with only one compatible property, reg = <irqstatus0 irqmask0 irqstatus1 irqmask1>; interrupts = <irq0>, <irq1>; Could then mean either irq0 is for interrupts 0..31 (mask/status0) and irq1 for interrupts 32 .. 64 (mask/status1), or irq0 is for interrupts 0..31 on cpu0, and irq1 is for interrupts 0..31 on cpu1, and then would require an additional property to tell them apart, for which we then also could just use a different compatible name, and have (IMHO) a lot less headache. (I wonder why we couldn't just have had more than one instance of 7120-l2 in the dts for the first case) Jonas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 2:25 AM, Jonas Gorski <jogo@openwrt.org> wrote: > On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski <jogo@openwrt.org> wrote: >> As far as I can see, we have three distinct layouts here: >> >> a) An arbitrary number of 32-bit Mask/Status-pairs (3384/6838). No per >> thread support (well, not sure about 60333). >> >> b) An arbitrary length (32 to 128 bit) Mask register, followed by a >> same length Status register (63xx except 63381, 6818, 6828); repeated >> for each thread. >> >> c) A single arbitrary length (currently only 128 bit) Status register, >> followed by per thread same length Mask registers (63381). >> >> On a first glance this could translate to three distinct >> drivers/compatible properties, where each expects different reg = <>; >> contents. >> >> For a), it should be enough to expand the current 7120-l2 driver to >> accept/use more than one 0x8 length register, which should simplify >> the register map setup. >> >> For b) we could add a a new compatible name (maybe bcm6358-l2, because >> that was AFAICT the first one with blocks) that will use the 8 to 32 >> byte length regs (one for each block). For now we could ignore the SMP >> capability of it and make it a variant of the 7120-l2 driver, and when >> we add SMP support, split it into a second different driver if we want >> to avoid having all the spinlock for register accesses even for a). >> >> We could then even easily document/add the extra block registers / >> interrrupts in documentation / the dtsi files before actually >> supporting them, because we only have a fixed amount of regs/irqs to >> expect in the !SMP case and can easily ignore the extra >> registers/interrupts. >> >> For c) we could add a a third compatible name (bcm63381-l2), also with >> its own setup routine. I would guess it doesn't matter if both >> thread's irqstatus register pointers point to the same region. > > This split-up is especially tempting to me after I had a closer look > at the current 7120-l2 driver, which already accepts more than one > interrupt, but uses it in a different way. So even if we try to make > it very flexible with only one compatible property, > > reg = <irqstatus0 irqmask0 irqstatus1 irqmask1>; > interrupts = <irq0>, <irq1>; > > Could then mean either irq0 is for interrupts 0..31 (mask/status0) and > irq1 for interrupts 32 .. 64 (mask/status1), or irq0 is for interrupts > 0..31 on cpu0, and irq1 is for interrupts 0..31 on cpu1, and then > would require an additional property to tell them apart, for which we > then also could just use a different compatible name, and have (IMHO) > a lot less headache. (I wonder why we couldn't just have had more > than one instance of 7120-l2 in the dts for the first case) I don't think we've used this driver to implement the first case yet. The initial use of the driver was for the BCM7xxx IRQ0 block, which is wired up according to the ASCII art diagram in Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt . i.e. different sets of bits in a single irqstatus0/irqmask0 pair map to different parent IRQs. The bits handled by each parent IRQ are indicated in the brcm,int-map-mask property. And now on BCM3384, of course, we're seeing the output from two 32-bit irqstatus/irqmask words ORed together into a single parent IRQ, for periph_intc. The other instances do have their own DT nodes. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 7:54 PM, Kevin Cernekee <cernekee@gmail.com> wrote: > On Wed, Nov 26, 2014 at 2:25 AM, Jonas Gorski <jogo@openwrt.org> wrote: >> On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski <jogo@openwrt.org> wrote: >>> As far as I can see, we have three distinct layouts here: >>> >>> a) An arbitrary number of 32-bit Mask/Status-pairs (3384/6838). No per >>> thread support (well, not sure about 60333). >>> >>> b) An arbitrary length (32 to 128 bit) Mask register, followed by a >>> same length Status register (63xx except 63381, 6818, 6828); repeated >>> for each thread. >>> >>> c) A single arbitrary length (currently only 128 bit) Status register, >>> followed by per thread same length Mask registers (63381). >>> >>> On a first glance this could translate to three distinct >>> drivers/compatible properties, where each expects different reg = <>; >>> contents. >>> >>> For a), it should be enough to expand the current 7120-l2 driver to >>> accept/use more than one 0x8 length register, which should simplify >>> the register map setup. >>> >>> For b) we could add a a new compatible name (maybe bcm6358-l2, because >>> that was AFAICT the first one with blocks) that will use the 8 to 32 >>> byte length regs (one for each block). For now we could ignore the SMP >>> capability of it and make it a variant of the 7120-l2 driver, and when >>> we add SMP support, split it into a second different driver if we want >>> to avoid having all the spinlock for register accesses even for a). >>> >>> We could then even easily document/add the extra block registers / >>> interrrupts in documentation / the dtsi files before actually >>> supporting them, because we only have a fixed amount of regs/irqs to >>> expect in the !SMP case and can easily ignore the extra >>> registers/interrupts. >>> >>> For c) we could add a a third compatible name (bcm63381-l2), also with >>> its own setup routine. I would guess it doesn't matter if both >>> thread's irqstatus register pointers point to the same region. >> >> This split-up is especially tempting to me after I had a closer look >> at the current 7120-l2 driver, which already accepts more than one >> interrupt, but uses it in a different way. So even if we try to make >> it very flexible with only one compatible property, >> >> reg = <irqstatus0 irqmask0 irqstatus1 irqmask1>; >> interrupts = <irq0>, <irq1>; >> >> Could then mean either irq0 is for interrupts 0..31 (mask/status0) and >> irq1 for interrupts 32 .. 64 (mask/status1), or irq0 is for interrupts >> 0..31 on cpu0, and irq1 is for interrupts 0..31 on cpu1, and then >> would require an additional property to tell them apart, for which we >> then also could just use a different compatible name, and have (IMHO) >> a lot less headache. (I wonder why we couldn't just have had more >> than one instance of 7120-l2 in the dts for the first case) > > I don't think we've used this driver to implement the first case yet. > > The initial use of the driver was for the BCM7xxx IRQ0 block, which is > wired up according to the ASCII art diagram in > Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt > . i.e. different sets of bits in a single irqstatus0/irqmask0 pair > map to different parent IRQs. The bits handled by each parent IRQ are > indicated in the brcm,int-map-mask property. > > And now on BCM3384, of course, we're seeing the output from two 32-bit > irqstatus/irqmask words ORed together into a single parent IRQ, for > periph_intc. The other instances do have their own DT nodes. Ah indeed, I read it wrong. But it still the same "problem" of regs + > 1 parent interrupts already having a different meaning for bcm7120 than what they will have for bcm63xx. I just successfully* booted bcm63xx with my proposed changes to bcm7120-l2-intc with a hacked together bcm6358-l2-intc probe routine, and I now think even less that having these two in one driver merged is a good idea. Especially if we want to support the affinity stuff. There seems to be quite a bit that will need to be changed for it. Jonas * took me a while to find your OF_DECLARE_2() for the mips-intc - sneaky ;p. P.S: I wonder how this patchset is supposed to go, as it depends on earlier bcm7120/generic irqchip patches marked in patchwork as "other subsystem". -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 12:13 PM, Jonas Gorski <jogo@openwrt.org> wrote: >>> Could then mean either irq0 is for interrupts 0..31 (mask/status0) and >>> irq1 for interrupts 32 .. 64 (mask/status1), or irq0 is for interrupts >>> 0..31 on cpu0, and irq1 is for interrupts 0..31 on cpu1, and then >>> would require an additional property to tell them apart, for which we >>> then also could just use a different compatible name, and have (IMHO) >>> a lot less headache. (I wonder why we couldn't just have had more >>> than one instance of 7120-l2 in the dts for the first case) >> >> I don't think we've used this driver to implement the first case yet. >> >> The initial use of the driver was for the BCM7xxx IRQ0 block, which is >> wired up according to the ASCII art diagram in >> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt >> . i.e. different sets of bits in a single irqstatus0/irqmask0 pair >> map to different parent IRQs. The bits handled by each parent IRQ are >> indicated in the brcm,int-map-mask property. >> >> And now on BCM3384, of course, we're seeing the output from two 32-bit >> irqstatus/irqmask words ORed together into a single parent IRQ, for >> periph_intc. The other instances do have their own DT nodes. > > Ah indeed, I read it wrong. But it still the same "problem" of regs + >> 1 parent interrupts already having a different meaning for bcm7120 > than what they will have for bcm63xx. > > I just successfully* booted bcm63xx with my proposed changes to > bcm7120-l2-intc with a hacked together bcm6358-l2-intc probe routine, > and I now think even less that having these two in one driver merged > is a good idea. Especially if we want to support the affinity stuff. > There seems to be quite a bit that will need to be changed for it. My current line of thinking is: compatible="bcm7120-l2-intc" will expect a STB IRQ device with multiple outputs, and a brcm,int-map-mask property. IRQEN at 0x00, IRQMASK at 0x04, single reg range: <addr 0x8>. compatible="bcm3380-l2-intc" will expect one or more reg pairs of <irqen_addr 0x4 irqstat_addr 0x4>, single output, no brcm,int-map-mask, no brcm,int-fwd-mask. In the short term this can be used to support bcm63xx controllers with one CPU. This can easily be handled by irq-bcm7120-l2.c (I'll just split out the reg parsing logic). compatible="bcm6358-l1-intc", "bcm63381-l1-intc", etc. can be supported by a separate driver at some future date. Similar to my new "bcm7038-l1-intc" driver, this would eliminate many of the special cases found in irq-bcm7120-l2.c, and it would add SMP affinity support. reg ranges would look something like <cpu0_block 0x20 cpu1_block 0x20>. Each range corresponds to a single parent IRQ. When this driver is ready, the DTS files can be upgraded to use the new code. In the unlikely event that the old DTB gets baked into a released bootloader image, that's still OK because we aren't changing the "bcm3380-l2-intc" bindings. IIRC there wasn't a nice way to implement SMP affinity with kernel/irq/generic-chip.c either, which is why I dropped it from the 7038 driver. > * took me a while to find your OF_DECLARE_2() for the mips-intc - sneaky ;p. I'm not real happy about how that currently looks, but I didn't know of another way to use mips_cpu_irq_of_init() in conjunction with irqchip_init() (covering the L1/L2 drivers). We only want to call of_irq_init() once, and it should be called with a list of all irqchip drivers built into the kernel. > P.S: I wonder how this patchset is supposed to go, as it depends on > earlier bcm7120/generic irqchip patches marked in patchwork as "other > subsystem". Right, I noted this somewhere in the cover-letter... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt index bae1f2187226..e3b0cba9489a 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt @@ -55,7 +55,10 @@ Required properties: - compatible: should be "brcm,bcm7120-l2-intc" - reg: specifies the base physical address and size of the registers; multiple pairs may be specified, with the first pair handling IRQ offsets - 0..31 and the second pair handling 32..63 + 0..31 and the second pair handling 32..63. A register pair may be + specified as either <base 0x8>, where IRQEN lives at base+0x00 and + IRQSTAT lives at base+0x04, or <enreg 0x4 statreg 0x4>, where the + address of each register is listed independently. - interrupt-controller: identifies the node as an interrupt controller - #interrupt-cells: specifies the number of cells needed to encode an interrupt source, should be 1. diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c index e8441ee7454c..576a92b34372 100644 --- a/drivers/irqchip/irq-bcm7120-l2.c +++ b/drivers/irqchip/irq-bcm7120-l2.c @@ -14,6 +14,7 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/kconfig.h> +#include <linux/kernel.h> #include <linux/platform_device.h> #include <linux/of.h> #include <linux/of_irq.h> @@ -22,6 +23,7 @@ #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/io.h> +#include <linux/ioport.h> #include <linux/irqdomain.h> #include <linux/reboot.h> #include <linux/bitops.h> @@ -29,12 +31,8 @@ #include "irqchip.h" -/* Register offset in the L2 interrupt controller */ -#define IRQEN 0x00 -#define IRQSTAT 0x04 - #define MAX_WORDS 4 -#define MAX_MAPPINGS MAX_WORDS +#define MAX_MAPPINGS (MAX_WORDS * 2) #define IRQS_PER_WORD 32 struct bcm7120_l2_intc_data { @@ -128,6 +126,61 @@ static int bcm7120_l2_intc_init_one(struct device_node *dn, return 0; } +static int __init bcm7120_l2_intc_map_regs(struct device_node *dn, + struct bcm7120_l2_intc_data *data) +{ + unsigned int idx, n_regs = 0, gc_idx = 0; + void __iomem *en_reg = NULL, *stat_reg = NULL; + + for (idx = 0; n_regs < MAX_WORDS * 2; idx++) { + struct resource res; + resource_size_t sz; + void __iomem *map_base; + + if (of_address_to_resource(dn, idx, &res)) + break; + sz = resource_size(&res); + map_base = data->map_base[idx] = ioremap(res.start, sz); + if (!map_base) + return -EINVAL; + + if (n_regs % 2 == 0) { + /* Accept either enable + status, or just enable: + * reg = <0x10000024 0x8>; + * reg = <0x10000024 0x4 0x1000002c 0x4>; + */ + en_reg = map_base; + if (sz == 8) { + stat_reg = map_base + 0x04; + n_regs += 2; + } else if (sz == 4) { + n_regs += 1; + continue; + } else { + return -EINVAL; + } + } else { + /* If the last register was enable, we're looking + * for its corresponding status register + */ + if (sz == 4) { + stat_reg = map_base; + n_regs += 1; + } else { + return -EINVAL; + } + } + + data->pair_base[gc_idx] = min(en_reg, stat_reg); + data->en_offset[gc_idx] = en_reg - data->pair_base[gc_idx]; + data->stat_offset[gc_idx] = stat_reg - data->pair_base[gc_idx]; + gc_idx++; + } + + data->n_words = gc_idx; + return gc_idx ? 0 : -ENOENT; +} + int __init bcm7120_l2_intc_of_init(struct device_node *dn, struct device_node *parent) { @@ -144,18 +197,7 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn, if (!data) return -ENOMEM; - for (idx = 0; idx < MAX_WORDS; idx++) { - data->map_base[idx] = of_iomap(dn, idx); - if (!data->map_base[idx]) - break; - - data->pair_base[idx] = data->map_base[idx]; - data->en_offset[idx] = IRQEN; - data->stat_offset[idx] = IRQSTAT; - - data->n_words = idx + 1; - } - if (!data->n_words) { + if (bcm7120_l2_intc_map_regs(dn, data) < 0) { pr_err("failed to remap intc L2 registers\n"); ret = -ENOMEM; goto out_unmap;
To date, all supported controllers have had the IRQEN register at offset 0x00 and the IRQSTAT register at 0x04. So in DT we would typically see something like: reg = <0xf0406800 0x8>; We still want to support this format, but we also need to support cases where IRQEN and IRQSTAT aren't adjacent. So, we will amend the driver to accept an alternate format that looks like this: reg = <0xf0406800 0x4 0xf0406804 0x4>; i.e. if the first resource_size() == 4, assume that the first resource points to IRQEN and that the next resource points to IRQSTAT. If the first resource_size() == 8, retain the current behavior. Signed-off-by: Kevin Cernekee <cernekee@gmail.com> --- .../interrupt-controller/brcm,bcm7120-l2-intc.txt | 5 +- drivers/irqchip/irq-bcm7120-l2.c | 76 +++++++++++++++++----- 2 files changed, 63 insertions(+), 18 deletions(-)