Re: [PATCH v4 08/10] arm64: dts: exynos: Add initial support for exynos8895 SoC

[Ivaylo Ivanov]

On 9/16/24 17:23, Krzysztof Kozlowski wrote:
> On Mon, Sep 09, 2024 at 02:00:15PM +0300, Ivaylo Ivanov wrote:
>
>> +&pinctrl_peric0 {
>> +	gpd0: gpd0-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpd1: gpd1-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpd2: gpd2-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpd3: gpd3-gpio-bank {
> These should be ordered by node name. Same in peric1.
Alright.
>
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpb1: gpb1-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpe7: gpe7-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>> +
>> +	gpf1: gpf1-gpio-bank {
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +	};
>
>> diff --git a/arch/arm64/boot/dts/exynos/exynos8895.dtsi b/arch/arm64/boot/dts/exynos/exynos8895.dtsi
>> new file mode 100644
>> index 000000000..59af33420
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/exynos/exynos8895.dtsi
>> @@ -0,0 +1,249 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>> +/*
>> + * Samsung's Exynos 8895 SoC device tree source
>> + *
>> + * Copyright (c) 2024, Ivaylo Ivanov <ivo.ivanov.ivanov1@xxxxxxxxx>
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> +	compatible = "samsung,exynos8895";
>> +	#address-cells = <2>;
>> +	#size-cells = <1>;
>> +
>> +	interrupt-parent = <&gic>;
>> +
>> +	aliases {
>> +		pinctrl0 = &pinctrl_alive;
>> +		pinctrl1 = &pinctrl_abox;
>> +		pinctrl2 = &pinctrl_vts;
>> +		pinctrl3 = &pinctrl_fsys0;
>> +		pinctrl4 = &pinctrl_fsys1;
>> +		pinctrl5 = &pinctrl_busc;
>> +		pinctrl6 = &pinctrl_peric0;
>> +		pinctrl7 = &pinctrl_peric1;
>> +	};
>> +
>> +	arm-a53-pmu {
>> +		compatible = "arm,cortex-a53-pmu";
>> +		interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-affinity = <&cpu0>,
>> +				     <&cpu1>,
>> +				     <&cpu2>,
>> +				     <&cpu3>;
>> +	};
>> +
>> +	/* There's no PMU model for the Mongoose cores */
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		cpu-map {
>> +			cluster0 {
>> +				core0 {
>> +					cpu = <&cpu0>;
>> +				};
>> +				core1 {
>> +					cpu = <&cpu1>;
>> +				};
>> +				core2 {
>> +					cpu = <&cpu2>;
>> +				};
>> +				core3 {
>> +					cpu = <&cpu3>;
>> +				};
>> +			};
>> +
>> +			cluster1 {
>> +				core0 {
>> +					cpu = <&cpu4>;
>> +				};
>> +				core1 {
>> +					cpu = <&cpu5>;
>> +				};
>> +				core2 {
>> +					cpu = <&cpu6>;
>> +				};
>> +				core3 {
>> +					cpu = <&cpu7>;
>> +				};
>> +			};
>> +		};
>> +
>> +		cpu0: cpu@100 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x100>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu1: cpu@101 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x101>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu2: cpu@102 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x102>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu3: cpu@103 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x103>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu4: cpu@0 {
> Why cpu@0 is cpu4 not cpu0? Anyway, these should be ordered by unit
> address.

cpu@100 is the boot core of the first cluster consisting of cortex-a53

cores, hence why it's labelled as cpu0. The second cluster contains

the Mongoose cores, labelled and ordered after the first cluster.


It's ordered like so on a lot of SoCs for sanity's sake, hence why I

believe it should stay like that.


If you still think that they must be ordered by unit address, please

explicitly let me know so that I include that change in the v5.


Regards, Ivo.

>
>> +
>> +	oscclk: osc-clock {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-output-names = "oscclk";
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci";
>> +		method = "smc";
>> +		cpu_suspend = <0xc4000001>;
>> +		cpu_off = <0x84000002>;
>> +		cpu_on = <0xc4000003>;
>> +	};
>> +
>> +	timer {
> Keep order by names, timer goes after soc.
>
>> +		compatible = "arm,armv8-timer";
>> +		/* Hypervisor Virtual Timer interrupt is not wired to GIC */
>> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
>> +		/*
>> +		 * Non-updatable, broken stock Samsung bootloader does not
>> +		 * configure CNTFRQ_EL0
>> +		 */
>> +		clock-frequency = <26000000>;
>> +	};
>> +
>> +	soc: soc@0 {
>> +		compatible = "simple-bus";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x0 0x0 0x20000000>;
> ranges is the second property (see DTS coding style).
>
>> +
>> +		chipid@10000000 {
>> +			compatible = "samsung,exynos8895-chipid",
>> +				     "samsung,exynos850-chipid";
>> +			reg = <0x10000000 0x24>;
>> +		};
>> +
>> +		gic: interrupt-controller@10201000 {
>> +			compatible = "arm,gic-400";
>> +			#interrupt-cells = <3>;
>> +			#address-cells = <0>;
>> +			interrupt-controller;
>> +			reg = <0x10201000 0x1000>,
> reg is second, please follow DTS coding style for new submissions.
>
>> +			      <0x10202000 0x1000>,
>> +			      <0x10204000 0x2000>,
>> +			      <0x10206000 0x2000>;
>> +			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) |
>> +						 IRQ_TYPE_LEVEL_HIGH)>;
>> +		};
> Best regards,
> Krzysztof
>