[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Tsahee Zidenberg]

This patch introduces an initial device-tree for the Alpine platform.

Signed-off-by: Barak Wasserstrom <barak@annapurnalabs.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 .../bindings/arm/annapurna-labs,alpine.txt         |  96 +++++++++++
 .../cpu-enable-method/annapurna-labs,alpine-smp    |  64 ++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/arm/boot/dts/Makefile                         |   2 +
 arch/arm/boot/dts/alpine.dts                       | 181 +++++++++++++++++++++
 5 files changed, 344 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
 create mode 100644 Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
 create mode 100644 arch/arm/boot/dts/alpine.dts

diff --git a/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt b/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
new file mode 100644
index 0000000..b10a058
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
@@ -0,0 +1,96 @@
+Annapurna Labs Alpine Platform Device Tree Bindings
+---------------------------------------------------------------
+
+Boards in the Alpine family shall have the following properties:
+
+* Required root node properties:
+compatible: must contain "annapurna-labs,alpine"
+
+* Example:
+
+/ {
+	model = "Annapurna Labs Alpine Dev Board";
+	compatible = "annapurna-labs,alpine";
+
+	...
+}
+
+* CPU node:
+
+The Alpine platform includes cortex-a15 cores.
+enable-method: must be "annapurna-labs,alpine-smp" to allow smp  [1]
+
+Example:
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	enable-method = "annapurna-labs,alpine-smp";
+
+	cpu at 0 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <0>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+	};
+
+	cpu at 1 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <1>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+	};
+
+	cpu at 2 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <2>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+	};
+
+	cpu at 3 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <3>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+	};
+};
+
+
+* Alpine CPU resume registers
+
+The CPU resume register are used to define required resume address after
+reset.
+
+Properties:
+- compatible : Should contain "annapurna-labs,al-cpu-resume".
+- reg : Offset and length of the register set for the device
+
+Example:
+
+cpu_resume {
+	compatible = "annapurna-labs,al-cpu-resume";
+	reg = <0xfbff5ed0 0x30>;
+};
+
+* Alpine System-Fabric Service Registers
+
+The System-Fabric Service Registers allow various operation on CPU and
+system fabric, like powering CPUs off.
+
+Properties:
+- compatible : Should contain "annapurna-labs,al-sysfabric-service".
+- reg : Offset and length of the register set for the device
+
+Example:
+
+nb_service {
+        compatible = "annapurna-labs,al-sysfabric-service";
+        reg = <0xfb070000 0x10000>;
+};
+
+[1] arm/cpu-enable-method/annapurna-labs,alpine-smp
diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
new file mode 100644
index 0000000..6245aa9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
@@ -0,0 +1,64 @@
+========================================================
+Secondary CPU enable-method "annapurna-labs,alpine-smp" binding
+========================================================
+
+This document describes the "annapurna-labs,alpine-smp" method for
+enabling secondary CPUs. To apply to all CPUs, a single
+"annapurna-labs,alpine-smp" enable method should be defined in the
+"cpus" node.
+
+Enable method name:	"annapurna-labs,alpine-smp"
+Compatible machines:	"annapurna-labs,alpine"
+Compatible CPUs:	"arm,cortex-a15"
+Related properties:	(none)
+
+Note:
+This enable method needs valid nodes compatible with
+"annapurna-labs,al-cpu-resume" and "annapurna-labs,al-nb-service"[1].
+
+Example:
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	enable-method = "annapurna-labs,alpine-smp";
+
+	cpu at 0 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <0>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+		clock-frequency = <0>; /* Filled by loader */
+	};
+
+	cpu at 1 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <1>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+		clock-frequency = <0>; /* Filled by loader */
+	};
+
+	cpu at 2 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <2>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+		clock-frequency = <0>; /* Filled by loader */
+	};
+
+	cpu at 3 {
+		compatible = "arm,cortex-a15";
+		device_type = "cpu";
+		reg = <3>;
+		clocks = <&cpuclk>;
+		clock-names = "cpu";
+		clock-frequency = <0>; /* Filled by loader */
+	};
+};
+
+--
+[1] arm/annapurna-labs,alpine.txt
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b1df0ad..e1225b8 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -17,6 +17,7 @@ amd	Advanced Micro Devices (AMD), Inc.
 amlogic	Amlogic, Inc.
 ams	AMS AG
 amstaos	AMS-Taos Inc.
+annapurna-labs Annapurna Labs
 apm	Applied Micro Circuits Corporation (APM)
 arm	ARM Ltd.
 armadeus	ARMadeus Systems SARL
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 91bd5bd..71610ea 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1,5 +1,7 @@
 ifeq ($(CONFIG_OF),y)
 
+dtb-$(CONFIG_ARCH_ALPINE) += alpine_db.dtb
+
 # Keep at91 dtb files sorted alphabetically for each SoC
 # rm9200
 dtb-$(CONFIG_ARCH_AT91) += at91rm9200ek.dtb
diff --git a/arch/arm/boot/dts/alpine.dts b/arch/arm/boot/dts/alpine.dts
new file mode 100644
index 0000000..fa0da66
--- /dev/null
+++ b/arch/arm/boot/dts/alpine.dts
@@ -0,0 +1,181 @@
+/*
+ * Copyright 2015 Annapurna Labs Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include "skeleton64.dtsi"
+
+/ {
+	version = "2.4";
+	compatible = "annapurna-labs,alpine";
+	#address-cells = <2>;
+	#size-cells = <2>;
+	clock-ranges;
+
+	/* CPU Configuration */
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		enable-method = "annapurna-labs,alpine-smp";
+
+		cpu at 0 {
+			compatible = "arm,cortex-a15";
+			device_type = "cpu";
+			reg = <0>;
+			clocks = <&cpuclk>;
+			clock-names = "cpu";
+		};
+
+		cpu at 1 {
+			compatible = "arm,cortex-a15";
+			device_type = "cpu";
+			reg = <1>;
+			clocks = <&cpuclk>;
+			clock-names = "cpu";
+		};
+
+		cpu at 2 {
+			compatible = "arm,cortex-a15";
+			device_type = "cpu";
+			reg = <2>;
+			clocks = <&cpuclk>;
+			clock-names = "cpu";
+		};
+
+		cpu at 3 {
+			compatible = "arm,cortex-a15";
+			device_type = "cpu";
+			reg = <3>;
+			clocks = <&cpuclk>;
+			clock-names = "cpu";
+		};
+	};
+
+	soc {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		compatible = "simple-bus";
+		interrupt-parent = <&gic_main>;
+		ranges;
+
+		arch-timer {
+			compatible = "arm,cortex-a15-timer",
+				     "arm,armv7-timer";
+			interrupts =
+				<GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+				<GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+				<GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+				<GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+			clock-frequency = <0>; /* Filled by loader */
+		};
+
+		/* Interrupt Controller */
+		gic_main: gic_main {
+			compatible = "arm,cortex-a15-gic";
+			#interrupt-cells = <3>;
+			#size-cells = <0>;
+			#address-cells = <0>;
+			interrupt-controller;
+			reg = <0x0 0xfb001000 0x0 0x1000>,
+			      <0x0 0xfb002000 0x0 0x2000>,
+			      <0x0 0xfb004000 0x0 0x1000>,
+			      <0x0 0xfb006000 0x0 0x2000>;
+		    interrupts = <GIC_PPI 9
+			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+		};
+
+		/* CPU Resume registers */
+		cpu_resume {
+			compatible = "annapurna-labs,al-cpu-resume";
+			reg = <0x0 0xfbff5ec0 0x0 0x30>;
+		};
+
+		/* North Bridge Service Registers */
+		nb_service {
+			compatible = "annapurna-labs,al-sysfabric-service";
+			reg = <0x0 0xfb070000 0x0 0x10000>;
+		};
+
+		/* Performance Monitor Unit */
+		pmu {
+			compatible = "arm,cortex-a15-pmu";
+			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH
+					GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH
+					GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH
+					GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		wdt0 {
+			compatible = "arm,sp805", "arm,primecell";
+			reg = <0x0 0xfd88c000 0x0 0x1000>;
+			interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&sbclk>;
+			clock-names = "apb_pclk";
+		};
+
+		uart0 {
+			compatible = "ns16550a";
+			reg = <0x0 0xfd883000 0x0 0x1000>;
+			clock-frequency = <0>; /* Filled by loader */
+			interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+		};
+
+		uart1 {
+			compatible = "ns16550a";
+			reg = <0x0 0xfd884000 0x0 0x1000>;
+			clock-frequency = <0>; /* Filled by loader */
+			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+		};
+
+		clocks {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/* Reference clock */
+			refclk: refclk {
+				#clock-cells = <0>;
+				compatible = "fixed-clock";
+				clock-frequency = <0>; /* Filled by loader */
+			};
+
+			/* South Bridge Clock */
+			sbclk: sbclk {
+				#clock-cells = <0>;
+				compatible = "fixed-clock";
+				clock-frequency = <0>; /* Filled by loader */
+			};
+
+			/* North Bridge Clock */
+			nbclk: nbclk {
+				#clock-cells = <0>;
+				compatible = "fixed-clock";
+				clock-frequency = <0>; /* Filled by loader */
+			};
+
+			/* CPU Clock */
+			cpuclk: cpuclk {
+				#clock-cells = <0>;
+				compatible = "fixed-clock";
+				clock-frequency = <0>; /* Filled by loader */
+			};
+		};
+	};
+};
-- 
1.9.1

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Arnd Bergmann]

On Sunday 25 January 2015 20:30:59 Tsahee Zidenberg wrote:
> This patch introduces an initial device-tree for the Alpine platform.
> 
> Signed-off-by: Barak Wasserstrom <barak@annapurnalabs.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
> ---
>  .../bindings/arm/annapurna-labs,alpine.txt         |  96 +++++++++++
>  .../cpu-enable-method/annapurna-labs,alpine-smp    |  64 ++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>  arch/arm/boot/dts/Makefile                         |   2 +
>  arch/arm/boot/dts/alpine.dts                       | 181 +++++++++++++++++++++
>  5 files changed, 344 insertions(+)

I'd suggest splitting this into two patches.

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
> @@ -0,0 +1,96 @@
> +Annapurna Labs Alpine Platform Device Tree Bindings
> +---------------------------------------------------------------
> +
> +Boards in the Alpine family shall have the following properties:
> +
> +* Required root node properties:
> +compatible: must contain "annapurna-labs,alpine"
> +
> +* Example:
> +
> +/ {
> +	model = "Annapurna Labs Alpine Dev Board";
> +	compatible = "annapurna-labs,alpine";
> +
> +	...
> +}
> +
> +* CPU node:
> +
> +The Alpine platform includes cortex-a15 cores.
> +enable-method: must be "annapurna-labs,alpine-smp" to allow smp  [1]

Any reason for not using PSCI on this platform?

> diff --git a/arch/arm/boot/dts/alpine.dts b/arch/arm/boot/dts/alpine.dts
> new file mode 100644
> index 0000000..fa0da66
> --- /dev/null
> +++ b/arch/arm/boot/dts/alpine.dts

Most people split this into one .dtsi file for the SoC and one .dts file
for a specific machine, to reduce the amount of duplication when there
are multiple machines that use the same chip.

> @@ -0,0 +1,181 @@
> +/*
> + * Copyright 2015 Annapurna Labs Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */

If you don't mind, please use a dual license for the dts file, to allow
distributing the file with other OSs that might to run on the same
hardware.

> +/dts-v1/;
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "skeleton64.dtsi"
> +
> +/ {
> +	version = "2.4";

This is not documented anywhere.

> +	compatible = "annapurna-labs,alpine";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +	clock-ranges;

No "model" property?

> +		/* North Bridge Service Registers */
> +		nb_service {
> +			compatible = "annapurna-labs,al-sysfabric-service";
> +			reg = <0x0 0xfb070000 0x0 0x10000>;
> +		};

What kinds of other registers are there in this device? Should it
be marked as "syscon" so you can use it through a regmap?

	Arnd

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Arnd Bergmann]

On Monday 26 January 2015 16:14:11 Tsahee Zidenberg wrote:
> On 26 January 2015 at 13:15, Arnd Bergmann <arnd@arndb.de> wrote:> +* CPU
> node:
> 
> > > +
> > > +The Alpine platform includes cortex-a15 cores.
> > > +enable-method: must be "annapurna-labs,alpine-smp" to allow smp  [1]
> >
> > Any reason for not using PSCI on this platform?
> >
> >
> PSCI is in the roadmap, but not supported by current firmware.

As your initial submission is very minimal regarding driver support,
would it be sensible to wait for the firmware to catch up then?

That way you lose SMP support for the moment but could still work
on adding the device drivers while testing in uniprocessor mode and
then we will see if your firmware is ready once there are more
drivers upstream. Once the firmware has PSCI working, it will all
just work without the need for extra devices in DT or the custom
SMP operations.

	Ard

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Saeed Bishara]

On 26 January 2015 at 16:32, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 26 January 2015 16:14:11 Tsahee Zidenberg wrote:
>> On 26 January 2015 at 13:15, Arnd Bergmann <arnd@arndb.de> wrote:> +* CPU
>> node:
>>
>> > > +
>> > > +The Alpine platform includes cortex-a15 cores.
>> > > +enable-method: must be "annapurna-labs,alpine-smp" to allow smp  [1]
>> >
>> > Any reason for not using PSCI on this platform?
>> >
>> >
>> PSCI is in the roadmap, but not supported by current firmware.
>
> As your initial submission is very minimal regarding driver support,
> would it be sensible to wait for the firmware to catch up then?
Arnd,
there are already systems with this soc that shipped without  PSCI firmware.
and we still would like to support mainline kernel on those systems
without forcing firmware upgrade.
saeed

>
> That way you lose SMP support for the moment but could still work
> on adding the device drivers while testing in uniprocessor mode and
> then we will see if your firmware is ready once there are more
> drivers upstream. Once the firmware has PSCI working, it will all
> just work without the need for extra devices in DT or the custom
> SMP operations.
>
>         Ard

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Mark Rutland]

On Sun, Jan 25, 2015 at 06:30:59PM +0000, Tsahee Zidenberg wrote:
> This patch introduces an initial device-tree for the Alpine platform.
>
> Signed-off-by: Barak Wasserstrom <barak@annapurnalabs.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
> ---
>  .../bindings/arm/annapurna-labs,alpine.txt         |  96 +++++++++++
>  .../cpu-enable-method/annapurna-labs,alpine-smp    |  64 ++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>  arch/arm/boot/dts/Makefile                         |   2 +
>  arch/arm/boot/dts/alpine.dts                       | 181 +++++++++++++++++++++
>  5 files changed, 344 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
>  create mode 100644 arch/arm/boot/dts/alpine.dts

[...]

> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
> new file mode 100644
> index 0000000..6245aa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
> @@ -0,0 +1,64 @@
> +========================================================
> +Secondary CPU enable-method "annapurna-labs,alpine-smp" binding
> +========================================================
> +
> +This document describes the "annapurna-labs,alpine-smp" method for
> +enabling secondary CPUs. To apply to all CPUs, a single
> +"annapurna-labs,alpine-smp" enable method should be defined in the
> +"cpus" node.
> +
> +Enable method name:    "annapurna-labs,alpine-smp"
> +Compatible machines:   "annapurna-labs,alpine"
> +Compatible CPUs:       "arm,cortex-a15"
> +Related properties:    (none)

Please describe what the contract of the method is? What's expected of
the system, FW, and kernel?

Otherwise the documentation is practically useless.

[...]

> diff --git a/arch/arm/boot/dts/alpine.dts b/arch/arm/boot/dts/alpine.dts
> new file mode 100644
> index 0000000..fa0da66
> --- /dev/null
> +++ b/arch/arm/boot/dts/alpine.dts
> @@ -0,0 +1,181 @@
> +/*
> + * Copyright 2015 Annapurna Labs Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "skeleton64.dtsi"
> +
> +/ {
> +       version = "2.4";

What's this for?

> +       compatible = "annapurna-labs,alpine";
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +       clock-ranges;

Surely this doesn't mean anything at the root node?

[...]

> +       soc {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               compatible = "simple-bus";
> +               interrupt-parent = <&gic_main>;
> +               ranges;
> +
> +               arch-timer {
> +                       compatible = "arm,cortex-a15-timer",
> +                                    "arm,armv7-timer";
> +                       interrupts =
> +                               <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +                       clock-frequency = <0>; /* Filled by loader */
> +               };

Please fix your bootloader/FW to configure CNTFRQ on all CPUs whenever
they are brought up.

Are CPUs booted at Hyp? If not, is CNTVOFF configured uniformly across
CPUs by the bootloader/FW?

If the answer to both those questions is no, timekeeping will not work
on this platform.

> +
> +               /* Interrupt Controller */
> +               gic_main: gic_main {
> +                       compatible = "arm,cortex-a15-gic";
> +                       #interrupt-cells = <3>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
> +                       interrupt-controller;
> +                       reg = <0x0 0xfb001000 0x0 0x1000>,
> +                             <0x0 0xfb002000 0x0 0x2000>,
> +                             <0x0 0xfb004000 0x0 0x1000>,
> +                             <0x0 0xfb006000 0x0 0x2000>;
> +                   interrupts = <GIC_PPI 9
> +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +               };
> +
> +               /* CPU Resume registers */
> +               cpu_resume {
> +                       compatible = "annapurna-labs,al-cpu-resume";
> +                       reg = <0x0 0xfbff5ec0 0x0 0x30>;
> +               };
> +
> +               /* North Bridge Service Registers */
> +               nb_service {
> +                       compatible = "annapurna-labs,al-sysfabric-service";
> +                       reg = <0x0 0xfb070000 0x0 0x10000>;
> +               };
> +
> +               /* Performance Monitor Unit */
> +               pmu {
> +                       compatible = "arm,cortex-a15-pmu";
> +                       interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> +               };

Nit: please bracket list entries individually, as you have done
elsewhere.

[...]

> +               clocks {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;

There is absolutely no need for a clock container node. Please get rid
of it, and just place the clocks here without it.

Thanks,
Mark.

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Tsahee Zidenberg]

On 26 January 2015 at 13:42, Mark Rutland <mark.rutland@arm.com> wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
>> new file mode 100644
>> index 0000000..6245aa9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
>> @@ -0,0 +1,64 @@
>> +========================================================
>> +Secondary CPU enable-method "annapurna-labs,alpine-smp" binding
>> +========================================================
>> +
>> +This document describes the "annapurna-labs,alpine-smp" method for
>> +enabling secondary CPUs. To apply to all CPUs, a single
>> +"annapurna-labs,alpine-smp" enable method should be defined in the
>> +"cpus" node.
>> +
>> +Enable method name:    "annapurna-labs,alpine-smp"
>> +Compatible machines:   "annapurna-labs,alpine"
>> +Compatible CPUs:       "arm,cortex-a15"
>> +Related properties:    (none)
>
>Please describe what the contract of the method is? What's expected of
>the system, FW, and kernel?
>
>Otherwise the documentation is practically useless.
I'm not sure I understand exactly what you ask.
This is simply documenting the device-tree binding, not much more or
less informational than the existing documentation for "berlin-smp".
The method is implemented in the previous patch. I will add a little
more detail to the implementation itself on the next patchset.
The node
Basically: the FW contract is pretty straight forward and tries to
keep up with arm/linux requirements. All CPUs boot to the same state,
firmware handles necessary initializations in the cases the CPU can't
make them because of it's mode.

>> +       soc {
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               compatible = "simple-bus";
>> +               interrupt-parent = <&gic_main>;
>> +               ranges;
>> +
>> +               arch-timer {
>> +                       compatible = "arm,cortex-a15-timer",
>> +                                    "arm,armv7-timer";
>> +                       interrupts =
>> +                               <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +                               <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +                               <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +                               <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> +                       clock-frequency = <0>; /* Filled by loader */
>> +               };
>
> Please fix your bootloader/FW to configure CNTFRQ on all CPUs whenever
> they are brought up.
>
For completeness, I agree. However, arch_timer_detect_rate in
drivers/clocksource/arm_arch_timer.c seems to first try reading the
frequency from devicetree, and fall-back to CNTFRQ only if that fails.

Either way, filling the arch-timer in bootloader seems right.

> Are CPUs booted at Hyp? If not, is CNTVOFF configured uniformly across
> CPUs by the bootloader/FW?
>
> If the answer to both those questions is no, timekeeping will not work
> on this platform.
>
Generally yes, all CPUs boot to HYP. We do plan for other options - so
thanks for the heads up!

All other comments (unnecessary nodes/properties, bad brackets, etc..)
- will be fixed.

Thank you!
tsahee.

[PATCH 4/4] ARM: dts: Add initial support for Alpine platform

[Olof Johansson]

Hi,

Some comments below. Some of these might be duplicated by other
reviewers, apologies if that's the case.

On Sun, Jan 25, 2015 at 10:30 AM, Tsahee Zidenberg
<tsahee@annapurnalabs.com> wrote:
> This patch introduces an initial device-tree for the Alpine platform.
>
> Signed-off-by: Barak Wasserstrom <barak@annapurnalabs.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
> ---
>  .../bindings/arm/annapurna-labs,alpine.txt         |  96 +++++++++++
>  .../cpu-enable-method/annapurna-labs,alpine-smp    |  64 ++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>  arch/arm/boot/dts/Makefile                         |   2 +
>  arch/arm/boot/dts/alpine.dts                       | 181 +++++++++++++++++++++
>  5 files changed, 344 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/cpu-enable-method/annapurna-labs,alpine-smp
>  create mode 100644 arch/arm/boot/dts/alpine.dts
>
> diff --git a/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt b/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
> new file mode 100644
> index 0000000..b10a058
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/annapurna-labs,alpine.txt
> @@ -0,0 +1,96 @@
> +Annapurna Labs Alpine Platform Device Tree Bindings
> +---------------------------------------------------------------
> +
> +Boards in the Alpine family shall have the following properties:
> +
> +* Required root node properties:
> +compatible: must contain "annapurna-labs,alpine"
> +
> +* Example:
> +
> +/ {
> +       model = "Annapurna Labs Alpine Dev Board";
> +       compatible = "annapurna-labs,alpine";

1. Bikeshed comment, feel free to ignore: "annapurna-labs" is a pretty
long prefix, feel free to use some sort of abbreviation instead (most
do).

2. More important: You should probably have a fallback compatible to
the SoC, and not just the board name. That way, if you have several
derivative but similar boards you don't have to update the compatible
table for every board name.


> +* Alpine System-Fabric Service Registers
> +
> +The System-Fabric Service Registers allow various operation on CPU and
> +system fabric, like powering CPUs off.
> +
> +Properties:
> +- compatible : Should contain "annapurna-labs,al-sysfabric-service".
> +- reg : Offset and length of the register set for the device

Double naming: annapurna-labs  and "al-*". One is enough.

Actually, "al" seems like it's an unused abbreviation. Feel free to
use it per above.

> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 91bd5bd..71610ea 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1,5 +1,7 @@
>  ifeq ($(CONFIG_OF),y)
>
> +dtb-$(CONFIG_ARCH_ALPINE) += alpine_db.dtb
> +
>  # Keep at91 dtb files sorted alphabetically for each SoC
>  # rm9200
>  dtb-$(CONFIG_ARCH_AT91) += at91rm9200ek.dtb


Common format of these are: <platform_prefix>-boardname.dtb

alpine-devboard.dtb or alpine-db.dtb would be appropriate here.


> diff --git a/arch/arm/boot/dts/alpine.dts b/arch/arm/boot/dts/alpine.dts
> new file mode 100644
> index 0000000..fa0da66
> --- /dev/null
> +++ b/arch/arm/boot/dts/alpine.dts

As mentioned elsewhere, a dtsi/dts combo is preferred here.

> @@ -0,0 +1,181 @@
> +/*
> + * Copyright 2015 Annapurna Labs Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.

We're asking for dual-license for DTS these days, so that they can be
incorporated in non-GPL firmware if needed. See other files in
arch/arm/boot/dts for the standard copyright header.


> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "skeleton64.dtsi"
> +
> +/ {
> +       version = "2.4";
> +       compatible = "annapurna-labs,alpine";
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +       clock-ranges;
> +
> +       /* CPU Configuration */
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               enable-method = "annapurna-labs,alpine-smp";
> +
> +               cpu at 0 {
> +                       compatible = "arm,cortex-a15";
> +                       device_type = "cpu";
> +                       reg = <0>;
> +                       clocks = <&cpuclk>;
> +                       clock-names = "cpu";
> +               };
> +
> +               cpu at 1 {
> +                       compatible = "arm,cortex-a15";
> +                       device_type = "cpu";
> +                       reg = <1>;
> +                       clocks = <&cpuclk>;
> +                       clock-names = "cpu";
> +               };
> +
> +               cpu at 2 {
> +                       compatible = "arm,cortex-a15";
> +                       device_type = "cpu";
> +                       reg = <2>;
> +                       clocks = <&cpuclk>;
> +                       clock-names = "cpu";
> +               };
> +
> +               cpu at 3 {
> +                       compatible = "arm,cortex-a15";
> +                       device_type = "cpu";
> +                       reg = <3>;
> +                       clocks = <&cpuclk>;
> +                       clock-names = "cpu";
> +               };
> +       };
> +
> +       soc {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               compatible = "simple-bus";
> +               interrupt-parent = <&gic_main>;
> +               ranges;
> +
> +               arch-timer {
> +                       compatible = "arm,cortex-a15-timer",
> +                                    "arm,armv7-timer";
> +                       interrupts =
> +                               <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                               <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +                       clock-frequency = <0>; /* Filled by loader */
> +               };
> +
> +               /* Interrupt Controller */
> +               gic_main: gic_main {
> +                       compatible = "arm,cortex-a15-gic";
> +                       #interrupt-cells = <3>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
> +                       interrupt-controller;
> +                       reg = <0x0 0xfb001000 0x0 0x1000>,
> +                             <0x0 0xfb002000 0x0 0x2000>,
> +                             <0x0 0xfb004000 0x0 0x1000>,
> +                             <0x0 0xfb006000 0x0 0x2000>;
> +                   interrupts = <GIC_PPI 9
> +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +               };
> +
> +               /* CPU Resume registers */
> +               cpu_resume {

Underscores are generally uncommon in node names, here and elsewhere
in this file.

Also, please use an unit address @0xfbff5ec0.

> +                       compatible = "annapurna-labs,al-cpu-resume";
> +                       reg = <0x0 0xfbff5ec0 0x0 0x30>;
> +               };
> +
> +               /* North Bridge Service Registers */
> +               nb_service {
> +                       compatible = "annapurna-labs,al-sysfabric-service";
> +                       reg = <0x0 0xfb070000 0x0 0x10000>;
> +               };
> +
> +               /* Performance Monitor Unit */
> +               pmu {
> +                       compatible = "arm,cortex-a15-pmu";
> +                       interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH
> +                                       GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> +               };
> +
> +               wdt0 {
> +                       compatible = "arm,sp805", "arm,primecell";
> +                       reg = <0x0 0xfd88c000 0x0 0x1000>;
> +                       interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&sbclk>;
> +                       clock-names = "apb_pclk";
> +               };
> +
> +               uart0 {

The label can be uart0, but the node should be generic, and not
uart0/uart1 for the two nodes. To distinguish them, include unit
address.

> +                       compatible = "ns16550a";
> +                       reg = <0x0 0xfd883000 0x0 0x1000>;
> +                       clock-frequency = <0>; /* Filled by loader */
> +                       interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> +                       reg-shift = <2>;
> +                       reg-io-width = <4>;
> +               };
> +
> +               uart1 {
> +                       compatible = "ns16550a";
> +                       reg = <0x0 0xfd884000 0x0 0x1000>;
> +                       clock-frequency = <0>; /* Filled by loader */
> +                       interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +                       reg-shift = <2>;
> +                       reg-io-width = <4>;
> +               };
> +
> +               clocks {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       /* Reference clock */
> +                       refclk: refclk {
> +                               #clock-cells = <0>;
> +                               compatible = "fixed-clock";
> +                               clock-frequency = <0>; /* Filled by loader */
> +                       };
> +
> +                       /* South Bridge Clock */
> +                       sbclk: sbclk {
> +                               #clock-cells = <0>;
> +                               compatible = "fixed-clock";
> +                               clock-frequency = <0>; /* Filled by loader */
> +                       };
> +
> +                       /* North Bridge Clock */
> +                       nbclk: nbclk {
> +                               #clock-cells = <0>;
> +                               compatible = "fixed-clock";
> +                               clock-frequency = <0>; /* Filled by loader */
> +                       };
> +
> +                       /* CPU Clock */
> +                       cpuclk: cpuclk {
> +                               #clock-cells = <0>;
> +                               compatible = "fixed-clock";
> +                               clock-frequency = <0>; /* Filled by loader */
> +                       };
> +               };
> +       };
> +};


-Olof