[12/12] arm64: dts: exynos: Add Exynos850 SoC support

Commit Message

Sam Protsenko July 30, 2021, 2:49 p.m. UTC

Samsung Exynos850 is ARMv8-based mobile-oriented SoC.

Features:
 * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
 * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
 * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
 * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
 * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
 * GPU: Mali-G52 MP1
 * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
 * Display: Full HD+ (2520x1080)@60fps LCD
 * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
 * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, Audio

This patch adds minimal SoC support. Particular board device tree files
can include exynos850.dtsi file to get SoC related nodes, and then
reference those nodes further as needed.

Signed-off-by: Sam Protsenko <[e-mail]>
---
 .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
 arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
 3 files changed, 1057 insertions(+)
 create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
 create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
 create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi

Comments

Marc Zyngier July 30, 2021, 4:50 p.m. UTC | #1

On 2021-07-30 15:49, Sam Protsenko wrote:
> Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
> 
> Features:
>  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
>  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
>  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
>  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
>  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
>  * GPU: Mali-G52 MP1
>  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
>  * Display: Full HD+ (2520x1080)@60fps LCD
>  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
>  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, 
> Audio
> 
> This patch adds minimal SoC support. Particular board device tree files
> can include exynos850.dtsi file to get SoC related nodes, and then
> reference those nodes further as needed.
> 
> Signed-off-by: Sam Protsenko <[e-mail]>
> ---
>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>  3 files changed, 1057 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> new file mode 100644
> index 000000000000..4cf0a22cc6db

[...]

> +	gic: interrupt-controller@12a00000 {
> +		compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";

One thing for sure, it cannot be both. And given that it is
an A55-based SoC, it isn't either. It is more likely a GIC400.

> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg = <0x0 0x12a01000 0x1000>,
> +		      <0x0 0x12a02000 0x1000>,

This is wrong. It is architecturally set to 8kB.

> +		      <0x0 0x12a04000 0x2000>,
> +		      <0x0 0x12a06000 0x2000>;
> +		interrupts = <GIC_PPI 9
> +				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;

4? With 8 CPUs?

I also find it curious that you went through the unusual
(and IMO confusing) effort to allocate a name to each and
every SPI in the system, but didn't do it for any on the PPIs...


> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		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>;

No, please. Fix the firmware to program CNTFRQ_EL0 on each
and every CPU. This isn't 2012 anymore.

You are also missing the hypervisor virtual timer interrupt.

> +		use-clocksource-only;
> +		use-physical-timer;

Thankfully, these two properties do not exist.

Thanks,

         M.

Krzysztof Kozlowski July 31, 2021, 9:03 a.m. UTC | #2

On 30/07/2021 16:49, Sam Protsenko wrote:
> Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
> 
> Features:
>  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
>  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
>  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
>  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
>  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
>  * GPU: Mali-G52 MP1
>  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
>  * Display: Full HD+ (2520x1080)@60fps LCD
>  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
>  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, Audio

Please document first the features you add (and are working) and
afterwards mention all others capabilities.

> 
> This patch adds minimal SoC support. Particular board device tree files
> can include exynos850.dtsi file to get SoC related nodes, and then
> reference those nodes further as needed.
> 
> Signed-off-by: Sam Protsenko <[e-mail]>
> ---
>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++

Not buildable. Missing Makefile, missing DTS. Please submit with initial
DTS, otherwise no one is able to verify it even compiles.

>  3 files changed, 1057 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> new file mode 100644
> index 000000000000..4cf0a22cc6db
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> @@ -0,0 +1,782 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung's Exynos850 SoC pin-mux and pin-config device tree source
> + *
> + * Copyright (C) 2017 Samsung Electronics Co., Ltd.
> + * Copyright (C) 2021 Linaro Ltd.
> + *
> + * Samsung's Exynos850 SoC pin-mux and pin-config options are listed as device
> + * tree nodes in this file.
> + */
> +
> +#include <dt-bindings/interrupt-controller/exynos850.h>
> +
> +/ {
> +	/* ALIVE */
> +	pinctrl@11850000 {
> +		gpa0: gpa0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;

That's odd a little, why three cells? How this would be used/referenced?

> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			    <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpa1: gpa1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			    <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpa2: gpa2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			    <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpa3: gpa3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			    <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpa4: gpa4 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			    <GIC_SPI INTREQ__ALIVE_EINT32 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT33 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT34 IRQ_TYPE_LEVEL_HIGH>,
> +			    <GIC_SPI INTREQ__ALIVE_EINT35 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpq0: gpq0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		/* USI_PERI_UART_DBG */
> +		uart0_bus: uart0-bus {
> +			samsung,pins = "gpq0-0", "gpq0-1";
> +			samsung,pin-function = <2>;

EXYNOS_PIN_FUNC_2

> +			samsung,pin-pud = <0>;

EXYNOS_PIN_PULL_xx?

> +		};
> +
> +		decon_f_te_on: decon_f_te_on {

1. Where is it used?
2. Use hyphens in node names.
Please build it with W=1 and fix the warnings.

> +			samsung,pins = "gpa4-1";

Order the nodes based on first pin name, so:
i2c5_bus
i2c6_bus
decon_f_te_on
uart0_bus

> +			samsung,pin-function = <0xf>;
> +		};
> +
> +		decon_f_te_off: decon_f_te_off {

Where is it used?

> +			samsung,pins = "gpa4-1";
> +			samsung,pin-function = <0x0>;
> +		};
> +
> +		/* I2C_5 | CAM_PMIC_I2C */

This comment is confusing. I2C-5 is obvious from node name and label.
CAM_PMIC_I2C does not look like property of SoC but board.

> +		i2c5_bus: i2c5-bus {
> +			samsung,pins = "gpa3-5", "gpa3-6";
> +			samsung,pin-function = <3>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* I2C_6 | MOTOR_I2C */
> +		i2c6_bus: i2c6-bus {
> +			samsung,pins = "gpa3-7", "gpa4-0";
> +			samsung,pin-function = <3>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +	};
> +
> +	/* CMGP */
> +	pinctrl@11c30000 {
> +		gpm0: gpm0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpm1: gpm1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpm2: gpm2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpm3: gpm3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpm4: gpm4 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpm5: gpm5 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupt-parent = <&gic>;
> +			interrupts =
> +			  <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		/* usi_cmgp00 */
> +		hsi2c3_bus: hsi2c3-bus {
> +			samsung,pins = "gpm0-0", "gpm1-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* usi_cmgp01 */
> +		hsi2c4_bus: hsi2c4-bus {
> +			samsung,pins = "gpm4-0", "gpm5-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* spi usi_cmgp00 */
> +		spi1_bus: spi1-bus {
> +			samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi1_cs: spi1-cs {
> +			samsung,pins = "gpm3-0";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi1_cs_func: spi1-cs-func {
> +			samsung,pins = "gpm3-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* spi usi_cmgp01 */
> +		spi2_bus: spi2-bus {
> +			samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi2_cs: spi2-cs {
> +			samsung,pins = "gpm7-0";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi2_cs_func: spi2-cs-func {
> +			samsung,pins = "gpm7-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* usi_cmgp00_uart */
> +		uart1_bus_single: uart1-bus {
> +			samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0", "gpm3-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +		};
> +
> +		uart1_bus_dual: uart1-bus-dual {
> +			samsung,pins = "gpm0-0", "gpm1-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +		};
> +
> +		/* usi_cmgp01_uart */
> +		uart2_bus_single: uart2-bus {
> +			samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0", "gpm7-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +		};
> +
> +		uart2_bus_dual: uart2-bus-dual {
> +			samsung,pins = "gpm4-0", "gpm5-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +		};
> +	};
> +
> +	/* AUD */
> +	pinctrl@14a60000 {
> +		gpb0: gpb0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpb1: gpb1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		aud_codec_mclk: aud-codec-mclk {
> +			samsung,pins = "gpb0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_codec_mclk_idle: aud-codec-mclk-idle {
> +			samsung,pins = "gpb0-0";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_i2s0_bus: aud-i2s0-bus {
> +			samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_i2s0_idle: aud-i2s0-idle {
> +			samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_i2s1_bus: aud-i2s1-bus {
> +			samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_i2s1_idle: aud-i2s1-idle {
> +			samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_fm_bus: aud-fm-bus {
> +			samsung,pins = "gpb1-4";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <1>;
> +		};
> +
> +		aud_fm_idle: aud-fm-idle {
> +			samsung,pins = "gpb1-4";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <1>;
> +		};
> +	};
> +
> +	/* HSI */
> +	pinctrl@13430000 {
> +		gpf2: gpf2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		sd2_clk: sd2-clk {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd2_cmd: sd2-cmd {
> +			samsung,pins = "gpf2-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <2>;
> +		 };
> +
> +		sd2_bus1: sd2-bus-width1 {
> +			samsung,pins = "gpf2-2";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd2_bus4: sd2-bus-width4 {
> +			samsung,pins = "gpf2-3", "gpf2-4", "gpf2-5";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_1x: sd2-clk_fast_slew_rate_1x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_1_5x: sd2-clk_fast_slew_rate_1_5x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <1>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_2x: sd2-clk_fast_slew_rate_2x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_2_5x: sd2-clk_fast_slew_rate_2_5x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_3x: sd2-clk_fast_slew_rate_3x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <4>;
> +		};
> +
> +		sd2_clk_fast_slew_rate_4x: sd2-clk_fast_slew_rate_4x {
> +			samsung,pins = "gpf2-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <5>;
> +		};
> +
> +		sd2_pins_as_pdn: sd2-pins-as-pdn {
> +			samsung,pins = "gpf2-0", "gpf2-1", "gpf2-2", "gpf2-3",
> +				       "gpf2-4", "gpf2-5";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <2>;
> +		};
> +

No need for blank line.

> +	};
> +
> +	/* CORE */
> +	pinctrl@12070000 {
> +		gpf0: gpf0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		sd0_clk: sd0-clk {
> +			samsung,pins = "gpf0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_cmd: sd0-cmd {
> +			samsung,pins = "gpf0-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_rdqs: sd0-rdqs {
> +			samsung,pins = "gpf0-2";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <1>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_nreset: sd0-nreset {
> +			samsung,pins = "gpf0-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_clk_fast_slew_rate_1x: sd0-clk_fast_slew_rate_1x {
> +			samsung,pins = "gpf0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <1>;
> +		};
> +
> +		sd0_clk_fast_slew_rate_2x: sd0-clk_fast_slew_rate_2x {
> +			samsung,pins = "gpf0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd0_clk_fast_slew_rate_3x: sd0-clk_fast_slew_rate_3x {
> +			samsung,pins = "gpf0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sd0_clk_fast_slew_rate_4x: sd0-clk_fast_slew_rate_4x {
> +			samsung,pins = "gpf0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		gpf1: gpf1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		sd0_bus1: sd0-bus-width1 {
> +			samsung,pins = "gpf1-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_bus4: sd0-bus-width4 {
> +			samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <3>;
> +		};
> +
> +		sd0_bus8: sd0-bus-width8 {
> +			samsung,pins = "gpf1-4", "gpf1-5", "gpf1-6", "gpf1-7";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <3>;
> +		};
> +	};
> +
> +	/* PERI */
> +	pinctrl@139b0000 {
> +		gpg0: gpg0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpp0: gpp0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +		gpp1: gpp1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpp2: gpp2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpg1: gpg1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpg2: gpg2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpg3: gpg3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpc0: gpc0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		gpc1: gpc1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		xclkout: xclkout {
> +			samsung,pins = "gpq0-2";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +		};
> +
> +		/* usi_hsi2c_0 */

Comment seems to duplicate node name/label.

> +		hsi2c0_bus: hsi2c0-bus {
> +			samsung,pins = "gpc1-0", "gpc1-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* usi_hsi2c_1 */
> +		hsi2c1_bus: hsi2c1-bus {
> +			samsung,pins = "gpc1-2", "gpc1-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* usi_hsi2c_2 */
> +		hsi2c2_bus: hsi2c2-bus {
> +			samsung,pins = "gpc1-4", "gpc1-5";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		/* usi_spi_0 */
> +		spi0_bus: spi0-bus {
> +			samsung,pins = "gpp2-0", "gpp2-2", "gpp2-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi0_cs: spi0-cs {
> +			samsung,pins = "gpp2-1";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		spi0_cs_func: spi0-cs-func {
> +			samsung,pins = "gpp2-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		i2c0_bus: i2c0-bus {
> +			samsung,pins = "gpp0-0", "gpp0-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		i2c1_bus: i2c1-bus {
> +			samsung,pins = "gpp0-2", "gpp0-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		i2c2_bus: i2c2-bus {
> +			samsung,pins = "gpp0-4", "gpp0-5";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		i2c3_bus: i2c3-bus {
> +			samsung,pins = "gpp1-0", "gpp1-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		i2c4_bus: i2c4-bus {
> +			samsung,pins = "gpp1-2", "gpp1-3";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};
> +
> +		fm_lna_en: fm-lna-en {
> +			samsung,pins = "gpg2-3";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-val = <0>;
> +		};
> +
> +		sensor_mclk0_in: sensor-mclk0-in {
> +			samsung,pins = "gpc0-0";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk0_out: sensor-mclk0-out {
> +			samsung,pins = "gpc0-0";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <1>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk0_fn: sensor-mclk0-fn {

No, seriously. What sensor is it? In SoC?

> +			samsung,pins = "gpc0-0";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk1_in: sensor-mclk1-in {
> +			samsung,pins = "gpc0-1";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk1_out: sensor-mclk1-out {
> +			samsung,pins = "gpc0-1";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <1>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk1_fn: sensor-mclk1-fn {
> +			samsung,pins = "gpc0-1";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk2_in: sensor-mclk2-in {
> +			samsung,pins = "gpc0-2";
> +			samsung,pin-function = <0>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk2_out: sensor-mclk2-out {
> +			samsung,pins = "gpc0-2";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <1>;
> +			samsung,pin-drv = <2>;
> +		};
> +
> +		sensor_mclk2_fn: sensor-mclk2-fn {
> +			samsung,pins = "gpc0-2";
> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <2>;
> +		};
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> new file mode 100644
> index 000000000000..fb243e0a6260
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung's Exynos850 SoC USI device tree source
> + *
> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> + * Copyright (C) 2021 Linaro Ltd.
> + *
> + * Samsung's Exynos850 SoC USI channels are listed in this file as device tree
> + * nodes.

Why here not in exynos850.dtsi?

> + */
> +
> +#include <dt-bindings/clock/exynos850.h>
> +
> +/ {
> +	aliases {
> +		uart0 = &serial_0;
> +	};
> +
> +	/* USI_UART */
> +	serial_0: uart@13820000 {

This should ne in soc node.

> +		compatible = "samsung,exynos850-uart";
> +		reg = <0x0 0x13820000 0x100>;
> +		interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&uart0_bus>;
> +		clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>;
> +		clock-names = "gate_uart_clk0", "uart";
> +		status = "disabled";
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> new file mode 100644
> index 000000000000..ed2d1c8ae0c3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung Exynos850 SoC device tree source
> + *
> + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> + * Copyright (C) 2021 Linaro Ltd.
> + *
> + * Samsung Exynos850 SoC device nodes are listed in this file.
> + * Exynos based board files can include this file and provide
> + * values for board specific bindings.
> + */
> +
> +#include <dt-bindings/interrupt-controller/exynos850.h>
> +#include <dt-bindings/clock/exynos850.h>
> +#include "exynos850-pinctrl.dtsi"
> +#include "exynos850-usi.dtsi"
> +
> +/ {

Add a comment like:
/* Also known under engineering name exynos3830 */

> +	compatible = "samsung,exynos850";

Undocumented compatible. Checkpatch should complain.

> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	aliases {
> +		pinctrl0 = &pinctrl_0;
> +		pinctrl1 = &pinctrl_1;
> +		pinctrl2 = &pinctrl_2;
> +		pinctrl3 = &pinctrl_3;
> +		pinctrl4 = &pinctrl_4;
> +		pinctrl5 = &pinctrl_5;
> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#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@0000 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0000>;

reg = <0x0 0x0>;
(in following places similarly)

> +			enable-method = "psci";
> +		};
> +		cpu1: cpu@0001 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0001>;
> +			enable-method = "psci";
> +		};
> +		cpu2: cpu@0002 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0002>;
> +			enable-method = "psci";
> +		};
> +		cpu3: cpu@0003 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0003>;
> +			enable-method = "psci";
> +		};
> +		cpu4: cpu@0004 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0100>;
> +			enable-method = "psci";
> +		};
> +		cpu5: cpu@0005 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0101>;
> +			enable-method = "psci";
> +		};
> +		cpu6: cpu@0006 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0102>;
> +			enable-method = "psci";
> +		};
> +		cpu7: cpu@0007 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55", "arm,armv8";
> +			reg = <0x0 0x0103>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	gic: interrupt-controller@12a00000 {
> +		compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg = <0x0 0x12a01000 0x1000>,
> +		      <0x0 0x12a02000 0x1000>,
> +		      <0x0 0x12a04000 0x2000>,
> +		      <0x0 0x12a06000 0x2000>;
> +		interrupts = <GIC_PPI 9
> +				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		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>;
> +		use-clocksource-only;
> +		use-physical-timer;
> +	};
> +

All below should be under soc node.

Please don't write new DTS/DTSI from scratch but use exynos5433.dtsi as
template/example.

> +	clock: clock-controller@0x120e0000 {
> +		compatible = "samsung,exynos850-clock";
> +		reg = <0x0 0x120e0000 0x8000>;
> +		#clock-cells = <1>;
> +	};
> +
> +	/* ALIVE */
> +	pinctrl_0: pinctrl@11850000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x11850000 0x1000>;
> +		interrupts = <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		wakeup-interrupt-controller {
> +			compatible = "samsung,exynos7-wakeup-eint";
> +		};
> +	};
> +
> +	/* CMGP */
> +	pinctrl_1: pinctrl@11c30000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x11c30000 0x1000>;
> +		interrupts =
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM06 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI INTREQ__CMGP_EXT_INTM07 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		wakeup-interrupt-controller {
> +			compatible = "samsung,exynos7-wakeup-eint";
> +		};
> +	};
> +
> +	/* AUD */
> +	pinctrl_2: pinctrl@14a60000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x14a60000 0x1000>;
> +	};
> +
> +	/* HSI */
> +	pinctrl_3: pinctrl@13430000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x13430000 0x1000>;
> +		interrupts = <GIC_SPI INTREQ__GPIO_HSI IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	/* CORE */
> +	pinctrl_4: pinctrl@12070000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x12070000 0x1000>;
> +		interrupts = <GIC_SPI INTREQ__GPIO_CORE IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	/* PERI */
> +	pinctrl_5: pinctrl@139b0000 {
> +		compatible = "samsung,exynos850-pinctrl";
> +		reg = <0x0 0x139b0000 0x1000>;
> +		interrupts = <GIC_SPI INTREQ__GPIO_PERI IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +};
> 


Best regards,
Krzysztof

Sam Protsenko Aug. 4, 2021, 2:39 p.m. UTC | #3

Hi Marc,

On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <[e-mail]> wrote:
>
> On 2021-07-30 15:49, Sam Protsenko wrote:
> > Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
> >
> > Features:
> >  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
> >  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
> >  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
> >  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
> >  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
> >  * GPU: Mali-G52 MP1
> >  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
> >  * Display: Full HD+ (2520x1080)@60fps LCD
> >  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
> >  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC,
> > Audio
> >
> > This patch adds minimal SoC support. Particular board device tree files
> > can include exynos850.dtsi file to get SoC related nodes, and then
> > reference those nodes further as needed.
> >
> > Signed-off-by: Sam Protsenko <[e-mail]>
> > ---
> >  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >  3 files changed, 1057 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> > b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> > new file mode 100644
> > index 000000000000..4cf0a22cc6db
>
> [...]
>
> > +     gic: interrupt-controller@12a00000 {
> > +             compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
>
> One thing for sure, it cannot be both. And given that it is
> an A55-based SoC, it isn't either. It is more likely a GIC400.
>

Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2.

> > +             #interrupt-cells = <3>;
> > +             #address-cells = <0>;
> > +             interrupt-controller;
> > +             reg = <0x0 0x12a01000 0x1000>,
> > +                   <0x0 0x12a02000 0x1000>,
>
> This is wrong. It is architecturally set to 8kB.
>

Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
that Virtual Interface Control Register starts at 0x3000 offset (from
0x12a00000), where it obviously should be 0x4000, that's probably
where this dts error originates from. Btw, I'm also seeing the same
error in exynos7.dtsi. Though I don't have a TRM for Exynos7 SoCs, so
not sure if I should go ahead and fix that too. Anyway, for Exynos850,
I'll fix that in v2 series.

> > +                   <0x0 0x12a04000 0x2000>,
> > +                   <0x0 0x12a06000 0x2000>;
> > +             interrupts = <GIC_PPI 9
> > +                             (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>
> 4? With 8 CPUs?
>

Will be fixed in v2, thanks.

> I also find it curious that you went through the unusual
> (and IMO confusing) effort to allocate a name to each and
> every SPI in the system, but didn't do it for any on the PPIs...
>

Yeah, after some consideration I removed the whole interrupts header
and used hard-coded values instead. I probably felt it would be right
thing to have, just because there is no public TRM for Exynos850, thus
documenting interrupts somewhere would be nice. But that reasoning is
wrong, as trying to mix that kind of documentation with code just
clutters it. The right thing to do is probably just provide a public
TRM, but that's not for me to decide, alas :) Anyway, will be fixed in
v2.

>
> > +     };
> > +
> > +     timer {
> > +             compatible = "arm,armv8-timer";
> > +             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>;
>
> No, please. Fix the firmware to program CNTFRQ_EL0 on each
> and every CPU. This isn't 2012 anymore.
>

Ok, will remove that property in v2. Though it looks like CNTFRQ_EL0
register can be only changed in EL3 execution level, so I'll have to
ask the vendor to fix their BL31 or whatever. But that might take some
time, so I'll have to keep "clock-frequency" workaround in my local
tree for now, to make scheduler work.

> You are also missing the hypervisor virtual timer interrupt.
>

Checked SoC TRM, there is no PPI for hypervisor virtual timer
interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
ARM and TRM, almost no description of it. Also, I checked other
platforms, and seems like everyone does the same (having only 4
interrupts). And I wasn't able to find any documentation on that, so I
guess I'll leave it as is, if you don't mind.

> > +             use-clocksource-only;
> > +             use-physical-timer;
>
> Thankfully, these two properties do not exist.
>

Yeah, that's just some leftover from vendor's kernel, overlook by
me... Will remove in v2, thanks.

> Thanks,
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Marc Zyngier Aug. 4, 2021, 3:01 p.m. UTC | #4

On Wed, 04 Aug 2021 15:39:38 +0100,
Sam Protsenko <[e-mail]> wrote:

> > You are also missing the hypervisor virtual timer interrupt.
> >
> 
> Checked SoC TRM, there is no PPI for hypervisor virtual timer
> interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> ARM and TRM, almost no description of it. Also, I checked other
> platforms, and seems like everyone does the same (having only 4
> interrupts). And I wasn't able to find any documentation on that, so I
> guess I'll leave it as is, if you don't mind.

I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)

From the ARMv8 ARM (ARM DDI 0487G.b)
<quote>
D11.2.4 Timers

In an implementation of the Generic Timer that includes EL3, if EL3
can use AArch64, the following timers are implemented:

* An EL1 physical timer, that:
  - In Secure state, can be accessed from EL1.
  - In Non-secure state, can be accessed from EL1 unless those
    accesses are trapped to EL2.
    When this timer can be accessed from EL1, an EL1 control
    determines whether it can be accessed from EL0.
* A Non-secure EL2 physical timer.
* A Secure EL3 physical timer. An EL3 control determines whether this
  register is accessible from Secure EL1.
* An EL1 virtual timer.
* When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
* When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
* When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
</quote>

Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
it does have a NS-EL2 virtual timer. This is further confirmed by the
TRM which documents CNTHV*_EL2 as valid system registers[1].

So the timer exists, the signal is routed out of the core, and it
is likely that it is connected to the GIC.

If the designers have omitted it, then it needs to be documented as
such.

Thanks,

	M.

[1] https://developer.arm.com/documentation/100442/0100/register-descriptions/aarch64-system-registers/aarch64-architectural-system-register-summary

Krzysztof Kozlowski Aug. 4, 2021, 6:36 p.m. UTC | #5

On 04/08/2021 16:39, Sam Protsenko wrote:
> Hi Marc,
> 
> On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <[e-mail]> wrote:
>>
>> On 2021-07-30 15:49, Sam Protsenko wrote:
>>> Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
>>>
>>> Features:
>>>  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
>>>  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
>>>  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
>>>  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
>>>  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
>>>  * GPU: Mali-G52 MP1
>>>  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
>>>  * Display: Full HD+ (2520x1080)@60fps LCD
>>>  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
>>>  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC,
>>> Audio
>>>
>>> This patch adds minimal SoC support. Particular board device tree files
>>> can include exynos850.dtsi file to get SoC related nodes, and then
>>> reference those nodes further as needed.
>>>
>>> Signed-off-by: Sam Protsenko <[e-mail]>
>>> ---
>>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>>>  3 files changed, 1057 insertions(+)
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>> b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>> new file mode 100644
>>> index 000000000000..4cf0a22cc6db
>>
>> [...]
>>
>>> +     gic: interrupt-controller@12a00000 {
>>> +             compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
>>
>> One thing for sure, it cannot be both. And given that it is
>> an A55-based SoC, it isn't either. It is more likely a GIC400.
>>
> 
> Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2.
> 
>>> +             #interrupt-cells = <3>;
>>> +             #address-cells = <0>;
>>> +             interrupt-controller;
>>> +             reg = <0x0 0x12a01000 0x1000>,
>>> +                   <0x0 0x12a02000 0x1000>,
>>
>> This is wrong. It is architecturally set to 8kB.
>>
> 
> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
> that Virtual Interface Control Register starts at 0x3000 offset (from
> 0x12a00000), where it obviously should be 0x4000, that's probably
> where this dts error originates from. Btw, I'm also seeing the same
> error in exynos7.dtsi.

What's the error exactly? The "Virtual interface control register"
offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
the Exynos5433 looks correct.

> Though I don't have a TRM for Exynos7 SoCs, so
> not sure if I should go ahead and fix that too. Anyway, for Exynos850,
> I'll fix that in v2 series.


However while we are at addresses - why are you using address-cells 2?
It adds everywhere additional 0x0 before actual address.


Best regards,
Krzysztof

Sam Protsenko Aug. 4, 2021, 6:37 p.m. UTC | #6

On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <[e-mail]> wrote:
>
> On Wed, 04 Aug 2021 15:39:38 +0100,
> Sam Protsenko <[e-mail]> wrote:
>
> > > You are also missing the hypervisor virtual timer interrupt.
> > >
> >
> > Checked SoC TRM, there is no PPI for hypervisor virtual timer
> > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> > ARM and TRM, almost no description of it. Also, I checked other
> > platforms, and seems like everyone does the same (having only 4
> > interrupts). And I wasn't able to find any documentation on that, so I
> > guess I'll leave it as is, if you don't mind.
>
> I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)
>
> From the ARMv8 ARM (ARM DDI 0487G.b)
> <quote>
> D11.2.4 Timers
>
> In an implementation of the Generic Timer that includes EL3, if EL3
> can use AArch64, the following timers are implemented:
>
> * An EL1 physical timer, that:
>   - In Secure state, can be accessed from EL1.
>   - In Non-secure state, can be accessed from EL1 unless those
>     accesses are trapped to EL2.
>     When this timer can be accessed from EL1, an EL1 control
>     determines whether it can be accessed from EL0.
> * A Non-secure EL2 physical timer.
> * A Secure EL3 physical timer. An EL3 control determines whether this
>   register is accessible from Secure EL1.
> * An EL1 virtual timer.
> * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
> * When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
> * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
> </quote>
>
> Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
> it does have a NS-EL2 virtual timer. This is further confirmed by the
> TRM which documents CNTHV*_EL2 as valid system registers[1].
>
> So the timer exists, the signal is routed out of the core, and it
> is likely that it is connected to the GIC.
>
> If the designers have omitted it, then it needs to be documented as
> such.
>

Ok, I've checked thoroughly all docs again, and it seems like there is
no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in
Exynos850 SoC. The timer instance itself might exist of course, but
interrupt line is probably wasn't connected to GIC by SoC designers,
at least it's not documented.

Moreover, from [1,2] it looks like if it were existing it would have
been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to
"Legacy FIQ signal", and all there is no PPI for Hypervisor Virtual
Timer documented there as well. In Exynos850 TRM the source for this
PPI's interrupt source is marked as "-", which means it's not used.

So if you know something that I don't know -- please point me out the
doc where this PPI line is documented. Otherwise I can add the comment
to device tree, stating that this interrupt line is not present in
SoC's GIC, i.e. something like this:

8<------------------------------------------------------------------------------->8
    timer {
        compatible = "arm,armv8-timer";
        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)>;
        /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */
    };
8<------------------------------------------------------------------------------->8

Is that ok with you?

[1] https://developer.arm.com/documentation/102379/0000/The-processor-timers?lang=en
[2] https://gem5.googlesource.com/public/gem5/+/refs/heads/master/src/arch/arm/fastmodel/CortexA76/FastModelCortexA76.py#150

> Thanks,
>
>         M.
>
> [1] https://developer.arm.com/documentation/100442/0100/register-descriptions/aarch64-system-registers/aarch64-architectural-system-register-summary
>
> --
> Without deviation from the norm, progress is not possible.

Sam Protsenko Aug. 4, 2021, 9:30 p.m. UTC | #7

On Wed, 4 Aug 2021 at 21:36, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 04/08/2021 16:39, Sam Protsenko wrote:
> > Hi Marc,
> >
> > On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <[e-mail]> wrote:
> >>
> >> On 2021-07-30 15:49, Sam Protsenko wrote:
> >>> Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
> >>>
> >>> Features:
> >>>  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
> >>>  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
> >>>  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
> >>>  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
> >>>  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
> >>>  * GPU: Mali-G52 MP1
> >>>  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
> >>>  * Display: Full HD+ (2520x1080)@60fps LCD
> >>>  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
> >>>  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC,
> >>> Audio
> >>>
> >>> This patch adds minimal SoC support. Particular board device tree files
> >>> can include exynos850.dtsi file to get SoC related nodes, and then
> >>> reference those nodes further as needed.
> >>>
> >>> Signed-off-by: Sam Protsenko <[e-mail]>
> >>> ---
> >>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >>>  3 files changed, 1057 insertions(+)
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> >>>
> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>> b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>> new file mode 100644
> >>> index 000000000000..4cf0a22cc6db
> >>
> >> [...]
> >>
> >>> +     gic: interrupt-controller@12a00000 {
> >>> +             compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
> >>
> >> One thing for sure, it cannot be both. And given that it is
> >> an A55-based SoC, it isn't either. It is more likely a GIC400.
> >>
> >
> > Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2.
> >
> >>> +             #interrupt-cells = <3>;
> >>> +             #address-cells = <0>;
> >>> +             interrupt-controller;
> >>> +             reg = <0x0 0x12a01000 0x1000>,
> >>> +                   <0x0 0x12a02000 0x1000>,
> >>
> >> This is wrong. It is architecturally set to 8kB.
> >>
> >
> > Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
> > that Virtual Interface Control Register starts at 0x3000 offset (from
> > 0x12a00000), where it obviously should be 0x4000, that's probably
> > where this dts error originates from. Btw, I'm also seeing the same
> > error in exynos7.dtsi.
>
> What's the error exactly? The "Virtual interface control register"
> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
> the Exynos5433 looks correct.
>

The issue is that 2nd region's size is 0x1000, but it must be 0x2000.
It's defined by GIC-400 architecture, as I understand. Please look at
[1], table 3-1 has very specific offsets and sizes for each functional
block, and each particular SoC must adhere to that spec. So having
0x1000 for 2nd region can't be correct. And because exynos7.dtsi has
GIC-400 as well, and 0x1000 is specified there for 2nd region size
too, so I presume there is the same mistake there.

Can you please check the TRM for Exynos7 SoC (if you have one in your
possession), and see if there is a typo there? E.g. in case of
Exynos850 TRM I can see that in "Register Map Summary" section the
offset for the first register (GICH_HCR) in "Virtual Interface Control
Register" region is specified as 0x3000, where it should be 0x4000, so
it's probably a typo. But the register description is correct, saying
that: "Address = Base Address + 0x4000".

[1] https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map

> > Though I don't have a TRM for Exynos7 SoCs, so
> > not sure if I should go ahead and fix that too. Anyway, for Exynos850,
> > I'll fix that in v2 series.
>
>
> However while we are at addresses - why are you using address-cells 2?
> It adds everywhere additional 0x0 before actual address.
>

Right. For "cpus" node I'll change the address-cells to 1 in my v2
series. I'll keep address-cells=2 for the root node, but I'm going to
encapsulate some nodes into soc node (as you suggested earlier), where
I'll make address-cells=1. That's pretty much how it's done in
exynos7.dtsi and in exynos5433.dtsi, so I guess that's should be fine
(to get rid of superfluous 0x0 and conform with other Exynos DTs)?

>
> Best regards,
> Krzysztof

Krzysztof Kozlowski Aug. 5, 2021, 7:17 a.m. UTC | #8

On 04/08/2021 23:30, Sam Protsenko wrote:
>>>
>>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
>>> that Virtual Interface Control Register starts at 0x3000 offset (from
>>> 0x12a00000), where it obviously should be 0x4000, that's probably
>>> where this dts error originates from. Btw, I'm also seeing the same
>>> error in exynos7.dtsi.
>>
>> What's the error exactly? The "Virtual interface control register"
>> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
>> the Exynos5433 looks correct.
>>
> 
> The issue is that 2nd region's size is 0x1000, but it must be 0x2000.
> It's defined by GIC-400 architecture, as I understand. Please look at
> [1], table 3-1 has very specific offsets and sizes for each functional
> block, and each particular SoC must adhere to that spec. So having
> 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has
> GIC-400 as well, and 0x1000 is specified there for 2nd region size
> too, so I presume there is the same mistake there.

I understand, the range length has indeed same mistake. However it does
not matter that much There are no registers pass 0x10C (so pass 0x1000).
This address space is not used.

> Can you please check the TRM for Exynos7 SoC (if you have one in your
> possession), and see if there is a typo there? E.g. in case of
> Exynos850 TRM I can see that in "Register Map Summary" section the
> offset for the first register (GICH_HCR) in "Virtual Interface Control
> Register" region is specified as 0x3000, where it should be 0x4000, so
> it's probably a typo. But the register description is correct, saying
> that: "Address = Base Address + 0x4000".

The starting addresses of each registers range is different issue and
this one matters. Except same typo as you say, all looks good - they
start at 0x4000.

> 
> [1] https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map

> 
>>> Though I don't have a TRM for Exynos7 SoCs, so
>>> not sure if I should go ahead and fix that too. Anyway, for Exynos850,
>>> I'll fix that in v2 series.
>>
>>
>> However while we are at addresses - why are you using address-cells 2?
>> It adds everywhere additional 0x0 before actual address.
>>
> 
> Right. For "cpus" node I'll change the address-cells to 1 in my v2
> series. I'll keep address-cells=2 for the root node, but I'm going to
> encapsulate some nodes into soc node (as you suggested earlier), where
> I'll make address-cells=1. That's pretty much how it's done in
> exynos7.dtsi and in exynos5433.dtsi, so I guess that's should be fine
> (to get rid of superfluous 0x0 and conform with other Exynos DTs)?

Yes, thanks.


Best regards,
Krzysztof

Marc Zyngier Aug. 5, 2021, 7:30 a.m. UTC | #9

On Thu, 05 Aug 2021 08:17:14 +0100,
Krzysztof Kozlowski <[e-mail]> wrote:
> 
> On 04/08/2021 23:30, Sam Protsenko wrote:
> >>>
> >>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
> >>> that Virtual Interface Control Register starts at 0x3000 offset (from
> >>> 0x12a00000), where it obviously should be 0x4000, that's probably
> >>> where this dts error originates from. Btw, I'm also seeing the same
> >>> error in exynos7.dtsi.
> >>
> >> What's the error exactly? The "Virtual interface control register"
> >> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
> >> the Exynos5433 looks correct.
> >>
> > 
> > The issue is that 2nd region's size is 0x1000, but it must be 0x2000.
> > It's defined by GIC-400 architecture, as I understand. Please look at
> > [1], table 3-1 has very specific offsets and sizes for each functional
> > block, and each particular SoC must adhere to that spec. So having
> > 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has
> > GIC-400 as well, and 0x1000 is specified there for 2nd region size
> > too, so I presume there is the same mistake there.
> 
> I understand, the range length has indeed same mistake. However it does
> not matter that much There are no registers pass 0x10C (so pass 0x1000).
> This address space is not used.

I have no idea which spec you are looking at, but the GICv2
architecture (of which GIC400 is an implementation) definitely has a
register in the second 4kB page of the CPU interface. It contains the
GICC_DIR register, which is used to deactivate an interrupt when
EOIMode==1.

Linux actively uses it when started at EL2.

	M.

Krzysztof Kozlowski Aug. 5, 2021, 7:35 a.m. UTC | #10

On 05/08/2021 09:30, Marc Zyngier wrote:
> On Thu, 05 Aug 2021 08:17:14 +0100,
> Krzysztof Kozlowski <[e-mail]> wrote:
>>
>> On 04/08/2021 23:30, Sam Protsenko wrote:
>>>>>
>>>>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
>>>>> that Virtual Interface Control Register starts at 0x3000 offset (from
>>>>> 0x12a00000), where it obviously should be 0x4000, that's probably
>>>>> where this dts error originates from. Btw, I'm also seeing the same
>>>>> error in exynos7.dtsi.
>>>>
>>>> What's the error exactly? The "Virtual interface control register"
>>>> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
>>>> the Exynos5433 looks correct.
>>>>
>>>
>>> The issue is that 2nd region's size is 0x1000, but it must be 0x2000.
>>> It's defined by GIC-400 architecture, as I understand. Please look at
>>> [1], table 3-1 has very specific offsets and sizes for each functional
>>> block, and each particular SoC must adhere to that spec. So having
>>> 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has
>>> GIC-400 as well, and 0x1000 is specified there for 2nd region size
>>> too, so I presume there is the same mistake there.
>>
>> I understand, the range length has indeed same mistake. However it does
>> not matter that much There are no registers pass 0x10C (so pass 0x1000).
>> This address space is not used.
> 
> I have no idea which spec you are looking at, but the GICv2
> architecture (of which GIC400 is an implementation) definitely has a
> register in the second 4kB page of the CPU interface. It contains the
> GICC_DIR register, which is used to deactivate an interrupt when
> EOIMode==1.
> 
> Linux actively uses it when started at EL2.

I was checking Exynos TRM and it seems it has one more bug... The ARM
datasheet [1] indeed mentions GICC_DIR at 0x1000. I'll add "Fixes" tag
to my fix for Exynos7.

https://developer.arm.com/documentation/ddi0471/b/programmers-model/cpu-interface-register-summary



Best regards,
Krzysztof

Marc Zyngier Aug. 5, 2021, 7:39 a.m. UTC | #11

On Wed, 04 Aug 2021 19:37:24 +0100,
Sam Protsenko <[e-mail]> wrote:
> 
> On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <[e-mail]> wrote:
> >
> > On Wed, 04 Aug 2021 15:39:38 +0100,
> > Sam Protsenko <[e-mail]> wrote:
> >
> > > > You are also missing the hypervisor virtual timer interrupt.
> > > >
> > >
> > > Checked SoC TRM, there is no PPI for hypervisor virtual timer
> > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> > > ARM and TRM, almost no description of it. Also, I checked other
> > > platforms, and seems like everyone does the same (having only 4
> > > interrupts). And I wasn't able to find any documentation on that, so I
> > > guess I'll leave it as is, if you don't mind.
> >
> > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)
> >
> > From the ARMv8 ARM (ARM DDI 0487G.b)
> > <quote>
> > D11.2.4 Timers
> >
> > In an implementation of the Generic Timer that includes EL3, if EL3
> > can use AArch64, the following timers are implemented:
> >
> > * An EL1 physical timer, that:
> >   - In Secure state, can be accessed from EL1.
> >   - In Non-secure state, can be accessed from EL1 unless those
> >     accesses are trapped to EL2.
> >     When this timer can be accessed from EL1, an EL1 control
> >     determines whether it can be accessed from EL0.
> > * A Non-secure EL2 physical timer.
> > * A Secure EL3 physical timer. An EL3 control determines whether this
> >   register is accessible from Secure EL1.
> > * An EL1 virtual timer.
> > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
> > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
> > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
> > </quote>
> >
> > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
> > it does have a NS-EL2 virtual timer. This is further confirmed by the
> > TRM which documents CNTHV*_EL2 as valid system registers[1].
> >
> > So the timer exists, the signal is routed out of the core, and it
> > is likely that it is connected to the GIC.
> >
> > If the designers have omitted it, then it needs to be documented as
> > such.
> >
> 
> Ok, I've checked thoroughly all docs again, and it seems like there is
> no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in
> Exynos850 SoC. The timer instance itself might exist of course, but
> interrupt line is probably wasn't connected to GIC by SoC designers,
> at least it's not documented.

Can you try and check this? You can directly program the virtual timer
so that it has a pending interrupt, and then check the pending
register on the same CPU to see if there is anything appearing there.

> Moreover, from [1,2] it looks like if it were existing it would have
> been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to
> "Legacy FIQ signal",

No. That's only if you set the bypass bits in GICD_CTLR, which nobody
with half a brain would consider doing.

> and all there is no PPI for Hypervisor Virtual
> Timer documented there as well. In Exynos850 TRM the source for this
> PPI's interrupt source is marked as "-", which means it's not used.
>
> So if you know something that I don't know -- please point me out the
> doc where this PPI line is documented. Otherwise I can add the comment
> to device tree, stating that this interrupt line is not present in
> SoC's GIC, i.e. something like this:
> 
> 8<------------------------------------------------------------------------------->8
>     timer {
>         compatible = "arm,armv8-timer";
>         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)>;
>         /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */
>     };
> 8<------------------------------------------------------------------------------->8
> 
> Is that ok with you?

I'd rather you verify the above first. And if you can't, I'd like a
comment that is a bit more explicit:

/* The vendor couldn't be bothered to wire the EL2 Virtual Timers */

Thanks,

	M.

Sam Protsenko Aug. 5, 2021, 3:30 p.m. UTC | #12

On Thu, 5 Aug 2021 at 10:39, Marc Zyngier <[e-mail]> wrote:
>
> On Wed, 04 Aug 2021 19:37:24 +0100,
> Sam Protsenko <[e-mail]> wrote:
> >
> > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <[e-mail]> wrote:
> > >
> > > On Wed, 04 Aug 2021 15:39:38 +0100,
> > > Sam Protsenko <[e-mail]> wrote:
> > >
> > > > > You are also missing the hypervisor virtual timer interrupt.
> > > > >
> > > >
> > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer
> > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> > > > ARM and TRM, almost no description of it. Also, I checked other
> > > > platforms, and seems like everyone does the same (having only 4
> > > > interrupts). And I wasn't able to find any documentation on that, so I
> > > > guess I'll leave it as is, if you don't mind.
> > >
> > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)
> > >
> > > From the ARMv8 ARM (ARM DDI 0487G.b)
> > > <quote>
> > > D11.2.4 Timers
> > >
> > > In an implementation of the Generic Timer that includes EL3, if EL3
> > > can use AArch64, the following timers are implemented:
> > >
> > > * An EL1 physical timer, that:
> > >   - In Secure state, can be accessed from EL1.
> > >   - In Non-secure state, can be accessed from EL1 unless those
> > >     accesses are trapped to EL2.
> > >     When this timer can be accessed from EL1, an EL1 control
> > >     determines whether it can be accessed from EL0.
> > > * A Non-secure EL2 physical timer.
> > > * A Secure EL3 physical timer. An EL3 control determines whether this
> > >   register is accessible from Secure EL1.
> > > * An EL1 virtual timer.
> > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
> > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
> > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
> > > </quote>
> > >
> > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
> > > it does have a NS-EL2 virtual timer. This is further confirmed by the
> > > TRM which documents CNTHV*_EL2 as valid system registers[1].
> > >
> > > So the timer exists, the signal is routed out of the core, and it
> > > is likely that it is connected to the GIC.
> > >
> > > If the designers have omitted it, then it needs to be documented as
> > > such.
> > >
> >
> > Ok, I've checked thoroughly all docs again, and it seems like there is
> > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in
> > Exynos850 SoC. The timer instance itself might exist of course, but
> > interrupt line is probably wasn't connected to GIC by SoC designers,
> > at least it's not documented.
>
> Can you try and check this? You can directly program the virtual timer
> so that it has a pending interrupt, and then check the pending
> register on the same CPU to see if there is anything appearing there.
>
> > Moreover, from [1,2] it looks like if it were existing it would have
> > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to
> > "Legacy FIQ signal",
>
> No. That's only if you set the bypass bits in GICD_CTLR, which nobody
> with half a brain would consider doing.
>
> > and all there is no PPI for Hypervisor Virtual
> > Timer documented there as well. In Exynos850 TRM the source for this
> > PPI's interrupt source is marked as "-", which means it's not used.
> >
> > So if you know something that I don't know -- please point me out the
> > doc where this PPI line is documented. Otherwise I can add the comment
> > to device tree, stating that this interrupt line is not present in
> > SoC's GIC, i.e. something like this:
> >
> > 8<------------------------------------------------------------------------------->8
> >     timer {
> >         compatible = "arm,armv8-timer";
> >         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)>;
> >         /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */
> >     };
> > 8<------------------------------------------------------------------------------->8
> >
> > Is that ok with you?
>
> I'd rather you verify the above first. And if you can't, I'd like a
> comment that is a bit more explicit:
>

I'm afraid I won't be able to verify your idea: seems like CNTHV_EL2
can be only modified (or read) in EL2. I tried to read that reg
anyway, which unsurprisingly resulted in el1_undef() BUG. The kernel
on my board is running in EL1, and I don't have access to the source
code for EL3 bootloaders. I have the source code for the last
bootloader, but it's already running in EL1.

> /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */
>

I'll add the comment as you suggested. I propose we come back to this
issue later, either when the need for HV timer arises or when I have
some means to test your theory about existing PPI.

Thanks!

> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Aug. 5, 2021, 3:50 p.m. UTC | #13

On Thu, 05 Aug 2021 16:30:23 +0100,
Sam Protsenko <[e-mail]> wrote:
> 
> On Thu, 5 Aug 2021 at 10:39, Marc Zyngier <[e-mail]> wrote:
> >
> > On Wed, 04 Aug 2021 19:37:24 +0100,
> > Sam Protsenko <[e-mail]> wrote:
> > >
> > > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <[e-mail]> wrote:
> > > >
> > > > On Wed, 04 Aug 2021 15:39:38 +0100,
> > > > Sam Protsenko <[e-mail]> wrote:
> > > >
> > > > > > You are also missing the hypervisor virtual timer interrupt.
> > > > > >
> > > > >
> > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer
> > > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> > > > > ARM and TRM, almost no description of it. Also, I checked other
> > > > > platforms, and seems like everyone does the same (having only 4
> > > > > interrupts). And I wasn't able to find any documentation on that, so I
> > > > > guess I'll leave it as is, if you don't mind.
> > > >
> > > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)
> > > >
> > > > From the ARMv8 ARM (ARM DDI 0487G.b)
> > > > <quote>
> > > > D11.2.4 Timers
> > > >
> > > > In an implementation of the Generic Timer that includes EL3, if EL3
> > > > can use AArch64, the following timers are implemented:
> > > >
> > > > * An EL1 physical timer, that:
> > > >   - In Secure state, can be accessed from EL1.
> > > >   - In Non-secure state, can be accessed from EL1 unless those
> > > >     accesses are trapped to EL2.
> > > >     When this timer can be accessed from EL1, an EL1 control
> > > >     determines whether it can be accessed from EL0.
> > > > * A Non-secure EL2 physical timer.
> > > > * A Secure EL3 physical timer. An EL3 control determines whether this
> > > >   register is accessible from Secure EL1.
> > > > * An EL1 virtual timer.
> > > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
> > > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
> > > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
> > > > </quote>
> > > >
> > > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
> > > > it does have a NS-EL2 virtual timer. This is further confirmed by the
> > > > TRM which documents CNTHV*_EL2 as valid system registers[1].
> > > >
> > > > So the timer exists, the signal is routed out of the core, and it
> > > > is likely that it is connected to the GIC.
> > > >
> > > > If the designers have omitted it, then it needs to be documented as
> > > > such.
> > > >
> > >
> > > Ok, I've checked thoroughly all docs again, and it seems like there is
> > > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in
> > > Exynos850 SoC. The timer instance itself might exist of course, but
> > > interrupt line is probably wasn't connected to GIC by SoC designers,
> > > at least it's not documented.
> >
> > Can you try and check this? You can directly program the virtual timer
> > so that it has a pending interrupt, and then check the pending
> > register on the same CPU to see if there is anything appearing there.
> >
> > > Moreover, from [1,2] it looks like if it were existing it would have
> > > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to
> > > "Legacy FIQ signal",
> >
> > No. That's only if you set the bypass bits in GICD_CTLR, which nobody
> > with half a brain would consider doing.
> >
> > > and all there is no PPI for Hypervisor Virtual
> > > Timer documented there as well. In Exynos850 TRM the source for this
> > > PPI's interrupt source is marked as "-", which means it's not used.
> > >
> > > So if you know something that I don't know -- please point me out the
> > > doc where this PPI line is documented. Otherwise I can add the comment
> > > to device tree, stating that this interrupt line is not present in
> > > SoC's GIC, i.e. something like this:
> > >
> > > 8<------------------------------------------------------------------------------->8
> > >     timer {
> > >         compatible = "arm,armv8-timer";
> > >         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)>;
> > >         /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */
> > >     };
> > > 8<------------------------------------------------------------------------------->8
> > >
> > > Is that ok with you?
> >
> > I'd rather you verify the above first. And if you can't, I'd like a
> > comment that is a bit more explicit:
> >
> 
> I'm afraid I won't be able to verify your idea: seems like CNTHV_EL2
> can be only modified (or read) in EL2. I tried to read that reg
> anyway, which unsurprisingly resulted in el1_undef() BUG. The kernel
> on my board is running in EL1, and I don't have access to the source
> code for EL3 bootloaders. I have the source code for the last
> bootloader, but it's already running in EL1.

Excellent. Yet another half-usable machine on the block. Just what we
need.

> 
> > /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */
> >
> 
> I'll add the comment as you suggested. I propose we come back to this
> issue later, either when the need for HV timer arises or when I have
> some means to test your theory about existing PPI.

If you depend on the vendor to get EL2 access, it is a lost cause,
unfortunately.

	M.

Sam Protsenko Aug. 5, 2021, 11:06 p.m. UTC | #14

On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 30/07/2021 16:49, Sam Protsenko wrote:
> > Samsung Exynos850 is ARMv8-based mobile-oriented SoC.
> >
> > Features:
> >  * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz
> >  * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s)
> >  * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit
> >  * Modem: 4G LTE, 3G, GSM/GPRS/EDGE
> >  * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0
> >  * GPU: Mali-G52 MP1
> >  * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec
> >  * Display: Full HD+ (2520x1080)@60fps LCD
> >  * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC
> >  * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, Audio
>
> Please document first the features you add (and are working) and
> afterwards mention all others capabilities.
>

I'll remove SoC features, as it's easy to find and it's not needed in
commit message anyway. Instead I'll describe which features (nodes)
are added in DT by this commit.

> >
> > This patch adds minimal SoC support. Particular board device tree files
> > can include exynos850.dtsi file to get SoC related nodes, and then
> > reference those nodes further as needed.
> >
> > Signed-off-by: Sam Protsenko <[e-mail]>
> > ---
> >  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>
> Not buildable. Missing Makefile, missing DTS. Please submit with initial
> DTS, otherwise no one is able to verify it even compiles.
>

This device is not available for purchase yet. I'll send the patch for
board dts once it's announced. I can do all the testing for now, if
you have any specific requests. Would it be possible for us to review
and apply only SoC support for now? Will send v2 soon...

> >  3 files changed, 1057 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> > new file mode 100644
> > index 000000000000..4cf0a22cc6db
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> > @@ -0,0 +1,782 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's Exynos850 SoC pin-mux and pin-config device tree source
> > + *
> > + * Copyright (C) 2017 Samsung Electronics Co., Ltd.
> > + * Copyright (C) 2021 Linaro Ltd.
> > + *
> > + * Samsung's Exynos850 SoC pin-mux and pin-config options are listed as device
> > + * tree nodes in this file.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/exynos850.h>
> > +
> > +/ {
> > +     /* ALIVE */
> > +     pinctrl@11850000 {
> > +             gpa0: gpa0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
>
> That's odd a little, why three cells? How this would be used/referenced?
>

Fixed. You are right, irq_domain_xlate_twocell() is used in
pinctrl-exynos.c. Btw, that fixed the use-case when gpX is specified
as interrupt-parrent for other nodes.

> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                         <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpa1: gpa1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                         <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpa2: gpa2 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                         <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpa3: gpa3 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                         <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpa4: gpa4 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                         <GIC_SPI INTREQ__ALIVE_EINT32 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT33 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT34 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI INTREQ__ALIVE_EINT35 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpq0: gpq0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             /* USI_PERI_UART_DBG */
> > +             uart0_bus: uart0-bus {
> > +                     samsung,pins = "gpq0-0", "gpq0-1";
> > +                     samsung,pin-function = <2>;
>
> EXYNOS_PIN_FUNC_2
>

Done, thanks.

> > +                     samsung,pin-pud = <0>;
>
> EXYNOS_PIN_PULL_xx?
>

Yes, replaced pin-pud values with these constants, thanks. Won't touch
pin-drv properties, though, as different domains have different
meanings for the same values, so don't want to introduce the whole
bunch of consts.

> > +             };
> > +
> > +             decon_f_te_on: decon_f_te_on {
>
> 1. Where is it used?
> 2. Use hyphens in node names.
> Please build it with W=1 and fix the warnings.
>

decon* nodes seem like leftover from vendor kernel. Those are not
connected anywhere on my board, so I removed those nodes.

Also fixed node names as you suggested, and fixed all stuff found with
W=1 build, thanks for reminding me that.

> > +                     samsung,pins = "gpa4-1";
>
> Order the nodes based on first pin name, so:
> i2c5_bus
> i2c6_bus
> decon_f_te_on
> uart0_bus
>

Done. Also rearranged the nodes in that way for other pinctrl domains.

> > +                     samsung,pin-function = <0xf>;
> > +             };
> > +
> > +             decon_f_te_off: decon_f_te_off {
>
> Where is it used?
>

Removed, as explained above.

> > +                     samsung,pins = "gpa4-1";
> > +                     samsung,pin-function = <0x0>;
> > +             };
> > +
> > +             /* I2C_5 | CAM_PMIC_I2C */
>
> This comment is confusing. I2C-5 is obvious from node name and label.
> CAM_PMIC_I2C does not look like property of SoC but board.
>

Yeah, unfortunately this confusing I2C naming (CAM_PMIC_I2C and
MOTOR_I2C) actually comes from TRM and SoC schematic symbol. So in
Device Tree it's called just i2c5 and i2c6 (because it's a regular I2C
really), but in TRM it's a mix of both names. I guess it's just a poor
pin function naming, done by SoC designers.

That said, I suggest changing these comments to something like this:

    /* I2C5 (also called CAM_PMIC_I2C in TRM) */
    /* I2C6 (also called MOTOR_I2C in TRM) */

> > +             i2c5_bus: i2c5-bus {
> > +                     samsung,pins = "gpa3-5", "gpa3-6";
> > +                     samsung,pin-function = <3>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* I2C_6 | MOTOR_I2C */
> > +             i2c6_bus: i2c6-bus {
> > +                     samsung,pins = "gpa3-7", "gpa4-0";
> > +                     samsung,pin-function = <3>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +     };
> > +
> > +     /* CMGP */
> > +     pinctrl@11c30000 {
> > +             gpm0: gpm0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpm1: gpm1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpm2: gpm2 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpm3: gpm3 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpm4: gpm4 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             gpm5: gpm5 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <3>;
> > +                     interrupt-parent = <&gic>;
> > +                     interrupts =
> > +                       <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             /* usi_cmgp00 */
> > +             hsi2c3_bus: hsi2c3-bus {
> > +                     samsung,pins = "gpm0-0", "gpm1-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* usi_cmgp01 */
> > +             hsi2c4_bus: hsi2c4-bus {
> > +                     samsung,pins = "gpm4-0", "gpm5-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* spi usi_cmgp00 */
> > +             spi1_bus: spi1-bus {
> > +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi1_cs: spi1-cs {
> > +                     samsung,pins = "gpm3-0";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi1_cs_func: spi1-cs-func {
> > +                     samsung,pins = "gpm3-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* spi usi_cmgp01 */
> > +             spi2_bus: spi2-bus {
> > +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi2_cs: spi2-cs {
> > +                     samsung,pins = "gpm7-0";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi2_cs_func: spi2-cs-func {
> > +                     samsung,pins = "gpm7-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* usi_cmgp00_uart */
> > +             uart1_bus_single: uart1-bus {
> > +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0", "gpm3-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +             };
> > +
> > +             uart1_bus_dual: uart1-bus-dual {
> > +                     samsung,pins = "gpm0-0", "gpm1-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +             };
> > +
> > +             /* usi_cmgp01_uart */
> > +             uart2_bus_single: uart2-bus {
> > +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0", "gpm7-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +             };
> > +
> > +             uart2_bus_dual: uart2-bus-dual {
> > +                     samsung,pins = "gpm4-0", "gpm5-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +             };
> > +     };
> > +
> > +     /* AUD */
> > +     pinctrl@14a60000 {
> > +             gpb0: gpb0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpb1: gpb1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             aud_codec_mclk: aud-codec-mclk {
> > +                     samsung,pins = "gpb0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_codec_mclk_idle: aud-codec-mclk-idle {
> > +                     samsung,pins = "gpb0-0";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_i2s0_bus: aud-i2s0-bus {
> > +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_i2s0_idle: aud-i2s0-idle {
> > +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_i2s1_bus: aud-i2s1-bus {
> > +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_i2s1_idle: aud-i2s1-idle {
> > +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_fm_bus: aud-fm-bus {
> > +                     samsung,pins = "gpb1-4";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +
> > +             aud_fm_idle: aud-fm-idle {
> > +                     samsung,pins = "gpb1-4";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <1>;
> > +             };
> > +     };
> > +
> > +     /* HSI */
> > +     pinctrl@13430000 {
> > +             gpf2: gpf2 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             sd2_clk: sd2-clk {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd2_cmd: sd2-cmd {
> > +                     samsung,pins = "gpf2-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <2>;
> > +              };
> > +
> > +             sd2_bus1: sd2-bus-width1 {
> > +                     samsung,pins = "gpf2-2";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd2_bus4: sd2-bus-width4 {
> > +                     samsung,pins = "gpf2-3", "gpf2-4", "gpf2-5";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_1x: sd2-clk_fast_slew_rate_1x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_1_5x: sd2-clk_fast_slew_rate_1_5x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <1>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_2x: sd2-clk_fast_slew_rate_2x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_2_5x: sd2-clk_fast_slew_rate_2_5x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_3x: sd2-clk_fast_slew_rate_3x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <4>;
> > +             };
> > +
> > +             sd2_clk_fast_slew_rate_4x: sd2-clk_fast_slew_rate_4x {
> > +                     samsung,pins = "gpf2-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <5>;
> > +             };
> > +
> > +             sd2_pins_as_pdn: sd2-pins-as-pdn {
> > +                     samsung,pins = "gpf2-0", "gpf2-1", "gpf2-2", "gpf2-3",
> > +                                    "gpf2-4", "gpf2-5";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <2>;
> > +             };
> > +
>
> No need for blank line.
>

Fixed.

> > +     };
> > +
> > +     /* CORE */
> > +     pinctrl@12070000 {
> > +             gpf0: gpf0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             sd0_clk: sd0-clk {
> > +                     samsung,pins = "gpf0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_cmd: sd0-cmd {
> > +                     samsung,pins = "gpf0-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_rdqs: sd0-rdqs {
> > +                     samsung,pins = "gpf0-2";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <1>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_nreset: sd0-nreset {
> > +                     samsung,pins = "gpf0-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_clk_fast_slew_rate_1x: sd0-clk_fast_slew_rate_1x {
> > +                     samsung,pins = "gpf0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <1>;
> > +             };
> > +
> > +             sd0_clk_fast_slew_rate_2x: sd0-clk_fast_slew_rate_2x {
> > +                     samsung,pins = "gpf0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd0_clk_fast_slew_rate_3x: sd0-clk_fast_slew_rate_3x {
> > +                     samsung,pins = "gpf0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sd0_clk_fast_slew_rate_4x: sd0-clk_fast_slew_rate_4x {
> > +                     samsung,pins = "gpf0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             gpf1: gpf1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             sd0_bus1: sd0-bus-width1 {
> > +                     samsung,pins = "gpf1-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_bus4: sd0-bus-width4 {
> > +                     samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +
> > +             sd0_bus8: sd0-bus-width8 {
> > +                     samsung,pins = "gpf1-4", "gpf1-5", "gpf1-6", "gpf1-7";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <3>;
> > +             };
> > +     };
> > +
> > +     /* PERI */
> > +     pinctrl@139b0000 {
> > +             gpg0: gpg0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpp0: gpp0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +             gpp1: gpp1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpp2: gpp2 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpg1: gpg1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpg2: gpg2 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpg3: gpg3 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpc0: gpc0 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             gpc1: gpc1 {
> > +                     gpio-controller;
> > +                     #gpio-cells = <2>;
> > +
> > +                     interrupt-controller;
> > +                     #interrupt-cells = <2>;
> > +             };
> > +
> > +             xclkout: xclkout {
> > +                     samsung,pins = "gpq0-2";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +             };
> > +
> > +             /* usi_hsi2c_0 */
>
> Comment seems to duplicate node name/label.
>

I think the comment is useful, maybe just designed poorly. It's
pointing out that this HS-I2C block is based on USI IP-core. Basically
we have 7 USI blocks in this SoC:
  - 5 USIs that pre-defined: HSI2C0, HSI2C1, HSI2C2, UART0 and SPI0;
although AFAIU they can be changed via System Register as well, but
seems like nobody does that
  - 2 USIs from CMGP blocks (CMGP0 and CMGP1), which are actually configurable

I will revise all such comments like this, just to show those blocks
are based on USI design:

    /* USI: HSI2C0 */

At least when I was bringing up this board, I was really confused for
long time about this USI business. So I'm sure some kind of comments
might be actually helpful. And that's one reason to keep
exynos850-usi.dtsi file: although I removed it for now, squashing it
into exynos850.dtsi, I think it might be a good idea to bring that
back once all USI nodes are described. Just thinking aloud :)

> > +             hsi2c0_bus: hsi2c0-bus {
> > +                     samsung,pins = "gpc1-0", "gpc1-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* usi_hsi2c_1 */
> > +             hsi2c1_bus: hsi2c1-bus {
> > +                     samsung,pins = "gpc1-2", "gpc1-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* usi_hsi2c_2 */
> > +             hsi2c2_bus: hsi2c2-bus {
> > +                     samsung,pins = "gpc1-4", "gpc1-5";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             /* usi_spi_0 */
> > +             spi0_bus: spi0-bus {
> > +                     samsung,pins = "gpp2-0", "gpp2-2", "gpp2-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi0_cs: spi0-cs {
> > +                     samsung,pins = "gpp2-1";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             spi0_cs_func: spi0-cs-func {
> > +                     samsung,pins = "gpp2-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             i2c0_bus: i2c0-bus {
> > +                     samsung,pins = "gpp0-0", "gpp0-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             i2c1_bus: i2c1-bus {
> > +                     samsung,pins = "gpp0-2", "gpp0-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             i2c2_bus: i2c2-bus {
> > +                     samsung,pins = "gpp0-4", "gpp0-5";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             i2c3_bus: i2c3-bus {
> > +                     samsung,pins = "gpp1-0", "gpp1-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             i2c4_bus: i2c4-bus {
> > +                     samsung,pins = "gpp1-2", "gpp1-3";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <3>;
> > +                     samsung,pin-drv = <0>;
> > +             };
> > +
> > +             fm_lna_en: fm-lna-en {
> > +                     samsung,pins = "gpg2-3";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-val = <0>;
> > +             };
> > +
> > +             sensor_mclk0_in: sensor-mclk0-in {
> > +                     samsung,pins = "gpc0-0";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk0_out: sensor-mclk0-out {
> > +                     samsung,pins = "gpc0-0";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <1>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk0_fn: sensor-mclk0-fn {
>
> No, seriously. What sensor is it? In SoC?
>

In this context, "sensor" stands for "camera". Of course, it resides
(or rather *may* reside) outside of the SoC. This is just useful pin
configuration for camera master clock lines (3 of them). On schematic
those 3 lines are grouped into "MCLK" block, and those pins main
function is CAM_MCLK. I guess it makes sense to keep those
definitions, as I doubt somebody would actually consider using those
lines for somthing else. Or I can remove all those sensor*
definitions, if you'd like, then those can be added later, when camera
support cames in.

> > +                     samsung,pins = "gpc0-0";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk1_in: sensor-mclk1-in {
> > +                     samsung,pins = "gpc0-1";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk1_out: sensor-mclk1-out {
> > +                     samsung,pins = "gpc0-1";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <1>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk1_fn: sensor-mclk1-fn {
> > +                     samsung,pins = "gpc0-1";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk2_in: sensor-mclk2-in {
> > +                     samsung,pins = "gpc0-2";
> > +                     samsung,pin-function = <0>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk2_out: sensor-mclk2-out {
> > +                     samsung,pins = "gpc0-2";
> > +                     samsung,pin-function = <1>;
> > +                     samsung,pin-pud = <1>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +
> > +             sensor_mclk2_fn: sensor-mclk2-fn {
> > +                     samsung,pins = "gpc0-2";
> > +                     samsung,pin-function = <2>;
> > +                     samsung,pin-pud = <0>;
> > +                     samsung,pin-drv = <2>;
> > +             };
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> > new file mode 100644
> > index 000000000000..fb243e0a6260
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's Exynos850 SoC USI device tree source
> > + *
> > + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> > + * Copyright (C) 2021 Linaro Ltd.
> > + *
> > + * Samsung's Exynos850 SoC USI channels are listed in this file as device tree
> > + * nodes.
>
> Why here not in exynos850.dtsi?
>

Yeah, you're right. As it's only serial_0 for now, I've moved that
into exynos850.dtsi and removed exynos850-usi.dtsi.

> > + */
> > +
> > +#include <dt-bindings/clock/exynos850.h>
> > +
> > +/ {
> > +     aliases {
> > +             uart0 = &serial_0;
> > +     };
> > +
> > +     /* USI_UART */
> > +     serial_0: uart@13820000 {
>
> This should ne in soc node.
>

Done.

> > +             compatible = "samsung,exynos850-uart";
> > +             reg = <0x0 0x13820000 0x100>;
> > +             interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&uart0_bus>;
> > +             clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>;
> > +             clock-names = "gate_uart_clk0", "uart";
> > +             status = "disabled";
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> > new file mode 100644
> > index 000000000000..ed2d1c8ae0c3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> > @@ -0,0 +1,245 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung Exynos850 SoC device tree source
> > + *
> > + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> > + * Copyright (C) 2021 Linaro Ltd.
> > + *
> > + * Samsung Exynos850 SoC device nodes are listed in this file.
> > + * Exynos based board files can include this file and provide
> > + * values for board specific bindings.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/exynos850.h>
> > +#include <dt-bindings/clock/exynos850.h>
> > +#include "exynos850-pinctrl.dtsi"
> > +#include "exynos850-usi.dtsi"
> > +
> > +/ {
>
> Add a comment like:
> /* Also known under engineering name exynos3830 */
>

Sure.

> > +     compatible = "samsung,exynos850";
>
> Undocumented compatible. Checkpatch should complain.
>

It actually doesn't, though it does complain about other undocumented
compatibles. I understand that it should be documented in
samsung-boards.yaml, but is it ok with you if I do that later, when
adding some actual board's dts? Just don't want to have two patches
for that.

> > +     interrupt-parent = <&gic>;
> > +     #address-cells = <2>;
> > +     #size-cells = <1>;
> > +
> > +     aliases {
> > +             pinctrl0 = &pinctrl_0;
> > +             pinctrl1 = &pinctrl_1;
> > +             pinctrl2 = &pinctrl_2;
> > +             pinctrl3 = &pinctrl_3;
> > +             pinctrl4 = &pinctrl_4;
> > +             pinctrl5 = &pinctrl_5;
> > +     };
> > +
> > +     cpus {
> > +             #address-cells = <2>;
> > +             #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@0000 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0000>;
>
> reg = <0x0 0x0>;
> (in following places similarly)
>

Done, and also fixed @0000 stuff, while at it.

> > +                     enable-method = "psci";
> > +             };
> > +             cpu1: cpu@0001 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0001>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu2: cpu@0002 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0002>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu3: cpu@0003 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0003>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu4: cpu@0004 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0100>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu5: cpu@0005 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0101>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu6: cpu@0006 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0102>;
> > +                     enable-method = "psci";
> > +             };
> > +             cpu7: cpu@0007 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,cortex-a55", "arm,armv8";
> > +                     reg = <0x0 0x0103>;
> > +                     enable-method = "psci";
> > +             };
> > +     };
> > +
> > +     psci {
> > +             compatible = "arm,psci-1.0";
> > +             method = "smc";
> > +     };
> > +
> > +     gic: interrupt-controller@12a00000 {
> > +             compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
> > +             #interrupt-cells = <3>;
> > +             #address-cells = <0>;
> > +             interrupt-controller;
> > +             reg = <0x0 0x12a01000 0x1000>,
> > +                   <0x0 0x12a02000 0x1000>,
> > +                   <0x0 0x12a04000 0x2000>,
> > +                   <0x0 0x12a06000 0x2000>;
> > +             interrupts = <GIC_PPI 9
> > +                             (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> > +     };
> > +
> > +     timer {
> > +             compatible = "arm,armv8-timer";
> > +             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>;
> > +             use-clocksource-only;
> > +             use-physical-timer;
> > +     };
> > +
>
> All below should be under soc node.
>

Done.

> Please don't write new DTS/DTSI from scratch but use exynos5433.dtsi as
> template/example.
>

Well, I didn't implement that from scratch :) As you probably figured,
I reworked the vendor's dts (heavily, that is). But yeah, I definitely
re-checked with existing DTs this time (exynos5433 and exynos7
mostly), and tried to follow best practices.

> > +     clock: clock-controller@0x120e0000 {
> > +             compatible = "samsung,exynos850-clock";
> > +             reg = <0x0 0x120e0000 0x8000>;
> > +             #clock-cells = <1>;
> > +     };
> > +
> > +     /* ALIVE */
> > +     pinctrl_0: pinctrl@11850000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x11850000 0x1000>;
> > +             interrupts = <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +             wakeup-interrupt-controller {
> > +                     compatible = "samsung,exynos7-wakeup-eint";
> > +             };
> > +     };
> > +
> > +     /* CMGP */
> > +     pinctrl_1: pinctrl@11c30000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x11c30000 0x1000>;
> > +             interrupts =
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM06 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI INTREQ__CMGP_EXT_INTM07 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +             wakeup-interrupt-controller {
> > +                     compatible = "samsung,exynos7-wakeup-eint";
> > +             };
> > +     };
> > +
> > +     /* AUD */
> > +     pinctrl_2: pinctrl@14a60000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x14a60000 0x1000>;
> > +     };
> > +
> > +     /* HSI */
> > +     pinctrl_3: pinctrl@13430000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x13430000 0x1000>;
> > +             interrupts = <GIC_SPI INTREQ__GPIO_HSI IRQ_TYPE_LEVEL_HIGH>;
> > +     };
> > +
> > +     /* CORE */
> > +     pinctrl_4: pinctrl@12070000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x12070000 0x1000>;
> > +             interrupts = <GIC_SPI INTREQ__GPIO_CORE IRQ_TYPE_LEVEL_HIGH>;
> > +     };
> > +
> > +     /* PERI */
> > +     pinctrl_5: pinctrl@139b0000 {
> > +             compatible = "samsung,exynos850-pinctrl";
> > +             reg = <0x0 0x139b0000 0x1000>;
> > +             interrupts = <GIC_SPI INTREQ__GPIO_PERI IRQ_TYPE_LEVEL_HIGH>;
> > +     };
> > +};
> >
>
>
> Best regards,
> Krzysztof

Krzysztof Kozlowski Aug. 6, 2021, 7:48 a.m. UTC | #15

On 06/08/2021 01:06, Sam Protsenko wrote:
> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
> <[e-mail]> wrote:
> 
>>>
>>> This patch adds minimal SoC support. Particular board device tree files
>>> can include exynos850.dtsi file to get SoC related nodes, and then
>>> reference those nodes further as needed.
>>>
>>> Signed-off-by: Sam Protsenko <[e-mail]>
>>> ---
>>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>>
>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
>> DTS, otherwise no one is able to verify it even compiles.
>>
> 
> This device is not available for purchase yet. I'll send the patch for
> board dts once it's announced. I can do all the testing for now, if
> you have any specific requests. Would it be possible for us to review
> and apply only SoC support for now? Will send v2 soon...

What you propose is equal to adding a driver (C source code) without
ability to compile it. What's the point of having it in the kernel? It's
unverifiable, unbuildable and unusable.

We can review the DTSI however merging has to be with a DTS. Usually the
SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
for Exynos850? Otherwise if you cannot disclose the actual board, the
DTSI will have to wait. You can submit drivers, though.

> 
>>>  3 files changed, 1057 insertions(+)
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>> new file mode 100644
>>> index 000000000000..4cf0a22cc6db
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
>>> @@ -0,0 +1,782 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Samsung's Exynos850 SoC pin-mux and pin-config device tree source
>>> + *
>>> + * Copyright (C) 2017 Samsung Electronics Co., Ltd.
>>> + * Copyright (C) 2021 Linaro Ltd.
>>> + *
>>> + * Samsung's Exynos850 SoC pin-mux and pin-config options are listed as device
>>> + * tree nodes in this file.
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/exynos850.h>
>>> +
>>> +/ {
>>> +     /* ALIVE */
>>> +     pinctrl@11850000 {
>>> +             gpa0: gpa0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>
>> That's odd a little, why three cells? How this would be used/referenced?
>>
> 
> Fixed. You are right, irq_domain_xlate_twocell() is used in
> pinctrl-exynos.c. Btw, that fixed the use-case when gpX is specified
> as interrupt-parrent for other nodes.
> 
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpa1: gpa1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpa2: gpa2 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpa3: gpa3 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpa4: gpa4 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT32 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT33 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT34 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI INTREQ__ALIVE_EINT35 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpq0: gpq0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             /* USI_PERI_UART_DBG */
>>> +             uart0_bus: uart0-bus {
>>> +                     samsung,pins = "gpq0-0", "gpq0-1";
>>> +                     samsung,pin-function = <2>;
>>
>> EXYNOS_PIN_FUNC_2
>>
> 
> Done, thanks.
> 
>>> +                     samsung,pin-pud = <0>;
>>
>> EXYNOS_PIN_PULL_xx?
>>
> 
> Yes, replaced pin-pud values with these constants, thanks. Won't touch
> pin-drv properties, though, as different domains have different
> meanings for the same values, so don't want to introduce the whole
> bunch of consts.
> 
>>> +             };
>>> +
>>> +             decon_f_te_on: decon_f_te_on {
>>
>> 1. Where is it used?
>> 2. Use hyphens in node names.
>> Please build it with W=1 and fix the warnings.
>>
> 
> decon* nodes seem like leftover from vendor kernel. Those are not
> connected anywhere on my board, so I removed those nodes.
> 
> Also fixed node names as you suggested, and fixed all stuff found with
> W=1 build, thanks for reminding me that.
> 
>>> +                     samsung,pins = "gpa4-1";
>>
>> Order the nodes based on first pin name, so:
>> i2c5_bus
>> i2c6_bus
>> decon_f_te_on
>> uart0_bus
>>
> 
> Done. Also rearranged the nodes in that way for other pinctrl domains.
> 
>>> +                     samsung,pin-function = <0xf>;
>>> +             };
>>> +
>>> +             decon_f_te_off: decon_f_te_off {
>>
>> Where is it used?
>>
> 
> Removed, as explained above.
> 
>>> +                     samsung,pins = "gpa4-1";
>>> +                     samsung,pin-function = <0x0>;
>>> +             };
>>> +
>>> +             /* I2C_5 | CAM_PMIC_I2C */
>>
>> This comment is confusing. I2C-5 is obvious from node name and label.
>> CAM_PMIC_I2C does not look like property of SoC but board.
>>
> 
> Yeah, unfortunately this confusing I2C naming (CAM_PMIC_I2C and
> MOTOR_I2C) actually comes from TRM and SoC schematic symbol.

Schematic symbol of SoC? Interesting document... What do you have there?
Internals of SoC?


> So in
> Device Tree it's called just i2c5 and i2c6 (because it's a regular I2C
> really), but in TRM it's a mix of both names. I guess it's just a poor
> pin function naming, done by SoC designers.
> 
> That said, I suggest changing these comments to something like this:
> 
>     /* I2C5 (also called CAM_PMIC_I2C in TRM) */
>     /* I2C6 (also called MOTOR_I2C in TRM) */

OK, assuming that by TRM you mean Exynos850 TRM.

> 
>>> +             i2c5_bus: i2c5-bus {
>>> +                     samsung,pins = "gpa3-5", "gpa3-6";
>>> +                     samsung,pin-function = <3>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* I2C_6 | MOTOR_I2C */
>>> +             i2c6_bus: i2c6-bus {
>>> +                     samsung,pins = "gpa3-7", "gpa4-0";
>>> +                     samsung,pin-function = <3>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +     };
>>> +
>>> +     /* CMGP */
>>> +     pinctrl@11c30000 {
>>> +             gpm0: gpm0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpm1: gpm1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpm2: gpm2 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpm3: gpm3 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpm4: gpm4 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             gpm5: gpm5 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-parent = <&gic>;
>>> +                     interrupts =
>>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>;
>>> +             };
>>> +
>>> +             /* usi_cmgp00 */
>>> +             hsi2c3_bus: hsi2c3-bus {
>>> +                     samsung,pins = "gpm0-0", "gpm1-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* usi_cmgp01 */
>>> +             hsi2c4_bus: hsi2c4-bus {
>>> +                     samsung,pins = "gpm4-0", "gpm5-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* spi usi_cmgp00 */
>>> +             spi1_bus: spi1-bus {
>>> +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi1_cs: spi1-cs {
>>> +                     samsung,pins = "gpm3-0";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi1_cs_func: spi1-cs-func {
>>> +                     samsung,pins = "gpm3-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* spi usi_cmgp01 */
>>> +             spi2_bus: spi2-bus {
>>> +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi2_cs: spi2-cs {
>>> +                     samsung,pins = "gpm7-0";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi2_cs_func: spi2-cs-func {
>>> +                     samsung,pins = "gpm7-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* usi_cmgp00_uart */
>>> +             uart1_bus_single: uart1-bus {
>>> +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0", "gpm3-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +             };
>>> +
>>> +             uart1_bus_dual: uart1-bus-dual {
>>> +                     samsung,pins = "gpm0-0", "gpm1-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +             };
>>> +
>>> +             /* usi_cmgp01_uart */
>>> +             uart2_bus_single: uart2-bus {
>>> +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0", "gpm7-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +             };
>>> +
>>> +             uart2_bus_dual: uart2-bus-dual {
>>> +                     samsung,pins = "gpm4-0", "gpm5-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +             };
>>> +     };
>>> +
>>> +     /* AUD */
>>> +     pinctrl@14a60000 {
>>> +             gpb0: gpb0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpb1: gpb1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             aud_codec_mclk: aud-codec-mclk {
>>> +                     samsung,pins = "gpb0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_codec_mclk_idle: aud-codec-mclk-idle {
>>> +                     samsung,pins = "gpb0-0";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_i2s0_bus: aud-i2s0-bus {
>>> +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_i2s0_idle: aud-i2s0-idle {
>>> +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_i2s1_bus: aud-i2s1-bus {
>>> +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_i2s1_idle: aud-i2s1-idle {
>>> +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_fm_bus: aud-fm-bus {
>>> +                     samsung,pins = "gpb1-4";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +
>>> +             aud_fm_idle: aud-fm-idle {
>>> +                     samsung,pins = "gpb1-4";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <1>;
>>> +             };
>>> +     };
>>> +
>>> +     /* HSI */
>>> +     pinctrl@13430000 {
>>> +             gpf2: gpf2 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             sd2_clk: sd2-clk {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd2_cmd: sd2-cmd {
>>> +                     samsung,pins = "gpf2-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <2>;
>>> +              };
>>> +
>>> +             sd2_bus1: sd2-bus-width1 {
>>> +                     samsung,pins = "gpf2-2";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd2_bus4: sd2-bus-width4 {
>>> +                     samsung,pins = "gpf2-3", "gpf2-4", "gpf2-5";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_1x: sd2-clk_fast_slew_rate_1x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_1_5x: sd2-clk_fast_slew_rate_1_5x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <1>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_2x: sd2-clk_fast_slew_rate_2x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_2_5x: sd2-clk_fast_slew_rate_2_5x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_3x: sd2-clk_fast_slew_rate_3x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <4>;
>>> +             };
>>> +
>>> +             sd2_clk_fast_slew_rate_4x: sd2-clk_fast_slew_rate_4x {
>>> +                     samsung,pins = "gpf2-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <5>;
>>> +             };
>>> +
>>> +             sd2_pins_as_pdn: sd2-pins-as-pdn {
>>> +                     samsung,pins = "gpf2-0", "gpf2-1", "gpf2-2", "gpf2-3",
>>> +                                    "gpf2-4", "gpf2-5";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <2>;
>>> +             };
>>> +
>>
>> No need for blank line.
>>
> 
> Fixed.
> 
>>> +     };
>>> +
>>> +     /* CORE */
>>> +     pinctrl@12070000 {
>>> +             gpf0: gpf0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             sd0_clk: sd0-clk {
>>> +                     samsung,pins = "gpf0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_cmd: sd0-cmd {
>>> +                     samsung,pins = "gpf0-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_rdqs: sd0-rdqs {
>>> +                     samsung,pins = "gpf0-2";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <1>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_nreset: sd0-nreset {
>>> +                     samsung,pins = "gpf0-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_clk_fast_slew_rate_1x: sd0-clk_fast_slew_rate_1x {
>>> +                     samsung,pins = "gpf0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <1>;
>>> +             };
>>> +
>>> +             sd0_clk_fast_slew_rate_2x: sd0-clk_fast_slew_rate_2x {
>>> +                     samsung,pins = "gpf0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd0_clk_fast_slew_rate_3x: sd0-clk_fast_slew_rate_3x {
>>> +                     samsung,pins = "gpf0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sd0_clk_fast_slew_rate_4x: sd0-clk_fast_slew_rate_4x {
>>> +                     samsung,pins = "gpf0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             gpf1: gpf1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             sd0_bus1: sd0-bus-width1 {
>>> +                     samsung,pins = "gpf1-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_bus4: sd0-bus-width4 {
>>> +                     samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +
>>> +             sd0_bus8: sd0-bus-width8 {
>>> +                     samsung,pins = "gpf1-4", "gpf1-5", "gpf1-6", "gpf1-7";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <3>;
>>> +             };
>>> +     };
>>> +
>>> +     /* PERI */
>>> +     pinctrl@139b0000 {
>>> +             gpg0: gpg0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpp0: gpp0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +             gpp1: gpp1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpp2: gpp2 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpg1: gpg1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpg2: gpg2 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpg3: gpg3 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpc0: gpc0 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             gpc1: gpc1 {
>>> +                     gpio-controller;
>>> +                     #gpio-cells = <2>;
>>> +
>>> +                     interrupt-controller;
>>> +                     #interrupt-cells = <2>;
>>> +             };
>>> +
>>> +             xclkout: xclkout {
>>> +                     samsung,pins = "gpq0-2";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +             };
>>> +
>>> +             /* usi_hsi2c_0 */
>>
>> Comment seems to duplicate node name/label.
>>
> 
> I think the comment is useful, maybe just designed poorly. It's
> pointing out that this HS-I2C block is based on USI IP-core. Basically
> we have 7 USI blocks in this SoC:
>   - 5 USIs that pre-defined: HSI2C0, HSI2C1, HSI2C2, UART0 and SPI0;
> although AFAIU they can be changed via System Register as well, but
> seems like nobody does that
>   - 2 USIs from CMGP blocks (CMGP0 and CMGP1), which are actually configurable
> 
> I will revise all such comments like this, just to show those blocks
> are based on USI design:
> 
>     /* USI: HSI2C0 */

OK

> 
> At least when I was bringing up this board, I was really confused for
> long time about this USI business. So I'm sure some kind of comments
> might be actually helpful. And that's one reason to keep
> exynos850-usi.dtsi file: although I removed it for now, squashing it
> into exynos850.dtsi, I think it might be a good idea to bring that
> back once all USI nodes are described. Just thinking aloud :)
> 
>>> +             hsi2c0_bus: hsi2c0-bus {
>>> +                     samsung,pins = "gpc1-0", "gpc1-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* usi_hsi2c_1 */
>>> +             hsi2c1_bus: hsi2c1-bus {
>>> +                     samsung,pins = "gpc1-2", "gpc1-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* usi_hsi2c_2 */
>>> +             hsi2c2_bus: hsi2c2-bus {
>>> +                     samsung,pins = "gpc1-4", "gpc1-5";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             /* usi_spi_0 */
>>> +             spi0_bus: spi0-bus {
>>> +                     samsung,pins = "gpp2-0", "gpp2-2", "gpp2-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi0_cs: spi0-cs {
>>> +                     samsung,pins = "gpp2-1";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             spi0_cs_func: spi0-cs-func {
>>> +                     samsung,pins = "gpp2-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             i2c0_bus: i2c0-bus {
>>> +                     samsung,pins = "gpp0-0", "gpp0-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             i2c1_bus: i2c1-bus {
>>> +                     samsung,pins = "gpp0-2", "gpp0-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             i2c2_bus: i2c2-bus {
>>> +                     samsung,pins = "gpp0-4", "gpp0-5";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             i2c3_bus: i2c3-bus {
>>> +                     samsung,pins = "gpp1-0", "gpp1-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             i2c4_bus: i2c4-bus {
>>> +                     samsung,pins = "gpp1-2", "gpp1-3";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <3>;
>>> +                     samsung,pin-drv = <0>;
>>> +             };
>>> +
>>> +             fm_lna_en: fm-lna-en {
>>> +                     samsung,pins = "gpg2-3";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-val = <0>;
>>> +             };
>>> +
>>> +             sensor_mclk0_in: sensor-mclk0-in {
>>> +                     samsung,pins = "gpc0-0";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk0_out: sensor-mclk0-out {
>>> +                     samsung,pins = "gpc0-0";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <1>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk0_fn: sensor-mclk0-fn {
>>
>> No, seriously. What sensor is it? In SoC?
>>
> 
> In this context, "sensor" stands for "camera". Of course, it resides
> (or rather *may* reside) outside of the SoC. This is just useful pin
> configuration for camera master clock lines (3 of them). On schematic
> those 3 lines are grouped into "MCLK" block, and those pins main
> function is CAM_MCLK. I guess it makes sense to keep those
> definitions, as I doubt somebody would actually consider using those
> lines for somthing else. Or I can remove all those sensor*
> definitions, if you'd like, then those can be added later, when camera
> support cames in.

Hmmm, indeed we define in pinctrl useful typical functions and I
understand this one is such. Let's keep them then.

> 
>>> +                     samsung,pins = "gpc0-0";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk1_in: sensor-mclk1-in {
>>> +                     samsung,pins = "gpc0-1";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk1_out: sensor-mclk1-out {
>>> +                     samsung,pins = "gpc0-1";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <1>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk1_fn: sensor-mclk1-fn {
>>> +                     samsung,pins = "gpc0-1";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk2_in: sensor-mclk2-in {
>>> +                     samsung,pins = "gpc0-2";
>>> +                     samsung,pin-function = <0>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk2_out: sensor-mclk2-out {
>>> +                     samsung,pins = "gpc0-2";
>>> +                     samsung,pin-function = <1>;
>>> +                     samsung,pin-pud = <1>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +
>>> +             sensor_mclk2_fn: sensor-mclk2-fn {
>>> +                     samsung,pins = "gpc0-2";
>>> +                     samsung,pin-function = <2>;
>>> +                     samsung,pin-pud = <0>;
>>> +                     samsung,pin-drv = <2>;
>>> +             };
>>> +     };
>>> +};
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>>> new file mode 100644
>>> index 000000000000..fb243e0a6260
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
>>> @@ -0,0 +1,30 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Samsung's Exynos850 SoC USI device tree source
>>> + *
>>> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
>>> + * Copyright (C) 2021 Linaro Ltd.
>>> + *
>>> + * Samsung's Exynos850 SoC USI channels are listed in this file as device tree
>>> + * nodes.
>>
>> Why here not in exynos850.dtsi?
>>
> 
> Yeah, you're right. As it's only serial_0 for now, I've moved that
> into exynos850.dtsi and removed exynos850-usi.dtsi.
> 
>>> + */
>>> +
>>> +#include <dt-bindings/clock/exynos850.h>
>>> +
>>> +/ {
>>> +     aliases {
>>> +             uart0 = &serial_0;
>>> +     };
>>> +
>>> +     /* USI_UART */
>>> +     serial_0: uart@13820000 {
>>
>> This should ne in soc node.
>>
> 
> Done.
> 
>>> +             compatible = "samsung,exynos850-uart";
>>> +             reg = <0x0 0x13820000 0x100>;
>>> +             interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&uart0_bus>;
>>> +             clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>;
>>> +             clock-names = "gate_uart_clk0", "uart";
>>> +             status = "disabled";
>>> +     };
>>> +};
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
>>> new file mode 100644
>>> index 000000000000..ed2d1c8ae0c3
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
>>> @@ -0,0 +1,245 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Samsung Exynos850 SoC device tree source
>>> + *
>>> + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
>>> + * Copyright (C) 2021 Linaro Ltd.
>>> + *
>>> + * Samsung Exynos850 SoC device nodes are listed in this file.
>>> + * Exynos based board files can include this file and provide
>>> + * values for board specific bindings.
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/exynos850.h>
>>> +#include <dt-bindings/clock/exynos850.h>
>>> +#include "exynos850-pinctrl.dtsi"
>>> +#include "exynos850-usi.dtsi"
>>> +
>>> +/ {
>>
>> Add a comment like:
>> /* Also known under engineering name exynos3830 */
>>
> 
> Sure.
> 
>>> +     compatible = "samsung,exynos850";
>>
>> Undocumented compatible. Checkpatch should complain.
>>
> 
> It actually doesn't, though it does complain about other undocumented
> compatibles. I understand that it should be documented in
> samsung-boards.yaml, but is it ok with you if I do that later, when
> adding some actual board's dts? Just don't want to have two patches
> for that.

We're back to the first question - it needs to come with DTS and then
you document both compatibles.

Best regards,
Krzysztof

Sam Protsenko Aug. 6, 2021, 12:07 p.m. UTC | #16

On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 06/08/2021 01:06, Sam Protsenko wrote:
> > On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
> > <[e-mail]> wrote:
> >
> >>>
> >>> This patch adds minimal SoC support. Particular board device tree files
> >>> can include exynos850.dtsi file to get SoC related nodes, and then
> >>> reference those nodes further as needed.
> >>>
> >>> Signed-off-by: Sam Protsenko <[e-mail]>
> >>> ---
> >>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >>
> >> Not buildable. Missing Makefile, missing DTS. Please submit with initial
> >> DTS, otherwise no one is able to verify it even compiles.
> >>
> >
> > This device is not available for purchase yet. I'll send the patch for
> > board dts once it's announced. I can do all the testing for now, if
> > you have any specific requests. Would it be possible for us to review
> > and apply only SoC support for now? Will send v2 soon...
>
> What you propose is equal to adding a driver (C source code) without
> ability to compile it. What's the point of having it in the kernel? It's
> unverifiable, unbuildable and unusable.
>

Yes, I understand. That's adding code with no users, and it's not a
good practice.

> We can review the DTSI however merging has to be with a DTS. Usually the
> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
> for Exynos850? Otherwise if you cannot disclose the actual board, the
> DTSI will have to wait. You can submit drivers, though.
>

Sure, let's go this way. I'll send v2 soon. Improving patches and
having Reviewed-by tag for those would good enough for me at this
point. I'll continue to prepare another Exynos850 related patches
until the actual board is announced, like proper clock driver, reset,
MMC, etc. Is it ok if I send those for a review too (so I can fix all
issues ahead)? And should I maybe add RFC tag for those?

Anyway, once the board is out, I'll re-send this series including
board's dts, as you suggested. Good thing is, I'll have some time to
address any outstanding issues by that time, hopefully.

Thanks!

> >
> >>>  3 files changed, 1057 insertions(+)
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi
> >>>
> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>> new file mode 100644
> >>> index 000000000000..4cf0a22cc6db
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
> >>> @@ -0,0 +1,782 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Samsung's Exynos850 SoC pin-mux and pin-config device tree source
> >>> + *
> >>> + * Copyright (C) 2017 Samsung Electronics Co., Ltd.
> >>> + * Copyright (C) 2021 Linaro Ltd.
> >>> + *
> >>> + * Samsung's Exynos850 SoC pin-mux and pin-config options are listed as device
> >>> + * tree nodes in this file.
> >>> + */
> >>> +
> >>> +#include <dt-bindings/interrupt-controller/exynos850.h>
> >>> +
> >>> +/ {
> >>> +     /* ALIVE */
> >>> +     pinctrl@11850000 {
> >>> +             gpa0: gpa0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>
> >> That's odd a little, why three cells? How this would be used/referenced?
> >>
> >
> > Fixed. You are right, irq_domain_xlate_twocell() is used in
> > pinctrl-exynos.c. Btw, that fixed the use-case when gpX is specified
> > as interrupt-parrent for other nodes.
> >
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpa1: gpa1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpa2: gpa2 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpa3: gpa3 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpa4: gpa4 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT32 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT33 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT34 IRQ_TYPE_LEVEL_HIGH>,
> >>> +                         <GIC_SPI INTREQ__ALIVE_EINT35 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpq0: gpq0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             /* USI_PERI_UART_DBG */
> >>> +             uart0_bus: uart0-bus {
> >>> +                     samsung,pins = "gpq0-0", "gpq0-1";
> >>> +                     samsung,pin-function = <2>;
> >>
> >> EXYNOS_PIN_FUNC_2
> >>
> >
> > Done, thanks.
> >
> >>> +                     samsung,pin-pud = <0>;
> >>
> >> EXYNOS_PIN_PULL_xx?
> >>
> >
> > Yes, replaced pin-pud values with these constants, thanks. Won't touch
> > pin-drv properties, though, as different domains have different
> > meanings for the same values, so don't want to introduce the whole
> > bunch of consts.
> >
> >>> +             };
> >>> +
> >>> +             decon_f_te_on: decon_f_te_on {
> >>
> >> 1. Where is it used?
> >> 2. Use hyphens in node names.
> >> Please build it with W=1 and fix the warnings.
> >>
> >
> > decon* nodes seem like leftover from vendor kernel. Those are not
> > connected anywhere on my board, so I removed those nodes.
> >
> > Also fixed node names as you suggested, and fixed all stuff found with
> > W=1 build, thanks for reminding me that.
> >
> >>> +                     samsung,pins = "gpa4-1";
> >>
> >> Order the nodes based on first pin name, so:
> >> i2c5_bus
> >> i2c6_bus
> >> decon_f_te_on
> >> uart0_bus
> >>
> >
> > Done. Also rearranged the nodes in that way for other pinctrl domains.
> >
> >>> +                     samsung,pin-function = <0xf>;
> >>> +             };
> >>> +
> >>> +             decon_f_te_off: decon_f_te_off {
> >>
> >> Where is it used?
> >>
> >
> > Removed, as explained above.
> >
> >>> +                     samsung,pins = "gpa4-1";
> >>> +                     samsung,pin-function = <0x0>;
> >>> +             };
> >>> +
> >>> +             /* I2C_5 | CAM_PMIC_I2C */
> >>
> >> This comment is confusing. I2C-5 is obvious from node name and label.
> >> CAM_PMIC_I2C does not look like property of SoC but board.
> >>
> >
> > Yeah, unfortunately this confusing I2C naming (CAM_PMIC_I2C and
> > MOTOR_I2C) actually comes from TRM and SoC schematic symbol.
>
> Schematic symbol of SoC? Interesting document... What do you have there?
> Internals of SoC?
>

Yeah, of course schematic symbol can't be considered a document :)
What I meant was it might be confusing for users to see one bus name
in TRM and schematics, and another one in device tree.

>
> > So in
> > Device Tree it's called just i2c5 and i2c6 (because it's a regular I2C
> > really), but in TRM it's a mix of both names. I guess it's just a poor
> > pin function naming, done by SoC designers.
> >
> > That said, I suggest changing these comments to something like this:
> >
> >     /* I2C5 (also called CAM_PMIC_I2C in TRM) */
> >     /* I2C6 (also called MOTOR_I2C in TRM) */
>
> OK, assuming that by TRM you mean Exynos850 TRM.
>
> >
> >>> +             i2c5_bus: i2c5-bus {
> >>> +                     samsung,pins = "gpa3-5", "gpa3-6";
> >>> +                     samsung,pin-function = <3>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* I2C_6 | MOTOR_I2C */
> >>> +             i2c6_bus: i2c6-bus {
> >>> +                     samsung,pins = "gpa3-7", "gpa4-0";
> >>> +                     samsung,pin-function = <3>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /* CMGP */
> >>> +     pinctrl@11c30000 {
> >>> +             gpm0: gpm0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpm1: gpm1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpm2: gpm2 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpm3: gpm3 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpm4: gpm4 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             gpm5: gpm5 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <3>;
> >>> +                     interrupt-parent = <&gic>;
> >>> +                     interrupts =
> >>> +                       <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             };
> >>> +
> >>> +             /* usi_cmgp00 */
> >>> +             hsi2c3_bus: hsi2c3-bus {
> >>> +                     samsung,pins = "gpm0-0", "gpm1-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_cmgp01 */
> >>> +             hsi2c4_bus: hsi2c4-bus {
> >>> +                     samsung,pins = "gpm4-0", "gpm5-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* spi usi_cmgp00 */
> >>> +             spi1_bus: spi1-bus {
> >>> +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi1_cs: spi1-cs {
> >>> +                     samsung,pins = "gpm3-0";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi1_cs_func: spi1-cs-func {
> >>> +                     samsung,pins = "gpm3-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* spi usi_cmgp01 */
> >>> +             spi2_bus: spi2-bus {
> >>> +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi2_cs: spi2-cs {
> >>> +                     samsung,pins = "gpm7-0";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi2_cs_func: spi2-cs-func {
> >>> +                     samsung,pins = "gpm7-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_cmgp00_uart */
> >>> +             uart1_bus_single: uart1-bus {
> >>> +                     samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0", "gpm3-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +             };
> >>> +
> >>> +             uart1_bus_dual: uart1-bus-dual {
> >>> +                     samsung,pins = "gpm0-0", "gpm1-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_cmgp01_uart */
> >>> +             uart2_bus_single: uart2-bus {
> >>> +                     samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0", "gpm7-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +             };
> >>> +
> >>> +             uart2_bus_dual: uart2-bus-dual {
> >>> +                     samsung,pins = "gpm4-0", "gpm5-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /* AUD */
> >>> +     pinctrl@14a60000 {
> >>> +             gpb0: gpb0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpb1: gpb1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             aud_codec_mclk: aud-codec-mclk {
> >>> +                     samsung,pins = "gpb0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_codec_mclk_idle: aud-codec-mclk-idle {
> >>> +                     samsung,pins = "gpb0-0";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_i2s0_bus: aud-i2s0-bus {
> >>> +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_i2s0_idle: aud-i2s0-idle {
> >>> +                     samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_i2s1_bus: aud-i2s1-bus {
> >>> +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_i2s1_idle: aud-i2s1-idle {
> >>> +                     samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_fm_bus: aud-fm-bus {
> >>> +                     samsung,pins = "gpb1-4";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +
> >>> +             aud_fm_idle: aud-fm-idle {
> >>> +                     samsung,pins = "gpb1-4";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /* HSI */
> >>> +     pinctrl@13430000 {
> >>> +             gpf2: gpf2 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             sd2_clk: sd2-clk {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd2_cmd: sd2-cmd {
> >>> +                     samsung,pins = "gpf2-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +              };
> >>> +
> >>> +             sd2_bus1: sd2-bus-width1 {
> >>> +                     samsung,pins = "gpf2-2";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd2_bus4: sd2-bus-width4 {
> >>> +                     samsung,pins = "gpf2-3", "gpf2-4", "gpf2-5";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_1x: sd2-clk_fast_slew_rate_1x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_1_5x: sd2-clk_fast_slew_rate_1_5x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <1>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_2x: sd2-clk_fast_slew_rate_2x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_2_5x: sd2-clk_fast_slew_rate_2_5x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_3x: sd2-clk_fast_slew_rate_3x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <4>;
> >>> +             };
> >>> +
> >>> +             sd2_clk_fast_slew_rate_4x: sd2-clk_fast_slew_rate_4x {
> >>> +                     samsung,pins = "gpf2-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <5>;
> >>> +             };
> >>> +
> >>> +             sd2_pins_as_pdn: sd2-pins-as-pdn {
> >>> +                     samsung,pins = "gpf2-0", "gpf2-1", "gpf2-2", "gpf2-3",
> >>> +                                    "gpf2-4", "gpf2-5";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <2>;
> >>> +             };
> >>> +
> >>
> >> No need for blank line.
> >>
> >
> > Fixed.
> >
> >>> +     };
> >>> +
> >>> +     /* CORE */
> >>> +     pinctrl@12070000 {
> >>> +             gpf0: gpf0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             sd0_clk: sd0-clk {
> >>> +                     samsung,pins = "gpf0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_cmd: sd0-cmd {
> >>> +                     samsung,pins = "gpf0-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_rdqs: sd0-rdqs {
> >>> +                     samsung,pins = "gpf0-2";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_nreset: sd0-nreset {
> >>> +                     samsung,pins = "gpf0-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_clk_fast_slew_rate_1x: sd0-clk_fast_slew_rate_1x {
> >>> +                     samsung,pins = "gpf0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <1>;
> >>> +             };
> >>> +
> >>> +             sd0_clk_fast_slew_rate_2x: sd0-clk_fast_slew_rate_2x {
> >>> +                     samsung,pins = "gpf0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd0_clk_fast_slew_rate_3x: sd0-clk_fast_slew_rate_3x {
> >>> +                     samsung,pins = "gpf0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sd0_clk_fast_slew_rate_4x: sd0-clk_fast_slew_rate_4x {
> >>> +                     samsung,pins = "gpf0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             gpf1: gpf1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             sd0_bus1: sd0-bus-width1 {
> >>> +                     samsung,pins = "gpf1-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_bus4: sd0-bus-width4 {
> >>> +                     samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +
> >>> +             sd0_bus8: sd0-bus-width8 {
> >>> +                     samsung,pins = "gpf1-4", "gpf1-5", "gpf1-6", "gpf1-7";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <3>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /* PERI */
> >>> +     pinctrl@139b0000 {
> >>> +             gpg0: gpg0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpp0: gpp0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +             gpp1: gpp1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpp2: gpp2 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpg1: gpg1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpg2: gpg2 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpg3: gpg3 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpc0: gpc0 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             gpc1: gpc1 {
> >>> +                     gpio-controller;
> >>> +                     #gpio-cells = <2>;
> >>> +
> >>> +                     interrupt-controller;
> >>> +                     #interrupt-cells = <2>;
> >>> +             };
> >>> +
> >>> +             xclkout: xclkout {
> >>> +                     samsung,pins = "gpq0-2";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_hsi2c_0 */
> >>
> >> Comment seems to duplicate node name/label.
> >>
> >
> > I think the comment is useful, maybe just designed poorly. It's
> > pointing out that this HS-I2C block is based on USI IP-core. Basically
> > we have 7 USI blocks in this SoC:
> >   - 5 USIs that pre-defined: HSI2C0, HSI2C1, HSI2C2, UART0 and SPI0;
> > although AFAIU they can be changed via System Register as well, but
> > seems like nobody does that
> >   - 2 USIs from CMGP blocks (CMGP0 and CMGP1), which are actually configurable
> >
> > I will revise all such comments like this, just to show those blocks
> > are based on USI design:
> >
> >     /* USI: HSI2C0 */
>
> OK
>
> >
> > At least when I was bringing up this board, I was really confused for
> > long time about this USI business. So I'm sure some kind of comments
> > might be actually helpful. And that's one reason to keep
> > exynos850-usi.dtsi file: although I removed it for now, squashing it
> > into exynos850.dtsi, I think it might be a good idea to bring that
> > back once all USI nodes are described. Just thinking aloud :)
> >
> >>> +             hsi2c0_bus: hsi2c0-bus {
> >>> +                     samsung,pins = "gpc1-0", "gpc1-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_hsi2c_1 */
> >>> +             hsi2c1_bus: hsi2c1-bus {
> >>> +                     samsung,pins = "gpc1-2", "gpc1-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_hsi2c_2 */
> >>> +             hsi2c2_bus: hsi2c2-bus {
> >>> +                     samsung,pins = "gpc1-4", "gpc1-5";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             /* usi_spi_0 */
> >>> +             spi0_bus: spi0-bus {
> >>> +                     samsung,pins = "gpp2-0", "gpp2-2", "gpp2-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi0_cs: spi0-cs {
> >>> +                     samsung,pins = "gpp2-1";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             spi0_cs_func: spi0-cs-func {
> >>> +                     samsung,pins = "gpp2-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             i2c0_bus: i2c0-bus {
> >>> +                     samsung,pins = "gpp0-0", "gpp0-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             i2c1_bus: i2c1-bus {
> >>> +                     samsung,pins = "gpp0-2", "gpp0-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             i2c2_bus: i2c2-bus {
> >>> +                     samsung,pins = "gpp0-4", "gpp0-5";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             i2c3_bus: i2c3-bus {
> >>> +                     samsung,pins = "gpp1-0", "gpp1-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             i2c4_bus: i2c4-bus {
> >>> +                     samsung,pins = "gpp1-2", "gpp1-3";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <3>;
> >>> +                     samsung,pin-drv = <0>;
> >>> +             };
> >>> +
> >>> +             fm_lna_en: fm-lna-en {
> >>> +                     samsung,pins = "gpg2-3";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-val = <0>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk0_in: sensor-mclk0-in {
> >>> +                     samsung,pins = "gpc0-0";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk0_out: sensor-mclk0-out {
> >>> +                     samsung,pins = "gpc0-0";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk0_fn: sensor-mclk0-fn {
> >>
> >> No, seriously. What sensor is it? In SoC?
> >>
> >
> > In this context, "sensor" stands for "camera". Of course, it resides
> > (or rather *may* reside) outside of the SoC. This is just useful pin
> > configuration for camera master clock lines (3 of them). On schematic
> > those 3 lines are grouped into "MCLK" block, and those pins main
> > function is CAM_MCLK. I guess it makes sense to keep those
> > definitions, as I doubt somebody would actually consider using those
> > lines for somthing else. Or I can remove all those sensor*
> > definitions, if you'd like, then those can be added later, when camera
> > support cames in.
>
> Hmmm, indeed we define in pinctrl useful typical functions and I
> understand this one is such. Let's keep them then.
>
> >
> >>> +                     samsung,pins = "gpc0-0";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk1_in: sensor-mclk1-in {
> >>> +                     samsung,pins = "gpc0-1";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk1_out: sensor-mclk1-out {
> >>> +                     samsung,pins = "gpc0-1";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk1_fn: sensor-mclk1-fn {
> >>> +                     samsung,pins = "gpc0-1";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk2_in: sensor-mclk2-in {
> >>> +                     samsung,pins = "gpc0-2";
> >>> +                     samsung,pin-function = <0>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk2_out: sensor-mclk2-out {
> >>> +                     samsung,pins = "gpc0-2";
> >>> +                     samsung,pin-function = <1>;
> >>> +                     samsung,pin-pud = <1>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +
> >>> +             sensor_mclk2_fn: sensor-mclk2-fn {
> >>> +                     samsung,pins = "gpc0-2";
> >>> +                     samsung,pin-function = <2>;
> >>> +                     samsung,pin-pud = <0>;
> >>> +                     samsung,pin-drv = <2>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >>> new file mode 100644
> >>> index 000000000000..fb243e0a6260
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
> >>> @@ -0,0 +1,30 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Samsung's Exynos850 SoC USI device tree source
> >>> + *
> >>> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> >>> + * Copyright (C) 2021 Linaro Ltd.
> >>> + *
> >>> + * Samsung's Exynos850 SoC USI channels are listed in this file as device tree
> >>> + * nodes.
> >>
> >> Why here not in exynos850.dtsi?
> >>
> >
> > Yeah, you're right. As it's only serial_0 for now, I've moved that
> > into exynos850.dtsi and removed exynos850-usi.dtsi.
> >
> >>> + */
> >>> +
> >>> +#include <dt-bindings/clock/exynos850.h>
> >>> +
> >>> +/ {
> >>> +     aliases {
> >>> +             uart0 = &serial_0;
> >>> +     };
> >>> +
> >>> +     /* USI_UART */
> >>> +     serial_0: uart@13820000 {
> >>
> >> This should ne in soc node.
> >>
> >
> > Done.
> >
> >>> +             compatible = "samsung,exynos850-uart";
> >>> +             reg = <0x0 0x13820000 0x100>;
> >>> +             interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>;
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&uart0_bus>;
> >>> +             clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>;
> >>> +             clock-names = "gate_uart_clk0", "uart";
> >>> +             status = "disabled";
> >>> +     };
> >>> +};
> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> >>> new file mode 100644
> >>> index 000000000000..ed2d1c8ae0c3
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
> >>> @@ -0,0 +1,245 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Samsung Exynos850 SoC device tree source
> >>> + *
> >>> + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> >>> + * Copyright (C) 2021 Linaro Ltd.
> >>> + *
> >>> + * Samsung Exynos850 SoC device nodes are listed in this file.
> >>> + * Exynos based board files can include this file and provide
> >>> + * values for board specific bindings.
> >>> + */
> >>> +
> >>> +#include <dt-bindings/interrupt-controller/exynos850.h>
> >>> +#include <dt-bindings/clock/exynos850.h>
> >>> +#include "exynos850-pinctrl.dtsi"
> >>> +#include "exynos850-usi.dtsi"
> >>> +
> >>> +/ {
> >>
> >> Add a comment like:
> >> /* Also known under engineering name exynos3830 */
> >>
> >
> > Sure.
> >
> >>> +     compatible = "samsung,exynos850";
> >>
> >> Undocumented compatible. Checkpatch should complain.
> >>
> >
> > It actually doesn't, though it does complain about other undocumented
> > compatibles. I understand that it should be documented in
> > samsung-boards.yaml, but is it ok with you if I do that later, when
> > adding some actual board's dts? Just don't want to have two patches
> > for that.
>
> We're back to the first question - it needs to come with DTS and then
> you document both compatibles.
>
> Best regards,
> Krzysztof

Krzysztof Kozlowski Aug. 6, 2021, 12:32 p.m. UTC | #17

On 06/08/2021 14:07, Sam Protsenko wrote:
> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
> <[e-mail]> wrote:
>>
>> On 06/08/2021 01:06, Sam Protsenko wrote:
>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
>>> <[e-mail]> wrote:
>>>
>>>>>
>>>>> This patch adds minimal SoC support. Particular board device tree files
>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
>>>>> reference those nodes further as needed.
>>>>>
>>>>> Signed-off-by: Sam Protsenko <[e-mail]>
>>>>> ---
>>>>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>>>>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>>>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>>>>
>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
>>>> DTS, otherwise no one is able to verify it even compiles.
>>>>
>>>
>>> This device is not available for purchase yet. I'll send the patch for
>>> board dts once it's announced. I can do all the testing for now, if
>>> you have any specific requests. Would it be possible for us to review
>>> and apply only SoC support for now? Will send v2 soon...
>>
>> What you propose is equal to adding a driver (C source code) without
>> ability to compile it. What's the point of having it in the kernel? It's
>> unverifiable, unbuildable and unusable.
>>
> 
> Yes, I understand. That's adding code with no users, and it's not a
> good practice.
> 
>> We can review the DTSI however merging has to be with a DTS. Usually the
>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
>> for Exynos850? Otherwise if you cannot disclose the actual board, the
>> DTSI will have to wait. You can submit drivers, though.
>>
> 
> Sure, let's go this way. I'll send v2 soon. Improving patches and
> having Reviewed-by tag for those would good enough for me at this
> point. I'll continue to prepare another Exynos850 related patches
> until the actual board is announced, like proper clock driver, reset,
> MMC, etc. Is it ok if I send those for a review too (so I can fix all
> issues ahead)?

Sure, prepare all necessary drivers earlier. I suspect clocks will be a
real pain because of significant changes modeled in vendor kernel. I
remember Paweł Chmiel (+Cc) was doing something for these:
https://github.com/PabloPL/linux/tree/exynos7420

I mentioned before - you should also modify the chipid driver. Check
also other drivers in drivers/soc/samsung, although some are needed only
for suspend&resume.

BTW, Paweł,
How is your Exynos7420 progress? :)

> And should I maybe add RFC tag for those?

No need. Drivers can be merged before DTS users.

Best regards,
Krzysztof

Krzysztof Kozlowski Aug. 6, 2021, 12:48 p.m. UTC | #18

On 06/08/2021 14:32, Krzysztof Kozlowski wrote:
> On 06/08/2021 14:07, Sam Protsenko wrote:
>> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
>> <[e-mail]> wrote:
>>>
>>> On 06/08/2021 01:06, Sam Protsenko wrote:
>>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
>>>> <[e-mail]> wrote:
>>>>
>>>>>>
>>>>>> This patch adds minimal SoC support. Particular board device tree files
>>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
>>>>>> reference those nodes further as needed.
>>>>>>
>>>>>> Signed-off-by: Sam Protsenko <[e-mail]>
>>>>>> ---
>>>>>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>>>>>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>>>>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>>>>>
>>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
>>>>> DTS, otherwise no one is able to verify it even compiles.
>>>>>
>>>>
>>>> This device is not available for purchase yet. I'll send the patch for
>>>> board dts once it's announced. I can do all the testing for now, if
>>>> you have any specific requests. Would it be possible for us to review
>>>> and apply only SoC support for now? Will send v2 soon...
>>>
>>> What you propose is equal to adding a driver (C source code) without
>>> ability to compile it. What's the point of having it in the kernel? It's
>>> unverifiable, unbuildable and unusable.
>>>
>>
>> Yes, I understand. That's adding code with no users, and it's not a
>> good practice.
>>
>>> We can review the DTSI however merging has to be with a DTS. Usually the
>>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
>>> for Exynos850? Otherwise if you cannot disclose the actual board, the
>>> DTSI will have to wait. You can submit drivers, though.
>>>
>>
>> Sure, let's go this way. I'll send v2 soon. Improving patches and
>> having Reviewed-by tag for those would good enough for me at this
>> point. I'll continue to prepare another Exynos850 related patches
>> until the actual board is announced, like proper clock driver, reset,
>> MMC, etc. Is it ok if I send those for a review too (so I can fix all
>> issues ahead)?
> 
> Sure, prepare all necessary drivers earlier. I suspect clocks will be a
> real pain because of significant changes modeled in vendor kernel. I
> remember Paweł Chmiel (+Cc) was doing something for these:
> https://github.com/PabloPL/linux/tree/exynos7420
> 
> I mentioned before - you should also modify the chipid driver. Check
> also other drivers in drivers/soc/samsung, although some are needed only
> for suspend&resume.
> 

You can also take a look at Exynos8895 efforts:
https://github.com/ivoszbg/linux/commits/for-upstream/exynos8895

Since knowledge, datasheets and efforts are quite spread all over, I
keep track of some work here:
https://exynos.wiki.kernel.org/community


Best regards,
Krzysztof

Sam Protsenko Aug. 6, 2021, 4:57 p.m. UTC | #19

On Fri, 6 Aug 2021 at 15:48, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 06/08/2021 14:32, Krzysztof Kozlowski wrote:
> > On 06/08/2021 14:07, Sam Protsenko wrote:
> >> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
> >> <[e-mail]> wrote:
> >>>
> >>> On 06/08/2021 01:06, Sam Protsenko wrote:
> >>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
> >>>> <[e-mail]> wrote:
> >>>>
> >>>>>>
> >>>>>> This patch adds minimal SoC support. Particular board device tree files
> >>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
> >>>>>> reference those nodes further as needed.
> >>>>>>
> >>>>>> Signed-off-by: Sam Protsenko <[e-mail]>
> >>>>>> ---
> >>>>>>  .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >>>>>>  arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >>>>>>  arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >>>>>
> >>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
> >>>>> DTS, otherwise no one is able to verify it even compiles.
> >>>>>
> >>>>
> >>>> This device is not available for purchase yet. I'll send the patch for
> >>>> board dts once it's announced. I can do all the testing for now, if
> >>>> you have any specific requests. Would it be possible for us to review
> >>>> and apply only SoC support for now? Will send v2 soon...
> >>>
> >>> What you propose is equal to adding a driver (C source code) without
> >>> ability to compile it. What's the point of having it in the kernel? It's
> >>> unverifiable, unbuildable and unusable.
> >>>
> >>
> >> Yes, I understand. That's adding code with no users, and it's not a
> >> good practice.
> >>
> >>> We can review the DTSI however merging has to be with a DTS. Usually the
> >>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
> >>> for Exynos850? Otherwise if you cannot disclose the actual board, the
> >>> DTSI will have to wait. You can submit drivers, though.
> >>>
> >>
> >> Sure, let's go this way. I'll send v2 soon. Improving patches and
> >> having Reviewed-by tag for those would good enough for me at this
> >> point. I'll continue to prepare another Exynos850 related patches
> >> until the actual board is announced, like proper clock driver, reset,
> >> MMC, etc. Is it ok if I send those for a review too (so I can fix all
> >> issues ahead)?
> >
> > Sure, prepare all necessary drivers earlier. I suspect clocks will be a
> > real pain because of significant changes modeled in vendor kernel. I
> > remember Paweł Chmiel (+Cc) was doing something for these:
> > https://github.com/PabloPL/linux/tree/exynos7420
> >
> > I mentioned before - you should also modify the chipid driver. Check
> > also other drivers in drivers/soc/samsung, although some are needed only
> > for suspend&resume.
> >

Yes, in due course... There won't be much problems with chip-id
driver. But pretty much all platform drivers depend on clk, and that
one I'm afraid I'll have to implement from scratch. Will take me a
while, as vendor's codebase for clk driver is huge and depend on
another huge non-upstreamable abstraction layer framework they have,
and also they have kind of strange home brewed composite clocks...
Anyway, that's what I'm going to look into next.

Btw, I just sent v2 :)

>
> You can also take a look at Exynos8895 efforts:
> https://github.com/ivoszbg/linux/commits/for-upstream/exynos8895
>
> Since knowledge, datasheets and efforts are quite spread all over, I
> keep track of some work here:
> https://exynos.wiki.kernel.org/community
>

Thanks, Krzysztof! I'm sure that info will help a lot with further
work, like clock driver.

>
> Best regards,
> Krzysztof

Paweł Chmiel Aug. 6, 2021, 8:32 p.m. UTC | #20

W dniu 06.08.2021 o 14:32, Krzysztof Kozlowski pisze:
> On 06/08/2021 14:07, Sam Protsenko wrote:
>> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
>> <[e-mail]> wrote:
>>>
>>> On 06/08/2021 01:06, Sam Protsenko wrote:
>>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
>>>> <[e-mail]> wrote:
>>>>
>>>>>>
>>>>>> This patch adds minimal SoC support. Particular board device tree files
>>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
>>>>>> reference those nodes further as needed.
>>>>>>
>>>>>> Signed-off-by: Sam Protsenko <[e-mail]>
>>>>>> ---
>>>>>>   .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
>>>>>>   arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
>>>>>>   arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
>>>>>
>>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
>>>>> DTS, otherwise no one is able to verify it even compiles.
>>>>>
>>>>
>>>> This device is not available for purchase yet. I'll send the patch for
>>>> board dts once it's announced. I can do all the testing for now, if
>>>> you have any specific requests. Would it be possible for us to review
>>>> and apply only SoC support for now? Will send v2 soon...
>>>
>>> What you propose is equal to adding a driver (C source code) without
>>> ability to compile it. What's the point of having it in the kernel? It's
>>> unverifiable, unbuildable and unusable.
>>>
>>
>> Yes, I understand. That's adding code with no users, and it's not a
>> good practice.
>>
>>> We can review the DTSI however merging has to be with a DTS. Usually the
>>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
>>> for Exynos850? Otherwise if you cannot disclose the actual board, the
>>> DTSI will have to wait. You can submit drivers, though.
>>>
>>
>> Sure, let's go this way. I'll send v2 soon. Improving patches and
>> having Reviewed-by tag for those would good enough for me at this
>> point. I'll continue to prepare another Exynos850 related patches
>> until the actual board is announced, like proper clock driver, reset,
>> MMC, etc. Is it ok if I send those for a review too (so I can fix all
>> issues ahead)?
> 
> Sure, prepare all necessary drivers earlier. I suspect clocks will be a
> real pain because of significant changes modeled in vendor kernel. I
> remember Paweł Chmiel (+Cc) was doing something for these:
> https://github.com/PabloPL/linux/tree/exynos7420
> 
> I mentioned before - you should also modify the chipid driver. Check
> also other drivers in drivers/soc/samsung, although some are needed only
> for suspend&resume.
> 
> BTW, Paweł,
> How is your Exynos7420 progress? :)
Hi

Sadly i had to postpone it for a while. Maybe will have more time now to 
get back to it.

About clock driver. In vendor sources there is clk driver with something 
called virtual clocks (different than real ones). That driver calls 
another driver called pwrcal, responsible for real manipulation of 
clocks in hardware. This one has info about real clocks and also 
additional info about for example rate for some of them, which is read 
from binary from memory, by another driver called ect_parser in case of 
devices at which i did looked.

In my case i was able to find some more info about real clocks there - 
for example register names and offsets 
https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmusfr.h 
and some clocks hierarchy info inside 
https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmu.c 
but there was still many info missing.

Finding a way (which could be applied to other Exynos SOC) to "convert" 
or use that vendor code and turn it into mainline driver, especially 
without TRM which is not available for all/most of them, would be great.

I'm wondering if Exynos850 device has the same issue as on 7420 (and 
probably 8890/7578 and maybe also other 64 bit Exynos devices) - broken 
firmware. For example i had to specify in dts timer clock frequency, on 
few devices there is also a problem with timer registers not properly 
configured by FW, which probably won't be fixed by vendor and patches 
with workaround for it in kernel were rejected :/.
> 
>> And should I maybe add RFC tag for those?
> 
> No need. Drivers can be merged before DTS users.
> 
> Best regards,
> Krzysztof
>

Sam Protsenko Sept. 6, 2021, 3:16 p.m. UTC | #21

On Fri, 6 Aug 2021 at 23:32, Paweł Chmiel
<[e-mail]> wrote:
>
> W dniu 06.08.2021 o 14:32, Krzysztof Kozlowski pisze:
> > On 06/08/2021 14:07, Sam Protsenko wrote:
> >> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
> >> <[e-mail]> wrote:
> >>>
> >>> On 06/08/2021 01:06, Sam Protsenko wrote:
> >>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
> >>>> <[e-mail]> wrote:
> >>>>
> >>>>>>
> >>>>>> This patch adds minimal SoC support. Particular board device tree files
> >>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
> >>>>>> reference those nodes further as needed.
> >>>>>>
> >>>>>> Signed-off-by: Sam Protsenko <[e-mail]>
> >>>>>> ---
> >>>>>>   .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >>>>>>   arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >>>>>>   arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >>>>>
> >>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
> >>>>> DTS, otherwise no one is able to verify it even compiles.
> >>>>>
> >>>>
> >>>> This device is not available for purchase yet. I'll send the patch for
> >>>> board dts once it's announced. I can do all the testing for now, if
> >>>> you have any specific requests. Would it be possible for us to review
> >>>> and apply only SoC support for now? Will send v2 soon...
> >>>
> >>> What you propose is equal to adding a driver (C source code) without
> >>> ability to compile it. What's the point of having it in the kernel? It's
> >>> unverifiable, unbuildable and unusable.
> >>>
> >>
> >> Yes, I understand. That's adding code with no users, and it's not a
> >> good practice.
> >>
> >>> We can review the DTSI however merging has to be with a DTS. Usually the
> >>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
> >>> for Exynos850? Otherwise if you cannot disclose the actual board, the
> >>> DTSI will have to wait. You can submit drivers, though.
> >>>
> >>
> >> Sure, let's go this way. I'll send v2 soon. Improving patches and
> >> having Reviewed-by tag for those would good enough for me at this
> >> point. I'll continue to prepare another Exynos850 related patches
> >> until the actual board is announced, like proper clock driver, reset,
> >> MMC, etc. Is it ok if I send those for a review too (so I can fix all
> >> issues ahead)?
> >
> > Sure, prepare all necessary drivers earlier. I suspect clocks will be a
> > real pain because of significant changes modeled in vendor kernel. I
> > remember Paweł Chmiel (+Cc) was doing something for these:
> > https://github.com/PabloPL/linux/tree/exynos7420
> >
> > I mentioned before - you should also modify the chipid driver. Check
> > also other drivers in drivers/soc/samsung, although some are needed only
> > for suspend&resume.
> >
> > BTW, Paweł,
> > How is your Exynos7420 progress? :)
> Hi
>
> Sadly i had to postpone it for a while. Maybe will have more time now to
> get back to it.
>
> About clock driver. In vendor sources there is clk driver with something
> called virtual clocks (different than real ones). That driver calls
> another driver called pwrcal, responsible for real manipulation of
> clocks in hardware. This one has info about real clocks and also
> additional info about for example rate for some of them, which is read
> from binary from memory, by another driver called ect_parser in case of
> devices at which i did looked.
>
> In my case i was able to find some more info about real clocks there -
> for example register names and offsets
> https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmusfr.h
> and some clocks hierarchy info inside
> https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmu.c
> but there was still many info missing.
>
> Finding a way (which could be applied to other Exynos SOC) to "convert"
> or use that vendor code and turn it into mainline driver, especially
> without TRM which is not available for all/most of them, would be great.
>
> I'm wondering if Exynos850 device has the same issue as on 7420 (and
> probably 8890/7578 and maybe also other 64 bit Exynos devices) - broken
> firmware. For example i had to specify in dts timer clock frequency, on
> few devices there is also a problem with timer registers not properly
> configured by FW, which probably won't be fixed by vendor and patches
> with workaround for it in kernel were rejected :/.

Hi Paweł,

Sorry for the late reply. Thanks for your input! I just started
implementing the clock driver, and maybe I can share some useful stuff
in exchange.

ECT parser: in downstream kernel there is an option to dump ECT tables
via some DebugFS file. I did that, and it seems to me it would be
easier to just hard code necessary table in corresponding drivers
code. E.g., PLL tables can be hard-coded in the clock driver (which is
how it seems to be implemented for other Exynos SoC upstream anyway).
So I don't think there is a huge need to upstream ECT parser itself.
But dumping the tables can be useful to implement corresponding
drivers (clocks, DVFS, APM, etc).

Investigating downstream clock driver for Exynos850 and its
dependencies (like VCLK, RA, CMUCAL), I figured it's much easier to
implement the clock driver completely from scratch. Looking into
clk-exynos7.c and clk-exynos5433.c implementation, this is probably
how upstream design should look like. And it has nothing to do with
downstream implementation. E.g., downstream driver has one single CMU
node in dts (for the whole clock subsystem), but for upstream drivers
we want to have separate nodes for each particular CMU. Also
downstream implementation is over-complicated; that might have some
sense for the vendor, if they have a bunch of similar SoCs or sharing
driver code between different OS kernels, but for upstream kernel it's
just unneeded complexity (several layers of abstraction that should be
just removed, and useful stuff should be integrated in already
existing upstream infrastructure). So I ended up using TRM and trying
to make something similar to mentioned upstream drivers. Of course,
there are some questions on whether we should use manual clocks
control, or rely on automatic clock gating and Q-channel
communication. But I'm having some progress already, and hopefully
will submit the minimal driver in a week or so.

As for downstream design and how to make a sense of it, for converting
that to something more upstreamable, here is what I figured.

1. Clock driver (clk-exynosXXXX.c) registers some vclk clocks
("Virtual Clocks"). Those are clocks which will be actually used. But
those are lacking any hardware specific data yet (like register
offsets, etc).

2. ".calid" field from those vclk's is used to find more
hardware-related info (later in run-time) for those registered clocks,
from cmucal-node.c file. That file contains actual parent information
and some info on HW stuff, but not real addresses/offset, but only
indexes.

3. Those indexes are resolved further (in run-time), obtaining actual
offsets from cmucal-sfr.c file, in ra_init() function.

4. Instead of using existing CCF clocks, custom vclk clock ops are
used. Those are defined in composite.c (has nothing to do with CCF
composite clocks). So the whole VCLK type looks like vendor
re-implementation of composite clocks. For example, one VCLK clock can
have the whole list of clocks which will be controlled via that single
VCLK clock, and all operations will be called for each clock from the
list.

5. Further those operations are actually calling the code from ra.c,
different functions are used for different clock types (PLL_TYPE,
MUX_TYPE, etc). So if you're curious about actual ops implementation
(like PLL, muxes, etc), just look there.

That should be enough to convert downstream driver to upstream one.
Basically one can use downstream source code to figure out info about
registers and offsets (from cmucal-sfr.c) and info about internal
clocks structure from cmucal-node.c and cmucal-vclk.c, and implement
everything from scratch using clk-exynos7.c/clk-exynos5433.c as a
template. I'd say it's easier to just use TRM for that, though :)

As for the broken firmware and clock freq registers: yes, CNTFRQ_EL0
registers wasn't set in EL3 firmware too, so I'm setting the
"clock-frequency" property for arch timer in dts as a workaround right
now, in my local tree. But I already let vendor guys know about that
problem, and they are trying to fix that right now (at least for
Exynos850 SoC).

Thanks!

> >
> >> And should I maybe add RFC tag for those?
> >
> > No need. Drivers can be merged before DTS users.
> >
> > Best regards,
> > Krzysztof
> >
>

Patch

diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
new file mode 100644
index 000000000000..4cf0a22cc6db
--- /dev/null
+++ b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi
@@ -0,0 +1,782 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung's Exynos850 SoC pin-mux and pin-config device tree source
+ *
+ * Copyright (C) 2017 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2021 Linaro Ltd.
+ *
+ * Samsung's Exynos850 SoC pin-mux and pin-config options are listed as device
+ * tree nodes in this file.
+ */
+
+#include <dt-bindings/interrupt-controller/exynos850.h>
+
+/ {
+	/* ALIVE */
+	pinctrl@11850000 {
+		gpa0: gpa0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			    <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpa1: gpa1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			    <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpa2: gpa2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			    <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpa3: gpa3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			    <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpa4: gpa4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			    <GIC_SPI INTREQ__ALIVE_EINT32 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT33 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT34 IRQ_TYPE_LEVEL_HIGH>,
+			    <GIC_SPI INTREQ__ALIVE_EINT35 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpq0: gpq0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		/* USI_PERI_UART_DBG */
+		uart0_bus: uart0-bus {
+			samsung,pins = "gpq0-0", "gpq0-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+
+		decon_f_te_on: decon_f_te_on {
+			samsung,pins = "gpa4-1";
+			samsung,pin-function = <0xf>;
+		};
+
+		decon_f_te_off: decon_f_te_off {
+			samsung,pins = "gpa4-1";
+			samsung,pin-function = <0x0>;
+		};
+
+		/* I2C_5 | CAM_PMIC_I2C */
+		i2c5_bus: i2c5-bus {
+			samsung,pins = "gpa3-5", "gpa3-6";
+			samsung,pin-function = <3>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* I2C_6 | MOTOR_I2C */
+		i2c6_bus: i2c6-bus {
+			samsung,pins = "gpa3-7", "gpa4-0";
+			samsung,pin-function = <3>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+	};
+
+	/* CMGP */
+	pinctrl@11c30000 {
+		gpm0: gpm0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpm1: gpm1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpm2: gpm2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpm3: gpm3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpm4: gpm4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpm5: gpm5 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			interrupts =
+			  <GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		/* usi_cmgp00 */
+		hsi2c3_bus: hsi2c3-bus {
+			samsung,pins = "gpm0-0", "gpm1-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* usi_cmgp01 */
+		hsi2c4_bus: hsi2c4-bus {
+			samsung,pins = "gpm4-0", "gpm5-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* spi usi_cmgp00 */
+		spi1_bus: spi1-bus {
+			samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi1_cs: spi1-cs {
+			samsung,pins = "gpm3-0";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi1_cs_func: spi1-cs-func {
+			samsung,pins = "gpm3-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* spi usi_cmgp01 */
+		spi2_bus: spi2-bus {
+			samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi2_cs: spi2-cs {
+			samsung,pins = "gpm7-0";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi2_cs_func: spi2-cs-func {
+			samsung,pins = "gpm7-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* usi_cmgp00_uart */
+		uart1_bus_single: uart1-bus {
+			samsung,pins = "gpm0-0", "gpm1-0", "gpm2-0", "gpm3-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+
+		uart1_bus_dual: uart1-bus-dual {
+			samsung,pins = "gpm0-0", "gpm1-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+
+		/* usi_cmgp01_uart */
+		uart2_bus_single: uart2-bus {
+			samsung,pins = "gpm4-0", "gpm5-0", "gpm6-0", "gpm7-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+
+		uart2_bus_dual: uart2-bus-dual {
+			samsung,pins = "gpm4-0", "gpm5-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+	};
+
+	/* AUD */
+	pinctrl@14a60000 {
+		gpb0: gpb0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpb1: gpb1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		aud_codec_mclk: aud-codec-mclk {
+			samsung,pins = "gpb0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_codec_mclk_idle: aud-codec-mclk-idle {
+			samsung,pins = "gpb0-0";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_i2s0_bus: aud-i2s0-bus {
+			samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_i2s0_idle: aud-i2s0-idle {
+			samsung,pins = "gpb0-1", "gpb0-2", "gpb0-3", "gpb0-4";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_i2s1_bus: aud-i2s1-bus {
+			samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_i2s1_idle: aud-i2s1-idle {
+			samsung,pins = "gpb1-0", "gpb1-1", "gpb1-2", "gpb1-3";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_fm_bus: aud-fm-bus {
+			samsung,pins = "gpb1-4";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <1>;
+		};
+
+		aud_fm_idle: aud-fm-idle {
+			samsung,pins = "gpb1-4";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <1>;
+		};
+	};
+
+	/* HSI */
+	pinctrl@13430000 {
+		gpf2: gpf2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sd2_clk: sd2-clk {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd2_cmd: sd2-cmd {
+			samsung,pins = "gpf2-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <2>;
+		 };
+
+		sd2_bus1: sd2-bus-width1 {
+			samsung,pins = "gpf2-2";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd2_bus4: sd2-bus-width4 {
+			samsung,pins = "gpf2-3", "gpf2-4", "gpf2-5";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd2_clk_fast_slew_rate_1x: sd2-clk_fast_slew_rate_1x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		sd2_clk_fast_slew_rate_1_5x: sd2-clk_fast_slew_rate_1_5x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <1>;
+		};
+
+		sd2_clk_fast_slew_rate_2x: sd2-clk_fast_slew_rate_2x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd2_clk_fast_slew_rate_2_5x: sd2-clk_fast_slew_rate_2_5x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd2_clk_fast_slew_rate_3x: sd2-clk_fast_slew_rate_3x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <4>;
+		};
+
+		sd2_clk_fast_slew_rate_4x: sd2-clk_fast_slew_rate_4x {
+			samsung,pins = "gpf2-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <5>;
+		};
+
+		sd2_pins_as_pdn: sd2-pins-as-pdn {
+			samsung,pins = "gpf2-0", "gpf2-1", "gpf2-2", "gpf2-3",
+				       "gpf2-4", "gpf2-5";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <2>;
+		};
+
+	};
+
+	/* CORE */
+	pinctrl@12070000 {
+		gpf0: gpf0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sd0_clk: sd0-clk {
+			samsung,pins = "gpf0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_cmd: sd0-cmd {
+			samsung,pins = "gpf0-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_rdqs: sd0-rdqs {
+			samsung,pins = "gpf0-2";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <1>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_nreset: sd0-nreset {
+			samsung,pins = "gpf0-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_clk_fast_slew_rate_1x: sd0-clk_fast_slew_rate_1x {
+			samsung,pins = "gpf0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <1>;
+		};
+
+		sd0_clk_fast_slew_rate_2x: sd0-clk_fast_slew_rate_2x {
+			samsung,pins = "gpf0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd0_clk_fast_slew_rate_3x: sd0-clk_fast_slew_rate_3x {
+			samsung,pins = "gpf0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sd0_clk_fast_slew_rate_4x: sd0-clk_fast_slew_rate_4x {
+			samsung,pins = "gpf0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <3>;
+		};
+
+		gpf1: gpf1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sd0_bus1: sd0-bus-width1 {
+			samsung,pins = "gpf1-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_bus4: sd0-bus-width4 {
+			samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_bus8: sd0-bus-width8 {
+			samsung,pins = "gpf1-4", "gpf1-5", "gpf1-6", "gpf1-7";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+	};
+
+	/* PERI */
+	pinctrl@139b0000 {
+		gpg0: gpg0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpp0: gpp0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+		gpp1: gpp1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpp2: gpp2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpg1: gpg1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpg2: gpg2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpg3: gpg3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpc0: gpc0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		gpc1: gpc1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		xclkout: xclkout {
+			samsung,pins = "gpq0-2";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+		};
+
+		/* usi_hsi2c_0 */
+		hsi2c0_bus: hsi2c0-bus {
+			samsung,pins = "gpc1-0", "gpc1-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* usi_hsi2c_1 */
+		hsi2c1_bus: hsi2c1-bus {
+			samsung,pins = "gpc1-2", "gpc1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* usi_hsi2c_2 */
+		hsi2c2_bus: hsi2c2-bus {
+			samsung,pins = "gpc1-4", "gpc1-5";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		/* usi_spi_0 */
+		spi0_bus: spi0-bus {
+			samsung,pins = "gpp2-0", "gpp2-2", "gpp2-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi0_cs: spi0-cs {
+			samsung,pins = "gpp2-1";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		spi0_cs_func: spi0-cs-func {
+			samsung,pins = "gpp2-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		i2c0_bus: i2c0-bus {
+			samsung,pins = "gpp0-0", "gpp0-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
+
+		i2c1_bus: i2c1-bus {
+			samsung,pins = "gpp0-2", "gpp0-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		i2c2_bus: i2c2-bus {
+			samsung,pins = "gpp0-4", "gpp0-5";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		i2c3_bus: i2c3-bus {
+			samsung,pins = "gpp1-0", "gpp1-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		i2c4_bus: i2c4-bus {
+			samsung,pins = "gpp1-2", "gpp1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		fm_lna_en: fm-lna-en {
+			samsung,pins = "gpg2-3";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-val = <0>;
+		};
+
+		sensor_mclk0_in: sensor-mclk0-in {
+			samsung,pins = "gpc0-0";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk0_out: sensor-mclk0-out {
+			samsung,pins = "gpc0-0";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <1>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk0_fn: sensor-mclk0-fn {
+			samsung,pins = "gpc0-0";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk1_in: sensor-mclk1-in {
+			samsung,pins = "gpc0-1";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk1_out: sensor-mclk1-out {
+			samsung,pins = "gpc0-1";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <1>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk1_fn: sensor-mclk1-fn {
+			samsung,pins = "gpc0-1";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk2_in: sensor-mclk2-in {
+			samsung,pins = "gpc0-2";
+			samsung,pin-function = <0>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk2_out: sensor-mclk2-out {
+			samsung,pins = "gpc0-2";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <1>;
+			samsung,pin-drv = <2>;
+		};
+
+		sensor_mclk2_fn: sensor-mclk2-fn {
+			samsung,pins = "gpc0-2";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <2>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
new file mode 100644
index 000000000000..fb243e0a6260
--- /dev/null
+++ b/arch/arm64/boot/dts/exynos/exynos850-usi.dtsi
@@ -0,0 +1,30 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung's Exynos850 SoC USI device tree source
+ *
+ * Copyright (C) 2019 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2021 Linaro Ltd.
+ *
+ * Samsung's Exynos850 SoC USI channels are listed in this file as device tree
+ * nodes.
+ */
+
+#include <dt-bindings/clock/exynos850.h>
+
+/ {
+	aliases {
+		uart0 = &serial_0;
+	};
+
+	/* USI_UART */
+	serial_0: uart@13820000 {
+		compatible = "samsung,exynos850-uart";
+		reg = <0x0 0x13820000 0x100>;
+		interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&uart0_bus>;
+		clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>;
+		clock-names = "gate_uart_clk0", "uart";
+		status = "disabled";
+	};
+};
diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
new file mode 100644
index 000000000000..ed2d1c8ae0c3
--- /dev/null
+++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
@@ -0,0 +1,245 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung Exynos850 SoC device tree source
+ *
+ * Copyright (C) 2018 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2021 Linaro Ltd.
+ *
+ * Samsung Exynos850 SoC device nodes are listed in this file.
+ * Exynos based board files can include this file and provide
+ * values for board specific bindings.
+ */
+
+#include <dt-bindings/interrupt-controller/exynos850.h>
+#include <dt-bindings/clock/exynos850.h>
+#include "exynos850-pinctrl.dtsi"
+#include "exynos850-usi.dtsi"
+
+/ {
+	compatible = "samsung,exynos850";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <1>;
+
+	aliases {
+		pinctrl0 = &pinctrl_0;
+		pinctrl1 = &pinctrl_1;
+		pinctrl2 = &pinctrl_2;
+		pinctrl3 = &pinctrl_3;
+		pinctrl4 = &pinctrl_4;
+		pinctrl5 = &pinctrl_5;
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#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@0000 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0000>;
+			enable-method = "psci";
+		};
+		cpu1: cpu@0001 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0001>;
+			enable-method = "psci";
+		};
+		cpu2: cpu@0002 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0002>;
+			enable-method = "psci";
+		};
+		cpu3: cpu@0003 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0003>;
+			enable-method = "psci";
+		};
+		cpu4: cpu@0004 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0100>;
+			enable-method = "psci";
+		};
+		cpu5: cpu@0005 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0101>;
+			enable-method = "psci";
+		};
+		cpu6: cpu@0006 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0102>;
+			enable-method = "psci";
+		};
+		cpu7: cpu@0007 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55", "arm,armv8";
+			reg = <0x0 0x0103>;
+			enable-method = "psci";
+		};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
+	gic: interrupt-controller@12a00000 {
+		compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+		#interrupt-cells = <3>;
+		#address-cells = <0>;
+		interrupt-controller;
+		reg = <0x0 0x12a01000 0x1000>,
+		      <0x0 0x12a02000 0x1000>,
+		      <0x0 0x12a04000 0x2000>,
+		      <0x0 0x12a06000 0x2000>;
+		interrupts = <GIC_PPI 9
+				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		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>;
+		use-clocksource-only;
+		use-physical-timer;
+	};
+
+	clock: clock-controller@0x120e0000 {
+		compatible = "samsung,exynos850-clock";
+		reg = <0x0 0x120e0000 0x8000>;
+		#clock-cells = <1>;
+	};
+
+	/* ALIVE */
+	pinctrl_0: pinctrl@11850000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x11850000 0x1000>;
+		interrupts = <GIC_SPI INTREQ__ALIVE_EINT0 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT1 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT2 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT3 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT4 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT5 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT6 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT7 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT8 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT9 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT10 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT11 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT12 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT13 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT14 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT15 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT16 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT17 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT18 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT19 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT20 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT21 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT22 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT23 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT24 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT25 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT26 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT27 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT28 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT29 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT30 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI INTREQ__ALIVE_EINT31 IRQ_TYPE_LEVEL_HIGH>;
+
+		wakeup-interrupt-controller {
+			compatible = "samsung,exynos7-wakeup-eint";
+		};
+	};
+
+	/* CMGP */
+	pinctrl_1: pinctrl@11c30000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x11c30000 0x1000>;
+		interrupts =
+			<GIC_SPI INTREQ__CMGP_EXT_INTM00 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM01 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM02 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM03 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM04 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM05 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM06 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI INTREQ__CMGP_EXT_INTM07 IRQ_TYPE_LEVEL_HIGH>;
+
+		wakeup-interrupt-controller {
+			compatible = "samsung,exynos7-wakeup-eint";
+		};
+	};
+
+	/* AUD */
+	pinctrl_2: pinctrl@14a60000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x14a60000 0x1000>;
+	};
+
+	/* HSI */
+	pinctrl_3: pinctrl@13430000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x13430000 0x1000>;
+		interrupts = <GIC_SPI INTREQ__GPIO_HSI IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	/* CORE */
+	pinctrl_4: pinctrl@12070000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x12070000 0x1000>;
+		interrupts = <GIC_SPI INTREQ__GPIO_CORE IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	/* PERI */
+	pinctrl_5: pinctrl@139b0000 {
+		compatible = "samsung,exynos850-pinctrl";
+		reg = <0x0 0x139b0000 0x1000>;
+		interrupts = <GIC_SPI INTREQ__GPIO_PERI IRQ_TYPE_LEVEL_HIGH>;
+	};
+};