| Message ID | 20140116013840.GA674@codeaurora.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote: > On 01/15, Stephen Boyd wrote: > > > > Ah sorry, I forgot to put the compatible property here like in > > the dts change. I'll do that in the next revision. Yes we need a > > compatible property here to match the platform driver. > > > > This is the replacement patch > > -----8<------ > From: Stephen Boyd <sboyd@codeaurora.org> > Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC > > The Krait CPU/L1 error reporting device is made up a per-CPU > interrupt. While we're here, document the next-level-cache > property that's used by the Krait EDAC driver. > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Kumar Gala <galak@codeaurora.org> > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > index 91304353eea4..03a529e791c4 100644 > --- a/Documentation/devicetree/bindings/arm/cpus.txt > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below. > Value type: <u32> > Definition: must be set to 0 > > + - compatible > + Usage: optional > + Value type: <string> > + Definition: should be one of the compatible strings listed > + in the cpu node compatible property. This property > + shall only be present if all the cpu nodes have the > + same compatible property. Do we really want to do that ? I am not sure. A cpus node is supposed to be a container node, we should not define this binding just because we know the kernel creates a platform device for it then. interrupts is a cpu node property and I think it should be kept as such. I know it will be duplicated and I know you can't rely on a platform device for probing (since if I am not mistaken, removing a compatible string from cpus prevents its platform device creation), but that's an issue related to how the kernel works, you should not define DT bindings to solve that IMHO. Lorenzo > + > + - interrupts > + Usage: required when node contains cpus with compatible > + string "qcom,krait". > + Value type: <prop-encoded-array> > + Definition: L1/CPU error interrupt > + > - cpu node > > Description: Describes a CPU in an ARM based system > @@ -191,6 +205,11 @@ nodes to be present and contain the properties described below. > property identifying a 64-bit zero-initialised > memory location. > > + - next-level-cache > + Usage: optional > + Value type: <phandle> > + Definition: phandle pointing to the next level cache > + > Example 1 (dual-cluster big.LITTLE system 32-bit): > > cpus { > @@ -382,3 +401,42 @@ cpus { > cpu-release-addr = <0 0x20000000>; > }; > }; > + > + > +Example 5 (Krait 32-bit system): > + > +cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + interrupts = <1 9 0xf04>; > + compatible = "qcom,krait"; > + > + cpu@0 { > + device_type = "cpu"; > + reg = <0>; > + next-level-cache = <&L2>; > + }; > + > + cpu@1 { > + device_type = "cpu"; > + reg = <1>; > + next-level-cache = <&L2>; > + }; > + > + cpu@2 { > + device_type = "cpu"; > + reg = <2>; > + next-level-cache = <&L2>; > + }; > + > + cpu@3 { > + device_type = "cpu"; > + reg = <3>; > + next-level-cache = <&L2>; > + }; > + > + L2: l2-cache { > + compatible = "cache"; > + interrupts = <0 2 0x4>; > + }; > +}; > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16, Lorenzo Pieralisi wrote: > On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote: > > On 01/15, Stephen Boyd wrote: > > > > > > Ah sorry, I forgot to put the compatible property here like in > > > the dts change. I'll do that in the next revision. Yes we need a > > > compatible property here to match the platform driver. > > > > > > > This is the replacement patch > > > > -----8<------ > > From: Stephen Boyd <sboyd@codeaurora.org> > > Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC > > > > The Krait CPU/L1 error reporting device is made up a per-CPU > > interrupt. While we're here, document the next-level-cache > > property that's used by the Krait EDAC driver. > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Kumar Gala <galak@codeaurora.org> > > Cc: <devicetree@vger.kernel.org> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > --- > > Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > > index 91304353eea4..03a529e791c4 100644 > > --- a/Documentation/devicetree/bindings/arm/cpus.txt > > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > > @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below. > > Value type: <u32> > > Definition: must be set to 0 > > > > + - compatible > > + Usage: optional > > + Value type: <string> > > + Definition: should be one of the compatible strings listed > > + in the cpu node compatible property. This property > > + shall only be present if all the cpu nodes have the > > + same compatible property. > > Do we really want to do that ? I am not sure. A cpus node is supposed to > be a container node, we should not define this binding just because we > know the kernel creates a platform device for it then. This is just copying more of the ePAPR spec into this document. It just so happens that having a compatible field here allows a platform device to be created. I don't see why that's a problem. > > interrupts is a cpu node property and I think it should be kept as such. > > I know it will be duplicated and I know you can't rely on a platform > device for probing (since if I am not mistaken, removing a compatible > string from cpus prevents its platform device creation), but that's an issue > related to how the kernel works, you should not define DT bindings to solve > that IMHO. The interrupts property is also common for all cpus so it seems fine to collapse the value down into a PPI specifier indicating that all CPUs get the interrupt, similar to how we compress the information about the compatible string.
On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > On 01/16, Lorenzo Pieralisi wrote: > > On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote: > > > On 01/15, Stephen Boyd wrote: > > > > > > > > Ah sorry, I forgot to put the compatible property here like in > > > > the dts change. I'll do that in the next revision. Yes we need a > > > > compatible property here to match the platform driver. > > > > > > > > > > This is the replacement patch > > > > > > -----8<------ > > > From: Stephen Boyd <sboyd@codeaurora.org> > > > Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC > > > > > > The Krait CPU/L1 error reporting device is made up a per-CPU > > > interrupt. While we're here, document the next-level-cache > > > property that's used by the Krait EDAC driver. > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: Kumar Gala <galak@codeaurora.org> > > > Cc: <devicetree@vger.kernel.org> > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > > --- > > > Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++ > > > 1 file changed, 58 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > > > index 91304353eea4..03a529e791c4 100644 > > > --- a/Documentation/devicetree/bindings/arm/cpus.txt > > > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > > > @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below. > > > Value type: <u32> > > > Definition: must be set to 0 > > > > > > + - compatible > > > + Usage: optional > > > + Value type: <string> > > > + Definition: should be one of the compatible strings listed > > > + in the cpu node compatible property. This property > > > + shall only be present if all the cpu nodes have the > > > + same compatible property. > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to > > be a container node, we should not define this binding just because we > > know the kernel creates a platform device for it then. > > This is just copying more of the ePAPR spec into this document. > It just so happens that having a compatible field here allows a > platform device to be created. I don't see why that's a problem. I do not see why you cannot define a node like pmu or arch-timer and stick a compatible property in there. cpus node does not represent a device, and must not be created as a platform device, that's my opinion. What would you do for big.LITTLE systems ? We are going to create two cpus node because we need two platform devices ? I really think there must be a better way to implement this, but I will let DT maintainers make a decision. > > interrupts is a cpu node property and I think it should be kept as such. > > > > I know it will be duplicated and I know you can't rely on a platform > > device for probing (since if I am not mistaken, removing a compatible > > string from cpus prevents its platform device creation), but that's an issue > > related to how the kernel works, you should not define DT bindings to solve > > that IMHO. > > The interrupts property is also common for all cpus so it seems > fine to collapse the value down into a PPI specifier indicating > that all CPUs get the interrupt, similar to how we compress the > information about the compatible string. I think it is nicer to create a device node (as I said, like a pmu or an arch-timer) and define interrupts there along with a proper compatible property. This would serve the same purpose without adding properties in the cpus node. cpu-edac { compatible = "qcom,cpu-edac"; interrupts = <...>; }; Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16, Lorenzo Pieralisi wrote: > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > > On 01/16, Lorenzo Pieralisi wrote: > > > Do we really want to do that ? I am not sure. A cpus node is supposed to > > > be a container node, we should not define this binding just because we > > > know the kernel creates a platform device for it then. > > > > This is just copying more of the ePAPR spec into this document. > > It just so happens that having a compatible field here allows a > > platform device to be created. I don't see why that's a problem. > > I do not see why you cannot define a node like pmu or arch-timer and stick > a compatible property in there. cpus node does not represent a device, and > must not be created as a platform device, that's my opinion. > I had what you're suggesting before in the original revision of this patch. Please take a look at the original patch series[1]. I suppose it could be tweaked slightly to still have a cache node for the L2 interrupt and the next-level-cache pointer from the CPUs. > What would you do for big.LITTLE systems ? We are going to create two > cpus node because we need two platform devices ? I really think there > must be a better way to implement this, but I will let DT maintainers > make a decision. There is no such thing as big.LITTLE for Krait, so this is not a concern. > > > > interrupts is a cpu node property and I think it should be kept as such. > > > > > > I know it will be duplicated and I know you can't rely on a platform > > > device for probing (since if I am not mistaken, removing a compatible > > > string from cpus prevents its platform device creation), but that's an issue > > > related to how the kernel works, you should not define DT bindings to solve > > > that IMHO. > > > > The interrupts property is also common for all cpus so it seems > > fine to collapse the value down into a PPI specifier indicating > > that all CPUs get the interrupt, similar to how we compress the > > information about the compatible string. > > I think it is nicer to create a device node (as I said, like a pmu or an > arch-timer) and define interrupts there along with a proper compatible > property. This would serve the same purpose without adding properties in > the cpus node. > > cpu-edac { > compatible = "qcom,cpu-edac"; > interrupts = <...>; > }; Yes, please see the original thread. [1] https://lkml.org/lkml/2013/10/29/134
On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote: > On 01/16, Lorenzo Pieralisi wrote: > > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > > > On 01/16, Lorenzo Pieralisi wrote: > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to > > > > be a container node, we should not define this binding just because we > > > > know the kernel creates a platform device for it then. > > > > > > This is just copying more of the ePAPR spec into this document. > > > It just so happens that having a compatible field here allows a > > > platform device to be created. I don't see why that's a problem. > > > > I do not see why you cannot define a node like pmu or arch-timer and stick > > a compatible property in there. cpus node does not represent a device, and > > must not be created as a platform device, that's my opinion. > > > > I had what you're suggesting before in the original revision of > this patch. Please take a look at the original patch series[1]. I > suppose it could be tweaked slightly to still have a cache node > for the L2 interrupt and the next-level-cache pointer from the > CPUs. Ok, sorry, we are running around in circles here, basically you moved the node to cpus according to reviews. I still think that treating cpus as a device is not a great idea, even though I am in the same position with C-states and probably will add C-state tables in the cpus node. http://comments.gmane.org/gmane.linux.power-management.general/41012 I just would like to see under cpus nodes and properties that apply to all ARM systems, and avoid defining properties (eg interrupts) that have different meanings for different ARM cores. The question related to why the kernel should create a platform device out of cpus is still open. I really do not want to block your series for these simple issues but we have to make a decision and stick to that, I am fine either way if we have a plan. > > What would you do for big.LITTLE systems ? We are going to create two > > cpus node because we need two platform devices ? I really think there > > must be a better way to implement this, but I will let DT maintainers > > make a decision. > > There is no such thing as big.LITTLE for Krait, so this is not a > concern. Yes, but if a core supporting big.LITTLE requires the same concept you need here we have to cater for that because after all cpus is a node that exists on ALL ARM systems, we should not rush and find a solution du jour without thinking about all foreseeable use cases. Thank you, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(Sorry, this discussion stalled due to merge window + life events) On 01/17, Lorenzo Pieralisi wrote: > On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote: > > On 01/16, Lorenzo Pieralisi wrote: > > > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > > > > On 01/16, Lorenzo Pieralisi wrote: > > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to > > > > > be a container node, we should not define this binding just because we > > > > > know the kernel creates a platform device for it then. > > > > > > > > This is just copying more of the ePAPR spec into this document. > > > > It just so happens that having a compatible field here allows a > > > > platform device to be created. I don't see why that's a problem. > > > > > > I do not see why you cannot define a node like pmu or arch-timer and stick > > > a compatible property in there. cpus node does not represent a device, and > > > must not be created as a platform device, that's my opinion. > > > > > > > I had what you're suggesting before in the original revision of > > this patch. Please take a look at the original patch series[1]. I > > suppose it could be tweaked slightly to still have a cache node > > for the L2 interrupt and the next-level-cache pointer from the > > CPUs. > > Ok, sorry, we are running around in circles here, basically you moved > the node to cpus according to reviews. I still think that treating cpus > as a device is not a great idea, even though I am in the same > position with C-states and probably will add C-state tables in the cpus > node. > > http://comments.gmane.org/gmane.linux.power-management.general/41012 > > I just would like to see under cpus nodes and properties that apply to > all ARM systems, and avoid defining properties (eg interrupts) that > have different meanings for different ARM cores. > > The question related to why the kernel should create a platform device > out of cpus is still open. I really do not want to block your series > for these simple issues but we have to make a decision and stick to that, > I am fine either way if we have a plan. > Do you just want a backup plan in case we don't make a platform device out of the cpus node? I believe we can always add code somewhere to create a platform device at runtime if we detect the cpus node has a compatible string equal to "qcom,krait". We could probably change this driver's module_init() to scan the DT for such a compatible string and create the platform device right there. If we get more than one interrupt in the cpus node we can add interrupt-names and then have software look for interrupts by name instead of number.
Hi Stephen, On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote: > (Sorry, this discussion stalled due to merge window + life events) Sorry for the delay in replying on my side too. > On 01/17, Lorenzo Pieralisi wrote: > > On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote: > > > On 01/16, Lorenzo Pieralisi wrote: > > > > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > > > > > On 01/16, Lorenzo Pieralisi wrote: > > > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to > > > > > > be a container node, we should not define this binding just because we > > > > > > know the kernel creates a platform device for it then. > > > > > > > > > > This is just copying more of the ePAPR spec into this document. > > > > > It just so happens that having a compatible field here allows a > > > > > platform device to be created. I don't see why that's a problem. > > > > > > > > I do not see why you cannot define a node like pmu or arch-timer and stick > > > > a compatible property in there. cpus node does not represent a device, and > > > > must not be created as a platform device, that's my opinion. > > > > > > > > > > I had what you're suggesting before in the original revision of > > > this patch. Please take a look at the original patch series[1]. I > > > suppose it could be tweaked slightly to still have a cache node > > > for the L2 interrupt and the next-level-cache pointer from the > > > CPUs. > > > > Ok, sorry, we are running around in circles here, basically you moved > > the node to cpus according to reviews. I still think that treating cpus > > as a device is not a great idea, even though I am in the same > > position with C-states and probably will add C-state tables in the cpus > > node. > > > > http://comments.gmane.org/gmane.linux.power-management.general/41012 > > > > I just would like to see under cpus nodes and properties that apply to > > all ARM systems, and avoid defining properties (eg interrupts) that > > have different meanings for different ARM cores. > > > > The question related to why the kernel should create a platform device > > out of cpus is still open. I really do not want to block your series > > for these simple issues but we have to make a decision and stick to that, > > I am fine either way if we have a plan. > > > > Do you just want a backup plan in case we don't make a platform > device out of the cpus node? I believe we can always add code > somewhere to create a platform device at runtime if we detect the > cpus node has a compatible string equal to "qcom,krait". We could > probably change this driver's module_init() to scan the DT for > such a compatible string and create the platform device right > there. If we get more than one interrupt in the cpus node we can > add interrupt-names and then have software look for interrupts by > name instead of number. As I mentioned, I do not like the idea of adding compatible properties just to force the kernel to create platform devices out of device tree nodes. On top of that I would avoid adding a compatible property to the cpus node (after all properties like enable-method are common for all cpus but still duplicated), my only concern being backward compatibility here (ie if we do that for interrupts, we should do that also for other common cpu nodes properties, otherwise we have different rules for different properties). I think you can then add interrupts to cpu nodes ("qcom,krait" specific), and as you mentioned create a platform device for that. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > Hi Stephen, > > On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote: >> (Sorry, this discussion stalled due to merge window + life events) > > Sorry for the delay in replying on my side too. > >> On 01/17, Lorenzo Pieralisi wrote: >>> On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote: >>>> On 01/16, Lorenzo Pieralisi wrote: >>>>> On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: >>>>>> On 01/16, Lorenzo Pieralisi wrote: >>>>>>> Do we really want to do that ? I am not sure. A cpus node is supposed to >>>>>>> be a container node, we should not define this binding just because we >>>>>>> know the kernel creates a platform device for it then. >>>>>> >>>>>> This is just copying more of the ePAPR spec into this document. >>>>>> It just so happens that having a compatible field here allows a >>>>>> platform device to be created. I don't see why that's a problem. >>>>> >>>>> I do not see why you cannot define a node like pmu or arch-timer and stick >>>>> a compatible property in there. cpus node does not represent a device, and >>>>> must not be created as a platform device, that's my opinion. >>>>> >>>> >>>> I had what you're suggesting before in the original revision of >>>> this patch. Please take a look at the original patch series[1]. I >>>> suppose it could be tweaked slightly to still have a cache node >>>> for the L2 interrupt and the next-level-cache pointer from the >>>> CPUs. >>> >>> Ok, sorry, we are running around in circles here, basically you moved >>> the node to cpus according to reviews. I still think that treating cpus >>> as a device is not a great idea, even though I am in the same >>> position with C-states and probably will add C-state tables in the cpus >>> node. >>> >>> http://comments.gmane.org/gmane.linux.power-management.general/41012 >>> >>> I just would like to see under cpus nodes and properties that apply to >>> all ARM systems, and avoid defining properties (eg interrupts) that >>> have different meanings for different ARM cores. >>> >>> The question related to why the kernel should create a platform device >>> out of cpus is still open. I really do not want to block your series >>> for these simple issues but we have to make a decision and stick to that, >>> I am fine either way if we have a plan. >>> >> >> Do you just want a backup plan in case we don't make a platform >> device out of the cpus node? I believe we can always add code >> somewhere to create a platform device at runtime if we detect the >> cpus node has a compatible string equal to "qcom,krait". We could >> probably change this driver's module_init() to scan the DT for >> such a compatible string and create the platform device right >> there. If we get more than one interrupt in the cpus node we can >> add interrupt-names and then have software look for interrupts by >> name instead of number. > > As I mentioned, I do not like the idea of adding compatible properties > just to force the kernel to create platform devices out of device tree > nodes. On top of that I would avoid adding a compatible property > to the cpus node (after all properties like enable-method are common for all > cpus but still duplicated), my only concern being backward compatibility > here (ie if we do that for interrupts, we should do that also for other > common cpu nodes properties, otherwise we have different rules for > different properties). > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific), > and as you mentioned create a platform device for that. > > Thanks, > Lorenzo So I agree with the statement about adding compatibles just to create platform devices is wrong. However its seems perfectly reasonable for a cpu node to have a compatible property. I don’t see why a CPU is any different from any other device described in a DT. - k
On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote: > > On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > > Hi Stephen, > > > > On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote: > >> (Sorry, this discussion stalled due to merge window + life events) > > > > Sorry for the delay in replying on my side too. > > > >> On 01/17, Lorenzo Pieralisi wrote: > >>> On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote: > >>>> On 01/16, Lorenzo Pieralisi wrote: > >>>>> On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote: > >>>>>> On 01/16, Lorenzo Pieralisi wrote: > >>>>>>> Do we really want to do that ? I am not sure. A cpus node is supposed to > >>>>>>> be a container node, we should not define this binding just because we > >>>>>>> know the kernel creates a platform device for it then. > >>>>>> > >>>>>> This is just copying more of the ePAPR spec into this document. > >>>>>> It just so happens that having a compatible field here allows a > >>>>>> platform device to be created. I don't see why that's a problem. > >>>>> > >>>>> I do not see why you cannot define a node like pmu or arch-timer and stick > >>>>> a compatible property in there. cpus node does not represent a device, and > >>>>> must not be created as a platform device, that's my opinion. > >>>>> > >>>> > >>>> I had what you're suggesting before in the original revision of > >>>> this patch. Please take a look at the original patch series[1]. I > >>>> suppose it could be tweaked slightly to still have a cache node > >>>> for the L2 interrupt and the next-level-cache pointer from the > >>>> CPUs. > >>> > >>> Ok, sorry, we are running around in circles here, basically you moved > >>> the node to cpus according to reviews. I still think that treating cpus > >>> as a device is not a great idea, even though I am in the same > >>> position with C-states and probably will add C-state tables in the cpus > >>> node. > >>> > >>> http://comments.gmane.org/gmane.linux.power-management.general/41012 > >>> > >>> I just would like to see under cpus nodes and properties that apply to > >>> all ARM systems, and avoid defining properties (eg interrupts) that > >>> have different meanings for different ARM cores. > >>> > >>> The question related to why the kernel should create a platform device > >>> out of cpus is still open. I really do not want to block your series > >>> for these simple issues but we have to make a decision and stick to that, > >>> I am fine either way if we have a plan. > >>> > >> > >> Do you just want a backup plan in case we don't make a platform > >> device out of the cpus node? I believe we can always add code > >> somewhere to create a platform device at runtime if we detect the > >> cpus node has a compatible string equal to "qcom,krait". We could > >> probably change this driver's module_init() to scan the DT for > >> such a compatible string and create the platform device right > >> there. If we get more than one interrupt in the cpus node we can > >> add interrupt-names and then have software look for interrupts by > >> name instead of number. > > > > As I mentioned, I do not like the idea of adding compatible properties > > just to force the kernel to create platform devices out of device tree > > nodes. On top of that I would avoid adding a compatible property > > to the cpus node (after all properties like enable-method are common for all > > cpus but still duplicated), my only concern being backward compatibility > > here (ie if we do that for interrupts, we should do that also for other > > common cpu nodes properties, otherwise we have different rules for > > different properties). > > > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific), > > and as you mentioned create a platform device for that. > > > > Thanks, > > Lorenzo > > So I agree with the statement about adding compatibles just to create platform devices is wrong. However its seems perfectly reasonable for a cpu node to have a compatible property. I don't see why a CPU is any different from any other device described in a DT. I was referring to the /cpus node, not to individual cpu nodes, where the compatible property is already present now. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/26, Lorenzo Pieralisi wrote: > On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote: > > > > On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > > > > > As I mentioned, I do not like the idea of adding compatible properties > > > just to force the kernel to create platform devices out of device tree > > > nodes. On top of that I would avoid adding a compatible property > > > to the cpus node (after all properties like enable-method are common for all > > > cpus but still duplicated), my only concern being backward compatibility > > > here (ie if we do that for interrupts, we should do that also for other > > > common cpu nodes properties, otherwise we have different rules for > > > different properties). > > > > > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific), > > > and as you mentioned create a platform device for that. > > > > > > Thanks, > > > Lorenzo > > > > So I agree with the statement about adding compatibles just to create platform devices is wrong. However its seems perfectly reasonable for a cpu node to have a compatible property. I don't see why a CPU is any different from any other device described in a DT. > > I was referring to the /cpus node, not to individual cpu nodes, where > the compatible property is already present now. > Ok I think I'll go ahead with moving the interrupts into each cpu node, i.e.: cpus { #address-cells = <1>; #size-cells = <0>; cpu@0 { compatible = "qcom,krait"; device_type = "cpu"; reg = <0>; interrupts = <1 14 0x304>; next-level-cache = <&L2>; }; cpu@1 { compatible = "qcom,krait"; device_type = "cpu"; reg = <1>; interrupts = <1 14 0x304>; next-level-cache = <&L2>; }; L2: l2-cache { compatible = "cache"; interrupts = <0 2 0x4>; }; }; Or should we be expressing the L1 cache as well? Something like: cpus { #address-cells = <1>; #size-cells = <0>; cpu@0 { compatible = "qcom,krait"; device_type = "cpu"; reg = <0>; next-level-cache = <&L1_0>; L1_0: l1-cache { compatible = "arm,arch-cache"; interrupts = <1 14 0x304>; next-level-cache = <&L2>; } }; cpu@1 { compatible = "qcom,krait"; device_type = "cpu"; reg = <1>; next-level-cache = <&L1_1>; L1_1: l1-cache { compatible = "arm,arch-cache"; interrupts = <1 14 0x304>; next-level-cache = <&L2>; } }; L2: l2-cache { compatible = "arm,arch-cache"; interrupts = <0 2 0x4>; }; }; (I'm also wondering if the 3rd cell of the interrupt binding should only indicate the CPU that the interrupt property is inside?) Finally we can have the edac driver look for a "qcom,krait" compatible node in cpus that it can create a platform device for, i.e.. static int __init krait_edac_driver_init(void) { struct device_node *np; np = of_get_cpu_node(0, NULL); if (!np) return 0; if (!krait_edacp && of_device_is_compatible(np, "qcom,krait")) krait_edacp = of_platform_device_create(np, "krait_edac", NULL); of_node_put(np); return platform_driver_register(&krait_edac_driver); } module_init(krait_edac_driver_init);
On Fri, Mar 07, 2014 at 11:08:56PM +0000, Stephen Boyd wrote: > On 02/26, Lorenzo Pieralisi wrote: > > On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote: > > > > > > On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > As I mentioned, I do not like the idea of adding compatible properties > > > > just to force the kernel to create platform devices out of device tree > > > > nodes. On top of that I would avoid adding a compatible property > > > > to the cpus node (after all properties like enable-method are common for all > > > > cpus but still duplicated), my only concern being backward compatibility > > > > here (ie if we do that for interrupts, we should do that also for other > > > > common cpu nodes properties, otherwise we have different rules for > > > > different properties). > > > > > > > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific), > > > > and as you mentioned create a platform device for that. > > > > > > > > Thanks, > > > > Lorenzo > > > > > > So I agree with the statement about adding compatibles just to create platform devices is wrong. However its seems perfectly reasonable for a cpu node to have a compatible property. I don't see why a CPU is any different from any other device described in a DT. > > > > I was referring to the /cpus node, not to individual cpu nodes, where > > the compatible property is already present now. > > > > Ok I think I'll go ahead with moving the interrupts into each cpu node, i.e.: > > cpus { > #address-cells = <1>; > #size-cells = <0>; > > cpu@0 { > compatible = "qcom,krait"; > device_type = "cpu"; > reg = <0>; > interrupts = <1 14 0x304>; > next-level-cache = <&L2>; > }; > > cpu@1 { > compatible = "qcom,krait"; > device_type = "cpu"; > reg = <1>; > interrupts = <1 14 0x304>; > next-level-cache = <&L2>; > }; > > L2: l2-cache { > compatible = "cache"; > interrupts = <0 2 0x4>; > }; > }; > > Or should we be expressing the L1 cache as well? Something like: > > cpus { > #address-cells = <1>; > #size-cells = <0>; > > cpu@0 { > compatible = "qcom,krait"; > device_type = "cpu"; > reg = <0>; > next-level-cache = <&L1_0>; > > L1_0: l1-cache { > compatible = "arm,arch-cache"; > interrupts = <1 14 0x304>; > next-level-cache = <&L2>; > } > }; > > cpu@1 { > compatible = "qcom,krait"; > device_type = "cpu"; > reg = <1>; > next-level-cache = <&L1_1>; > > L1_1: l1-cache { > compatible = "arm,arch-cache"; > interrupts = <1 14 0x304>; > next-level-cache = <&L2>; > } > }; > > L2: l2-cache { > compatible = "arm,arch-cache"; > interrupts = <0 2 0x4>; > }; > }; > > (I'm also wondering if the 3rd cell of the interrupt binding > should only indicate the CPU that the interrupt property is > inside?) I am not aware of interrupts associated with vanilla :) "arm,arch-cache" objects, so I think that should be handled as a "qcom,krait" specific property (in the cpu node), or you should add another cache binding (compatible) for that. As you might have noticed (idle states thread) I am keen on defining objects for L1 caches explicitly, that patch still requires an ACK though (and you need to update it since you cannot add an interrupt property for all "arm,arch-cache" objects. I am sorry for being a pain, but I do not think that's correct from a HW description standpoint). > Finally we can have the edac driver look for a "qcom,krait" > compatible node in cpus that it can create a platform device for, > i.e.. > > static int __init krait_edac_driver_init(void) > { > struct device_node *np; > > np = of_get_cpu_node(0, NULL); > if (!np) > return 0; > > if (!krait_edacp && of_device_is_compatible(np, "qcom,krait")) > krait_edacp = of_platform_device_create(np, "krait_edac", NULL); > of_node_put(np); > > return platform_driver_register(&krait_edac_driver); > } > module_init(krait_edac_driver_init); It seems fine to me, but it requires an ACK from platform bus and DT maintainers. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11, Lorenzo Pieralisi wrote: > On Fri, Mar 07, 2014 at 11:08:56PM +0000, Stephen Boyd wrote: > > > > Or should we be expressing the L1 cache as well? Something like: > > > > cpus { > > #address-cells = <1>; > > #size-cells = <0>; > > > > cpu@0 { > > compatible = "qcom,krait"; > > device_type = "cpu"; > > reg = <0>; > > next-level-cache = <&L1_0>; > > > > L1_0: l1-cache { > > compatible = "arm,arch-cache"; > > interrupts = <1 14 0x304>; > > next-level-cache = <&L2>; > > } > > }; > > > > cpu@1 { > > compatible = "qcom,krait"; > > device_type = "cpu"; > > reg = <1>; > > next-level-cache = <&L1_1>; > > > > L1_1: l1-cache { > > compatible = "arm,arch-cache"; > > interrupts = <1 14 0x304>; > > next-level-cache = <&L2>; > > } > > }; > > > > L2: l2-cache { > > compatible = "arm,arch-cache"; > > interrupts = <0 2 0x4>; > > }; > > }; > > > > (I'm also wondering if the 3rd cell of the interrupt binding > > should only indicate the CPU that the interrupt property is > > inside?) > > I am not aware of interrupts associated with vanilla :) "arm,arch-cache" > objects, so I think that should be handled as a "qcom,krait" specific property > (in the cpu node), or you should add another cache binding (compatible) for > that. > > As you might have noticed (idle states thread) I am keen on defining objects > for L1 caches explicitly, that patch still requires an ACK though (and > you need to update it since you cannot add an interrupt property for all > "arm,arch-cache" objects. I am sorry for being a pain, but I do not > think that's correct from a HW description standpoint). > Ok. s/arm,arch-cache/qcom,arch-cache/ then. I imagine it is easy enough to add some bits in the cache binding once it's accepted.
diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt index 91304353eea4..03a529e791c4 100644 --- a/Documentation/devicetree/bindings/arm/cpus.txt +++ b/Documentation/devicetree/bindings/arm/cpus.txt @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below. Value type: <u32> Definition: must be set to 0 + - compatible + Usage: optional + Value type: <string> + Definition: should be one of the compatible strings listed + in the cpu node compatible property. This property + shall only be present if all the cpu nodes have the + same compatible property. + + - interrupts + Usage: required when node contains cpus with compatible + string "qcom,krait". + Value type: <prop-encoded-array> + Definition: L1/CPU error interrupt + - cpu node Description: Describes a CPU in an ARM based system @@ -191,6 +205,11 @@ nodes to be present and contain the properties described below. property identifying a 64-bit zero-initialised memory location. + - next-level-cache + Usage: optional + Value type: <phandle> + Definition: phandle pointing to the next level cache + Example 1 (dual-cluster big.LITTLE system 32-bit): cpus { @@ -382,3 +401,42 @@ cpus { cpu-release-addr = <0 0x20000000>; }; }; + + +Example 5 (Krait 32-bit system): + +cpus { + #address-cells = <1>; + #size-cells = <0>; + interrupts = <1 9 0xf04>; + compatible = "qcom,krait"; + + cpu@0 { + device_type = "cpu"; + reg = <0>; + next-level-cache = <&L2>; + }; + + cpu@1 { + device_type = "cpu"; + reg = <1>; + next-level-cache = <&L2>; + }; + + cpu@2 { + device_type = "cpu"; + reg = <2>; + next-level-cache = <&L2>; + }; + + cpu@3 { + device_type = "cpu"; + reg = <3>; + next-level-cache = <&L2>; + }; + + L2: l2-cache { + compatible = "cache"; + interrupts = <0 2 0x4>; + }; +};