Re: [PATCH v2 1/2] arm64: dts: Add Arm corstone500 platform support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/12/2022 14:24, Emekcan Aras wrote:
> From: Emekcan Aras <Emekcan.Aras@arm.com>
> 

Use subject prefixes matching the subsystem (git log --oneline -- ...).
You already got such comment for v1... so do not ignore it but implement
instead.

> Corstone500[0] is a platform from arm, which includes Cortex-A cores and
> ideal starting point for feature rich System on Chip (SoC) designs
> based on the Cortex-A5 core.
> 
> These device trees contains the necessary bits to support the
> Corstone 500 FVP (Fixed Virtual Platform) and the
> FPGA MPS3 board.
> 
> 0: https://developer.arm.com/documentation/102262/0000
> 
> Signed-off-by: Emekcan Aras <emekcan.aras@arm.com>
> ---
>  arch/arm/boot/dts/corstone500.dts | 181 ++++++++++++++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 arch/arm/boot/dts/corstone500.dts

How do you test it? I do not see a way to compile it...

You need to include it in maintainers entry and CC relevant maintainers.

> 
> diff --git a/arch/arm/boot/dts/corstone500.dts b/arch/arm/boot/dts/corstone500.dts
> new file mode 100644
> index 000000000000..976aa333ffbc
> --- /dev/null
> +++ b/arch/arm/boot/dts/corstone500.dts
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +/*
> + * Copyright (c) 2022, Arm Limited. All rights reserved.
> + *
> + */
> +
> +

Just one blank line

> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	model = "ARM Corstone500";
> +	compatible = "arm,corstone500";

No DTSI? No board or platform compatible? How is it going to be used by
anyone else?

> +	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0", "arm,psci-0.2", "arm,psci";
> +		method = "smc";
> +		cpu_on = <0x84000003>;
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		enable-method = "psci";
> +
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a5";
> +			reg = <0>;
> +			next-level-cache = <&L2>;
> +		};
> +
> +		cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a5";
> +			reg = <1>;
> +			next-level-cache = <&L2>;
> +		};
> +
> +		cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a5";
> +			reg = <2>;
> +			next-level-cache = <&L2>;
> +		};
> +
> +		cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a5";
> +			reg = <3>;
> +			next-level-cache = <&L2>;
> +		};
> +	};
> +
> +	memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0x7f000000>;
> +	};
> +
> +	L2: cache-controller@1c010000 {
> +		compatible = "arm,pl310-cache";
> +		reg = <0x1c010000 0x1000>;
> +		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> +		cache-level = <2>;
> +		cache-unified;
> +		arm,data-latency = <1 1 1>;
> +		arm,tag-latency = <1 1 1>;
> +	};
> +
> +	refclk7500khz: refclk7500khz {

Node names should be generic, so at least generic prefix or suffix
(clock-, -clock).
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <7500000>;
> +		clock-output-names = "apb_pclk";
> +	};
> +
> +	refclk24mhz: refclk24mhz {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "apb_pclk";
> +	};
> +
> +	smbclk: refclk24mhzx2 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <48000000>;
> +		clock-output-names = "smclk";
> +	};
> +
> +	rtc@1a220000 {
> +		compatible = "arm,pl031", "arm,primecell";
> +		reg = <0x1a220000 0x1000>;
> +		clocks = <&refclk24mhz>;
> +		interrupts = <GIC_SPI 6 (GIC_CPU_MASK_SIMPLE(4) |
> +			IRQ_TYPE_LEVEL_HIGH)>;
> +		clock-names = "apb_pclk";
> +	};
> +
> +	gic: interrupt-controller@1c001000 {
> +		compatible = "arm,cortex-a5-gic";
> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg =	<0x1c001000 0x1000>,
> +			<0x1c000100 0x100>;
> +		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) |
> +			IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	soc{
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		clock_frequency = <50000000>;
> +		interrupt-parent = <&gic>;
> +		ranges;
> +
> +		uart0: serial@1a200000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x1a200000 0x1000>;
> +			interrupts = <GIC_SPI 8 (GIC_CPU_MASK_SIMPLE(4) |
> +				IRQ_TYPE_LEVEL_HIGH)>;
> +			clocks = <&refclk7500khz>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		uart1: serial@1a210000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x1a210000 0x1000>;
> +			interrupts = <GIC_SPI 9 (GIC_CPU_MASK_SIMPLE(4) |
> +				IRQ_TYPE_LEVEL_HIGH)>;
> +			clocks = <&refclk7500khz>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		timer0: timer@1a040000 {
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0x1a040000 0x1000>;
> +			clock-frequency = <7500000>;
> +
> +			frame@1a050000 {
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 2 (GIC_CPU_MASK_SIMPLE(4) |
> +				IRQ_TYPE_LEVEL_HIGH)>;
> +				reg = <0x1a050000 0x1000>;
> +			};
> +		};
> +
> +		smsc: ethernet@4020000 {
> +			compatible = "smsc,lan9220", "smsc,lan9115";
> +			reg = <0x40200000 0x10000>;
> +			interrupts = <GIC_SPI 43 (GIC_CPU_MASK_SIMPLE(4) |
> +				IRQ_TYPE_LEVEL_HIGH)>;
> +			reg-io-width = <4>;
> +			phy-mode = "mii";
> +			smsc,irq-active-high;
> +			vdd33a-supply = <&v2m_fixed_3v3>;
> +			vddvario-supply = <&v2m_fixed_3v3>;
> +		};
> +	};
> +
> +	v2m_fixed_3v3: fixed-regulator-0 {

You have weird ordering of nodes. If it is by name, then this should be
regulator-0. Anyway drop "fixed" prefix as it is not relevant.


Best regards,
Krzysztof