On Wed, Nov 26, 2014 at 7:54 PM, Kevin Cernekee <cernekee@xxxxxxxxx> wrote: > On Wed, Nov 26, 2014 at 2:25 AM, Jonas Gorski <jogo@xxxxxxxxxxx> wrote: >> On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski <jogo@xxxxxxxxxxx> 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".
