On Thu, Jul 31, 2014 at 7:34 AM, Robert Richter [off-list ref] wrote:

On 30.07.14 11:37:38, Rob Herring wrote:

quoted

On Wed, Jul 30, 2014 at 10:46 AM, Mark Rutland [off-list ref] wrote:

quoted

On Wed, Jul 30, 2014 at 04:06:31PM +0100, Robert Richter wrote:

quoted

From: Radha Mohan Chintakuntla <redacted>

quoted

quoted

quoted

+/ {
+     model = "Cavium ThunderX CN88XX Family";
+     compatible = "cavium,thunder-88xx";

Please don't use wildcards in compatible strings. Give this an absolute
name, and override as necessary.

The naming 88xx refers to the processor family and arn't actually
wildcards. In the future we might need another dts file for 87xx, but
so far all SoCs of 88xx family should use the same dts files. In this
sense the naming is very specific.

Yes, but each implementation can have its own errata. You might not
need to distinguish them now, but you could in the future.

However, if the family is really all the same die and different parts
are just marketing, then the name is fine. Or if you can easily probe
the exact part and revision it's probably fine.

quoted

quoted

quoted

+     cpus {
+             #address-cells = <2>;
+             #size-cells = <0>;
+
+             cpu at 000 {
+                     device_type = "cpu";
+                     compatible = "cavium,thunder", "arm,armv8";
+                     reg = <0x0 0x000>;
+                     enable-method = "psci";
+             };

Just to check: both the SoC and CPU are called thunder?

The soc is called thunder-88xx, the cpu thunder. E.g. an 87xx soc will
have the same core in which is thunder.

And the next version of the core would be called something else?
thunder-v2? lightning? As long as they are distinguishable they should
be fine.

Rob

quoted

quoted

quoted

+     memory at 00000000 {
+             device_type = "memory";
+             reg = <0x0 0x00000000 0x0 0x80000000>;
+     };
+
+     gic0: interrupt-controller at 801000000000 {

To make this easier to read, please place a comma between 32-bit
portions of the unit address (e.g. here have 8010,00000000).

Changed this.

quoted

Mark, perhaps a dtc or checkpatch.pl check for this?

This should also be under a bus node.

Will do.

quoted

quoted

quoted

+             compatible = "arm,gic-v3";
+             #interrupt-cells = <3>;
+             #address-cells = <2>;
+             #size-cells = <2>;
+             ranges;

This has no children, so why have ranges, #address-cells, and
#size-cells?

Right, this is a leftover from a change in a follow on patch that
introduces a child for its. Will remove #address-cells, #size-cells
and ranges in this patch and move the change to the later patch.

quoted

quoted

quoted

+             interrupt-controller;
+             reg = <0x8010 0x00000000 0x0 0x010000>, /* GICD */
+                   <0x8010 0x80000000 0x0 0x200000>; /* GICR */
+             interrupts = <1 9 0xf04>;
+     };

quoted

quoted

quoted

+             clocks {
+                     #address-cells = <2>;
+                     #size-cells = <2>;
+                     ranges;
+
+                     refclk50mhz: refclk50mhz {
+                             compatible = "fixed-clock";
+                             #clock-cells = <0>;
+                             clock-frequency = <50000000>;
+                             clock-output-names = "refclk50mhz";
+                     };
+             };

Please get rid of the clocks node and just put the clocks here.

Will do.

quoted

quoted

quoted

+
+             uaa0: serial at 87e024000000 {
+                     compatible = "arm,pl011", "arm,primecell";
+                     reg = <0x87e0 0x24000000 0x0 0x1000>;
+                     interrupts = <1 21 4>;
+                     clocks = <&refclk50mhz>;
+                     clock-names = "apb_pclk";

Is this actually the apb_pclk, or is the the uartclk? I assume it's the
latter.

Shouldn't new bindings have both clocks here? A single clock was a
mistake I think (mine in fact).

Do you mean
                        clock-names = "uartclk", "apb_pclk";
here?

Yes, but Mark said this change never happened so maybe it is fine. In
any case, follow the pl011 binding documentation.

Rob