Re: Subject: L2x0 OF properties do not include interrupt #

Re: Subject: L2x0 OF properties do not include interrupt #

[Rob Herring]

Mark,

On 08/10/2011 07:48 AM, Mark Rutland wrote:
> Hi all,
> 
> I'm working on a driver for the PMU found in L220/PL310 L2 Cache Controllers
> (as an extension to the existing L2x0 code).
> 
> I've taken a look into what BSP support would be required and mocked up some
> platform_device support, though I notice that Rob Herring has provided
> devicetree bindings for the L2x0 ("l2x0: Add OF based initialization") which
> should probably be used to initialize the PMU (if present) from devicetree.
> 
> The L2x0 hardware supports a combined interrupt line, which is present on the
> Vexpress A9x4, EB11MPcore, PB1176, etc. This can be used to signal errors on
> read/write, parity checks, etc. The PMU also fires an interrupt on this line
> when any of its counters saturate.
> 
> Unfortunately the L2x0 OF bindings introduced by the patch don't include an
> (optional) interrupt property, which means the PMU driver won't be able to be
> initialised via devicetree. As we want to get as much as possible dynamically
> configured via devicetree, this is problematic.
> 
> I realise I'm a bit late to the party here, but I'd like to propose adding an
> optional interrupt parameter to the binding. I'm not aware of any
> implementations which use separate interrupts, but given the binding
> seems to be generic across L2CC implementations (and is not limited simply to
> the L2x0), having a list rather than a single interrupt may be appropriate for
> someone.

Good thing the party never ends...

> This would boil down to (for the moment) a Documentation change along the lines of:
> 
>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
>> index f50e021..d4b387b 100644
>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
>> @@ -28,6 +28,7 @@ Optional properties:
>>  - arm,filter-ranges : <start length> Starting address and length of window to
>>    filter. Addresses in the filter window are directed to the M1 port. Other
>>    addresses will go to the M0 port.
>> +- interrupt : A combined interrupt.
>>  
>>  Example:
>>  
>> @@ -39,4 +40,5 @@ L2: cache-controller {
>>          arm,filter-latency = <0x80000000 0x8000000>;
>>          cache-unified;
>>          cache-level = <2>;
>> +        interrupt = <45>;
>>  };
> 
> Any thoughts?

I think you should allow for either the single irq or individual irqs.
You can specify that the event counter interrupt must be first, then the
pmu driver could work either way ignoring the rest. The driver probably
needs to mark the handler as shared if there is only the combined
interrupt unless you expect all interrupts to be handled by 1 driver.

Rob

> 
> Thanks,
> Mark
> 
> 

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

Hi Rob,

On Wed, Aug 10, 2011 at 02:59:12PM +0100, Rob Herring wrote:
> I think you should allow for either the single irq or individual irqs.
> You can specify that the event counter interrupt must be first, then the
> pmu driver could work either way ignoring the rest. The driver probably
> needs to mark the handler as shared if there is only the combined
> interrupt unless you expect all interrupts to be handled by 1 driver.

I much prefer having seperate, individual IRQs with no requirement on
ordering.

Now, the L2 binding also doesn't fit too well for the L2CC on Cortex-A15,
which is an inner cache like the one on Cortex-A8. Because of this, it
doesn't have a base address but it *does* have an IRQ which is how external
aborts are raised.

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Arnd Bergmann]

On Wednesday 10 August 2011, Will Deacon wrote:
> On Wed, Aug 10, 2011 at 02:59:12PM +0100, Rob Herring wrote:
> > I think you should allow for either the single irq or individual irqs.
> > You can specify that the event counter interrupt must be first, then the
> > pmu driver could work either way ignoring the rest. The driver probably
> > needs to mark the handler as shared if there is only the combined
> > interrupt unless you expect all interrupts to be handled by 1 driver.
> 
> I much prefer having seperate, individual IRQs with no requirement on
> ordering.
> 

What do you mean with 'no requirement on ordering'? If we have multiple
interrupt sources, we definitely want to identify which one calls which
handler, and the only information we have is the position in the array
of interrupt numbers.

	Arnd

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Wed, Aug 10, 2011 at 03:24:27PM +0100, Arnd Bergmann wrote:
> On Wednesday 10 August 2011, Will Deacon wrote:
> > On Wed, Aug 10, 2011 at 02:59:12PM +0100, Rob Herring wrote:
> > > I think you should allow for either the single irq or individual irqs.
> > > You can specify that the event counter interrupt must be first, then the
> > > pmu driver could work either way ignoring the rest. The driver probably
> > > needs to mark the handler as shared if there is only the combined
> > > interrupt unless you expect all interrupts to be handled by 1 driver.
> > 
> > I much prefer having seperate, individual IRQs with no requirement on
> > ordering.
> > 
> 
> What do you mean with 'no requirement on ordering'? If we have multiple
> interrupt sources, we definitely want to identify which one calls which
> handler, and the only information we have is the position in the array
> of interrupt numbers.

I was hoping that it was possible to have separate properties which describe
the interrupt. So you could have something like pmu-interrupt <75> and
abort-interrupt <76> rather than interrupts <75, 76>.

I've not played with DT bindings before though, so if it's usually done with
an ordered list then so be it!

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Arnd Bergmann]

On Wednesday 10 August 2011, Will Deacon wrote:
> I was hoping that it was possible to have separate properties which describe
> the interrupt. So you could have something like pmu-interrupt <75> and
> abort-interrupt <76> rather than interrupts <75, 76>.

Ok, I see.

> I've not played with DT bindings before though, so if it's usually done with
> an ordered list then so be it!

A lot of the code assumes that the property is called 'interrupts' and that
it contains a fixed-length array of interrupt numbers, each for one specific
purpose.

Given that we have so many different meanings for the interrupts, I'm
not sure how this would work best in this case. According to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246f/CHDFHCFJ.html
this looks like a nested interrupt controller, i.e. the L2CC has its own mask
and status register with bits for each one of them. We could model these by
describing the l2cc interrupt controller with these registers and listing all
nine of the current inputs. I suspect however that it would be easier to just
assume that there is only one line for now, and treat the l2cc as a single
interrupt source with an internal status register.

	Arnd

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Thu, Aug 11, 2011 at 02:05:11PM +0100, Arnd Bergmann wrote:
> On Wednesday 10 August 2011, Will Deacon wrote:
> > I was hoping that it was possible to have separate properties which describe
> > the interrupt. So you could have something like pmu-interrupt <75> and
> > abort-interrupt <76> rather than interrupts <75, 76>.
> 
> Ok, I see.
> 
> > I've not played with DT bindings before though, so if it's usually done with
> > an ordered list then so be it!
> 
> A lot of the code assumes that the property is called 'interrupts' and that
> it contains a fixed-length array of interrupt numbers, each for one specific
> purpose.

Ok, I wondered if something like that might be the case.

> Given that we have so many different meanings for the interrupts, I'm
> not sure how this would work best in this case. According to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246f/CHDFHCFJ.html
> this looks like a nested interrupt controller, i.e. the L2CC has its own mask
> and status register with bits for each one of them. We could model these by
> describing the l2cc interrupt controller with these registers and listing all
> nine of the current inputs. I suspect however that it would be easier to just
> assume that there is only one line for now, and treat the l2cc as a single
> interrupt source with an internal status register.

Given that this binding is only for the l2x0 / pl310 and I don't know of any
implementation where > 1 interrupt line is wired up, I'm happy to assume a
single combined interrupt line for now.

Cheers,

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Rob Herring]

On 08/11/2011 08:09 AM, Will Deacon wrote:
> On Thu, Aug 11, 2011 at 02:05:11PM +0100, Arnd Bergmann wrote:
>> On Wednesday 10 August 2011, Will Deacon wrote:
>>> I was hoping that it was possible to have separate properties which describe
>>> the interrupt. So you could have something like pmu-interrupt <75> and
>>> abort-interrupt <76> rather than interrupts <75, 76>.
>>
>> Ok, I see.
>>
>>> I've not played with DT bindings before though, so if it's usually done with
>>> an ordered list then so be it!
>>
>> A lot of the code assumes that the property is called 'interrupts' and that
>> it contains a fixed-length array of interrupt numbers, each for one specific
>> purpose.
> 
> Ok, I wondered if something like that might be the case.
> 
>> Given that we have so many different meanings for the interrupts, I'm
>> not sure how this would work best in this case. According to
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246f/CHDFHCFJ.html
>> this looks like a nested interrupt controller, i.e. the L2CC has its own mask
>> and status register with bits for each one of them. We could model these by
>> describing the l2cc interrupt controller with these registers and listing all
>> nine of the current inputs. I suspect however that it would be easier to just
>> assume that there is only one line for now, and treat the l2cc as a single
>> interrupt source with an internal status register.
> 
> Given that this binding is only for the l2x0 / pl310 and I don't know of any
> implementation where > 1 interrupt line is wired up, I'm happy to assume a
> single combined interrupt line for now.
> 

I know of one. Although, we have the combined interrupt as well. The
binding should allow either way and specify the order. If the event
counter interrupt is 1st, then it should be the same to s/w.

Rob

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Thu, Aug 11, 2011 at 04:32:08PM +0100, Rob Herring wrote:
> On 08/11/2011 08:09 AM, Will Deacon wrote:
> > On Thu, Aug 11, 2011 at 02:05:11PM +0100, Arnd Bergmann wrote:
> >> On Wednesday 10 August 2011, Will Deacon wrote:
> >>> I was hoping that it was possible to have separate properties which describe
> >>> the interrupt. So you could have something like pmu-interrupt <75> and
> >>> abort-interrupt <76> rather than interrupts <75, 76>.
> >>
> >> Ok, I see.
> >>
> >>> I've not played with DT bindings before though, so if it's usually done with
> >>> an ordered list then so be it!
> >>
> >> A lot of the code assumes that the property is called 'interrupts' and that
> >> it contains a fixed-length array of interrupt numbers, each for one specific
> >> purpose.
> > 
> > Ok, I wondered if something like that might be the case.
> > 
> >> Given that we have so many different meanings for the interrupts, I'm
> >> not sure how this would work best in this case. According to
> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246f/CHDFHCFJ.html
> >> this looks like a nested interrupt controller, i.e. the L2CC has its own mask
> >> and status register with bits for each one of them. We could model these by
> >> describing the l2cc interrupt controller with these registers and listing all
> >> nine of the current inputs. I suspect however that it would be easier to just
> >> assume that there is only one line for now, and treat the l2cc as a single
> >> interrupt source with an internal status register.
> > 
> > Given that this binding is only for the l2x0 / pl310 and I don't know of any
> > implementation where > 1 interrupt line is wired up, I'm happy to assume a
> > single combined interrupt line for now.
> > 
> 
> I know of one. Although, we have the combined interrupt as well. The
> binding should allow either way and specify the order. If the event
> counter interrupt is 1st, then it should be the same to s/w.

You mean putting the combined interrupt first? If so, we may as well just
specify that until somebody builds a platform that doesn't have it.

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Rob Herring]

On 08/11/2011 10:38 AM, Will Deacon wrote:
> On Thu, Aug 11, 2011 at 04:32:08PM +0100, Rob Herring wrote:
>> On 08/11/2011 08:09 AM, Will Deacon wrote:
>>> On Thu, Aug 11, 2011 at 02:05:11PM +0100, Arnd Bergmann wrote:
>>>> On Wednesday 10 August 2011, Will Deacon wrote:
>>>>> I was hoping that it was possible to have separate properties which describe
>>>>> the interrupt. So you could have something like pmu-interrupt <75> and
>>>>> abort-interrupt <76> rather than interrupts <75, 76>.
>>>>
>>>> Ok, I see.
>>>>
>>>>> I've not played with DT bindings before though, so if it's usually done with
>>>>> an ordered list then so be it!
>>>>
>>>> A lot of the code assumes that the property is called 'interrupts' and that
>>>> it contains a fixed-length array of interrupt numbers, each for one specific
>>>> purpose.
>>>
>>> Ok, I wondered if something like that might be the case.
>>>
>>>> Given that we have so many different meanings for the interrupts, I'm
>>>> not sure how this would work best in this case. According to
>>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246f/CHDFHCFJ.html
>>>> this looks like a nested interrupt controller, i.e. the L2CC has its own mask
>>>> and status register with bits for each one of them. We could model these by
>>>> describing the l2cc interrupt controller with these registers and listing all
>>>> nine of the current inputs. I suspect however that it would be easier to just
>>>> assume that there is only one line for now, and treat the l2cc as a single
>>>> interrupt source with an internal status register.
>>>
>>> Given that this binding is only for the l2x0 / pl310 and I don't know of any
>>> implementation where > 1 interrupt line is wired up, I'm happy to assume a
>>> single combined interrupt line for now.
>>>
>>
>> I know of one. Although, we have the combined interrupt as well. The
>> binding should allow either way and specify the order. If the event
>> counter interrupt is 1st, then it should be the same to s/w.
> 
> You mean putting the combined interrupt first? If so, we may as well just
> specify that until somebody builds a platform that doesn't have it.
> 

No, either you have 1 interrupt and it is the combined one. or you have
the 9? separate interrupts.  Having both combined and separate hooked up
is a bit dumb, so I would not worry about that case. I would just define
the event counter interrupt 1st as that is probably the primary use.
Also, I think that was the only interrupt on the L2x0 controllers IIRC.

It's also conceivable that some of the interrupts get routed somewhere
else rather than just into the GIC.

Rob

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Thu, Aug 11, 2011 at 05:06:23PM +0100, Rob Herring wrote:
> On 08/11/2011 10:38 AM, Will Deacon wrote:
> > 
> > You mean putting the combined interrupt first? If so, we may as well just
> > specify that until somebody builds a platform that doesn't have it.
> > 
> 
> No, either you have 1 interrupt and it is the combined one. or you have
> the 9? separate interrupts.  Having both combined and separate hooked up
> is a bit dumb, so I would not worry about that case. I would just define
> the event counter interrupt 1st as that is probably the primary use.
> Also, I think that was the only interrupt on the L2x0 controllers IIRC.
> 
> It's also conceivable that some of the interrupts get routed somewhere
> else rather than just into the GIC.

Ok. I think the binding that Mark posted to devicetree-discuss:

http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-August/007349.html

will take care of this. We have a single interrupt (I don't think we even
need to specify what it is). Interrupt handlers can request this with
IRQ_SHARED and then check the status register to see whether they actually
need to do anything.

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Russell King - ARM Linux]

On Thu, Aug 11, 2011 at 11:06:23AM -0500, Rob Herring wrote:
> No, either you have 1 interrupt and it is the combined one. or you have
> the 9? separate interrupts.  Having both combined and separate hooked up
> is a bit dumb, so I would not worry about that case. I would just define
> the event counter interrupt 1st as that is probably the primary use.
> Also, I think that was the only interrupt on the L2x0 controllers IIRC.
> 
> It's also conceivable that some of the interrupts get routed somewhere
> else rather than just into the GIC.

A lot of other primecells have the one-combined and individual interrupts.
So far, in the last 10 years, no one's used the individual interrupts in
Linux (or even afaik wired them up.)  Not with either the original ARM
primecells or the vendor modified ones.

That tells us something about the individual interrupts.

Re: Subject: L2x0 OF properties do not include interrupt #

[Rob Herring]

On 08/10/2011 09:10 AM, Will Deacon wrote:
> Hi Rob,
> 
> On Wed, Aug 10, 2011 at 02:59:12PM +0100, Rob Herring wrote:
>> I think you should allow for either the single irq or individual irqs.
>> You can specify that the event counter interrupt must be first, then the
>> pmu driver could work either way ignoring the rest. The driver probably
>> needs to mark the handler as shared if there is only the combined
>> interrupt unless you expect all interrupts to be handled by 1 driver.
> 
> I much prefer having seperate, individual IRQs with no requirement on
> ordering.
> 
> Now, the L2 binding also doesn't fit too well for the L2CC on Cortex-A15,
> which is an inner cache like the one on Cortex-A8. Because of this, it
> doesn't have a base address but it *does* have an IRQ which is how external
> aborts are raised.

This is not a general L2 binding, but an L2x0/PL310 binding. A8/A15 L2
is a completely different binding and driver though. You would do
something like the current cpu pmu binding that is just interrupts.

Rob

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Wed, Aug 10, 2011 at 03:37:26PM +0100, Rob Herring wrote:
> On 08/10/2011 09:10 AM, Will Deacon wrote:
> > Hi Rob,
> > 
> > On Wed, Aug 10, 2011 at 02:59:12PM +0100, Rob Herring wrote:
> >> I think you should allow for either the single irq or individual irqs.
> >> You can specify that the event counter interrupt must be first, then the
> >> pmu driver could work either way ignoring the rest. The driver probably
> >> needs to mark the handler as shared if there is only the combined
> >> interrupt unless you expect all interrupts to be handled by 1 driver.
> > 
> > I much prefer having seperate, individual IRQs with no requirement on
> > ordering.
> > 
> > Now, the L2 binding also doesn't fit too well for the L2CC on Cortex-A15,
> > which is an inner cache like the one on Cortex-A8. Because of this, it
> > doesn't have a base address but it *does* have an IRQ which is how external
> > aborts are raised.
> 
> This is not a general L2 binding, but an L2x0/PL310 binding. A8/A15 L2
> is a completely different binding and driver though. You would do
> something like the current cpu pmu binding that is just interrupts.

Ok, I think that's a lot more sense to have a separate binding for inner and
outer L2 cache implementstions. I was just a bit concerned by the
documentation using the term 'l2cc' as it sounds like it's fairly catch-all.

Will

Re: Subject: L2x0 OF properties do not include interrupt #

[Arnd Bergmann]

On Wednesday 10 August 2011, Mark Rutland wrote:

> I realise I'm a bit late to the party here, but I'd like to propose adding an
> optional interrupt parameter to the binding. I'm not aware of any
> implementations which use separate interrupts, but given the binding
> seems to be generic across L2CC implementations (and is not limited simply to
> the L2x0), having a list rather than a single interrupt may be appropriate for
> someone.

Sounds good, thanks for pointing this out.

How many possible interrupt sources are there? If there is only a small number
of those (e.g. at most 4), we might just list all of them and register
them from the driver even if they are all the same.

> This would boil down to (for the moment) a Documentation change along the lines of:
> 
> > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
> > index f50e021..d4b387b 100644
> > --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> > @@ -28,6 +28,7 @@ Optional properties:
> >  - arm,filter-ranges : <start length> Starting address and length of window to
> >    filter. Addresses in the filter window are directed to the M1 port. Other
> >    addresses will go to the M0 port.
> > +- interrupt : A combined interrupt.
> >  
> >  Example:
> >  
> > @@ -39,4 +40,5 @@ L2: cache-controller {
> >          arm,filter-latency = <0x80000000 0x8000000>;
> >          cache-unified;
> >          cache-level = <2>;
> > +        interrupt = <45>;
> >  };
> 
> Any thoughts?

Do we also need to document an interrupt-parent property, or is that implied?

	Arnd

Re: Subject: L2x0 OF properties do not include interrupt #

[Rob Herring]

Arnd,

On 08/10/2011 09:09 AM, Arnd Bergmann wrote:
> On Wednesday 10 August 2011, Mark Rutland wrote:
> 
>> I realise I'm a bit late to the party here, but I'd like to propose adding an
>> optional interrupt parameter to the binding. I'm not aware of any
>> implementations which use separate interrupts, but given the binding
>> seems to be generic across L2CC implementations (and is not limited simply to
>> the L2x0), having a list rather than a single interrupt may be appropriate for
>> someone.
> 
> Sounds good, thanks for pointing this out.
> 
> How many possible interrupt sources are there? If there is only a small number
> of those (e.g. at most 4), we might just list all of them and register
> them from the driver even if they are all the same.
> 

Here's all the interrupts on the PL310:

DECERRINTR Decode error received on master ports from L3
SLVERRINTR Slave error received on master ports from L3
ERRRDINTR Error on L2 data RAM read
ERRRTINTR Error on L2 tag RAM read
ERRWDINTR Error on L2 data RAM write
ERRWTINTR Error on L2 tag RAM write
PARRDINTR Parity error on L2 data RAM read
PARRTINTR Parity error on L2 tag RAM read
ECNTRINTR Event Counter Overflow/Increment
L2CCINTR L2CC Combined Interrupt Output

It's likely that you would want to split these to different drivers.
EDAC for parity/bus errors and hw-events for event counters, for example.

Rob

>> This would boil down to (for the moment) a Documentation change along the lines of:
>>
>>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
>>> index f50e021..d4b387b 100644
>>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
>>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
>>> @@ -28,6 +28,7 @@ Optional properties:
>>>  - arm,filter-ranges : <start length> Starting address and length of window to
>>>    filter. Addresses in the filter window are directed to the M1 port. Other
>>>    addresses will go to the M0 port.
>>> +- interrupt : A combined interrupt.
>>>  
>>>  Example:
>>>  
>>> @@ -39,4 +40,5 @@ L2: cache-controller {
>>>          arm,filter-latency = <0x80000000 0x8000000>;
>>>          cache-unified;
>>>          cache-level = <2>;
>>> +        interrupt = <45>;
>>>  };
>>
>> Any thoughts?
> 
> Do we also need to document an interrupt-parent property, or is that implied?
> 
> 	Arnd

Re: Subject: L2x0 OF properties do not include interrupt #

[Will Deacon]

On Wed, Aug 10, 2011 at 03:31:54PM +0100, Rob Herring wrote:
> Here's all the interrupts on the PL310:
> 
> DECERRINTR Decode error received on master ports from L3
> SLVERRINTR Slave error received on master ports from L3
> ERRRDINTR Error on L2 data RAM read
> ERRRTINTR Error on L2 tag RAM read
> ERRWDINTR Error on L2 data RAM write
> ERRWTINTR Error on L2 tag RAM write
> PARRDINTR Parity error on L2 data RAM read
> PARRTINTR Parity error on L2 tag RAM read
> ECNTRINTR Event Counter Overflow/Increment
> L2CCINTR L2CC Combined Interrupt Output
> 
> It's likely that you would want to split these to different drivers.
> EDAC for parity/bus errors and hw-events for event counters, for example.

I completely agree with you here. In reality, I expect we'll be lucky if
even the combined interrupt is wired up so maybe the individual property
names should only be added on an as-needed basis? For the time being we can
happily get by with something like combined-interrupt = <...>.

Or would you rather specify everything up front?

Will