Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Thomas Gleixner]

You forgot to CC the device tree dudes. We want an ack on the bindings
before they materialize in Linus tree.

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../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"
> +- reg: specifies the base physical address and size of the registers
> +- 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.
> +- interrupt-parent: specifies the phandle to the parent interrupt controller
> +  this one is cascaded from
> +- interrupts: specifies the interrupt line(s) in the interrupt-parent domain
> +  to be used for cascading
> +- 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.
> +
> +Optional properties:
> +
> +- interrupt-names: if present, the litteral names for the parent interrupts
> +  specified in the 'interrupts' property.
> +
> +- brcm,irq-can-wake: if present, this means the L2 controller can be used as a
> +  wakeup source for system suspend/resume.
> +
> +- 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).
> +
> +Example:
> +
> +irq0_intc: interrupt-controller@f0406800 {
> +	compatible = "brcm,bcm7120-l2-intc";
> +	interrupt-parent = <&intc>;
> +	#interrupt-cells = <1>;
> +	reg = <0xf0406800 0x8>;
> +	interrupt-controller;
> +	interrupts = <0x0 0x42 0x0>, <0x0 0x40 0x0>;
> +	interrupt-names = "upg_main", "upg_bsc";
> +	brcm,int-map-mask = <0xeb8>, <0x140>;
> +	brcm,int-fwd-mask = <0x7>;
> +};
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Mark Rutland]

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@gmail.com>
> > ---
> >  .../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?

Does the IP block not have a more specific name?

> > +- reg: specifies the base physical address and size of the registers
> > +- 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.

... and valid values are?

> > +- interrupt-parent: specifies the phandle to the parent interrupt controller
> > +  this one is cascaded from
> > +- interrupts: specifies the interrupt line(s) in the interrupt-parent domain
> > +  to be used for cascading

The domain is a software construct, so I don't think it needs to be
mentuioned in the binding doc. All that this proeprty describes is the
lines.

> > +- 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?

> > +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.

This is useless as-is.

> > +- 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?

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.

> > +- 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?

Thanks,
Mark.

> > +Example:
> > +
> > +irq0_intc: interrupt-controller@f0406800 {
> > +	compatible = "brcm,bcm7120-l2-intc";
> > +	interrupt-parent = <&intc>;
> > +	#interrupt-cells = <1>;
> > +	reg = <0xf0406800 0x8>;
> > +	interrupt-controller;
> > +	interrupts = <0x0 0x42 0x0>, <0x0 0x40 0x0>;
> > +	interrupt-names = "upg_main", "upg_bsc";
> > +	brcm,int-map-mask = <0xeb8>, <0x140>;
> > +	brcm,int-fwd-mask = <0x7>;
> > +};
> > -- 
> > 1.9.1
> > 
> > 
> --
> 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
> 

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Florian Fainelli]

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@gmail.com>
>>> ---
>>>  .../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.

> 
> 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.

> 
>>> +- reg: specifies the base physical address and size of the registers
>>> +- 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.
> 
> ... and valid values are?

0 to 31, this is the interrupt number, will fix that.

> 
>>> +- interrupt-parent: specifies the phandle to the parent interrupt controller
>>> +  this one is cascaded from
>>> +- interrupts: specifies the interrupt line(s) in the interrupt-parent domain
>>> +  to be used for cascading
> 
> The domain is a software construct, so I don't think it needs to be
> mentuioned in the binding doc. All that this proeprty describes is the
> lines.

Will fix that.

> 
>>> +- 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

> 
>>> +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.

> 
> This is useless as-is.
> 
>>> +- 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).

> 
>>> +- 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.

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)

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,
> Mark.
> 
>>> +Example:
>>> +
>>> +irq0_intc: interrupt-controller@f0406800 {
>>> +	compatible = "brcm,bcm7120-l2-intc";
>>> +	interrupt-parent = <&intc>;
>>> +	#interrupt-cells = <1>;
>>> +	reg = <0xf0406800 0x8>;
>>> +	interrupt-controller;
>>> +	interrupts = <0x0 0x42 0x0>, <0x0 0x40 0x0>;
>>> +	interrupt-names = "upg_main", "upg_bsc";
>>> +	brcm,int-map-mask = <0xeb8>, <0x140>;
>>> +	brcm,int-fwd-mask = <0x7>;
>>> +};
>>> -- 
>>> 1.9.1
>>>
>>>
>> --
>> 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
>>

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Mark Rutland]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> ---
> >>>  .../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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Florian Fainelli]

On 09/05/2014 02:05 AM, Mark Rutland wrote:
> 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@gmail.com>
>>>>> ---
>>>>>  .../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.

Had I CC'd you correctly in the first place you could see how that is
used, my bad.

This bitmask gets bitwise OR'd to set the unused bits in gc->unused, to
make sure there was no loss of information, I wanted to have one mask
per interrupt outputs, even though the code does combine that into a
single 32-bits wide mask.

> 
>>>>> +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?

Right, this is what happens. We prefer to use the GIC interrupts because
that provides more flexibility.

> 
>> 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.

The other approach was to use the "interrupt-extended" property for the
UART nodes and have them reference both their GIC interrupt, and the
BCM7120-L2 interrupt, but that also requires UART driver/platform
modifications to account for that extra "interrupt", on which we are
only ever going to call enable_irq() and nothing more.

So, in the end, this turned out to be simpler to just read the
"brcm,irq-fwd-mask" property and apply it to the relevant register.

> 
>>
>> 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.
> 

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Thomas Gleixner]

On Fri, 5 Sep 2014, Florian Fainelli wrote:
> On 09/05/2014 02:05 AM, Mark Rutland wrote:
> >>>>> +- 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?
> 
> Right, this is what happens. We prefer to use the GIC interrupts because
> that provides more flexibility.
> 
> > 
> >> 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.
> 
> The other approach was to use the "interrupt-extended" property for the
> UART nodes and have them reference both their GIC interrupt, and the
> BCM7120-L2 interrupt, but that also requires UART driver/platform
> modifications to account for that extra "interrupt", on which we are
> only ever going to call enable_irq() and nothing more.
> 
> So, in the end, this turned out to be simpler to just read the
> "brcm,irq-fwd-mask" property and apply it to the relevant register.

So if I understand correctly what you have is:

                          /- GIC------------->   
 Device-irq ---- [routing]
                          \- BC irq chip ---->

and you implement it as

 Device-irq ---- [BC irq chip] ---- [GIC] --->
                            |
                            ----------------->

And the fwd mask is to tell the BC chip to use the GIC and which irq
of the GIC, so it can fiddle with the GIC under the hood, right?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Florian Fainelli]

On 09/05/2014 12:21 PM, Thomas Gleixner wrote:
> On Fri, 5 Sep 2014, Florian Fainelli wrote:
>> On 09/05/2014 02:05 AM, Mark Rutland wrote:
>>>>>>> +- 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?
>>
>> Right, this is what happens. We prefer to use the GIC interrupts because
>> that provides more flexibility.
>>
>>>
>>>> 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.
>>
>> The other approach was to use the "interrupt-extended" property for the
>> UART nodes and have them reference both their GIC interrupt, and the
>> BCM7120-L2 interrupt, but that also requires UART driver/platform
>> modifications to account for that extra "interrupt", on which we are
>> only ever going to call enable_irq() and nothing more.
>>
>> So, in the end, this turned out to be simpler to just read the
>> "brcm,irq-fwd-mask" property and apply it to the relevant register.
> 
> So if I understand correctly what you have is:
> 
>                           /- GIC------------->   
>  Device-irq ---- [routing]
>                           \- BC irq chip ---->
> 
> and you implement it as
> 
>  Device-irq ---- [BC irq chip] ---- [GIC] --->
>                             |
>                             ----------------->
> 
> And the fwd mask is to tell the BC chip to use the GIC and which irq
> of the GIC, so it can fiddle with the GIC under the hood, right?

The forward mask really is to tell the BCM7120 l2 interrupt controller:
bypass me, and output the UART interrupts directly at the GIC level, so
I think this does match your understanding.

Not setting the forward mask means you would get the UART interrupts at
the BCM7120 l2 interrupt controller level, and have to handle them here.

Hope this helps clarify what this funky piece of hardware does.
--
Florian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Thomas Gleixner]

On Fri, 5 Sep 2014, Florian Fainelli wrote:
> On 09/05/2014 12:21 PM, Thomas Gleixner wrote:
> > So if I understand correctly what you have is:
> > 
> >                           /- GIC------------->   
> >  Device-irq ---- [routing]
> >                           \- BC irq chip ---->
> > 
> > and you implement it as
> > 
> >  Device-irq ---- [BC irq chip] ---- [GIC] --->
> >                             |
> >                             ----------------->
> > 
> > And the fwd mask is to tell the BC chip to use the GIC and which irq
> > of the GIC, so it can fiddle with the GIC under the hood, right?
> 
> The forward mask really is to tell the BCM7120 l2 interrupt controller:
> bypass me, and output the UART interrupts directly at the GIC level, so
> I think this does match your understanding.
> 
> Not setting the forward mask means you would get the UART interrupts at
> the BCM7120 l2 interrupt controller level, and have to handle them here.
> 
> Hope this helps clarify what this funky piece of hardware does.

Sigh, this stacked interrupt chip nonsense is becoming a plague.

So if you set that bit then the UART driver only sees the GIC as its
interrupt controller and not the L2 thingy. So, the L2 chip only
enables its interrupt unconditionally for that line and the
enable/disable happens at the GIC level.

If that's the case, that's fine with me. It's not pretty, but at least
it does not involve L2 fiddling indirectly with the GIC.

Thanks,

	tglx

Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding

[Florian Fainelli]

On 09/05/2014 01:44 PM, Thomas Gleixner wrote:
> On Fri, 5 Sep 2014, Florian Fainelli wrote:
>> On 09/05/2014 12:21 PM, Thomas Gleixner wrote:
>>> So if I understand correctly what you have is:
>>>
>>>                           /- GIC------------->   
>>>  Device-irq ---- [routing]
>>>                           \- BC irq chip ---->
>>>
>>> and you implement it as
>>>
>>>  Device-irq ---- [BC irq chip] ---- [GIC] --->
>>>                             |
>>>                             ----------------->
>>>
>>> And the fwd mask is to tell the BC chip to use the GIC and which irq
>>> of the GIC, so it can fiddle with the GIC under the hood, right?
>>
>> The forward mask really is to tell the BCM7120 l2 interrupt controller:
>> bypass me, and output the UART interrupts directly at the GIC level, so
>> I think this does match your understanding.
>>
>> Not setting the forward mask means you would get the UART interrupts at
>> the BCM7120 l2 interrupt controller level, and have to handle them here.
>>
>> Hope this helps clarify what this funky piece of hardware does.
> 
> Sigh, this stacked interrupt chip nonsense is becoming a plague.

This is a HW design that we inherited, but fortunately we might be able
to fix that in the future.

> 
> So if you set that bit then the UART driver only sees the GIC as its
> interrupt controller and not the L2 thingy. So, the L2 chip only
> enables its interrupt unconditionally for that line and the
> enable/disable happens at the GIC level.

Exactly, this is completely transparent for the UART and the GIC.

> 
> If that's the case, that's fine with me. It's not pretty, but at least
> it does not involve L2 fiddling indirectly with the GIC.

Absolutely, we do not have to do the arch_gic_extn thingy.

Thanks for your feedback!
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html