Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752064AbaKZUNr (ORCPT ); Wed, 26 Nov 2014 15:13:47 -0500 Received: from arrakis.dune.hu ([78.24.191.176]:48137 "EHLO arrakis.dune.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750770AbaKZUNp (ORCPT ); Wed, 26 Nov 2014 15:13:45 -0500 MIME-Version: 1.0 In-Reply-To: References: <1416796846-28149-1-git-send-email-cernekee@gmail.com> <1416796846-28149-7-git-send-email-cernekee@gmail.com> From: Jonas Gorski Date: Wed, 26 Nov 2014 21:13:15 +0100 Message-ID: Subject: Re: [PATCH V3 06/11] irqchip: bcm7120-l2: Change DT binding to allow non-contiguous IRQEN/IRQSTAT To: Kevin Cernekee Cc: Ralf Baechle , Florian Fainelli , Jon Fraser , Dmitry Torokhov , Thomas Gleixner , Jason Cooper , Arnd Bergmann , Brian Norris , MIPS Mailing List , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 26, 2014 at 7:54 PM, Kevin Cernekee wrote: > On Wed, Nov 26, 2014 at 2:25 AM, Jonas Gorski wrote: >> On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski 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 = ; >> interrupts = , ; >> >> 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/