[PATCH 1/2] powerpc: document the MPIC device tree binding

[Yoder Stuart-B08248]

> -----Original Message-----
> From: Meador Inge [mailto:meador_inge at mentor.com]
> Sent: Wednesday, January 19, 2011 2:25 PM
> To: Yoder Stuart-B08248
> Cc: linuxppc-dev at lists.ozlabs.org; devicetree-discuss at lists.ozlabs.org;
> Blanchard, Hollis
> Subject: Re: [PATCH 1/2] powerpc: document the MPIC device tree binding
> 
> On 01/18/2011 02:21 PM, Yoder Stuart-B08248 wrote:
> >>   Documentation/powerpc/dts-bindings/mpic.txt |   78
> >
> > This is really the binding for an open-pic interrupt controller and I
> > think the name should reflect that-- open-pic.txt.
> 
> Yup, agreed.
> 
> >> +This binding specifies what properties and child nodes must be
> >> +available on the device tree representation of the "MPIC" interrupt
> >> +controller.  This binding is based on the binding defined for Open
> >> +PIC in [1] and is a superset of that binding.
> >
> > I think it would be better to base this on the ePAPR binding which was
> > based on the original chrp binding.  Properties like "name"
> > and "device_type" are deprecated not being used in flat device trees.
> >
> > <http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pd
> > f>
> >
> > The proposed new properties really should go back into the ePAPR.
> 
> I read portions of ePAPR while writing this binding and considered that.
>   My only worry was that ePAPR is focused on embedded systems and this
> binding will have to cover non-embedded systems that exist in the kernel.
> However, perhaps that is not a legitimate concern?

The ePAPR tried to codify what was previously implemented in
Linux, so I don't think lack of things like "name" and
"device_type" in the binding are an issue.

> >> +
> >> +** Required properties:
> >> +
> >> +   NOTE: Many of these descriptions were paraphrased from [1] to aid
> >> +         readability.
> >> +
> >> +   - name : Specifies the name of the MPIC.
> >
> > Drop this.  No DTS files use it.
> 
> Done.
> 
> >> +   - device_type : Specifies the device type of this MPIC.  The
> >> + value of this
> >> +                   property shall be "open-pic".
> >
> > device_type is deprecated, since this is not real open-firmware.  In
> > practice the kernel is matching on device_type, but we want to move
> > away from that to match on "compatible", just hasn't been implemented
> > yet.
> 
> I will drop this property with the expectation that the kernel will be
> fixed.  From a quick grep of '.../arch/powerpc' it looks like most uses are
> of the form:
> 
>      np = of_find_node_by_type(NULL, "open-pic");
>      if (np == NULL)
>         return;
> 
> In most of these cases I suppose the 'of_find_node_by_type' calls could
> just be replaced with calls to 'of_find_compatible_node(NULL, "open-pic")'.

For backwards compatibility, we should continue to accept
the old/deprecated device_type="open-pic", but in addition
we should accept the compatible="open-pic".

> >> +   - reg : Specifies the base physical address(s) and size(s) of this
> >> + MPIC's
> >> +           addressable register space.
> >> +   - compatible : Specifies the compatibility list for the MPIC.  The
> >> + property
> >> +                  value shall include "chrp,open-pic".
> >
> > In the ePAPR we modified this to just "open-pic", because this has
> > nothing to do with chrp anymore.   I think just "open-pic" is
> > what we want.
> 
> OK, but as a migration path we should allow the kernel to accept both
> (Scott mentioned this in another reply), but "open-pic" is the
> documented correct way.

Right.

> >> +   - interrupt-controller : The presence of this property identifies
> >> + the node
> >> +                            as a MPIC.  No property value should be
> >> defined.
> >> +   - #address-cells : Specifies the number of cells needed to encode an
> >> +                      address.  The value of this property shall always
> >> + be 0
> >> +                      so that 'interrupt-map' nodes do not have to
> >> + specify a
> >> +                      parent unit address.
> >> +   - #interrupt-cells : Specifies the number of cells needed to encode
> >> + an
> >> +                        interrupt source.
> >
> > Should be 2, correct?
> 
> Yup.
> 
> >> +** Optional properties:
> >> +
> >> +   - no-reset : The presence of this property indicates that the MPIC
> >> +                should not be reset during runtime initialization.
> >> +   - protected-sources : Specifies a list of interrupt sources that are
> >> + not
> >> +                         available for use and whose corresponding
> >> + vectors
> >> +                         should not be initialized.  A typical use case
> >> + for
> >> +                         this property is in AMP systems where multiple
> >> +                         independent operating systems need to share
> >> + the MPIC
> >> +                         without clobbering each other.
> >
> > I do think you need to include the definition of interrupt
> > specifiers here.   Feel free to cut/paste text from my
> > Freescale mpic binding.
> 
> OK, I will look into that.  Thanks.

I have a version 2 I hope to send out later today.

Stuart