Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Am 10.06.20 um 11:04 schrieb Daniel Palmer:
Adds initial dtsi for the base MStar ARMv7 SoCs, family dtsis for infinity
and mercury families, and then some chip level dtsis for chips in those
families.

Signed-off-by: Daniel Palmer <daniel@xxxxxxxx>
---
  MAINTAINERS                              |  3 +
  arch/arm/boot/dts/infinity-msc313.dtsi   | 14 +++++
  arch/arm/boot/dts/infinity.dtsi          | 10 ++++
  arch/arm/boot/dts/infinity3-msc313e.dtsi | 14 +++++
  arch/arm/boot/dts/infinity3.dtsi         | 10 ++++
  arch/arm/boot/dts/mercury5-ssc8336n.dtsi | 14 +++++
  arch/arm/boot/dts/mercury5.dtsi          | 10 ++++
  arch/arm/boot/dts/mstar-v7.dtsi          | 71 ++++++++++++++++++++++++
  8 files changed, 146 insertions(+)

Can you split this up into three parts for easier review?

  create mode 100644 arch/arm/boot/dts/infinity-msc313.dtsi
  create mode 100644 arch/arm/boot/dts/infinity.dtsi
  create mode 100644 arch/arm/boot/dts/infinity3-msc313e.dtsi
  create mode 100644 arch/arm/boot/dts/infinity3.dtsi
  create mode 100644 arch/arm/boot/dts/mercury5-ssc8336n.dtsi
  create mode 100644 arch/arm/boot/dts/mercury5.dtsi
  create mode 100644 arch/arm/boot/dts/mstar-v7.dtsi

diff --git a/MAINTAINERS b/MAINTAINERS
index 754521938303..839ae0250d3d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2114,6 +2114,9 @@ ARM/MStar/Sigmastar ARMv7 SoC support
  M:	Daniel Palmer <daniel@xxxxxxxxx>
  L:	linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
  S:	Maintained
+F:	arch/arm/boot/dts/infinity*.dtsi
+F:	arch/arm/boot/dts/mercury*.dtsi
+F:	arch/arm/boot/dts/mstar-v7.dtsi

Sort order wrt D.

  F:	arch/arm/mach-mstar/
  F:	Documentation/devicetree/bindings/arm/mstar.yaml
diff --git a/arch/arm/boot/dts/infinity-msc313.dtsi b/arch/arm/boot/dts/infinity-msc313.dtsi
new file mode 100644
index 000000000000..4eb522e6a75d
--- /dev/null
+++ b/arch/arm/boot/dts/infinity-msc313.dtsi
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0

DTs as hardware description should be dual-licensed as either MIT or BSD-2-Clause, similar to the schema.

Also, elsewhere, for any code that might get reused for OpenOCD (e.g., clk drivers and low-level init like machine - 2/5) or other non-kernel projects potentially incompatible with GPL-2.0-only, it would be useful to use the -or-later version of the GPL for code sharing - if the sources you used permit that.

+/*
+ * Copyright (c) 2019 thingy.jp.

2019-2020? Check elsewhere.

+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "infinity.dtsi"
+
+/ {
+	memory {
+		device_type = "memory";
+		reg = <0x20000000 0x4000000>;

The memory node needs to become memory@20000000 then.

+	};

I take it, this RAM is integrated? Maybe add some explanation of what this file is

+};
diff --git a/arch/arm/boot/dts/infinity.dtsi b/arch/arm/boot/dts/infinity.dtsi
new file mode 100644
index 000000000000..25d379028689
--- /dev/null
+++ b/arch/arm/boot/dts/infinity.dtsi
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "mstar-v7.dtsi"
+
+/ {
+};

What do you intend to add here? Is it really needed? (same below)

Pre-DT-Schema I used to add a compatible property in the .dtsi, to make sure we have at least the SoC's, in case someone neglects to add one in their board's .dts. With DT schema that's no longer valid (if enum/const is required), but Linux would still work better with than without.

diff --git a/arch/arm/boot/dts/infinity3-msc313e.dtsi b/arch/arm/boot/dts/infinity3-msc313e.dtsi
new file mode 100644
index 000000000000..d0c53153faad
--- /dev/null
+++ b/arch/arm/boot/dts/infinity3-msc313e.dtsi
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "infinity3.dtsi"
+
+/ {
+	memory {

Ditto, unit address missing.

+		device_type = "memory";
+		reg = <0x20000000 0x4000000>;
+	};
+};
diff --git a/arch/arm/boot/dts/infinity3.dtsi b/arch/arm/boot/dts/infinity3.dtsi
new file mode 100644
index 000000000000..cf5f18a07835
--- /dev/null
+++ b/arch/arm/boot/dts/infinity3.dtsi
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "infinity.dtsi"
+
+/ {
+};

Don't you anticipate incompatibilities between infinity and infinity3, i.e., things you don't want to inherit? Seems a bit optimistic. You can of course overwrite properties, but deleting is more difficult.

diff --git a/arch/arm/boot/dts/mercury5-ssc8336n.dtsi b/arch/arm/boot/dts/mercury5-ssc8336n.dtsi
new file mode 100644
index 000000000000..7513f903c838
--- /dev/null
+++ b/arch/arm/boot/dts/mercury5-ssc8336n.dtsi
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "mercury5.dtsi"
+
+/ {
+	memory {

Unit address.

+		device_type = "memory";
+		reg = <0x20000000 0x4000000>;
+	};
+};
diff --git a/arch/arm/boot/dts/mercury5.dtsi b/arch/arm/boot/dts/mercury5.dtsi
new file mode 100644
index 000000000000..25d379028689
--- /dev/null
+++ b/arch/arm/boot/dts/mercury5.dtsi
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include "mstar-v7.dtsi"
+
+/ {
+};
diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
new file mode 100644
index 000000000000..0fccc4ca52a4
--- /dev/null
+++ b/arch/arm/boot/dts/mstar-v7.dtsi

So this is the only file starting with mstar. Have you thought about prefixing infinity/mercury, so that they're grouped together?

@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 thingy.jp.
+ * Author: Daniel Palmer <daniel@xxxxxxxxx>
+ */
+
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	#address-cells = <1>;
+ #size-cells = <1>;> + interrupt-parent = <&gic>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x0>;
+		};
+	};
+
+	arch_timer {
+		compatible = "arm,armv7-timer";
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2)
+				| IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2)
+				| IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2)
+				| IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2)
+				| IRQ_TYPE_LEVEL_LOW)>;
+		clock-frequency = <6000000>;
+	};
+
+	pmu {
+		compatible = "arm,cortex-a7-pmu";
+		interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;

Lacking interrupt-affinity.

"This property should be present when there is more than a single SPI."

To deal with single- vs. dual-core models, you should give the pmu node a label, e.g., arm_pmu used elsewhere, I think. Depending on your preferences you could set it here in common code (less work) or in the SoC-specific .dtsi where you know the number of CPUs for sure (safer to not forget later).

+	};
+
+	soc: soc {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;

I had been instructed not to use full identity ranges but to exclude RAM ranges for safety reasons.

+
+		gic: interrupt-controller@16001000 {
+			compatible = "arm,cortex-a7-gic";
+			#interrupt-cells = <3>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			interrupt-controller;
+			reg = <0x16001000 0x1000>,
+			      <0x16002000 0x1000>;

In addition to Marc's comments, please reorder reg to below compatible for consistency. Suggest to also reorder interrupt-controller before the -cells properties, after reg.

+		};
+
+		pm_uart: uart@1f221000 {
+			compatible = "ns16550a";
+			reg = <0x1f221000 0x100>;
+			reg-shift = <3>;
+			clock-frequency = <172000000>;
+			status = "disabled";
+		};

If you have any decent manuals for these SoCs, I suggest to check whether there are any internal buses that you may want to model as simple-bus for grouping. In-tree examples include meson and recently merged rtd1195 - it affects the reg addresses and unit addresses via suitable ranges mappings.

+	};
+};

Regards,
Andreas

--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux