On Wed, Sep 03, 2014 at 05:59:58PM +0100, Florian Fainelli wrote: > On 09/03/2014 05:43 AM, Mark Rutland wrote: > > On Wed, Sep 03, 2014 at 01:13:02PM +0100, Thomas Gleixner wrote: > >> You forgot to CC the device tree dudes. We want an ack on the bindings > >> before they materialize in Linus tree. > > > > Thanks for the Cc. > > > > Florian, in future could you please Cc for both the binding and driver? > > So long as it's obvious which patch introduces the binding other can > > choose to ignore the driver, but for me it's useful context. > > > >> > >> Thanks, > >> > >> tglx > >> > >> On Thu, 28 Aug 2014, Florian Fainelli wrote: > >> > >>> This patch adds the Device Tree binding document for the Broadcom > >>> BCM7120-style Set-top-box Level 2 interrupt controller hardware. > >>> > >>> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx> > >>> --- > >>> .../interrupt-controller/brcm,bcm7120-l2-intc.txt | 44 ++++++++++++++++++++++ > >>> 1 file changed, 44 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt > >>> new file mode 100644 > >>> index 000000000000..3818ffed7347 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt > >>> @@ -0,0 +1,44 @@ > >>> +Broadcom BCM7120-style Level 2 interrupt controller > >>> + > >>> +Required properties: > >>> + > >>> +- compatible: should be "brcm,bcm7120-l2-intc" > > > > Is this a custom block for the bcm7120? > > This is a custom block that started being used with bcm7120 and that we > inherited, unmodified, in newer BCM7xxx designs since then, hence the > name we picked up. Ok. > > > > Does the IP block not have a more specific name? > > I wish there was one, I had to dig through the verilog sources to find > which chip this interrupt controller first started to appear, and the > filename was not too helpful either. Fair enough. [...] > >>> +- brcm,int-map-mask: 32-bits bit mask describing how the interrupts > >>> at this level > >>> + map to their respective parents. Should match exactly the number of interrupts > >>> + specified in the 'interrupts' property. > > > > I don't follow. > > > > Surely this should be static, and we know the 1-1 mapping, or this is > > dynamic and should be SW-configured? > > This is static for a given instance of this interrupt controller on a > particular brcmstb chip. BCM7445 will have something different here than > e.g: BCM7366 or BCM7439 which are also from the same family. > > Not all 32 bits within this interrupt controller will map to wired > interrupts, so the point of this bitmask is to: > > - tell how many valid interrupt sources there are > - tell how a given bitmask maps to its corresponding L1 interrupt line > number > > So this is not much to know the 1:1, but to know the the 1:many mapping, > the example might make it a little clearer how this works I'll have to give the example another scan, it's not immediately clear how the latter portion works. > >>> +Optional properties: > >>> + > >>> +- interrupt-names: if present, the litteral names for the parent interrupts > >>> + specified in the 'interrupts' property. > > > > If you use the interrupt-names property, it should contain the names of > > the interrupts from the POV of this device. Those names must be > > specified in the binding doc. > > Since this also varies on a per-chip basis, I can certainly add each > valid names for each chips out there, but that does not sound useful, > maybe let's just drop that property, we don't use it anyway since we > perform lookups by indexes, and that is safe to do. Ok. It sounds like the index would be the thing to use here. > >>> +- brcm,irq-can-wake: if present, this means the L2 controller can be used as a > >>> + wakeup source for system suspend/resume. > > > > How variable is this? > > There's two instances of this interrupt controller on most SoCs, one > that can wake up the system, one that cannot, see below. > > > > > I realise have properties like this elsewhere, but it seems to be > > hacking around the lack of a decent power domain interface for figuring > > this out. > > Humm, I kind of see your point here with the power domains, I don't see > a big problem with specifying that property though, at most this becomes > redundant when we have a power domain representation (which will be very > simple: always-on and everything else). Sure, we seem to have done that elsewhere. > >>> +- brcm,int-fwd-mask: if present, a 32-bits bit mask describing the interrupts > >>> + which need to be enabled in this controller to flow to the higher level > >>> + interrupt controller. This is typically needed for the UARTs interrupts to > >>> + flow through the top-level interrupt controller (e.g: ARM GIC on ARM-based > >>> + platforms). > >>> + > > > > I don't follow why this property is needed at all. Is this a mechanism > > to bypass this controller entirely? Why should this be described as a > > fixed HW property? > > This interrupt controller has traditionally (not necessarily for good > reasons) been the placeholder for special bits that control whether our > UARTs level 1 interrupts (wired to the ARM GIC) will flow to the L1 > interrupt. So basically setting these bits unmasks some irq lines inpout to the GIC? > We discussed initially with Arnd Bergman about how to best approach > this, and he was happy with a bitmask since: > > a) that is a one-time initialization thing that can happen anywhere in > the kernel before UART interrupts get used (so before user-space gets > scheduled) That feels a little dodgy to me, but perhaps that's ok. > > b) we need to save/restore that bitmask during suspend/resume > > c) this is not a real interrupt bit, we need to set this bit, but we > will not get any interrupt at this particular interrupt controller level > for UARTs, so this is totally transparent for the UART driver > > Once again, the bitmask values varies on a per-chip basis, though the > fundamentals are the same. Thanks for the explanation. Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
