diff mbox

[v2,1/3] ARM: dts: berlin2q: add the Marvell Armada 1500 pro

Message ID 1394719607-25018-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Antoine Tenart March 13, 2014, 2:06 p.m. UTC
Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family).
The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local
timer, apb timers and uarts for now.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)
 create mode 100644 arch/arm/boot/dts/berlin2q.dtsi

Comments

Mark Rutland March 13, 2014, 5:25 p.m. UTC | #1
On Thu, Mar 13, 2014 at 02:06:45PM +0000, Antoine Ténart wrote:
> Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family).
> The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local
> timer, apb timers and uarts for now.
> 
> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 167 insertions(+)
>  create mode 100644 arch/arm/boot/dts/berlin2q.dtsi
> 
> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
> new file mode 100644
> index 000000000000..1cb76031dfe6
> --- /dev/null
> +++ b/arch/arm/boot/dts/berlin2q.dtsi
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2014 Antoine Ténart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +#include "skeleton.dtsi"
> +
> +/ {
> +	model = "Marvell Armada 1500 pro (BG2-Q) SoC";
> +	compatible = "marvell,berlin2q", "marvell,berlin";
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			next-level-cache = <&l2>;
> +			reg = <0>;
> +		};
> +
> +		cpu@1 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			next-level-cache = <&l2>;
> +			reg = <1>;
> +		};
> +
> +		cpu@2 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			next-level-cache = <&l2>;
> +			reg = <2>;
> +		};
> +
> +		cpu@3 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			next-level-cache = <&l2>;
> +			reg = <3>;
> +		};
> +	};
> +
> +	clocks {
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +
> +		smclk: sysmgr-clock {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <25000000>;
> +		};
> +
> +		sysclk: system-clock {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <400000000>;
> +		};
> +	};

I am very much not keen on having container nodes like this.

A "clocks" container is non-standard, and not guarantee to probe. The
fact these clocks probe currently is an artifact of the current
organisation of Linux rather than any conscious decision.

The #address-cells and #size-cells properties are useless, as no
address space is defined. The node is not a simple-bus, has no ranges,
and doesn't have a compatible string that implies an address space.

Is there any reason you need to use a clocks container here and can't
put these directly under the root?

> +
> +	soc {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		ranges = <0 0xf7000000 0x1000000>;
> +		interrupt-parent = <&gic>;
> +
> +		l2: l2-cache-controller@ac0000 {
> +			compatible = "arm,pl310-cache";
> +			reg = <0xac0000 0x1000>;
> +			cache-level = <2>;
> +		};
> +
> +		local-timer@ad0600 {
> +			compatible = "arm,cortex-a9-twd-timer";
> +			reg = <0xad0600 0x20>;
> +			clocks = <&sysclk>;
> +			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "okay";

Given that nodes are assumed by default to be okay, this can be dropped.

I think it only makes sense to have a status property in a common dtsi
where the value is "disabled".

Otherwise, this dtsi looks sane to me.

Thanks,
Mark.
Sebastian Hesselbarth March 14, 2014, 9:31 a.m. UTC | #2
On 03/13/2014 03:06 PM, Antoine Ténart wrote:
> Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family).
> The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local
> timer, apb timers and uarts for now.
>
> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>   arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 167 insertions(+)
>   create mode 100644 arch/arm/boot/dts/berlin2q.dtsi
>
> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
> new file mode 100644
> index 000000000000..1cb76031dfe6
> --- /dev/null
> +++ b/arch/arm/boot/dts/berlin2q.dtsi
[...]
> +	clocks {
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +
> +		smclk: sysmgr-clock {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <25000000>;
> +		};
> +
> +		sysclk: system-clock {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <400000000>;
> +		};
> +	};
> +
> +	soc {
[...]
> +		local-timer@ad0600 {
> +			compatible = "arm,cortex-a9-twd-timer";
> +			reg = <0xad0600 0x20>;
> +			clocks = <&sysclk>;

If I understand Jisheng correctly, this should be cpuclk/3. When
removing the clocks {} container above, please also take care of
it.

You can do

cpuclk: cpu-clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <1200000000>; /* <- put correct freq here */
};

sysclk: system-clock {
	compatible = "fixed-factor-clock";
	#clock-cells = <0>;
	clocks = <&cpuclk>;
	clock-multi = <1>;
	clock-div = <3>;
};

Hopefully, we'll have proper clock drivers soon so we can just replace
referenced "fixed-*" clocks.

> +			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "okay";

As Mark Rutland mentioned, get rid of status = "okay" in dtsi.

> +		};
> +
[...]
> +		apb@e80000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			ranges = <0 0xe80000 0x10000>;
> +			interrupt-parent = <&aic>;
> +
> +			timer0: timer@2c00 {
> +				compatible = "snps,dw-apb-timer";
> +				reg = <0x2c00 0x14>;
> +				interrupts = <8>;
> +				clock-freq = <100000000>;
> +				status = "okay";
> +			};
> +
> +			timer1: timer@2c14 {
> +				compatible = "snps,dw-apb-timer";
> +				reg = <0x2c14 0x14>;
> +				clock-freq = <100000000>;
> +				status = "disabled";
> +			};

Please also add the remaining 6 apb timers.

Sebastian
Antoine Tenart March 14, 2014, 9:48 a.m. UTC | #3
Sebastian,

On 14/03/2014 10:31, Sebastian Hesselbarth wrote:
> On 03/13/2014 03:06 PM, Antoine Ténart wrote:
>> +    clocks {
>> +        #address-cells = <0>;
>> +        #size-cells = <0>;
>> +
>> +        smclk: sysmgr-clock {
>> +            compatible = "fixed-clock";
>> +            #clock-cells = <0>;
>> +            clock-frequency = <25000000>;
>> +        };
>> +
>> +        sysclk: system-clock {
>> +            compatible = "fixed-clock";
>> +            #clock-cells = <0>;
>> +            clock-frequency = <400000000>;
>> +        };
>> +    };
>> +
>> +    soc {
> [...]
>> +        local-timer@ad0600 {
>> +            compatible = "arm,cortex-a9-twd-timer";
>> +            reg = <0xad0600 0x20>;
>> +            clocks = <&sysclk>;
>
> If I understand Jisheng correctly, this should be cpuclk/3. When
> removing the clocks {} container above, please also take care of
> it.
>
> You can do
>
> cpuclk: cpu-clock {
>      compatible = "fixed-clock";
>      #clock-cells = <0>;
>      clock-frequency = <1200000000>; /* <- put correct freq here */
> };
>
> sysclk: system-clock {
>      compatible = "fixed-factor-clock";
>      #clock-cells = <0>;
>      clocks = <&cpuclk>;
>      clock-multi = <1>;
>      clock-div = <3>;
> };
>
> Hopefully, we'll have proper clock drivers soon so we can just replace
> referenced "fixed-*" clocks.

Sure, this is why I only kept one fixed clock in this patch.

I'll add the cpuclk here then.

>> +            timer0: timer@2c00 {
>> +                compatible = "snps,dw-apb-timer";
>> +                reg = <0x2c00 0x14>;
>> +                interrupts = <8>;
>> +                clock-freq = <100000000>;
>> +                status = "okay";
>> +            };
>> +
>> +            timer1: timer@2c14 {
>> +                compatible = "snps,dw-apb-timer";
>> +                reg = <0x2c14 0x14>;
>> +                clock-freq = <100000000>;
>> +                status = "disabled";
>> +            };
>
> Please also add the remaining 6 apb timers.

I don't have any information about the remaining 6 apb timers. I'll ask 
Jisheng.


Antoine
Sebastian Hesselbarth March 14, 2014, 9:53 a.m. UTC | #4
On 03/14/2014 10:48 AM, Antoine Ténart wrote:
> On 14/03/2014 10:31, Sebastian Hesselbarth wrote:
>> On 03/13/2014 03:06 PM, Antoine Ténart wrote:
>>> +    clocks {
>>> +        #address-cells = <0>;
>>> +        #size-cells = <0>;
>>> +
>>> +        smclk: sysmgr-clock {
>>> +            compatible = "fixed-clock";
>>> +            #clock-cells = <0>;
>>> +            clock-frequency = <25000000>;
>>> +        };
>>> +
>>> +        sysclk: system-clock {
>>> +            compatible = "fixed-clock";
>>> +            #clock-cells = <0>;
>>> +            clock-frequency = <400000000>;
>>> +        };
>>> +    };
>>> +
>>> +    soc {
>> [...]
>>> +        local-timer@ad0600 {
>>> +            compatible = "arm,cortex-a9-twd-timer";
>>> +            reg = <0xad0600 0x20>;
>>> +            clocks = <&sysclk>;
>>
>> If I understand Jisheng correctly, this should be cpuclk/3. When
>> removing the clocks {} container above, please also take care of
>> it.
>>
>> You can do
>>
>> cpuclk: cpu-clock {
>>      compatible = "fixed-clock";
>>      #clock-cells = <0>;
>>      clock-frequency = <1200000000>; /* <- put correct freq here */
>> };
>>
>> sysclk: system-clock {
>>      compatible = "fixed-factor-clock";
>>      #clock-cells = <0>;
>>      clocks = <&cpuclk>;
>>      clock-multi = <1>;
>>      clock-div = <3>;
>> };
>>
>> Hopefully, we'll have proper clock drivers soon so we can just replace
>> referenced "fixed-*" clocks.
>
> Sure, this is why I only kept one fixed clock in this patch.
>
> I'll add the cpuclk here then.
>
>>> +            timer0: timer@2c00 {
>>> +                compatible = "snps,dw-apb-timer";
>>> +                reg = <0x2c00 0x14>;
>>> +                interrupts = <8>;
>>> +                clock-freq = <100000000>;
>>> +                status = "okay";
>>> +            };
>>> +
>>> +            timer1: timer@2c14 {
>>> +                compatible = "snps,dw-apb-timer";
>>> +                reg = <0x2c14 0x14>;
>>> +                clock-freq = <100000000>;
>>> +                status = "disabled";
>>> +            };
>>
>> Please also add the remaining 6 apb timers.
>
> I don't have any information about the remaining 6 apb timers. I'll ask
> Jisheng.

He replied to v1 of this patch with

"We still have 8 apb timers"

Sebastian
Alexandre Belloni March 14, 2014, 11:32 a.m. UTC | #5
On 14/03/2014 at 10:31:55 +0100, Sebastian Hesselbarth wrote :
> On 03/13/2014 03:06 PM, Antoine Ténart wrote:
> >+		local-timer@ad0600 {
> >+			compatible = "arm,cortex-a9-twd-timer";
> >+			reg = <0xad0600 0x20>;
> >+			clocks = <&sysclk>;
> 
> If I understand Jisheng correctly, this should be cpuclk/3. When
> removing the clocks {} container above, please also take care of
> it.
> 

You understood correctly

> You can do
> 
> cpuclk: cpu-clock {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <1200000000>; /* <- put correct freq here */
> };
> 
> sysclk: system-clock {
> 	compatible = "fixed-factor-clock";
> 	#clock-cells = <0>;
> 	clocks = <&cpuclk>;
> 	clock-multi = <1>;
> 	clock-div = <3>;
> };
> 
> Hopefully, we'll have proper clock drivers soon so we can just replace
> referenced "fixed-*" clocks.
> 

I'm working on that, do you have some code that you can share right now ?
Sebastian Hesselbarth March 14, 2014, 11:36 a.m. UTC | #6
On 03/14/2014 12:32 PM, Alexandre Belloni wrote:
> On 14/03/2014 at 10:31:55 +0100, Sebastian Hesselbarth wrote :
>> On 03/13/2014 03:06 PM, Antoine Ténart wrote:
>>> +		local-timer@ad0600 {
>>> +			compatible = "arm,cortex-a9-twd-timer";
>>> +			reg = <0xad0600 0x20>;
>>> +			clocks = <&sysclk>;
>>
>> If I understand Jisheng correctly, this should be cpuclk/3. When
>> removing the clocks {} container above, please also take care of
>> it.
>>
>
> You understood correctly
>
>> You can do
>>
>> cpuclk: cpu-clock {
>> 	compatible = "fixed-clock";
>> 	#clock-cells = <0>;
>> 	clock-frequency = <1200000000>; /* <- put correct freq here */
>> };
>>
>> sysclk: system-clock {
>> 	compatible = "fixed-factor-clock";
>> 	#clock-cells = <0>;
>> 	clocks = <&cpuclk>;
>> 	clock-multi = <1>;
>> 	clock-div = <3>;
>> };
>>
>> Hopefully, we'll have proper clock drivers soon so we can just replace
>> referenced "fixed-*" clocks.
>>
>
> I'm working on that, do you have some code that you can share right now ?

No, nothing specific. You can have a look at drivers/clk/mvebu/ and see
how we deal with it on mvebu. For now, you can stick with bg2q and I'll
see how we extend that drivers for bg2/bg2cd then.

BTW: We really enjoy having a mvebu subfolder in drivers/clk as it makes
things easier. If you prepare any clk drivers for berlin, I'd suggest to
put them in drivers/clk/berlin from the beginning.

Sebastian
diff mbox

Patch

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
new file mode 100644
index 000000000000..1cb76031dfe6
--- /dev/null
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -0,0 +1,167 @@ 
+/*
+ * Copyright (C) 2014 Antoine Ténart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#include "skeleton.dtsi"
+
+/ {
+	model = "Marvell Armada 1500 pro (BG2-Q) SoC";
+	compatible = "marvell,berlin2q", "marvell,berlin";
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			next-level-cache = <&l2>;
+			reg = <0>;
+		};
+
+		cpu@1 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			next-level-cache = <&l2>;
+			reg = <1>;
+		};
+
+		cpu@2 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			next-level-cache = <&l2>;
+			reg = <2>;
+		};
+
+		cpu@3 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			next-level-cache = <&l2>;
+			reg = <3>;
+		};
+	};
+
+	clocks {
+		#address-cells = <0>;
+		#size-cells = <0>;
+
+		smclk: sysmgr-clock {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <25000000>;
+		};
+
+		sysclk: system-clock {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <400000000>;
+		};
+	};
+
+	soc {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		ranges = <0 0xf7000000 0x1000000>;
+		interrupt-parent = <&gic>;
+
+		l2: l2-cache-controller@ac0000 {
+			compatible = "arm,pl310-cache";
+			reg = <0xac0000 0x1000>;
+			cache-level = <2>;
+		};
+
+		local-timer@ad0600 {
+			compatible = "arm,cortex-a9-twd-timer";
+			reg = <0xad0600 0x20>;
+			clocks = <&sysclk>;
+			interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
+			status = "okay";
+		};
+
+		gic: interrupt-controller@ad1000 {
+			compatible = "arm,cortex-a9-gic";
+			reg = <0xad1000 0x1000>, <0xad0100 0x100>;
+			interrupt-controller;
+			#interrupt-cells = <3>;
+		};
+
+		apb@e80000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			ranges = <0 0xe80000 0x10000>;
+			interrupt-parent = <&aic>;
+
+			timer0: timer@2c00 {
+				compatible = "snps,dw-apb-timer";
+				reg = <0x2c00 0x14>;
+				interrupts = <8>;
+				clock-freq = <100000000>;
+				status = "okay";
+			};
+
+			timer1: timer@2c14 {
+				compatible = "snps,dw-apb-timer";
+				reg = <0x2c14 0x14>;
+				clock-freq = <100000000>;
+				status = "disabled";
+			};
+
+			aic: interrupt-controller@3800 {
+				compatible = "snps,dw-apb-ictl";
+				reg = <0x3800 0x30>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				interrupt-parent = <&gic>;
+				interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
+			};
+		};
+
+		apb@fc0000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			ranges = <0 0xfc0000 0x10000>;
+			interrupt-parent = <&sic>;
+
+			uart0: uart@9000 {
+				compatible = "snps,dw-apb-uart";
+				reg = <0x9000 0x100>;
+				interrupt-parent = <&sic>;
+				interrupts = <8>;
+				clock-frequency = <25000000>;
+				reg-shift = <2>;
+				status = "disabled";
+			};
+
+			uart1: uart@a000 {
+				compatible = "snps,dw-apb-uart";
+				reg = <0xa000 0x100>;
+				interrupt-parent = <&sic>;
+				interrupts = <9>;
+				clock-frequency = <25000000>;
+				reg-shift = <2>;
+				status = "disabled";
+			};
+
+			sic: interrupt-controller@e000 {
+				compatible = "snps,dw-apb-ictl";
+				reg = <0xe000 0x30>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				interrupt-parent = <&gic>;
+				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+			};
+		};
+	};
+};