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

From: Andreas FÃrber
Date: Thu Jun 11 2020 - 09:39:38 EST

Next message: Charan Teja Kalla: "[PATCH] dmabuf: use spinlock to access dmabuf->name"
Previous message: Colin King: "[PATCH] drm/bridge: make array frs_limits static, makes object smaller"
In reply to: Marc Zyngier: "Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi"
Next in thread: Daniel Palmer: "Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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)

Next message: Charan Teja Kalla: "[PATCH] dmabuf: use spinlock to access dmabuf->name"
Previous message: Colin King: "[PATCH] drm/bridge: make array frs_limits static, makes object smaller"
In reply to: Marc Zyngier: "Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi"
Next in thread: Daniel Palmer: "Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]