Re: [PATCH 2/2] arm64: dts: zena: Add support for Zena CSS

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/01/2026 18:37, Debbie Horsfall wrote:
> Introduce the Zena CSS Fixed Virtual Platform (FVP) dts. This is
> currently the only Zena CSS variant, however the common definitions are
> included in a common dtsi for extensibility.
> 
> Signed-off-by: Debbie Horsfall <debbie.horsfall@arm.com>
> ---
>  MAINTAINERS                              |   1 +
>  arch/arm64/boot/dts/arm/Makefile         |   1 +
>  arch/arm64/boot/dts/arm/zena-css-fvp.dts |  55 ++
>  arch/arm64/boot/dts/arm/zena-css.dtsi    | 826 +++++++++++++++++++++++++++++++
>  4 files changed, 883 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90d88137adf1..d1d2dae6a71e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3727,6 +3727,7 @@ ARM/ZENA CSS PLATFORM
>  M:	Debbie Horsfall <debbie.horsfall@arm.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/arm/arm,zena-css.yaml
> +F:	arch/arm64/boot/dts/arm/zena-css*

Eeeh, this is getting more and more messier.

All ARM designs or at least all similar like all ARM FVP should have one
group maintainers and that entry now claims "versatile express".

Additional entries for submaintainers is fine, but honestly with this
split of bindings this is getting more and more messier.

I know that ARM is kind of "special" but when it comes to SoCs it should
not be.

Sort out this mess, please, before get accept another platform.

...


> +		cpu-map {
> +

No opening blank lines.

> +			cluster0 {
> +
> +				core0 {
> +					cpu = <&CPU0>;

Labels are lowercase. See DTS coding style.


> +				};
> +
> +				core1 {
> +					cpu = <&CPU1>;
> +				};
> +
> +				core2 {
> +					cpu = <&CPU2>;
> +				};
> +
> +				core3 {
> +					cpu = <&CPU3>;
> +				};
> +			};
> +

...

> +	memory@80000000 {
> +		device_type = "memory";
> +
> +		/* Bank 0: start = 0x0000_0000_8000_0000, size = ~2 GiB (0x7F00_0000) */
> +		reg = <
> +			0x00000000  0x80000000  0x00000000  0x7F000000

Lowercase hex. Also,
> +			0x00000200  0x00000000  0x00000000  0x80000000
> +		>;
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> +			<GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> +			<GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> +			<GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>,
> +			<GIC_PPI 12 IRQ_TYPE_LEVEL_LOW>;
> +	};
> +
> +	soc_clk24mhz: clock-24000000 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "refclk24mhz";
> +	};
> +
> +	soc: soc {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		timer@1a810000 {
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0x0 0x1a810000 0 0x10000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			/* Map child space [0x0..0x30000) to parent @ 0x1a810000 */
> +			ranges = <0x0 0x0 0x1a810000 0x00030000>;
> +
> +			frame@20000 {
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0x20000 0x10000>;
> +			};
> +		};
> +
> +		gic: interrupt-controller@20800000 {
> +			compatible = "arm,gic-v3";
> +			#redistributor-regions = <16>;
> +			reg = <0x0 0x20800000 0x0 0x10000>,    /* GICD */
> +				<0x0 0x20880000 0x0 0x40000>,    /* 16 * GICR */
> +				<0x0 0x208c0000 0x0 0x40000>,
> +				<0x0 0x20900000 0x0 0x40000>,
> +				<0x0 0x20940000 0x0 0x40000>,
> +				<0x0 0x20980000 0x0 0x40000>,
> +				<0x0 0x209c0000 0x0 0x40000>,
> +				<0x0 0x20a00000 0x0 0x40000>,
> +				<0x0 0x20a40000 0x0 0x40000>,
> +				<0x0 0x20a80000 0x0 0x40000>,
> +				<0x0 0x20ac0000 0x0 0x40000>,
> +				<0x0 0x20b00000 0x0 0x40000>,
> +				<0x0 0x20b40000 0x0 0x40000>,
> +				<0x0 0x20b80000 0x0 0x40000>,
> +				<0x0 0x20bc0000 0x0 0x40000>,
> +				<0x0 0x20c00000 0x0 0x40000>,
> +				<0x0 0x20c40000 0x0 0x40000>;
> +			#interrupt-cells = <3>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			interrupt-controller;
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			its1: msi-controller@20840000 {
> +				compatible = "arm,gic-v3-its";
> +				reg = <0x0 0x20840000 0x0 0x40000>;
> +				msi-controller;
> +				#msi-cells = <1>;
> +			};
> +		};
> +
> +		/* UART is fixed as 24MHz, both UARTCLK and PCLK */
> +		soc_serial0: serial@1a400000 {

That's some messed ordering. What sort of ordering rule is followed by
ALL ARM SoCs? Not standard DTS coding style? If not, which one and why?

> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x0 0x1a400000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&soc_clk24mhz>, <&soc_clk24mhz>;
> +			clock-names = "uartclk", "apb_pclk";
> +		};
> +
> +		watchdog@1a420000 {
> +			compatible = "arm,sbsa-gwdt";
> +			reg = <0x0 0x1a420000 0x0 0x10000>,
> +			      <0x0 0x1a430000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		rtc@300d0000 {
> +			compatible = "arm,pl031", "arm,primecell";
> +			reg = <0x0 0x300d0000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&soc_clk24mhz>;
> +			clock-names = "apb_pclk";
> +		};
> +

No trailing blank lines either. Please clean up your code so it looks
intentional and well organized.


> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0", "arm,psci-0.2", "arm,psci";
> +		method = "smc";
> +		cpu_suspend = <0xc4000001>;
> +		cpu_off = <0x84000002>;
> +		cpu_on = <0xc4000003>;
> +	};
> +
> +	sram: sram@104000 {
> +		compatible = "mmio-sram";
> +		reg = <0x0 0x104000 0x0 0x00001000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x0 0x104000 0x00001000>;
> +
> +		scmi_shmem_tx: scpshmem-sram-section@0 {
> +			compatible = "arm,scmi-shmem";
> +			reg = <0x0 0x100>;
> +		};
> +		scmi_shmem_rx: scpshmem-sram-section@100 {
> +			compatible = "arm,scmi-shmem";
> +			reg = <0x100 0x100>;
> +		};
> +	};
> +
> +	mbox_db_tx: mailbox@40020000 {

MMIO nodes are under soc.


Best regards,
Krzysztof