| Message ID | 4D34E448.8000902@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> From: Meador Inge <meador_inge@mentor.com> > Date: Mon, Jan 17, 2011 at 6:52 PM > Subject: [PATCH 1/2] powerpc: document the MPIC device tree binding > To: linuxppc-dev@lists.ozlabs.org > Cc: minge <meador_inge@mentor.com>, > devicetree-discuss@lists.ozlabs.org, "Blanchard, Hollis" > <Hollis_Blanchard@mentor.com> > > > This binding documents several properties that have been in use for quite > some time, and adds one new property 'no-reset', which controls whether the > MPIC should be reset during runtime initialization. > > Signed-off-by: Meador Inge <meador_inge@mentor.com> > CC: Hollis Blanchard <hollis_blanchard@mentor.com> > --- > 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. > +++++++++++++++++++++++++++ > 1 files changed, 78 insertions(+), 0 deletions(-) > create mode 100644 Documentation/powerpc/dts-bindings/mpic.txt > > diff --git a/Documentation/powerpc/dts-bindings/mpic.txt > b/Documentation/powerpc/dts-bindings/mpic.txt > new file mode 100644 > index 0000000..3a67919 > --- /dev/null > +++ b/Documentation/powerpc/dts-bindings/mpic.txt > @@ -0,0 +1,78 @@ > +* MPIC Binding > + > +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.pdf> The proposed new properties really should go back into the ePAPR. > + > +** 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. > + - 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. > + - 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. > + - 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? > +** 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. Stuart
On Mon, 17 Jan 2011 18:52:24 -0600 Meador Inge <meador_inge@mentor.com> wrote: > +** Required properties: > + > + NOTE: Many of these descriptions were paraphrased from [1] to aid > + readability. > + > + - name : Specifies the name of the MPIC. "name" isn't really a property with flat trees. The appropriate node name, according to the Generic Names recommendation and ePAPR, is interrupt-controller. > + - device_type : Specifies the device type of this MPIC. The value > of this > + property shall be "open-pic". Can we drop device_type, and fix the kernel to look for compatible instead? > + - compatible : Specifies the compatibility list for the MPIC. The > property > + value shall include "chrp,open-pic". ePAPR wants just "open-pic". And while chrp,open-pic is common in existing trees, only one platform currently checks for it. I'd make in "open-pic" in the binding, and have the kernel accept either one. > +** 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. > + Can we define no-reset as meaning that all vectors are in a sane state (either directed at other cores, or masked)? If we do that, maybe we can get rid of protected-sources. -Scott
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.pdf> > > 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? >> + >> +** 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")'. >> + - 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. >> + - 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.
> -----Original Message----- > From: Meador Inge [mailto:meador_inge@mentor.com] > Sent: Wednesday, January 19, 2011 2:25 PM > To: Yoder Stuart-B08248 > Cc: linuxppc-dev@lists.ozlabs.org; devicetree-discuss@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
> +** 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. Is "protected-sources" really needed for AMP systems to tell the OSes not to clobber each other? Won't each OS be given a device tree with only its interrupt sources? ...so you know what you are allowed to touch. Stuart
On 01/19/2011 04:14 PM, Yoder Stuart-B08248 wrote: > >> +** 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. > > Is "protected-sources" really needed for AMP systems to > tell the OSes not to clobber each other? Won't each > OS be given a device tree with only its interrupt > sources? ...so you know what you are allowed to touch. This was discussed a little bit already [1, 2]. The MPIC driver currently initializes the VECPRI register for all interrupt sources, which can lead to the aforementioned clobbering.
> -----Original Message----- > From: Meador Inge [mailto:meador_inge@mentor.com] > Sent: Wednesday, January 19, 2011 6:08 PM > To: Yoder Stuart-B08248 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; devicetree- > discuss@lists.ozlabs.org; Blanchard, Hollis > Subject: Re: [PATCH 1/2] powerpc: document the MPIC device tree binding > > On 01/19/2011 04:14 PM, Yoder Stuart-B08248 wrote: > > > >> +** 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. > > > > Is "protected-sources" really needed for AMP systems to tell the OSes > > not to clobber each other? Won't each OS be given a device tree with > > only its interrupt sources? ...so you know what you are allowed to > > touch. > > This was discussed a little bit already [1, 2]. The MPIC driver currently > initializes the VECPRI register for all interrupt sources, which can lead > to the aforementioned clobbering. For sources that are protected and not to be touched, it seems that the other need is for the OS to know (be guaranteed?) that those sources have been put in state where the source is masked or directed to other cores. You can't have interrupts occurring on sources that you are not allowed to initialize. So the "no-reset" property could potentially cover this as well-- if it was defined to mean "don't reset the mpic" and boot firmware has put all sources in a sane initial state. And we wouldn't need the "protected-sources" property any longer. Stuart Right...so it would seem that the no-reset property if prop
On 01/20/2011 09:50 AM, Yoder Stuart-B08248 wrote: > > >> -----Original Message----- >> From: Meador Inge [mailto:meador_inge@mentor.com] >> Sent: Wednesday, January 19, 2011 6:08 PM >> To: Yoder Stuart-B08248 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; devicetree- >> discuss@lists.ozlabs.org; Blanchard, Hollis >> Subject: Re: [PATCH 1/2] powerpc: document the MPIC device tree binding >> >> On 01/19/2011 04:14 PM, Yoder Stuart-B08248 wrote: >>> >>>> +** 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. >>> >>> Is "protected-sources" really needed for AMP systems to tell the OSes >>> not to clobber each other? Won't each OS be given a device tree with >>> only its interrupt sources? ...so you know what you are allowed to >>> touch. >> >> This was discussed a little bit already [1, 2]. The MPIC driver currently >> initializes the VECPRI register for all interrupt sources, which can lead >> to the aforementioned clobbering. > > For sources that are protected and not to be touched, it seems > that the other need is for the OS to know (be guaranteed?) that > those sources have been put in state where the source is masked > or directed to other cores. You can't have interrupts occurring > on sources that you are not allowed to initialize. > > So the "no-reset" property could potentially cover this as well-- if it was > defined to mean "don't reset the mpic" and boot firmware has put all sources > in a sane initial state. And we wouldn't need the "protected-sources" > property any longer. > This seems reasonable to me. If "no-reset" is there, then we will not reset the MPIC and *only* sources explicitly listed in "interrupts" properties are up for any sort of initialization (e.g. the VECPRI init). If "no-reset" is not there, then anything is free game. In terms of implementation, I think we can (1) pull the protected sources code, (2) keep the VECPRI initialization in 'mpic_init' from happening when "no-reset" is present, and (3) "lazily" perform the VECPRI initialization in 'mpic_host_map' (this way only sources mentioned in the device tree are initialized). I will send out a patch with these updates tomorrow. I also CC'd Ben, who wrote the original protected sources work, to make sure something about the original use case is not being missed.
diff --git a/Documentation/powerpc/dts-bindings/mpic.txt b/Documentation/powerpc/dts-bindings/mpic.txt new file mode 100644 index 0000000..3a67919 --- /dev/null +++ b/Documentation/powerpc/dts-bindings/mpic.txt @@ -0,0 +1,78 @@ +* MPIC Binding + +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. + +** Required properties: + + NOTE: Many of these descriptions were paraphrased from [1] to aid + readability. + + - name : Specifies the name of the MPIC. + - device_type : Specifies the device type of this MPIC. The value of this + property shall be "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". + - 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. + +** 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. + +** Example: + + mpic: pic@40000 { + // This is an interrupt controller node. + interrupt-controller; + + // No address cells so that 'interrupt-map' nodes which reference + // this MPIC node do not need a parent address specifier. + #address-cells = <0>; + + // Two cell to encode interrupt sources. + #interrupt-cells = <2>; + + // Offset address of 0x40000 and size of 0x40000. + reg = <0x40000 0x40000>; + + // Compatible with Open PIC. + compatible = "chrp,open-pic"; + + // An Open PIC device. + device_type = "open-pic"; + + // The sources 0xb1 and 0xb2 are off limits for use and should not be + // initialized by the OS. + protected-sources = <0xb1 0xb2>; + + // The MPIC should not be reset. + no-reset;
This binding documents several properties that have been in use for quite some time, and adds one new property 'no-reset', which controls whether the MPIC should be reset during runtime initialization. Signed-off-by: Meador Inge <meador_inge@mentor.com> CC: Hollis Blanchard <hollis_blanchard@mentor.com> --- Documentation/powerpc/dts-bindings/mpic.txt | 78 +++++++++++++++++++++++++++ 1 files changed, 78 insertions(+), 0 deletions(-) create mode 100644 Documentation/powerpc/dts-bindings/mpic.txt + }; + +* References + +[1] PowerPC Microprocessor Common Hardware Reference Platform (CHRP) Binding, + Version 1.8, 1998. Published by the Open Firmware Working Group. + (http://playground.sun.com/1275/bindings/chrp/chrp1_8a.ps) +[2] Open Firmware Recommended Practice: Interrupt Mapping, Version 0.9. 1996. + Published by the Open Firmware Working Group. + (http://playground.sun.com/1275/practice/imap/imap0_9d.pdf) + -- 1.6.3.3