On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:

On 01/16, Lorenzo Pieralisi wrote:

quoted

On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote:

quoted

On 01/15, Stephen Boyd wrote:

quoted

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 <redacted>
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 <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Kumar Gala <redacted>
Cc: <redacted>
Signed-off-by: Stephen Boyd <redacted>
---
 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.

quoted

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