Re: [PATCH v2 2/5] arm64: dts: exynos: add initial devicetree support for exynos7870

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

On 2/3/25 22:46, Kaustabh Chakraborty wrote:
> Exynos7870 is an arm64 SoC manufactured by Samsung and announced in
> 2016. It is present in multiple mid-range Samsung phones and tablets.
>
> Add basic devicetree support for the SoC, which includes CMUs, pin
> controllers, I2C, UART, DW-MMC, and USB-DRD.
>
> Co-developed-by: Sergey Lisov <sleirsgoevy@gmail.com>
> Signed-off-by: Sergey Lisov <sleirsgoevy@gmail.com>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
>  arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi | 1035 ++++++++++++++++++++
>  arch/arm64/boot/dts/exynos/exynos7870.dtsi         |  722 ++++++++++++++
>  2 files changed, 1757 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..28ff409c4fdc5f766d92617ea2df7be2112c28d1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi
> @@ -0,0 +1,1035 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung Exynos7870 SoC pin-mux and pin-config device tree source
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "exynos-pinctrl.h"
> +
> +&pinctrl0 {

I haven't had enough time to look deeper, but these are my 2 cents:


Can you label them according to their block name rather than numbers?
For example, pinctrl_abox (make sure to keep them alphabetically sorted
as well).

> +	etc0: etc0-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	etc1: etc1-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	gpa0: gpa0-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gpa1: gpa1-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gpa2: gpa2-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	gpq0: gpq0-gpio-bank {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	uart2_bus: uart2-bus-pins {
> +		samsung,pins = "gpa1-1", "gpa1-0";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +	};
> +
> +	uart2_sleep: uart2-sleep-pins {
> +		samsung,pins = "gpa1-1";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_DOWN>;
> +	};
> +
> +	bt_btwake: bt-btwake-pins {
> +		samsung,pins = "gpa1-2";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> +		samsung,pin-con-pdn = <EXYNOS_PIN_PDN_PREV>;
> +		samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +	};
> +
> +	bt_hostwake: bt-hostwake-pins {

Sort these alphabetically, so you have:
all a-z-gpio-bank, after which all a-z-pin.

In this case bt_* can be first after all gpio bank nodes.

> +		samsung,pins = "gpa1-6";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +		samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +		samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +	};
> +
> +	gnss_sensor_irq: gnss-sensor-irq-pins {
> +		samsung,pins = "gpa2-3";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_6>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_UP>;
> +	};
> +
> +	gnss_sensor_i2c: gnss-sensor-i2c-pins {
> +		samsung,pins = "gpa2-5", "gpa2-4";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_6>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +	};
> +
...
> +		#interrupt-cells = <2>;
> +	};
> +
> +	hs_i2c0_bus: hs-i2c0-bus-pins {

s/hs_i2c/hsi2c/g

> +		samsung,pins = "gpm0-1", "gpm0-0";
> +		samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> +		samsung,pin-con-pdn = <EXYNOS_PIN_PDN_PREV>;
> +		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +		samsung,pin-drv = <EXYNOS5433_PIN_DRV_FAST_SR1>;
> +	};
...
> diff --git a/arch/arm64/boot/dts/exynos/exynos7870.dtsi b/arch/arm64/boot/dts/exynos/exynos7870.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..11129e37fc86ebaee01684ed6841c932dd6cbc8a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos7870.dtsi
> @@ -0,0 +1,722 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung Exynos7870 SoC device tree source
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + */
> +
> +#include <dt-bindings/clock/exynos7870.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/samsung,boot-mode.h>
> +
> +/ {
> +	compatible = "samsung,exynos7870";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	/* Samsung's bootloader (S-BOOT) checks for these DT properties */
> +	model_info-hw_rev = <0>;
> +	model_info-hw_rev_end = <255>;
> +	model_info-chip = <7870>;

You could skip the properties with a shim bootloader like uniLoader. Up to
Krzysztof to decide if you should keep these.

> +

...

> +	};
> +
> +	oscclk: oscclk {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +
> +		clock-frequency = <26000000>;

Frequency must be defined in the board device tree.

> +	};
> +
> +	timer {

S is before T, so this 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)>;
> +
> +		/* Samsung's bootloader (S-BOOT) doesn't set CNTFRQ_EL0 */
> +		clock-frequency = <26000000>;
> +	};
> +
> +	gic: interrupt-controller@104e0000 {
> +		compatible = "arm,cortex-a15-gic";
> +		reg = <0x0 0x104e1000 0x1000>,

So it should be @104e1000 and not @104e0000

> +		      <0x0 0x104e2000 0x1000>,
> +		      <0x0 0x104e4000 0x2000>,
> +		      <0x0 0x104e6000 0x2000>;
> +		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
> +		interrupt-controller;
> +		#address-cells = <0>;
> +		#interrupt-cells = <3>;
> +	};
> +
> +	soc: soc@0 {
> +		compatible = "simple-bus";
> +		ranges = <0x0 0x0 0x0 0x20000000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		chipid@10100000 {
> +			compatible = "samsung,exynos7870-chipid",
> +				     "samsung,exynos4210-chipid";
> +			reg = <0x10100000 0x100>;
> +		};
> +
> +		pmu_system_controller: system-controller@10480000 {
> +			compatible = "samsung,exynos7870-pmu",
> +				     "samsung,exynos7-pmu", "syscon";
> +			reg = <0x10480000 0x10000>;
> +			#clock-cells = <1>;
> +
> +			clock-names = "clkout8",
> +				      "clkout9",

Could fit more than one per line.

> +				      "clkout10",
> +				      "clkout11",
> +				      "clkout12";
> +			clocks = <&oscclk>,
> +				 <&oscclk>,
> +				 <&oscclk>,
> +				 <&oscclk>,
> +				 <&oscclk>;
> +
> +			reboot-mode {
> +				compatible = "syscon-reboot-mode";
> +				offset = <0x080c>;
> +				mode-bootloader = <EXYNOS7870_BOOT_BOOTLOADER>;
> +				mode-download = <EXYNOS7870_BOOT_DOWNLOAD>;
> +				mode-recovery = <EXYNOS7870_BOOT_RECOVERY>;
> +			};
> +		};
> +
> +		cmu_mif: clock-controller@10460000 {

Sort all of these by address.

> +			compatible = "samsung,exynos7870-cmu-mif";
> +			reg = <0x10460000 0x1000>;
> +			#clock-cells = <1>;
> +
> +			clock-names = "oscclk";
> +			clocks = <&oscclk>;
> +		};
> +
> +		cmu_dispaud: clock-controller@148d0000 {
> +			compatible = "samsung,exynos7870-cmu-dispaud";
> +			reg = <0x148d0000 0x1000>;
> +			#clock-cells = <1>;
> +
> +			clock-names = "oscclk",
> +				      "gout_mif_cmu_dispaud_bus",

bus

> +				      "gout_mif_cmu_dispaud_decon_eclk",

decon_eclk

> +				      "gout_mif_cmu_dispaud_decon_vclk";

decon_vclk

Clock names here shouldn't be long. You'll still describe them in the
clock YAML either way. Check other SoCs like 8895, autov920, 990.

> +			clocks = <&oscclk>,
> +				 <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_BUS>,
> +				 <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_DECON_ECLK>,
> +				 <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_DECON_VCLK>;
> +		};
> +

...

> +
> +		i2c1: i2c@13880000 {
> +			compatible = "samsung,exynos7870-i2c",
> +				     "samsung,s3c2440-i2c";
> +			reg = <0x13880000 0x100>;
> +			interrupts = <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";

Status always comes last. Make sure to sort all properties as per the DT
Coding Style [1].

[1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html

> +
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&i2c1_bus>;
> +
> +			clock-names = "i2c";
> +			clocks = <&cmu_peri CLK_GOUT_PERI_I2C1_PCLK>;
> +		};

...

> +		};
> +	};
> +};
> +
> +#include "exynos7870-pinctrl.dtsi"
> +#include "arm/samsung/exynos-syscon-restart.dtsi"

Didn't this already include a reboot node?

Best regards,
Ivaylo

>