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

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 8/7/24 12:20, Krzysztof Kozlowski wrote:
> On 07/08/2024 10:28, ivo.ivanov.ivanov1@gmail.com wrote:
>> From: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>>
>> Exynos 8895 SoC is an ARMv8 mobile SoC found in the Samsung Galaxy
>> S8 (dreamlte), S8 Plus (dream2lte), Note 8 (greatlte) and the Meizu
>> 15 Plus (m1891). Add minimal support for that SoC, including:
>>
>> - All 8 cores via PSCI
>> - ChipID
>> - Generic ARMV8 Timer
>> - Enumarate all pinctrl nodes
>>
>> Further platform support will be added over time.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> ---
>>  .../boot/dts/exynos/exynos8895-pinctrl.dtsi   | 1378 +++++++++++++++++
>>  arch/arm64/boot/dts/exynos/exynos8895.dtsi    |  253 +++
>>  2 files changed, 1631 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos8895-pinctrl.dtsi
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos8895.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/exynos/exynos8895-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos8895-pinctrl.dtsi
>> new file mode 100644
>> index 000000000..1dcb61e2e
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/exynos/exynos8895-pinctrl.dtsi
>> @@ -0,0 +1,1378 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Samsung's Exynos 8895 SoC pin-mux and pin-config device tree source
>> + *
>> + * Copyright (c) 2024, Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include "exynos-pinctrl.h"
>> +
>> +&pinctrl_alive {
>> +	gpa0: gpa0 {
> I do not believe this was tested. See maintainer SoC profile for Samsung
> Exynos.
>
> Limited review follows due to lack of testing.
>
>
>> +};
>> diff --git a/arch/arm64/boot/dts/exynos/exynos8895.dtsi b/arch/arm64/boot/dts/exynos/exynos8895.dtsi
>> new file mode 100644
>> index 000000000..3ed381ee5
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/exynos/exynos8895.dtsi
>> @@ -0,0 +1,253 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Samsung's Exynos 8895 SoC device tree source
>> + *
>> + * Copyright (c) 2024, Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> + */
>> +
>> +#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 {
> Are there two pmus?

Hm. The Downstream kernel has them all under one node with compatible

'arm,armv8-pmuv3', same as with Exynos 7885. So it should have two PMUs,

one for each cluster.


Considering the second cluster consists of Samsung's custom Mongoose M2

cores, what would be the most adequate thing to do? Keep the first PMU as

"arm,cortex-a53-pmu" and use the SW model "arm,armv8-pmuv3" for the

second PMU? I doubt guessing if these mongoose cores are based on already

existing cortex cores is a great idea.

>> +		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>,
>> +			     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
>> +			     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-affinity = <&cpu0>,
>> +				     <&cpu1>,
>> +				     <&cpu2>,
>> +				     <&cpu3>,
>> +				     <&cpu4>,
>> +				     <&cpu5>,
>> +				     <&cpu6>,
>> +				     <&cpu7>;
>> +	};
>> +
>> +	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 {
>> +			device_type = "cpu";
>> +			compatible = "samsung,mongoose-m2";
>> +			reg = <0x0>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu5: cpu@1 {
>> +			device_type = "cpu";
>> +			compatible = "samsung,mongoose-m2";
>> +			reg = <0x1>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu6: cpu@2 {
>> +			device_type = "cpu";
>> +			compatible = "samsung,mongoose-m2";
>> +			reg = <0x2>;
>> +			enable-method = "psci";
>> +		};
>> +
>> +		cpu7: cpu@3 {
>> +			device_type = "cpu";
>> +			compatible = "samsung,mongoose-m2";
>> +			reg = <0x3>;
>> +			enable-method = "psci";
>> +		};
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci";
>> +		method = "smc";
>> +		cpu_suspend = <0xc4000001>;
>> +		cpu_off = <0x84000002>;
>> +		cpu_on = <0xc4000003>;
>> +	};
>> +
>> +	timer {
>> +		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)>;
>> +		clock-frequency = <26000000>;
> Hm? I think this was explicitly disallowed.

It's weird. Without the clock-frequency property it fails early during the

boot process and I can't get any logs from pstore or simple-framebuffer.

Yet it's not set on similar platforms (exynos7885, autov9). Perhaps I

could alias the node and set it in the board device tree..? That doesn't

sound right.


Best regards,

Ivaylo

>> +	};
>> +
>> +	fixed-rate-clocks {
> Keep order of properties, just like DTS coding style asks.
>
> Anyway, fixed-rate-clocks wrapper is not needed, drop.
>
>> +		oscclk: osc-clock {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-output-names = "oscclk";
>> +		};
>> +	};
>> +
>> +	soc: soc@0 {
>> +		compatible = "simple-bus";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x0 0x0 0x20000000>;
>> +
>> +		chipid@10000000 {
>> +			compatible = "samsung,exynos8895-chipid",
>> +				     "samsung,exynos850-chipid";
>> +			reg = <0x10000000 0x24>;
>> +		};
>> +
>> +		gic: interrupt-controller@10200000 {
>> +			compatible = "arm,gic-400";
>> +			#interrupt-cells = <3>;
>> +			#address-cells = <0>;
>> +			interrupt-controller;
>> +			reg = <0x10201000 0x1000>,
>> +			      <0x10202000 0x1000>,
>> +			      <0x10204000 0x2000>,
>> +			      <0x10206000 0x2000>;
>> +			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) |
>> +						 IRQ_TYPE_LEVEL_HIGH)>;
>> +		};
>> +
>> +		pinctrl_alive: pinctrl@164b0000 {
>> +			compatible = "samsung,exynos8895-pinctrl";
>> +			reg = <0x164b0000 0x1000>;
>> +
>> +			wakeup-interrupt-controller {
>> +				compatible = "samsung,exynos8895-wakeup-eint",
>> +					     "samsung,exynos7-wakeup-eint";
>> +				interrupt-parent = <&gic>;
>> +				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
>> +			};
>> +		};
>> +
>> +		pinctrl_abox: pinctrl@13e60000 {
> This does not look ordered. See DTS coding style.
>
> Best regards,
> Krzysztof
>