On 03/01/2025 07:53, Krzysztof Kozlowski wrote:

On Thu, Jan 02, 2025 at 03:54:14PM +0000, Vincenzo Frascino wrote:

quoted

The Morello architecture is an experimental extension to Armv8.2-A,
which extends the AArch64 state with the principles proposed in
version 7 of the Capability Hardware Enhanced RISC Instructions
(CHERI) ISA.

The Morello Platform (soc) and the Fixed Virtual Platfom (fvp) share
some functionalities that have conveniently been included in
morello.dtsi to avoid duplication.

Introduce morello.dtsi.

Note: Morello fvp will be introduced with a future patch series.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/boot/dts/arm/morello.dtsi | 341 +++++++++++++++++++++++++++
 1 file changed, 341 insertions(+)
 create mode 100644 arch/arm64/boot/dts/arm/morello.dtsi

diff --git a/arch/arm64/boot/dts/arm/morello.dtsi b/arch/arm64/boot/dts/arm/morello.dtsi
new file mode 100644
index 000000000000..67bc960f4596
--- /dev/null
+++ b/arch/arm64/boot/dts/arm/morello.dtsi

@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * Copyright (c) 2020-2024, Arm Limited. All rights reserved.
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	interrupt-parent = <&gic>;
+
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	chosen { };

Drop

Fine, I will do it in the next iteration. The file you asked me to see
(sm8650.dtsi) has the same expression. Why this is incorrect? What's the rationale?

quoted

+
+	clocks {
+		soc_refclk50mhz: clock-50000000 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <50000000>;
+			clock-output-names = "apb_pclk";
+		};
+
+		soc_refclk85mhz: clock-85000000 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <85000000>;
+			clock-output-names = "iofpga:aclk";
+		};
+
+		soc_uartclk: clock-50000000-uart {

Keep nodes sorted by name.

I missed this one. I will reorder it in the next iteration.

quoted

+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <50000000>;
+			clock-output-names = "uartclk";
+		};
+
+		dpu_aclk: dpu-aclk {

Keep consistent naming. If others use "clock-" then why this has
different pattern?

Ok, I will change it.

quoted

+			/* 77.1 MHz derived from 24 MHz reference clock */
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <350000000>;
+			clock-output-names = "aclk";
+		};
+
+		dpu_pixel_clk: dpu-pixel-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <148500000>;
+			clock-output-names = "pxclk";
+		};
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu0: cpu0@0 {

cpu@0

Ok, will fix in the next iteration.

quoted

+			compatible = "arm,neoverse-n1";
+			reg = <0x0 0x0>;
+			device_type = "cpu";
+			enable-method = "psci";
+			clocks = <&scmi_dvfs 0>;
+		};
+
+		cpu1: cpu1@100 {

cpu@100

This applies to the entire patchset.

quoted

+			compatible = "arm,neoverse-n1";
+			reg = <0x0 0x100>;
+			device_type = "cpu";
+			enable-method = "psci";
+			clocks = <&scmi_dvfs 0>;
+		};
+
+		cpu2: cpu2@10000 {
+			compatible = "arm,neoverse-n1";
+			reg = <0x0 0x10000>;
+			device_type = "cpu";
+			enable-method = "psci";
+			clocks = <&scmi_dvfs 1>;
+		};
+
+		cpu3: cpu3@10100 {
+			compatible = "arm,neoverse-n1";
+			reg = <0x0 0x10100>;
+			device_type = "cpu";
+			enable-method = "psci";
+			clocks = <&scmi_dvfs 1>;
+		};

Missing cache nodes and properties.

All right, I will have a look.

quoted

+	};
+
+	firmware {
+		interrupt-parent = <&gic>;
+
+		scmi {
+			compatible = "arm,scmi";
+			mbox-names = "tx", "rx";
+			mboxes = <&mailbox 1 0>, <&mailbox 1 1>;
+			shmem = <&cpu_scp_hpri0>, <&cpu_scp_hpri1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			scmi_dvfs: protocol@13 {
+				reg = <0x13>;
+				#clock-cells = <1>;
+			};
+
+			scmi_clk: protocol@14 {
+				reg = <0x14>;
+				#clock-cells = <1>;
+			};
+		};
+	};
+
+	/* The first bank of memory, memory map is actually provided by UEFI. */
+	memory@80000000 {
+		device_type = "memory";
+		/* [0x80000000-0xffffffff] */
+		reg = <0x00000000 0x80000000 0x0 0x7F000000>;
+	};
+
+	memory@8080000000 {
+		device_type = "memory";
+		/* [0x8080000000-0x83f7ffffff] */
+		reg = <0x00000080 0x80000000 0x3 0x78000000>;
+	};
+
+	pmu {
+		compatible = "arm,armv8-pmuv3";
+		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		secure-firmware@ff000000 {
+			reg = <0 0xff000000 0 0x01000000>;
+			no-map;
+		};
+	};
+
+	spe-pmu {
+		compatible = "arm,statistical-profiling-extension-v1";
+		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	soc: soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		interrupt-parent = <&gic>;
+		ranges;
+
+		dp0: display@2cc00000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "arm,mali-d32", "arm,mali-d71";

I am pretty sure I asked for this: order properties according to DTS
coding style, not random.

You have, I missed this iteration.

quoted

+			reg = <0 0x2cc00000 0 0x20000>;
+			interrupts = <0 69 4>;
+			clocks = <&dpu_aclk>;
+			clock-names = "aclk";
+			iommus = <&smmu_dp 0>, <&smmu_dp 1>, <&smmu_dp 2>, <&smmu_dp 3>,
+				<&smmu_dp 8>;
+
+			pl0: pipeline@0 {
+				reg = <0>;
+				clocks = <&dpu_pixel_clk>;
+				clock-names = "pxclk";
+				port {
+					dp_pl0_out0: endpoint {
+						remote-endpoint = <&tda998x_0_input>;
+					};
+				};
+			};
+		};
+
+		i2c: i2c@1c0f0000 {
+			compatible = "cdns,i2c-r1p14";
+			reg = <0x0 0x1c0f0000 0x0 0x1000>;

And here order is correct... why each node is formatted differently?

I tried to format them all in the same way. But I clearly missed some iterations.
Maybe providing an automated tool (e.g. extend checkpatch.pl) would save time to
developers and reviewers.

quoted

+			#address-cells = <1>;
+			#size-cells = <0>;
+			clock-frequency = <100000>;

This is usually property of the board, unless this is somehow blurred
here.

Ok, I will move it around.

quoted

+			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&dpu_aclk>;
+
+			hdmi_tx: hdmi-transmitter@70 {
+				compatible = "nxp,tda998x";
+				reg = <0x70>;
+				video-ports = <0x234501>;
+				port {
+					tda998x_0_input: endpoint {
+						remote-endpoint = <&dp_pl0_out0>;
+					};
+				};
+			};
+		};
+
+		gic: interrupt-controller@2c010000 {
+			compatible = "arm,gic-v3";
+			#address-cells = <2>;
+			#interrupt-cells = <3>;
+			#size-cells = <2>;
+			ranges;

Missing reg.... and why do you have ranges here? No children.

I will remove it in the next iteration.

quoted

+			interrupt-controller;
+		};
+
+		smmu_dp: iommu@2ce00000 {
+			compatible = "arm,smmu-v3";
+			reg = <0 0x2ce00000 0 0x40000>;
+			interrupts = <GIC_SPI 76 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 80 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 78 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "eventq", "gerror", "cmdq-sync";
+			#iommu-cells = <1>;
+		};
+
+		smmu_ccix: iommu@4f000000 {
+			compatible = "arm,smmu-v3";
+			reg = <0 0x4f000000 0 0x40000>;

No hex here in reg but...

Fine.

quoted

+			interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 230 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 41 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "eventq", "gerror", "priq", "cmdq-sync";
+			msi-parent = <&its1 0>;
+			#iommu-cells = <1>;
+			dma-coherent;
+		};
+
+		smmu_pcie: iommu@4f400000 {
+			compatible = "arm,smmu-v3";
+			reg = <0 0x4f400000 0 0x40000>;
+			interrupts = <GIC_SPI 235 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 237 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 40 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 236 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "eventq", "gerror", "priq", "cmdq-sync";
+			msi-parent = <&its2 0>;
+			#iommu-cells = <1>;
+			dma-coherent;
+		};
+
+		mailbox: mhu@45000000 {
+			compatible = "arm,mhu-doorbell", "arm,primecell";
+			reg = <0x0 0x45000000 0x0 0x1000>;

here hex. Well, one more inconsistency in the same file. It's usually
hex everywhere in reg and ranges.

Fine.

quoted

+			interrupts = <GIC_SPI 318 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <2>;
+			clocks = <&soc_refclk50mhz>;
+			clock-names = "apb_pclk";
+		};
+
+		pcie_ctlr: pcie@28c0000000 {
+			compatible = "pci-host-ecam-generic";
+			device_type = "pci";
+			reg = <0x28 0xC0000000 0 0x10000000>;
+			ranges = <0x01000000 0x00 0x00000000 0x00 0x6F000000 0x00 0x00800000>,
+				 <0x02000000 0x00 0x60000000 0x00 0x60000000 0x00 0x0F000000>,
+				 <0x42000000 0x09 0x00000000 0x09 0x00000000 0x1F 0xC0000000>;

lowercase hex

Ok.

quoted

+			bus-range = <0 255>;
+			linux,pci-domain = <0>;
+			#address-cells = <3>;

Best regards,
Krzysztof

-- 
Regards,
Vincenzo