[PATCH v5 0/4] Krait L1/L2 EDAC driver

[PATCH v5 0/4] Krait L1/L2 EDAC driver

[Stephen Boyd]

This patchset adds support for the Krait L1/L2 cache error detection
hardware. The first patch adds the Krait l2 indirection
register code. This patch is in need of an ACK from ARM folks.
The next two patches add the driver and the binding and 
the final patch hooks it all up by adding the device tree node.

I think Boris will pick up the first and third patches since they depend
on each other. The final dts change could go through arm-soc via davidb's tree
and the Documentation patch could go through the devicetree tree. Or patches 1
through 3 can go through Boris' tree.

Changes since v4:
 * Prefixed l2 accessors functions with krait_
 * Dropped first two patches as Boris says he picked them up

Changes since v3:
 * Fixed l1_irq handler to properly dereference dev_id

Changes since v2:
 * Picked up acks
 * s/an/a/ in DT binding

Stephen Boyd (4):
  ARM: Add Krait L2 register accessor functions
  devicetree: bindings: Document Krait CPU/L1 EDAC
  edac: Add support for Krait CPU cache error detection
  ARM: dts: msm: Add Krait CPU/L2 nodes

 Documentation/devicetree/bindings/arm/cpus.txt |  52 ++++
 arch/arm/boot/dts/qcom-msm8974.dtsi            |  41 +++
 arch/arm/common/Kconfig                        |   3 +
 arch/arm/common/Makefile                       |   1 +
 arch/arm/common/krait-l2-accessors.c           |  58 +++++
 arch/arm/include/asm/krait-l2-accessors.h      |  20 ++
 drivers/edac/Kconfig                           |   8 +
 drivers/edac/Makefile                          |   2 +
 drivers/edac/krait_edac.c                      | 346 +++++++++++++++++++++++++
 9 files changed, 531 insertions(+)
 create mode 100644 arch/arm/common/krait-l2-accessors.c
 create mode 100644 arch/arm/include/asm/krait-l2-accessors.h
 create mode 100644 drivers/edac/krait_edac.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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 | 52 ++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 91304353eea4..c332b5168456 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- interrupts
+		Usage: required for cpus with compatible string "qcom,krait".
+		Value type: <prop-encoded-array>
+		Definition: L1/CPU error interrupt
+
+	- 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 +392,45 @@ cpus {
 		cpu-release-addr = <0 0x20000000>;
 	};
 };
+
+
+Example 5 (Krait 32-bit system):
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupts = <1 9 0xf04>;
+
+	cpu@0 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <0>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@1 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@2 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@3 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+
+	L2: l2-cache {
+		compatible = "cache";
+		interrupts = <0 2 0x4>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 0/4] Krait L1/L2 EDAC driver

[Borislav Petkov]

On Tue, Jan 14, 2014 at 01:30:30PM -0800, Stephen Boyd wrote:
> This patchset adds support for the Krait L1/L2 cache error detection
> hardware. The first patch adds the Krait l2 indirection
> register code. This patch is in need of an ACK from ARM folks.
> The next two patches add the driver and the binding and 
> the final patch hooks it all up by adding the device tree node.
> 
> I think Boris will pick up the first and third patches since they depend
> on each other. The final dts change could go through arm-soc via davidb's tree
> and the Documentation patch could go through the devicetree tree. Or patches 1
> through 3 can go through Boris' tree.
> 
> Changes since v4:
>  * Prefixed l2 accessors functions with krait_
>  * Dropped first two patches as Boris says he picked them up

Hey Stephen,

I'm going to send upstream only

[PATCH v4 1/6] edac: Don't try to cancel workqueue when it's never setup

as it is a clear bugfix.

The second one

[PATCH v4 2/6] genirq: export percpu irq functions for module usage

will have to wait for when the driver goes in. But the driver depends on
"ARM: Add Krait L2 register accessor functions" which still waits for
ACKs, right?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v5 0/4] Krait L1/L2 EDAC driver

[Stephen Boyd]

On 01/14/14 13:48, Borislav Petkov wrote:
> On Tue, Jan 14, 2014 at 01:30:30PM -0800, Stephen Boyd wrote:
>> This patchset adds support for the Krait L1/L2 cache error detection
>> hardware. The first patch adds the Krait l2 indirection
>> register code. This patch is in need of an ACK from ARM folks.
>> The next two patches add the driver and the binding and 
>> the final patch hooks it all up by adding the device tree node.
>>
>> I think Boris will pick up the first and third patches since they depend
>> on each other. The final dts change could go through arm-soc via davidb's tree
>> and the Documentation patch could go through the devicetree tree. Or patches 1
>> through 3 can go through Boris' tree.
>>
>> Changes since v4:
>>  * Prefixed l2 accessors functions with krait_
>>  * Dropped first two patches as Boris says he picked them up
> Hey Stephen,
>
> I'm going to send upstream only
>
> [PATCH v4 1/6] edac: Don't try to cancel workqueue when it's never setup
>
> as it is a clear bugfix.
>
> The second one
>
> [PATCH v4 2/6] genirq: export percpu irq functions for module usage
>
> will have to wait for when the driver goes in. But the driver depends on
> "ARM: Add Krait L2 register accessor functions" which still waits for
> ACKs, right?
>

Ok that sounds fine. The ARM patch is fairly minor so I'm not sure if
anyone is willing to ack it, but I'm willing to wait a little while to
see if anyone will.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Tue, Jan 14, 2014 at 09:30:32PM +0000, Stephen Boyd wrote:
> 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-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 52 ++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 91304353eea4..c332b5168456 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
>  			  property identifying a 64-bit zero-initialised
>  			  memory location.
>  
> +	- interrupts
> +		Usage: required for cpus with compatible string "qcom,krait".
> +		Value type: <prop-encoded-array>
> +		Definition: L1/CPU error interrupt

I reckon you want this property to belong in the cpus node (example below),
not in cpu nodes, right ?

Are you relying on a platform device to be created for /cpus node in
order for this series to work ? I guess that's why you want the
interrupts property to be defined in /cpus so that it becomes a platform
device resource (and you also add a compatible property in /cpus that is
missing in these bindings).

> +
> +	- 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 +392,45 @@ cpus {
>  		cpu-release-addr = <0 0x20000000>;
>  	};
>  };
> +
> +
> +Example 5 (Krait 32-bit system):
> +
> +cpus {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	interrupts = <1 9 0xf04>;

In patch 4 you also add a compatible property here, and that's not documented,
and honestly I do not think that's acceptable either. I guess you want a
compatible property here to match the platform driver, right ?

Thank you,
Lorenzo

> +
> +	cpu@0 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <0>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@1 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <1>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@2 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <2>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@3 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <3>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	L2: l2-cache {
> +		compatible = "cache";
> +		interrupts = <0 2 0x4>;
> +	};
> +};
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 01/15, Lorenzo Pieralisi wrote:
> On Tue, Jan 14, 2014 at 09:30:32PM +0000, Stephen Boyd wrote:
> > 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-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/arm/cpus.txt | 52 ++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> > index 91304353eea4..c332b5168456 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> > @@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
> >  			  property identifying a 64-bit zero-initialised
> >  			  memory location.
> >  
> > +	- interrupts
> > +		Usage: required for cpus with compatible string "qcom,krait".
> > +		Value type: <prop-encoded-array>
> > +		Definition: L1/CPU error interrupt
> 
> I reckon you want this property to belong in the cpus node (example below),
> not in cpu nodes, right ?

Yes.

> 
> Are you relying on a platform device to be created for /cpus node in
> order for this series to work ? I guess that's why you want the
> interrupts property to be defined in /cpus so that it becomes a platform
> device resource (and you also add a compatible property in /cpus that is
> missing in these bindings).

Ah yes. I'll move this to the /cpus section.

> 
> > +
> > +	- 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 +392,45 @@ cpus {
> >  		cpu-release-addr = <0 0x20000000>;
> >  	};
> >  };
> > +
> > +
> > +Example 5 (Krait 32-bit system):
> > +
> > +cpus {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	interrupts = <1 9 0xf04>;
> 
> In patch 4 you also add a compatible property here, and that's not documented,
> and honestly I do not think that's acceptable either. I guess you want a
> compatible property here to match the platform driver, right ?

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.

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.
+
+	- 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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
> 

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

(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.

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Kumar Gala]

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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

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

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

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.

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html