Re: [PATCH v2 3/4] arm64: dts: morello: Add support for soc dts

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/12/2024 17:20, Vincenzo Frascino wrote:
> 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.
> 
> Introduce Morello SoC dts.
> 
> Note: Morello is at the same time the name of an Architecture [1], an SoC
> [2] and a Board [2].
> To distinguish in between Architecture/SoC and Board we refer to the first
> as arm,morello and to the second as arm,morello-sdp.
> 
> [1] https://developer.arm.com/Architectures/Morello
> [2] https://www.morello-project.org/

Drop both above paragraphs, you already said this in previous commit.
This still does not help me (See further questions).
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under the
--- separator.

Or just use b4 and none of these are needed.

> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/boot/dts/arm/Makefile        |   1 +
>  arch/arm64/boot/dts/arm/morello-sdp.dts | 116 ++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/arm/morello-sdp.dts
> 
> diff --git a/arch/arm64/boot/dts/arm/Makefile b/arch/arm64/boot/dts/arm/Makefile
> index d908e96d7ddc..869667bef7c0 100644
> --- a/arch/arm64/boot/dts/arm/Makefile
> +++ b/arch/arm64/boot/dts/arm/Makefile
> @@ -7,3 +7,4 @@ dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb
>  dtb-$(CONFIG_ARCH_VEXPRESS) += vexpress-v2f-1xv7-ca53x2.dtb
>  dtb-$(CONFIG_ARCH_VEXPRESS) += fvp-base-revc.dtb
>  dtb-$(CONFIG_ARCH_VEXPRESS) += corstone1000-fvp.dtb corstone1000-mps3.dtb
> +dtb-$(CONFIG_ARCH_VEXPRESS) += morello-sdp.dtb
> diff --git a/arch/arm64/boot/dts/arm/morello-sdp.dts b/arch/arm64/boot/dts/arm/morello-sdp.dts
> new file mode 100644
> index 000000000000..143e644361e4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/arm/morello-sdp.dts
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +/*
> + * Copyright (c) 2021-2024, Arm Limited. All rights reserved.
> +
> + */
> +
> +/dts-v1/;
> +#include "morello.dtsi"
> +
> +/ {
> +	model = "Arm Morello System Development Platform";
> +	compatible = "arm,morello-sdp";
> +
> +	smmu_dp: iommu@2ce00000 {

Well, this is confusing. Boards are almost never adding things to the
soc node, where this belongs to.

Your commit msg should explain this.

> +		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>;
> +	};
> +
> +	dp0: display@2cc00000 {

display/GPU is outside of SoC? Really? Please explain the hardware in
the commit msg.

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "arm,mali-d32", "arm,mali-d71";
> +		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@1c0f0000 {
> +		compatible = "cdns,i2c-r1p14";

I really doubt that you can package cdns IP block outside of SoC.

> +		reg = <0x0 0x1c0f0000 0x0 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clock-frequency = <100000>;
> +		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>;
> +				};
> +			};
> +		};
> +	};
> +
> +	dpu_aclk: dpu_aclk {

Use consistent names

> +		/* 77.1 MHz derived from 24 MHz reference clock */

77.1 or 35?

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <350000000>;
> +		clock-output-names = "aclk";

aclk? Sounds like something belonging to the clock controller.

> +	};
> +
> +	dpu_pixel_clk: dpu-pixel-clk {

Same comment

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <148500000>;
> +		clock-output-names = "pxclk";
> +	};
> +};
> +
> +&gic {
> +	reg = <0x0 0x30000000 0 0x10000>,	/* GICD */
> +	      <0x0 0x300c0000 0 0x80000>;	/* GICR */
> +	interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +
> +	its1: msi-controller@30040000 {
> +		compatible = "arm,gic-v3-its";
> +		msi-controller;
> +		#msi-cells = <1>;
> +		reg = <0x0 0x30040000 0x0 0x20000>;
> +	};
> +
> +	its2: msi-controller@30060000 {
> +		compatible = "arm,gic-v3-its";
> +		msi-controller;
> +		#msi-cells = <1>;
> +		reg = <0x0 0x30060000 0x0 0x20000>;
> +	};
> +
> +	its_ccix: msi-controller@30080000 {
> +		compatible = "arm,gic-v3-its";
> +		msi-controller;
> +		#msi-cells = <1>;
> +		reg = <0x0 0x30080000 0x0 0x20000>;
> +	};
> +
> +	its_pcie: msi-controller@300a0000 {
> +		compatible = "arm,gic-v3-its";
> +		msi-controller;
> +		#msi-cells = <1>;
> +		reg = <0x0 0x300a0000 0x0 0x20000>;
> +	};
> +};


Best regards,
Krzysztof