diff mbox

[RESEND,3/5] ARM: BCM63XX: add BCM63138 minimal Device Tree

Message ID 1398130758-19456-4-git-send-email-f.fainelli@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Florian Fainelli April 22, 2014, 1:39 a.m. UTC
Add a very minimalistic BCM63138 Device Tree include file which
describes the BCM63138 SoC with only the basic set of required
peripherals:

- Cortex A9 CPU
- ARM GIC
- PL310 Level-2 cache controller
- ARM TWD & Global timers
- ARM TWD watchdog
- legacy MIPS bus (UBUS)

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/boot/dts/bcm63138.dtsi | 109 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)
 create mode 100644 arch/arm/boot/dts/bcm63138.dtsi

Comments

Arnd Bergmann April 22, 2014, 10:52 a.m. UTC | #1
On Monday 21 April 2014 18:39:16 Florian Fainelli wrote:
> 
> +#include "skeleton.dtsi"
> +
> +/ {
> +       compatible = "brcm,bcm63138";
> +       model = "Broadcom BCM63138 DSL SoC";
> +       interrupt-parent = <&gic>;
> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a9";
> +                       next-level-cache = <&L2>;
> +                       reg = <0>;
> +               };
> +       };

Even if you don't support SMP yet, I can see no reason not
to list both CPUs here. The binding is known and the code
should ignore the extra cores if it doesn't know how to
turn them on.

> +
> +       /* ARM bus */
> +       axi@80000000 {
> +               compatible = "simple-bus";
> +               ranges = <0 0x80000000 0x783003>;
> +               reg = <0x80000000 0x783003>;

The length seems odd, I would expect that the bus actually
translates all addresses in the 0x80000000 range to downstream
devices even if there is nothing connected. Just round it up
to your best knowledge.

I would also drop the 'reg' property. Since you don't have a
specific "compatible" value, there is no way to use the registers
in this node.

> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               L2: cache-controller@1d000 {
> +                       compatible = "arm,pl310-cache";
> +                       reg = <0x1d000 0x1000>;
> +                       cache-unified;
> +                       cache-level = <2>;
> +                       interrupts = <GIC_PPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +               };
> +
> +               mpcore@1e000 {
> +                       compatible = "simple-bus";
> +                       reg = <0x1e000 0x20000>;
> +                       ranges = <0 0x1e000 0x20000>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;


Same thing here.

Also, can you explain why there is a separate 'mpcore' bus, and why the
cache controller is not part of that?

Do you have reason to believe that this is how the hardware actually
looks?

> +
> +       /* Legacy UBUS base */
> +       ubus@fffe8000 {
> +               compatible = "simple-bus";
> +               reg = <0xfffe8000 0x8053>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0 0xfffe8000 0x8053>;
> +       };
> +};


Again, use a proper 'length' here and remove the 'reg' property.

	Arnd
Florian Fainelli May 2, 2014, 5:37 a.m. UTC | #2
2014-04-22 3:52 GMT-07:00 Arnd Bergmann <arnd@arndb.de>:
> On Monday 21 April 2014 18:39:16 Florian Fainelli wrote:
>>
>> +#include "skeleton.dtsi"
>> +
>> +/ {
>> +       compatible = "brcm,bcm63138";
>> +       model = "Broadcom BCM63138 DSL SoC";
>> +       interrupt-parent = <&gic>;
>> +
>> +       cpus {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               cpu@0 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a9";
>> +                       next-level-cache = <&L2>;
>> +                       reg = <0>;
>> +               };
>> +       };
>
> Even if you don't support SMP yet, I can see no reason not
> to list both CPUs here. The binding is known and the code
> should ignore the extra cores if it doesn't know how to
> turn them on.

OK.

>
>> +
>> +       /* ARM bus */
>> +       axi@80000000 {
>> +               compatible = "simple-bus";
>> +               ranges = <0 0x80000000 0x783003>;
>> +               reg = <0x80000000 0x783003>;
>
> The length seems odd, I would expect that the bus actually
> translates all addresses in the 0x80000000 range to downstream
> devices even if there is nothing connected. Just round it up
> to your best knowledge.
>
> I would also drop the 'reg' property. Since you don't have a
> specific "compatible" value, there is no way to use the registers
> in this node.

OK.

>
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +
>> +               L2: cache-controller@1d000 {
>> +                       compatible = "arm,pl310-cache";
>> +                       reg = <0x1d000 0x1000>;
>> +                       cache-unified;
>> +                       cache-level = <2>;
>> +                       interrupts = <GIC_PPI 0 IRQ_TYPE_LEVEL_HIGH>;
>> +               };
>> +
>> +               mpcore@1e000 {
>> +                       compatible = "simple-bus";
>> +                       reg = <0x1e000 0x20000>;
>> +                       ranges = <0 0x1e000 0x20000>;
>> +                       #address-cells = <1>;
>> +                       #size-cells = <1>;
>
>
> Same thing here.
>
> Also, can you explain why there is a separate 'mpcore' bus, and why the
> cache controller is not part of that?
>
> Do you have reason to believe that this is how the hardware actually
> looks?

I think I was just mistaken by how the register space looks like and
is named, but there probably is not a separate underlying bus, so I
will move this up one level as it should.

>
>> +
>> +       /* Legacy UBUS base */
>> +       ubus@fffe8000 {
>> +               compatible = "simple-bus";
>> +               reg = <0xfffe8000 0x8053>;
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges = <0 0xfffe8000 0x8053>;
>> +       };
>> +};
>
>
> Again, use a proper 'length' here and remove the 'reg' property.


>
>         Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

diff --git a/arch/arm/boot/dts/bcm63138.dtsi b/arch/arm/boot/dts/bcm63138.dtsi
new file mode 100644
index 000000000000..190d6e53a85a
--- /dev/null
+++ b/arch/arm/boot/dts/bcm63138.dtsi
@@ -0,0 +1,109 @@ 
+/*
+ * Broadcom BCM63138 DSL SoCs Device Tree
+ *
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * Licensed under the GNU/GPL. See COPYING for details
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+#include "skeleton.dtsi"
+
+/ {
+	compatible = "brcm,bcm63138";
+	model = "Broadcom BCM63138 DSL SoC";
+	interrupt-parent = <&gic>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a9";
+			next-level-cache = <&L2>;
+			reg = <0>;
+		};
+	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		arm_timer_clk: arm_timer_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <500000000>;
+		};
+	};
+
+	/* ARM bus */
+	axi@80000000 {
+		compatible = "simple-bus";
+		ranges = <0 0x80000000 0x783003>;
+		reg = <0x80000000 0x783003>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		L2: cache-controller@1d000 {
+			compatible = "arm,pl310-cache";
+			reg = <0x1d000 0x1000>;
+			cache-unified;
+			cache-level = <2>;
+			interrupts = <GIC_PPI 0 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		mpcore@1e000 {
+			compatible = "simple-bus";
+			reg = <0x1e000 0x20000>;
+			ranges = <0 0x1e000 0x20000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			scu: scu@0 {
+				compatible = "arm,cortex-a9-scu";
+				reg = <0x0 0x100>;
+			};
+
+			gic: interrupt-controller@100 {
+				compatible = "arm,cortex-a9-gic";
+				reg = <0x1000 0x1000
+					0x100 0x100>;
+				#interrupt-cells = <3>;
+				#address-cells = <0>;
+				interrupt-controller;
+			};
+
+			global_timer: timer@200 {
+				compatible = "arm,cortex-a9-global-timer";
+				reg = <0x200 0x20>;
+				interrupts = <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&arm_timer_clk>;
+			};
+
+			local_timer: local-timer@600 {
+				compatible = "arm,cortex-a9-twd-timer";
+				reg = <0x600 0x20>;
+				interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&arm_timer_clk>;
+			};
+
+			twd_watchdog: watchdog@620 {
+				compatible = "arm,cortex-a9-twd-wdt";
+				reg = <0x620 0x20>;
+				interupts = <GIC_PPI 14 IRQ_TYPE_LEVEL_HIGH>;
+			};
+		};
+	};
+
+	/* Legacy UBUS base */
+	ubus@fffe8000 {
+		compatible = "simple-bus";
+		reg = <0xfffe8000 0x8053>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0xfffe8000 0x8053>;
+	};
+};