All of lore.kernel.org
 help / color / mirror / Atom feed
* RFC: proposal to extend the open-pic interrupt specifier definition
@ 2010-01-05 23:28 Yoder Stuart-B08248
       [not found] ` <9696D7A991D0824DBA8DFAC74A9C5FA30590506E-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Yoder Stuart-B08248 @ 2010-01-05 23:28 UTC (permalink / raw)
  To: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg


The current open-pic binding defines that interrupt specifiers
have 2 cells-- an interrupt number and level/sense encoding.

With chips like the P4080 this is no longer sufficient to
represent the various types of interrupt sources handled by
the interrupt controller.  A linear list of interrupt numbers
doesn't handle all interrupt types-- there are at least 4 different
kinds of interrupts on the P4080.

We have a proposal to extend the open-pic binding in
a backwards compatible way to encode additional information
in the level/sense field.

The current definition of level/sense is:
  0 = low to high edge sensitive type enabled
  1 = active low level sensitive type enabled
  2 = active high level sensitive type enabled
  3 = high to low edge sensitive type enabled

Those 2 bits would retain their current meaning, but the
full encoding would be extended as follows:

     bits      meaning
     ----------------------------------------------
     0-7       interrupt sub-type
     8-15      interrupt type
     16-23     implementation dependent
     24-29     reserved
     30-31     level/sense encoding

     interrupt type
     --------------
     The interrupt type field numberspace is divided as
     follows:

        0x00 to 0x7f - architected interrupt types
        0x80 to 0xff - implementation dependent interrupt
                       types

     The interrupt type determines the meaning of the 
     sub-type.  Implementation dependent types are
     specified by the interrupt controller binding.

     architected interrupt types
     ---------------------------
     0x00 - unspecified / I/O device
     0x01 - interprocessor interrupt
     0x02 - timer interrupt
     0x03 - message signaled interrupt

The advantage of the above approach is backwards compatibility.
Existing interrupt specifiers (and device trees) are valid with
this proposal. 

Regards,
Stuart Yoder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found] ` <9696D7A991D0824DBA8DFAC74A9C5FA30590506E-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
@ 2010-01-07  0:50   ` David Gibson
  2010-01-07  3:33     ` RFC: proposal to extend the open-pic interrupt specifierdefinition Yoder Stuart-B08248
  2010-01-08  1:02     ` [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifier definition Benjamin Herrenschmidt
  2010-01-12 18:17   ` Segher Boessenkool
  1 sibling, 2 replies; 24+ messages in thread
From: David Gibson @ 2010-01-07  0:50 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder Stuart-B08248 wrote:
> 
> The current open-pic binding defines that interrupt specifiers
> have 2 cells-- an interrupt number and level/sense encoding.
> 
> With chips like the P4080 this is no longer sufficient to
> represent the various types of interrupt sources handled by
> the interrupt controller.  A linear list of interrupt numbers
> doesn't handle all interrupt types-- there are at least 4 different
> kinds of interrupts on the P4080.
> 
> We have a proposal to extend the open-pic binding in
> a backwards compatible way to encode additional information
> in the level/sense field.
> 
> The current definition of level/sense is:
>   0 = low to high edge sensitive type enabled
>   1 = active low level sensitive type enabled
>   2 = active high level sensitive type enabled
>   3 = high to low edge sensitive type enabled
> 
> Those 2 bits would retain their current meaning, but the
> full encoding would be extended as follows:
> 
>      bits      meaning
>      ----------------------------------------------
>      0-7       interrupt sub-type
>      8-15      interrupt type
>      16-23     implementation dependent
>      24-29     reserved
>      30-31     level/sense encoding

Um.. what do "type" and "sub-type" mean in this context?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: RFC: proposal to extend the open-pic interrupt specifierdefinition
  2010-01-07  0:50   ` David Gibson
@ 2010-01-07  3:33     ` Yoder Stuart-B08248
       [not found]       ` <9696D7A991D0824DBA8DFAC74A9C5FA305987E61-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  2010-01-08  1:02     ` [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifier definition Benjamin Herrenschmidt
  1 sibling, 1 reply; 24+ messages in thread
From: Yoder Stuart-B08248 @ 2010-01-07  3:33 UTC (permalink / raw)
  To: David Gibson
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

 

> -----Original Message-----
> From: 
> devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
labs.org [mailto:devicetree-discuss->
bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On 
> Behalf Of David Gibson
> Sent: Wednesday, January 06, 2010 6:51 PM
> To: Yoder Stuart-B08248
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> Subject: Re: RFC: proposal to extend the open-pic interrupt 
> specifierdefinition
> 
> On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder Stuart-B08248 wrote:
> > 
> > The current open-pic binding defines that interrupt specifiers
> > have 2 cells-- an interrupt number and level/sense encoding.
> > 
> > With chips like the P4080 this is no longer sufficient to
> > represent the various types of interrupt sources handled by
> > the interrupt controller.  A linear list of interrupt numbers
> > doesn't handle all interrupt types-- there are at least 4 different
> > kinds of interrupts on the P4080.
> > 
> > We have a proposal to extend the open-pic binding in
> > a backwards compatible way to encode additional information
> > in the level/sense field.
> > 
> > The current definition of level/sense is:
> >   0 = low to high edge sensitive type enabled
> >   1 = active low level sensitive type enabled
> >   2 = active high level sensitive type enabled
> >   3 = high to low edge sensitive type enabled
> > 
> > Those 2 bits would retain their current meaning, but the
> > full encoding would be extended as follows:
> > 
> >      bits      meaning
> >      ----------------------------------------------
> >      0-7       interrupt sub-type
> >      8-15      interrupt type
> >      16-23     implementation dependent
> >      24-29     reserved
> >      30-31     level/sense encoding
> 
> Um.. what do "type" and "sub-type" mean in this context?

"type" specifies the type of interrupt-- example timer, MSI,
etc and would define the meaning of the interrupt number
portion of the interrupt specifier.   A given "type" may or
may not have a "subtype" depending on the binding.

As described in the proposal, "type" is a range of numbers,
divided between standard/architected types and implementation
specific types.

We (Freescale) have at least one interrupt type "error" in the P4080
that would have a "sub-type" that would indicate a related bit in
another
status register.

Stuart

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]       ` <9696D7A991D0824DBA8DFAC74A9C5FA305987E61-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
@ 2010-01-07  4:55         ` David Gibson
  2010-01-07 11:17           ` [Power.org:parch] " Sethi Varun-B16395
                             ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: David Gibson @ 2010-01-07  4:55 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder Stuart-B08248 wrote:
> > From: 
> > devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
> labs.org [mailto:devicetree-discuss->
> bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On 
> > Behalf Of David Gibson
> > Sent: Wednesday, January 06, 2010 6:51 PM
> > To: Yoder Stuart-B08248
> > Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> > Subject: Re: RFC: proposal to extend the open-pic interrupt 
> > specifierdefinition
> > 
> > On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder Stuart-B08248 wrote:
> > > 
> > > The current open-pic binding defines that interrupt specifiers
> > > have 2 cells-- an interrupt number and level/sense encoding.
> > > 
> > > With chips like the P4080 this is no longer sufficient to
> > > represent the various types of interrupt sources handled by
> > > the interrupt controller.  A linear list of interrupt numbers
> > > doesn't handle all interrupt types-- there are at least 4 different
> > > kinds of interrupts on the P4080.
> > > 
> > > We have a proposal to extend the open-pic binding in
> > > a backwards compatible way to encode additional information
> > > in the level/sense field.
> > > 
> > > The current definition of level/sense is:
> > >   0 = low to high edge sensitive type enabled
> > >   1 = active low level sensitive type enabled
> > >   2 = active high level sensitive type enabled
> > >   3 = high to low edge sensitive type enabled
> > > 
> > > Those 2 bits would retain their current meaning, but the
> > > full encoding would be extended as follows:
> > > 
> > >      bits      meaning
> > >      ----------------------------------------------
> > >      0-7       interrupt sub-type
> > >      8-15      interrupt type
> > >      16-23     implementation dependent
> > >      24-29     reserved
> > >      30-31     level/sense encoding
> > 
> > Um.. what do "type" and "sub-type" mean in this context?
> 
> "type" specifies the type of interrupt-- example timer, MSI,
> etc and would define the meaning of the interrupt number
> portion of the interrupt specifier.   A given "type" may or
> may not have a "subtype" depending on the binding.
> 
> As described in the proposal, "type" is a range of numbers,
> divided between standard/architected types and implementation
> specific types.
> 
> We (Freescale) have at least one interrupt type "error" in the P4080
> that would have a "sub-type" that would indicate a related bit in
> another
> status register.

And who is the type/subtype relevant to?  From what you've said here,
I don't see why it needs to be in the interrupt specifiers.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
  2010-01-07  4:55         ` David Gibson
@ 2010-01-07 11:17           ` Sethi Varun-B16395
  2010-01-07 17:18           ` Scott Wood
  2010-01-07 17:39           ` Yoder Stuart-B08248
  2 siblings, 0 replies; 24+ messages in thread
From: Sethi Varun-B16395 @ 2010-01-07 11:17 UTC (permalink / raw)
  To: David Gibson, Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

 

> -----Original Message-----
> From: parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org [mailto:parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org] On Behalf Of 
> David Gibson
> Sent: Thursday, January 07, 2010 10:25 AM
> To: Yoder Stuart-B08248
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> Subject: [Power.org:parch] Re: RFC: proposal to extend the 
> open-pic interrupt specifierdefinition
> 
> On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder Stuart-B08248 wrote:
> > > From: 
> > > devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
> > labs.org [mailto:devicetree-discuss->
> > bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On
> > > Behalf Of David Gibson
> > > Sent: Wednesday, January 06, 2010 6:51 PM
> > > To: Yoder Stuart-B08248
> > > Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> > > Subject: Re: RFC: proposal to extend the open-pic interrupt 
> > > specifierdefinition
> > > 
> > > On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder 
> Stuart-B08248 wrote:
> > > > 
> > > > The current open-pic binding defines that interrupt specifiers 
> > > > have 2 cells-- an interrupt number and level/sense encoding.
> > > > 
> > > > With chips like the P4080 this is no longer sufficient to 
> > > > represent the various types of interrupt sources handled by the 
> > > > interrupt controller.  A linear list of interrupt 
> numbers doesn't 
> > > > handle all interrupt types-- there are at least 4 
> different kinds 
> > > > of interrupts on the P4080.
> > > > 
> > > > We have a proposal to extend the open-pic binding in a 
> backwards 
> > > > compatible way to encode additional information in the 
> level/sense 
> > > > field.
> > > > 
> > > > The current definition of level/sense is:
> > > >   0 = low to high edge sensitive type enabled
> > > >   1 = active low level sensitive type enabled
> > > >   2 = active high level sensitive type enabled
> > > >   3 = high to low edge sensitive type enabled
> > > > 
> > > > Those 2 bits would retain their current meaning, but the full 
> > > > encoding would be extended as follows:
> > > > 
> > > >      bits      meaning
> > > >      ----------------------------------------------
> > > >      0-7       interrupt sub-type
> > > >      8-15      interrupt type
> > > >      16-23     implementation dependent
> > > >      24-29     reserved
> > > >      30-31     level/sense encoding
> > > 
> > > Um.. what do "type" and "sub-type" mean in this context?
> > 
> > "type" specifies the type of interrupt-- example timer, 
> MSI, etc and 
> > would define the meaning of the interrupt number
> > portion of the interrupt specifier.   A given "type" may or
> > may not have a "subtype" depending on the binding.
> > 
> > As described in the proposal, "type" is a range of numbers, divided 
> > between standard/architected types and implementation 
> specific types.
> > 
> > We (Freescale) have at least one interrupt type "error" in 
> the P4080 
> > that would have a "sub-type" that would indicate a related bit in 
> > another status register.
> 
> And who is the type/subtype relevant to?  From what you've 
> said here, I don't see why it needs to be in the interrupt specifiers.
> 
Error interrupt case would be relevant for a hypervisor in a virtual
environment. Hypervisor may have to virtualize access to a paltform
error register.

-Varun

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
  2010-01-07  4:55         ` David Gibson
  2010-01-07 11:17           ` [Power.org:parch] " Sethi Varun-B16395
@ 2010-01-07 17:18           ` Scott Wood
       [not found]             ` <4B46176F.2040600-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
  2010-01-07 17:39           ` Yoder Stuart-B08248
  2 siblings, 1 reply; 24+ messages in thread
From: Scott Wood @ 2010-01-07 17:18 UTC (permalink / raw)
  To: David Gibson
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg, Kumar Gala

David Gibson wrote:
> On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder Stuart-B08248 wrote:
>> "type" specifies the type of interrupt-- example timer, MSI,
>> etc and would define the meaning of the interrupt number
>> portion of the interrupt specifier.   A given "type" may or
>> may not have a "subtype" depending on the binding.
>>
>> As described in the proposal, "type" is a range of numbers,
>> divided between standard/architected types and implementation
>> specific types.
>>
>> We (Freescale) have at least one interrupt type "error" in the P4080
>> that would have a "sub-type" that would indicate a related bit in
>> another
>> status register.
> 
> And who is the type/subtype relevant to? 

The interrupt controller driver.

> From what you've said here,
> I don't see why it needs to be in the interrupt specifiers.

The case that this grew out of was the p4080 error interrupt, which is 
basically a cascaded interrupt controller within the mpic.  All errors 
arrive on the same mpic vector, but each has its own bit in summary and 
mask registers.  The subtype contains that bit number.

It could also be used for expressing interrupts such as IPIs and timers 
that aren't part of the numbered interrupts that start at offset 0x10000.

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: RFC: proposal to extend the open-pic interrupt specifierdefinition
  2010-01-07  4:55         ` David Gibson
  2010-01-07 11:17           ` [Power.org:parch] " Sethi Varun-B16395
  2010-01-07 17:18           ` Scott Wood
@ 2010-01-07 17:39           ` Yoder Stuart-B08248
       [not found]             ` <9696D7A991D0824DBA8DFAC74A9C5FA305987F94-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  2 siblings, 1 reply; 24+ messages in thread
From: Yoder Stuart-B08248 @ 2010-01-07 17:39 UTC (permalink / raw)
  To: David Gibson
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

 

> -----Original Message-----
> From: 
> devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
labs.org [mailto:devicetree-discuss->
bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On 
> Behalf Of David Gibson
> Sent: Wednesday, January 06, 2010 10:55 PM
> To: Yoder Stuart-B08248
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> Subject: Re: RFC: proposal to extend the open-pic interrupt 
> specifierdefinition
> 
> On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder Stuart-B08248 wrote:
> > > From: 
> > > devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
> > labs.org [mailto:devicetree-discuss->
> > bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On 
> > > Behalf Of David Gibson
> > > Sent: Wednesday, January 06, 2010 6:51 PM
> > > To: Yoder Stuart-B08248
> > > Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> > > Subject: Re: RFC: proposal to extend the open-pic interrupt 
> > > specifierdefinition
> > > 
> > > On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder 
> Stuart-B08248 wrote:
> > > > 
> > > > The current open-pic binding defines that interrupt specifiers
> > > > have 2 cells-- an interrupt number and level/sense encoding.
> > > > 
> > > > With chips like the P4080 this is no longer sufficient to
> > > > represent the various types of interrupt sources handled by
> > > > the interrupt controller.  A linear list of interrupt numbers
> > > > doesn't handle all interrupt types-- there are at least 
> 4 different
> > > > kinds of interrupts on the P4080.
> > > > 
> > > > We have a proposal to extend the open-pic binding in
> > > > a backwards compatible way to encode additional information
> > > > in the level/sense field.
> > > > 
> > > > The current definition of level/sense is:
> > > >   0 = low to high edge sensitive type enabled
> > > >   1 = active low level sensitive type enabled
> > > >   2 = active high level sensitive type enabled
> > > >   3 = high to low edge sensitive type enabled
> > > > 
> > > > Those 2 bits would retain their current meaning, but the
> > > > full encoding would be extended as follows:
> > > > 
> > > >      bits      meaning
> > > >      ----------------------------------------------
> > > >      0-7       interrupt sub-type
> > > >      8-15      interrupt type
> > > >      16-23     implementation dependent
> > > >      24-29     reserved
> > > >      30-31     level/sense encoding
> > > 
> > > Um.. what do "type" and "sub-type" mean in this context?
> > 
> > "type" specifies the type of interrupt-- example timer, MSI,
> > etc and would define the meaning of the interrupt number
> > portion of the interrupt specifier.   A given "type" may or
> > may not have a "subtype" depending on the binding.
> > 
> > As described in the proposal, "type" is a range of numbers,
> > divided between standard/architected types and implementation
> > specific types.
> > 
> > We (Freescale) have at least one interrupt type "error" in the P4080
> > that would have a "sub-type" that would indicate a related bit in
> > another
> > status register.
> 
> And who is the type/subtype relevant to?  From what you've said here,
> I don't see why it needs to be in the interrupt specifiers.

It is relevant to whatever code manages the interrupt
controller.

A bit more background information. The MPIC in Freescale chips
supports several types of interrupts-- SOC devices, external 
interrupts, MSIs, timers.   The registers to manage these interrupt
sources are not laid out in a way that is conducive to just enumerating
all the interrupt sources starting from zero.   They are in different
discrete areas of the MPIC register map.

We need the device tree to be able to represent all interrupt sources
and distinguish between timer interrupt 0 and SOC device interrupt
0 and MSI interrupt 0.

In the P4080 we have additional complexity in that SOC device
interrupt 0 is error interrupt that has multiple sources of
errors ORed into it.   We don't want to hardcode knowlege of the
specific topography of the error interrupts into software and
want to represent in the dev tree.

Stuart

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifier definition
  2010-01-07  0:50   ` David Gibson
  2010-01-07  3:33     ` RFC: proposal to extend the open-pic interrupt specifierdefinition Yoder Stuart-B08248
@ 2010-01-08  1:02     ` Benjamin Herrenschmidt
  1 sibling, 0 replies; 24+ messages in thread
From: Benjamin Herrenschmidt @ 2010-01-08  1:02 UTC (permalink / raw)
  To: David Gibson
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Thu, 2010-01-07 at 11:50 +1100, David Gibson wrote:
> On Tue, Jan 05, 2010 at 04:28:12PM -0700, Yoder Stuart-B08248 wrote:
> > 
> > The current open-pic binding defines that interrupt specifiers
> > have 2 cells-- an interrupt number and level/sense encoding.
> > 
> > With chips like the P4080 this is no longer sufficient to
> > represent the various types of interrupt sources handled by
> > the interrupt controller.  A linear list of interrupt numbers
> > doesn't handle all interrupt types-- there are at least 4 different
> > kinds of interrupts on the P4080.
> > 
> > We have a proposal to extend the open-pic binding in
> > a backwards compatible way to encode additional information
> > in the level/sense field.
> > 
> > The current definition of level/sense is:
> >   0 = low to high edge sensitive type enabled
> >   1 = active low level sensitive type enabled
> >   2 = active high level sensitive type enabled
> >   3 = high to low edge sensitive type enabled
> > 
> > Those 2 bits would retain their current meaning, but the
> > full encoding would be extended as follows:
> > 
> >      bits      meaning
> >      ----------------------------------------------
> >      0-7       interrupt sub-type
> >      8-15      interrupt type
> >      16-23     implementation dependent
> >      24-29     reserved
> >      30-31     level/sense encoding
> 
> Um.. what do "type" and "sub-type" mean in this context?

Also keep in mind that Apple has already been playing games with the
first cell of the interrupt specifier on mpic :-)

Not a big deal, but we'll have to be careful in the driver to properly
flag the "standard" extensions vs. the "apple" ones.

Cheers,
Ben.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found] ` <9696D7A991D0824DBA8DFAC74A9C5FA30590506E-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  2010-01-07  0:50   ` David Gibson
@ 2010-01-12 18:17   ` Segher Boessenkool
       [not found]     ` <50433.84.105.60.153.1263320254.squirrel-JorI+TVEvZrY24RiXHRV3ti2O/JbrIOy@public.gmane.org>
  1 sibling, 1 reply; 24+ messages in thread
From: Segher Boessenkool @ 2010-01-12 18:17 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

> The current open-pic binding defines that interrupt specifiers
> have 2 cells-- an interrupt number and level/sense encoding.
>
> With chips like the P4080 this is no longer sufficient to
> represent the various types of interrupt sources handled by
> the interrupt controller.  A linear list of interrupt numbers
> doesn't handle all interrupt types-- there are at least 4 different
> kinds of interrupts on the P4080.
>
> We have a proposal to extend the open-pic binding in
> a backwards compatible way to encode additional information
> in the level/sense field.

Why can you not have a particular "compatible" for your device,
i.e. have a new binding for it?  Changing the "base" binding is
asking for trouble.

You can of course base your binding on the openpic one.

> The advantage of the above approach is backwards compatibility.
> Existing interrupt specifiers (and device trees) are valid with
> this proposal.

Actually they're not, like BenH pointed out.


Segher

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]     ` <50433.84.105.60.153.1263320254.squirrel-JorI+TVEvZrY24RiXHRV3ti2O/JbrIOy@public.gmane.org>
@ 2010-01-12 18:36       ` Yoder Stuart-B08248
       [not found]         ` <9696D7A991D0824DBA8DFAC74A9C5FA305988778-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Yoder Stuart-B08248 @ 2010-01-12 18:36 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

 

> -----Original Message-----
> From: 
> devicetree-discuss-bounces+stuart.yoder=freescale.com-uLR06cmDAlY@public.gmane.org
labs.org [mailto:devicetree-discuss->
bounces+stuart.yoder=freescale.com-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org] On 
> Behalf Of Segher Boessenkool
> Sent: Tuesday, January 12, 2010 12:18 PM
> To: Yoder Stuart-B08248
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org
> Subject: Re: RFC: proposal to extend the open-pic interrupt 
> specifier definition
> 
> > The current open-pic binding defines that interrupt specifiers
> > have 2 cells-- an interrupt number and level/sense encoding.
> >
> > With chips like the P4080 this is no longer sufficient to
> > represent the various types of interrupt sources handled by
> > the interrupt controller.  A linear list of interrupt numbers
> > doesn't handle all interrupt types-- there are at least 4 different
> > kinds of interrupts on the P4080.
> >
> > We have a proposal to extend the open-pic binding in
> > a backwards compatible way to encode additional information
> > in the level/sense field.
> 
> Why can you not have a particular "compatible" for your device,
> i.e. have a new binding for it?  Changing the "base" binding is
> asking for trouble.
> 
> You can of course base your binding on the openpic one.

Sure we can do that, but it is nice to standardize if possible to
avoid gratuitous different solutions solving the same problem.

> > The advantage of the above approach is backwards compatibility.
> > Existing interrupt specifiers (and device trees) are valid with
> > this proposal.
> 
> Actually they're not, like BenH pointed out.

The proposal is backwards compatible.  An existing interrupt
specifier  (e.g. interrupts = <24 2>;) retains its exact
same meaning.  Old device trees do not need to change
to comply with the proposal.

I'm not directly familiar with the case Ben pointed out, but
it sounded like Apple used the 1st cell in some non-standard
way.

It is true that openpic drivers would need to change to handle
the new specifier-- minimally masking the level/sense field
to 2 bits.

Stuart

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]             ` <4B46176F.2040600-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
@ 2010-01-13  6:04               ` Grant Likely
       [not found]                 ` <fa686aa41001122204w3ce1a31m7debe52a4b5d4fc7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-13  6:04 UTC (permalink / raw)
  To: Scott Wood
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg, Kumar Gala, David Gibson

On Thu, Jan 7, 2010 at 10:18 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> David Gibson wrote:
>>
>> On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder Stuart-B08248 wrote:
>>>
>>> "type" specifies the type of interrupt-- example timer, MSI,
>>> etc and would define the meaning of the interrupt number
>>> portion of the interrupt specifier.   A given "type" may or
>>> may not have a "subtype" depending on the binding.
>>>
>>> As described in the proposal, "type" is a range of numbers,
>>> divided between standard/architected types and implementation
>>> specific types.
>>>
>>> We (Freescale) have at least one interrupt type "error" in the P4080
>>> that would have a "sub-type" that would indicate a related bit in
>>> another
>>> status register.
>>
>> And who is the type/subtype relevant to?
>
> The interrupt controller driver.
>
>> From what you've said here,
>> I don't see why it needs to be in the interrupt specifiers.
>
> The case that this grew out of was the p4080 error interrupt, which is
> basically a cascaded interrupt controller within the mpic.  All errors
> arrive on the same mpic vector, but each has its own bit in summary and mask
> registers.  The subtype contains that bit number.
>
> It could also be used for expressing interrupts such as IPIs and timers that
> aren't part of the numbered interrupts that start at offset 0x10000.

If it really does behave as a cascaded irq, then give is a separate
irq number space; either by a separate error-cascade interrupt
controller node, or by choosing a new range of hw irq numbers outside
of the current range handled by the mpic.  It does not sound sane or
particularly parseable to stuff it into bitfields within the second
cell.

Users have enough trouble parsing irq specifiers as is.  It makes me
nervous to see even more complicated irq specifiers being devised.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]         ` <9696D7A991D0824DBA8DFAC74A9C5FA305988778-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
@ 2010-01-13  6:06           ` Grant Likely
       [not found]             ` <fa686aa41001122206l232b270er58323a6409e575e9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-13  6:06 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Tue, Jan 12, 2010 at 11:36 AM, Yoder Stuart-B08248
<B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> > The advantage of the above approach is backwards compatibility.
>> > Existing interrupt specifiers (and device trees) are valid with
>> > this proposal.
>>
>> Actually they're not, like BenH pointed out.
>
> The proposal is backwards compatible.  An existing interrupt
> specifier  (e.g. interrupts = <24 2>;) retains its exact
> same meaning.  Old device trees do not need to change
> to comply with the proposal.

You also need to deal with the case of old drivers incorrectly binding
to and trying to understand the new data.

> I'm not directly familiar with the case Ben pointed out, but
> it sounded like Apple used the 1st cell in some non-standard
> way.
>
> It is true that openpic drivers would need to change to handle
> the new specifier-- minimally masking the level/sense field
> to 2 bits.

Which makes the new binding incompatible with old kernels/drivers
which just leads to confusion.  It's not worth toying with.  Just
create a new compatible value for this new binding and be done with
it.  When a driver gets modified to handle the new behaviour, then it
can be also changed to bind against the new compatible value too.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]             ` <9696D7A991D0824DBA8DFAC74A9C5FA305987F94-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
@ 2010-01-13  6:16               ` Grant Likely
  0 siblings, 0 replies; 24+ messages in thread
From: Grant Likely @ 2010-01-13  6:16 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg, David Gibson

On Thu, Jan 7, 2010 at 10:39 AM, Yoder Stuart-B08248
<B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> > > Um.. what do "type" and "sub-type" mean in this context?
>> >
>> > "type" specifies the type of interrupt-- example timer, MSI,
>> > etc and would define the meaning of the interrupt number
>> > portion of the interrupt specifier.   A given "type" may or
>> > may not have a "subtype" depending on the binding.
>> >
>> > As described in the proposal, "type" is a range of numbers,
>> > divided between standard/architected types and implementation
>> > specific types.
>> >
>> > We (Freescale) have at least one interrupt type "error" in the P4080
>> > that would have a "sub-type" that would indicate a related bit in
>> > another
>> > status register.
>>
>> And who is the type/subtype relevant to?  From what you've said here,
>> I don't see why it needs to be in the interrupt specifiers.
>
> It is relevant to whatever code manages the interrupt
> controller.
>
> A bit more background information. The MPIC in Freescale chips
> supports several types of interrupts-- SOC devices, external
> interrupts, MSIs, timers.   The registers to manage these interrupt
> sources are not laid out in a way that is conducive to just enumerating
> all the interrupt sources starting from zero.   They are in different
> discrete areas of the MPIC register map.
>
> We need the device tree to be able to represent all interrupt sources
> and distinguish between timer interrupt 0 and SOC device interrupt
> 0 and MSI interrupt 0.

Sounds to me that it would be far more sane to either use cascaded
interrupt nodes to handle these different irq number spaces, or to use
multiple cells for the hw irq number.  Trying to encode these things
into bit fields in the second cell sounds like trouble and confusion
to me, not to mention premature optimization to reduce the number of
cells being used per IRQ.

Oh, and *DOCUMENT THE HELL OUT OF IT*.  Give uses an easy reference to
figure out what the irq specifier needs to be for the device they're
using.  I had enough trouble from the MPC5200's 2 level irq number
scheme in this regard.  The confusion finally went away (mostly) when
I stopped trying to describe to everyone how it worked and instead
specifically documented that IRQ1 == <0 0 x>, IRQ2 == <1 1 x>, IRQ3 ==
<1 2 x>, IRQ4 == <1 3 x>.  :-)

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]             ` <fa686aa41001122206l232b270er58323a6409e575e9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-13  6:24               ` Grant Likely
       [not found]                 ` <fa686aa41001122224t2eeec9acy15b40420ebe18541-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2010-01-13 18:02               ` Scott Wood
  1 sibling, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-13  6:24 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Tue, Jan 12, 2010 at 11:06 PM, Grant Likely
<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Tue, Jan 12, 2010 at 11:36 AM, Yoder Stuart-B08248
> <B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>> > The advantage of the above approach is backwards compatibility.
>>> > Existing interrupt specifiers (and device trees) are valid with
>>> > this proposal.
>>>
>>> Actually they're not, like BenH pointed out.
>>
>> The proposal is backwards compatible.  An existing interrupt
>> specifier  (e.g. interrupts = <24 2>;) retains its exact
>> same meaning.  Old device trees do not need to change
>> to comply with the proposal.
>
> You also need to deal with the case of old drivers incorrectly binding
> to and trying to understand the new data.
>
>> I'm not directly familiar with the case Ben pointed out, but
>> it sounded like Apple used the 1st cell in some non-standard
>> way.
>>
>> It is true that openpic drivers would need to change to handle
>> the new specifier-- minimally masking the level/sense field
>> to 2 bits.
>
> Which makes the new binding incompatible with old kernels/drivers
> which just leads to confusion.  It's not worth toying with.  Just
> create a new compatible value for this new binding and be done with
> it.  When a driver gets modified to handle the new behaviour, then it
> can be also changed to bind against the new compatible value too.

Oh, and BTW, this is *exactly* why I advocate being explicit about
what part the node describes instead of depending on some generic
name.  ie. "fsl,p4080-mpic" instead of "chrp,open-pic".  So that you
can deal with part specific oddities and so you can create new
bindings when necessary.  Nodes can still put backwards compatible
entries in the compatible list after the specific device when
appropriate so that existing drivers can still bind to them.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]                 ` <fa686aa41001122204w3ce1a31m7debe52a4b5d4fc7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-13 14:19                   ` Yoder Stuart-B08248
       [not found]                     ` <9696D7A991D0824DBA8DFAC74A9C5FA305988946-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Yoder Stuart-B08248 @ 2010-01-13 14:19 UTC (permalink / raw)
  To: Grant Likely, Wood Scott-B07421
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg, Gala Kumar-B11780, David Gibson

 

> -----Original Message-----
> From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On 
> Behalf Of Grant Likely
> Sent: Wednesday, January 13, 2010 12:04 AM
> To: Wood Scott-B07421
> Cc: David Gibson; Yoder Stuart-B08248; 
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; parch-QRwYI7m9GJLYtjvyW6yDsg@public.gmane.org; Gala 
> Kumar-B11780
> Subject: Re: [Power.org:parch] Re: RFC: proposal to extend 
> the open-pic interrupt specifierdefinition
> 
> On Thu, Jan 7, 2010 at 10:18 AM, Scott Wood 
> <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > David Gibson wrote:
> >>
> >> On Wed, Jan 06, 2010 at 08:33:03PM -0700, Yoder 
> Stuart-B08248 wrote:
> >>>
> >>> "type" specifies the type of interrupt-- example timer, MSI,
> >>> etc and would define the meaning of the interrupt number
> >>> portion of the interrupt specifier.   A given "type" may or
> >>> may not have a "subtype" depending on the binding.
> >>>
> >>> As described in the proposal, "type" is a range of numbers,
> >>> divided between standard/architected types and implementation
> >>> specific types.
> >>>
> >>> We (Freescale) have at least one interrupt type "error" 
> in the P4080
> >>> that would have a "sub-type" that would indicate a related bit in
> >>> another
> >>> status register.
> >>
> >> And who is the type/subtype relevant to?
> >
> > The interrupt controller driver.
> >
> >> From what you've said here,
> >> I don't see why it needs to be in the interrupt specifiers.
> >
> > The case that this grew out of was the p4080 error 
> interrupt, which is
> > basically a cascaded interrupt controller within the mpic.  
All errors
> > arrive on the same mpic vector, but each has its own bit in 
> summary and mask
> > registers.  The subtype contains that bit number.
> >
> > It could also be used for expressing interrupts such as 
> IPIs and timers that
> > aren't part of the numbered interrupts that start at offset 0x10000.
> 
> If it really does behave as a cascaded irq, then give is a separate
> irq number space; either by a separate error-cascade interrupt
> controller node

We considered that-- treating this case as a separate interrupt
domain.  That starts getting really ugly in that a number of devices
will then have 2 interrupt controllers for the same device.  The
device tree does not support that nicely.  It requires creating
a interrupt-map under the device so that different interrupt
numbers can mapped to different interrupt controller.  It possible
but ugly.   Having numerous interrupt maps scattered around
under device nodes makes things must less human-readable and
less clear what is going on.

It also does not solve the problem of representing things
like the MPIC timers which do not behave as a cascaded
interrupt controller.

> or by choosing a new range of hw irq numbers outside
> of the current range handled by the mpic.

But where would the meaning of the new range live.  Yes, we
could pick interrupt numbers 2000-2032 to be error interrupts,
but we want to avoid hard coding assumptions like that
in the MPIC driver.

> It does not sound sane or
> particularly parseable to stuff it into bitfields within the second
> cell.

I think it is somewhat sane compared to the alternatives.  The
second cell encodes information about the interrupt source.  Allowing
some of those bits to encode information besides level/sense
doesn't seem that difficult.

> Users have enough trouble parsing irq specifiers as is.  It makes me
> nervous to see even more complicated irq specifiers being devised.

Yes, they become slightly more complicated, but the complexity needs to
go somewhere.

Stuart

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]                     ` <9696D7A991D0824DBA8DFAC74A9C5FA305988946-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
@ 2010-01-13 14:27                       ` Grant Likely
       [not found]                         ` <fa686aa41001130627g7ed3bd46p6ac50631576d89ef-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-13 14:27 UTC (permalink / raw)
  To: Yoder Stuart-B08248
  Cc: Wood Scott-B07421, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg, Gala Kumar-B11780, David Gibson

On Wed, Jan 13, 2010 at 7:19 AM, Yoder Stuart-B08248
<B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> It does not sound sane or
>> particularly parseable to stuff it into bitfields within the second
>> cell.
>
> I think it is somewhat sane compared to the alternatives.  The
> second cell encodes information about the interrupt source.  Allowing
> some of those bits to encode information besides level/sense
> doesn't seem that difficult.

Not difficult.  Ugly, unnecessary, and sounds like a premature optimization.

>> Users have enough trouble parsing irq specifiers as is.  It makes me
>> nervous to see even more complicated irq specifiers being devised.
>
> Yes, they become slightly more complicated, but the complexity needs to
> go somewhere.

Then at the very least do it as separate cells.  Carving cells into
multiple fields is pretty ugly when cells are cheap.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]                         ` <fa686aa41001130627g7ed3bd46p6ac50631576d89ef-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-13 17:23                           ` Scott Wood
       [not found]                             ` <4B4E01A8.5070509-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Scott Wood @ 2010-01-13 17:23 UTC (permalink / raw)
  To: Grant Likely
  Cc: Wood Scott-B07421, parch-QRwYI7m9GJLYtjvyW6yDsg,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David Gibson,
	Yoder Stuart-B08248, Gala Kumar-B11780

Grant Likely wrote:
> On Wed, Jan 13, 2010 at 7:19 AM, Yoder Stuart-B08248
> <B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>> It does not sound sane or
>>> particularly parseable to stuff it into bitfields within the second
>>> cell.
>> I think it is somewhat sane compared to the alternatives.  The
>> second cell encodes information about the interrupt source.  Allowing
>> some of those bits to encode information besides level/sense
>> doesn't seem that difficult.
> 
> Not difficult.  Ugly, unnecessary, and sounds like a premature optimization.

It is not optimization, but functionality.

>>> Users have enough trouble parsing irq specifiers as is.  It makes me
>>> nervous to see even more complicated irq specifiers being devised.
>> Yes, they become slightly more complicated, but the complexity needs to
>> go somewhere.
> 
> Then at the very least do it as separate cells.  Carving cells into
> multiple fields is pretty ugly when cells are cheap.

That means that all the interrupt specifiers in an existing tree have to 
be updated with the larger numer of cells whenever such an interrupt is 
added, since #interrupt-cells applies globally to the MPIC interrupt 
domain.  It's unnecessary churn.

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]             ` <fa686aa41001122206l232b270er58323a6409e575e9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2010-01-13  6:24               ` Grant Likely
@ 2010-01-13 18:02               ` Scott Wood
       [not found]                 ` <4B4E0AC7.7090502-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
  1 sibling, 1 reply; 24+ messages in thread
From: Scott Wood @ 2010-01-13 18:02 UTC (permalink / raw)
  To: Grant Likely
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

Grant Likely wrote:
> On Tue, Jan 12, 2010 at 11:36 AM, Yoder Stuart-B08248
> <B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>> The advantage of the above approach is backwards compatibility.
>>>> Existing interrupt specifiers (and device trees) are valid with
>>>> this proposal.
>>> Actually they're not, like BenH pointed out.
>> The proposal is backwards compatible.  An existing interrupt
>> specifier  (e.g. interrupts = <24 2>;) retains its exact
>> same meaning.  Old device trees do not need to change
>> to comply with the proposal.
> 
> You also need to deal with the case of old drivers incorrectly binding
> to and trying to understand the new data.
> 
>> I'm not directly familiar with the case Ben pointed out, but
>> it sounded like Apple used the 1st cell in some non-standard
>> way.
>>
>> It is true that openpic drivers would need to change to handle
>> the new specifier-- minimally masking the level/sense field
>> to 2 bits.
> 
> Which makes the new binding incompatible with old kernels/drivers
> which just leads to confusion.

FWIW, Linux already does mask those bits.

> It's not worth toying with.  Just create a new compatible value for this new binding and be done with
> it.  When a driver gets modified to handle the new behaviour, then it
> can be also changed to bind against the new compatible value too.

I agree that a new compatible is warranted from a theoretical 
perspetive, though from a practical compatibility perspective one should 
consider the odds of something breaking because old code chokes on the 
new bits, versus the old code not recognizing the new compatible and 
thus not binding the device at all.

Is there any interest in standardizing this with a new compatible, or 
should it be FSL-only?

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]                 ` <fa686aa41001122224t2eeec9acy15b40420ebe18541-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-13 18:06                   ` Scott Wood
       [not found]                     ` <4B4E0B96.1030204-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Scott Wood @ 2010-01-13 18:06 UTC (permalink / raw)
  To: Grant Likely
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

Grant Likely wrote:
> Oh, and BTW, this is *exactly* why I advocate being explicit about
> what part the node describes instead of depending on some generic
> name.  ie. "fsl,p4080-mpic" instead of "chrp,open-pic".  So that you
> can deal with part specific oddities and so you can create new
> bindings when necessary.  Nodes can still put backwards compatible
> entries in the compatible list after the specific device when
> appropriate so that existing drivers can still bind to them.

I fully agree that there should be a more specific compatible -- this 
doesn't replace that (indeed, it's required to interpret the 
implementation-specific sections of the specifier).  The question is 
whether we could still include chrp,open-pic in the compatible list if 
there are additional bits set in the interrupt specifier.

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]                     ` <4B4E0B96.1030204-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
@ 2010-01-17  6:42                       ` Grant Likely
  0 siblings, 0 replies; 24+ messages in thread
From: Grant Likely @ 2010-01-17  6:42 UTC (permalink / raw)
  To: Scott Wood
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	parch-QRwYI7m9GJLYtjvyW6yDsg

On Wed, Jan 13, 2010 at 11:06 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant Likely wrote:
>>
>> Oh, and BTW, this is *exactly* why I advocate being explicit about
>> what part the node describes instead of depending on some generic
>> name.  ie. "fsl,p4080-mpic" instead of "chrp,open-pic".  So that you
>> can deal with part specific oddities and so you can create new
>> bindings when necessary.  Nodes can still put backwards compatible
>> entries in the compatible list after the specific device when
>> appropriate so that existing drivers can still bind to them.
>
> I fully agree that there should be a more specific compatible -- this
> doesn't replace that (indeed, it's required to interpret the
> implementation-specific sections of the specifier).  The question is whether
> we could still include chrp,open-pic in the compatible list if there are
> additional bits set in the interrupt specifier.

My vote is no.

g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]                 ` <4B4E0AC7.7090502-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
@ 2010-01-17  7:06                   ` Grant Likely
       [not found]                     ` <fa686aa41001162306x7c4e3dedy196409a4c8420e35-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-17  7:06 UTC (permalink / raw)
  To: Scott Wood; +Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

On Wed, Jan 13, 2010 at 11:02 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant Likely wrote:
>> It's not worth toying with.  Just create a new compatible value for this
>> new binding and be done with
>> it.  When a driver gets modified to handle the new behaviour, then it
>> can be also changed to bind against the new compatible value too.
>
> I agree that a new compatible is warranted from a theoretical perspetive,
> though from a practical compatibility perspective one should consider the
> odds of something breaking because old code chokes on the new bits, versus
> the old code not recognizing the new compatible and thus not binding the
> device at all.

Letting old drivers bind against new bindings that aren't actually the
same isn't good practice and leaves little recourse if we really do
need to differentiate between them in the future.  Teaching the
current driver to match against the new value is a one line change.
Choosing a new compatible value is so inexpensive, trivial, and safe
that I think it is a no-brainer decision.

> Is there any interest in standardizing this with a new compatible, or should
> it be FSL-only?

Instead of trying to come up with a new "generic/standard" compatible
value, just adopt the name of the first part to use the new binding as
the standard compatible value.  If the binding catches on then great,
new parts will just claim compatibility with the old.  If it does not,
then we won't have wasted time trying to predict the future and
wrangling about what a so-called generic binding should look like.

So, instead of trying to do something like the following for, say, a
future 16 core version of the p4080:

compatible = "fsl,p4160-mpic", 'open-pic-enhanced";

Do this instead:

compatible = "fsl,p4160-mpic", "fsl,p4080-mpic"

This also has the advantage of the meaning of a fsl,p4080-mpic
compatible device is grounded in some real piece of hardware, not in
something made up who's definition can change over time.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]                             ` <4B4E01A8.5070509-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
@ 2010-01-17  7:13                               ` Grant Likely
       [not found]                                 ` <fa686aa41001162313u28ef0fb4s328ebc25bf5e551f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 24+ messages in thread
From: Grant Likely @ 2010-01-17  7:13 UTC (permalink / raw)
  To: Scott Wood
  Cc: Wood Scott-B07421, parch-QRwYI7m9GJLYtjvyW6yDsg,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David Gibson,
	Yoder Stuart-B08248, Gala Kumar-B11780

On Wed, Jan 13, 2010 at 10:23 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant Likely wrote:
>>
>> On Wed, Jan 13, 2010 at 7:19 AM, Yoder Stuart-B08248
>> <B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>>
>>>> It does not sound sane or
>>>> particularly parseable to stuff it into bitfields within the second
>>>> cell.
>>>
>>> I think it is somewhat sane compared to the alternatives.  The
>>> second cell encodes information about the interrupt source.  Allowing
>>> some of those bits to encode information besides level/sense
>>> doesn't seem that difficult.
>>
>> Not difficult.  Ugly, unnecessary, and sounds like a premature
>> optimization.
>
> It is not optimization, but functionality.

I don't doubt you need the data.  What I object to is stuffing it all
into a single cell since I think it is an unneeded optimization that
also makes it less user friendly.

>>>> Users have enough trouble parsing irq specifiers as is.  It makes me
>>>> nervous to see even more complicated irq specifiers being devised.
>>>
>>> Yes, they become slightly more complicated, but the complexity needs to
>>> go somewhere.
>>
>> Then at the very least do it as separate cells.  Carving cells into
>> multiple fields is pretty ugly when cells are cheap.
>
> That means that all the interrupt specifiers in an existing tree have to be
> updated with the larger numer of cells whenever such an interrupt is added,
> since #interrupt-cells applies globally to the MPIC interrupt domain.  It's
> unnecessary churn.

Pish.  You're talking about updating the interrupt properties in each
affected .dts file, and it only needs to be done when the new binding
is applied.  It will simply be adding a bunch of '0' cells to the
beginning of each existing interrupts property in the file.  Easy to
review, not a hard change to make, and easier to use/understand
binding for a long time to come.  BTW, how many .dts files do we
really have in the tree right now that will be using the new binding?
I'm guessing not many.  Heck, I'll even volunteer to make the change
in all affected .dts files when then new binding comes on-line.

Make the change now and don't burden us with a binding we'll be
cursing for the next 10 years.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: RFC: proposal to extend the open-pic interrupt specifier definition
       [not found]                     ` <fa686aa41001162306x7c4e3dedy196409a4c8420e35-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-18 17:11                       ` Scott Wood
  0 siblings, 0 replies; 24+ messages in thread
From: Scott Wood @ 2010-01-18 17:11 UTC (permalink / raw)
  To: Grant Likely
  Cc: Yoder Stuart-B08248, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Grant Likely wrote:
> On Wed, Jan 13, 2010 at 11:02 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> It's not worth toying with.  Just create a new compatible value for this
>>> new binding and be done with
>>> it.  When a driver gets modified to handle the new behaviour, then it
>>> can be also changed to bind against the new compatible value too.
>> I agree that a new compatible is warranted from a theoretical perspetive,
>> though from a practical compatibility perspective one should consider the
>> odds of something breaking because old code chokes on the new bits, versus
>> the old code not recognizing the new compatible and thus not binding the
>> device at all.
> 
> Letting old drivers bind against new bindings that aren't actually
> the same isn't good practice and leaves little recourse if we really
> do need to differentiate between them in the future. Teaching the 
> current driver to match against the new value is a one line change. 
> Choosing a new compatible value is so inexpensive, trivial, and safe 
> that I think it is a no-brainer decision.

Yes, I said that it's theoretically a bad thing to do, but in practice I 
really don't see what it would break that wouldn't be just as broken by 
a change in compatible -- unless we leave in the device_type that the 
kernel actually binds on at the moment :-P -- and I can see cases where 
it would improve compatibility.  Making that one-line change without a 
time machine won't stop the e-mails asking why an old kernel won't boot 
with a new device tree, especially if we ever get to the point where 
we're sharing device tree source fragments and don't want to keep 
separate old-binding and new-binding fragments for each device.

I really don't see anyone supporting both bindings and doing something 
different with one versus the other.  It's not like we're talking about 
leaving out chip information where there might be hardware quirks to be 
dealt with.  It's just a use of previously unused bits (check against 
zero if you care), or additional cells (check #interrupt-cells if you 
care).  The only thing that makes it a bit questionable is the ambiguity 
about whether the existing values are the low two bits of a cell, or a 
whole-cell enumeration of configurations -- the odd, non-orthogonal 
encoding does point to the latter.

>> Is there any interest in standardizing this with a new compatible, or should
>> it be FSL-only?
> 
> Instead of trying to come up with a new "generic/standard" compatible
> value, just adopt the name of the first part to use the new binding as
> the standard compatible value.  If the binding catches on then great,
> new parts will just claim compatibility with the old.

Assuming new parts are compatible with the previous part, which is less 
likely than being compatible with the base openpic design.

But even if there's nothing generic to match on, it would be nice if 
different openpic implementations didn't do gratuitously different 
things in their bindings, hence the RFC.

> This also has the advantage of the meaning of a fsl,p4080-mpic
> compatible device is grounded in some real piece of hardware, not in
> something made up who's definition can change over time.

Because nothing ever changes in new chip revisions of the same name. :-)

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifierdefinition
       [not found]                                 ` <fa686aa41001162313u28ef0fb4s328ebc25bf5e551f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-01-18 17:44                                   ` Scott Wood
  0 siblings, 0 replies; 24+ messages in thread
From: Scott Wood @ 2010-01-18 17:44 UTC (permalink / raw)
  To: Grant Likely
  Cc: Wood Scott-B07421, parch-QRwYI7m9GJLYtjvyW6yDsg,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David Gibson,
	Yoder Stuart-B08248, Gala Kumar-B11780

Grant Likely wrote:
> On Wed, Jan 13, 2010 at 10:23 AM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> On Wed, Jan 13, 2010 at 7:19 AM, Yoder Stuart-B08248
>>> <B08248-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>>> It does not sound sane or
>>>>> particularly parseable to stuff it into bitfields within the second
>>>>> cell.
>>>> I think it is somewhat sane compared to the alternatives.  The
>>>> second cell encodes information about the interrupt source.  Allowing
>>>> some of those bits to encode information besides level/sense
>>>> doesn't seem that difficult.
>>> Not difficult.  Ugly, unnecessary, and sounds like a premature
>>> optimization.
>> It is not optimization, but functionality.
> 
> I don't doubt you need the data.  What I object to is stuffing it all
> into a single cell since I think it is an unneeded optimization that
> also makes it less user friendly.

I'm fine with four cells (int config type subtype) if that's the 
consensus -- it's hard to guess in advance what will go over well.  User 
friendliness can be subjective.

>>>>> Users have enough trouble parsing irq specifiers as is.  It makes me
>>>>> nervous to see even more complicated irq specifiers being devised.
>>>> Yes, they become slightly more complicated, but the complexity needs to
>>>> go somewhere.
>>> Then at the very least do it as separate cells.  Carving cells into
>>> multiple fields is pretty ugly when cells are cheap.
>> That means that all the interrupt specifiers in an existing tree have to be
>> updated with the larger numer of cells whenever such an interrupt is added,
>> since #interrupt-cells applies globally to the MPIC interrupt domain.  It's
>> unnecessary churn.
> 
> Pish.  You're talking about updating the interrupt properties in each
> affected .dts file, and it only needs to be done when the new binding
> is applied.  It will simply be adding a bunch of '0' cells to the
> beginning of each existing interrupts property in the file.

More likely the end -- putting it at the beginning *will* break existing 
code and require software to know which binding it is dealing with.

And don't forget interrupt-maps (which are hard enough to read as is, 
due to too many cells with no delimiters of logical groupings), or 
interrupts properties with more than one interrupt.

> Easy to
> review, not a hard change to make, and easier to use/understand
> binding for a long time to come.  BTW, how many .dts files do we
> really have in the tree right now that will be using the new binding?

One that will need it, and a bunch of others that might want to use it 
either to describe things like MPIC timers or message register 
interrupts, or to be able to share device tree source fragments with 
newer chips if that effort ever gets revived.

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2010-01-18 17:44 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-05 23:28 RFC: proposal to extend the open-pic interrupt specifier definition Yoder Stuart-B08248
     [not found] ` <9696D7A991D0824DBA8DFAC74A9C5FA30590506E-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
2010-01-07  0:50   ` David Gibson
2010-01-07  3:33     ` RFC: proposal to extend the open-pic interrupt specifierdefinition Yoder Stuart-B08248
     [not found]       ` <9696D7A991D0824DBA8DFAC74A9C5FA305987E61-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
2010-01-07  4:55         ` David Gibson
2010-01-07 11:17           ` [Power.org:parch] " Sethi Varun-B16395
2010-01-07 17:18           ` Scott Wood
     [not found]             ` <4B46176F.2040600-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2010-01-13  6:04               ` Grant Likely
     [not found]                 ` <fa686aa41001122204w3ce1a31m7debe52a4b5d4fc7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-13 14:19                   ` Yoder Stuart-B08248
     [not found]                     ` <9696D7A991D0824DBA8DFAC74A9C5FA305988946-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
2010-01-13 14:27                       ` Grant Likely
     [not found]                         ` <fa686aa41001130627g7ed3bd46p6ac50631576d89ef-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-13 17:23                           ` Scott Wood
     [not found]                             ` <4B4E01A8.5070509-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2010-01-17  7:13                               ` Grant Likely
     [not found]                                 ` <fa686aa41001162313u28ef0fb4s328ebc25bf5e551f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-18 17:44                                   ` Scott Wood
2010-01-07 17:39           ` Yoder Stuart-B08248
     [not found]             ` <9696D7A991D0824DBA8DFAC74A9C5FA305987F94-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
2010-01-13  6:16               ` Grant Likely
2010-01-08  1:02     ` [Power.org:parch] Re: RFC: proposal to extend the open-pic interrupt specifier definition Benjamin Herrenschmidt
2010-01-12 18:17   ` Segher Boessenkool
     [not found]     ` <50433.84.105.60.153.1263320254.squirrel-JorI+TVEvZrY24RiXHRV3ti2O/JbrIOy@public.gmane.org>
2010-01-12 18:36       ` Yoder Stuart-B08248
     [not found]         ` <9696D7A991D0824DBA8DFAC74A9C5FA305988778-ofAVchDyotYzzZk0BCvKg5jmvxFtTJ+o0e7PPNI6Mm0@public.gmane.org>
2010-01-13  6:06           ` Grant Likely
     [not found]             ` <fa686aa41001122206l232b270er58323a6409e575e9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-13  6:24               ` Grant Likely
     [not found]                 ` <fa686aa41001122224t2eeec9acy15b40420ebe18541-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-13 18:06                   ` Scott Wood
     [not found]                     ` <4B4E0B96.1030204-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2010-01-17  6:42                       ` Grant Likely
2010-01-13 18:02               ` Scott Wood
     [not found]                 ` <4B4E0AC7.7090502-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2010-01-17  7:06                   ` Grant Likely
     [not found]                     ` <fa686aa41001162306x7c4e3dedy196409a4c8420e35-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-01-18 17:11                       ` Scott Wood

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.