Re: [PATCH v6 18/20] arm64: dts: exynos: google: Add initial Google gs101 SoC support

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

On 10/12/2023 00:31, Peter Griffin wrote:
> Google gs101 SoC is a ARMv8 mobile SoC found in the Pixel 6
> (oriole), Pixel 6a (bluejay) and Pixel 6 pro (raven) mobile
> phones.
> 
> It features:
> * 4xA55 Little cluster
> * 2xA76 Mid cluster
> * 2xX1 Big cluster
> 

...

> diff --git a/arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h b/arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h
> new file mode 100644
> index 000000000000..68b7bc47c91b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Pinctrl binding constants for GS101
> + *
> + * Copyright 2020-2023 Google LLC
> + */
> +
> +#ifndef __DT_BINDINGS_PINCTRL_GS101_H__
> +#define __DT_BINDINGS_PINCTRL_GS101_H__

Header guards don't really match location.

> +
> +#define GS101_PIN_PULL_NONE		0
> +#define GS101_PIN_PULL_DOWN		1
> +#define GS101_PIN_PULL_UP		3
> +
> +/* Pin function in power down mode */
> +#define GS101_PIN_PDN_OUT0		0
> +#define GS101_PIN_PDN_OUT1		1
> +#define GS101_PIN_PDN_INPUT		2
> +#define GS101_PIN_PDN_PREV		3
> +
> +/* GS101 drive strengths */
> +#define GS101_PIN_DRV_2_5_MA		0
> +#define GS101_PIN_DRV_5_MA		1
> +#define GS101_PIN_DRV_7_5_MA		2
> +#define GS101_PIN_DRV_10_MA		3
> +
> +#define GS101_PIN_FUNC_INPUT		0
> +#define GS101_PIN_FUNC_OUTPUT		1
> +#define GS101_PIN_FUNC_2		2
> +#define GS101_PIN_FUNC_3		3
> +#define GS101_PIN_FUNC_EINT		0xf
> +
> +#endif /* __DT_BINDINGS_PINCTRL_GS101_H__ */
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> new file mode 100644
> index 000000000000..60e112d25246
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * GS101 SoC
> + *
> + * Copyright 2019-2023 Google LLC
> + * Copyright 2023 Linaro Ltd - <peter.griffin@linaro.org>
> + */
> +
> +#include <dt-bindings/clock/google,gs101.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/samsung,exynos-usi.h>
> +
> +/ {
> +	compatible = "google,gs101";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	interrupt-parent = <&gic>;
> +
> +	aliases {
> +		pinctrl0 = &pinctrl_gpio_alive;
> +		pinctrl1 = &pinctrl_far_alive;
> +		pinctrl2 = &pinctrl_gsacore;
> +		pinctrl3 = &pinctrl_gsactrl;
> +		pinctrl4 = &pinctrl_peric0;
> +		pinctrl5 = &pinctrl_peric1;
> +		pinctrl6 = &pinctrl_hsi1;
> +		pinctrl7 = &pinctrl_hsi2;
> +	};
> +
> +	pmu-0 {
> +		compatible = "arm,cortex-a55-pmu";
> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster0>;
> +	};
> +
> +	pmu-1 {
> +		compatible = "arm,cortex-a76-pmu";
> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster1>;
> +	};
> +
> +	pmu-2 {
> +		compatible = "arm,cortex-x1-pmu";
> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster2>;
> +	};
> +
> +	pmu-3 {
> +		compatible = "arm,dsu-pmu";
> +		interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>;
> +		cpus = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>,
> +		       <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
> +	};

Keep alphabetical order of top-level nodes. pmu should be before psci

> +
> +	/* TODO replace with CCF clock */
> +	dummy_clk: oscillator {

clock-3

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12345>;
> +		clock-output-names = "pclk";
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&cpu0>;
> +				};
> +				core1 {
> +					cpu = <&cpu1>;
> +				};
> +				core2 {
> +					cpu = <&cpu2>;
> +				};
> +				core3 {
> +					cpu = <&cpu3>;
> +				};
> +			};
> +
> +			cluster1 {
> +				core0 {
> +					cpu = <&cpu4>;
> +				};
> +				core1 {
> +					cpu = <&cpu5>;
> +				};
> +			};
> +
> +			cluster2 {
> +				core0 {
> +					cpu = <&cpu6>;
> +				};
> +				core1 {
> +					cpu = <&cpu7>;
> +				};
> +			};
> +		};

...

> +
> +	/* ect node is required to be present by bootloader */
> +	ect {
> +	};

alphabetical order

> +
> +	ext_24_5m: clock-1 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-output-names = "oscclk";
> +	};
> +
> +	ext_200m: clock-2 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-output-names = "ext-200m";
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	reserved_memory: reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		gsa_reserved_protected: gsa@90200000 {
> +			reg = <0x0 0x90200000 0x400000>;
> +			no-map;
> +		};
> +
> +		tpu_fw_reserved: tpu-fw@93000000 {
> +			reg = <0x0 0x93000000 0x1000000>;
> +			no-map;
> +		};
> +
> +		aoc_reserve: aoc@94000000 {
> +			reg = <0x0 0x94000000 0x03000000>;
> +			no-map;
> +		};
> +
> +		abl_reserved: abl@f8800000 {
> +			reg = <0x0 0xf8800000 0x02000000>;
> +			no-map;
> +		};
> +
> +		dss_log_reserved: dss-log-reserved@fd3f0000 {
> +			reg = <0x0 0xfd3f0000 0x0000e000>;
> +			no-map;
> +		};
> +
> +		debug_kinfo_reserved: debug-kinfo-reserved@fd3fe000 {
> +			reg = <0x0 0xfd3fe000 0x00001000>;
> +			no-map;
> +		};
> +
> +		bldr_log_reserved: bldr-log-reserved@fd800000 {
> +			reg = <0x0 0xfd800000 0x00100000>;
> +			no-map;
> +		};
> +
> +		bldr_log_hist_reserved: bldr-log-hist-reserved@fd900000 {
> +			reg = <0x0 0xfd900000 0x00002000>;
> +			no-map;
> +		};
> +	};
> +
> +	timer {

alphabetical order, so this goes to the end

> +		compatible = "arm,armv8-timer";
> +		interrupts =
> +		   <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW) 0>,
> +		   <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW) 0>,
> +		   <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW) 0>,
> +		   <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW) 0>;
> +		clock-frequency = <24576000>;

I don't remember if you already got Marc's wrath, so just in case: are
you sure it is needed?

Anyway, this is board specific, not SoC.

> +	};
> +
> +	soc: soc@0 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x0 0x0 0x40000000>;
> +
> +		cmu_misc: clock-controller@10010000 {
> +			compatible = "google,gs101-cmu-misc";
> +			reg = <0x10010000 0x8000>;
> +			#clock-cells = <1>;
> +			clocks =  <&cmu_top CLK_DOUT_CMU_MISC_BUS>,

One space after =

> +				  <&cmu_top CLK_DOUT_CMU_MISC_SSS>;
> +			clock-names = "dout_cmu_misc_bus", "dout_cmu_misc_sss";
> +		};
> +
> +		watchdog_cl0: watchdog@10060000 {
> +			compatible = "google,gs101-wdt";
> +			reg = <0x10060000 0x100>;
> +			interrupts = <GIC_SPI 765 IRQ_TYPE_LEVEL_HIGH 0>;
> +			clocks =
> +			  <&cmu_misc CLK_GOUT_MISC_WDT_CLUSTER0_PCLK>,

Join lines (clocks = <foo bar>). Same in other places.

> +			  <&ext_24_5m>;
> +			clock-names = "watchdog", "watchdog_src";
> +			samsung,syscon-phandle = <&pmu_system_controller>;
> +			samsung,cluster-index = <0>;
> +			status = "disabled";
> +		};
> +
> +		watchdog_cl1: watchdog@10070000 {
> +			compatible = "google,gs101-wdt";
> +			reg = <0x10070000 0x100>;
> +			interrupts = <GIC_SPI 766 IRQ_TYPE_LEVEL_HIGH 0>;
> +			clocks =
> +			  <&cmu_misc CLK_GOUT_MISC_WDT_CLUSTER1_PCLK>,
> +			  <&ext_24_5m>;
> +			clock-names = "watchdog", "watchdog_src";
> +			samsung,syscon-phandle = <&pmu_system_controller>;
> +			samsung,cluster-index = <1>;
> +			status = "disabled";
> +		};
> +
> +		gic: interrupt-controller@10400000 {
> +			compatible = "arm,gic-v3";
> +			#interrupt-cells = <4>;
> +			interrupt-controller;
> +			reg = <0x10400000 0x10000>, /* GICD */
> +			      <0x10440000 0x100000>;/* GICR * 8 */
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
> +
> +			ppi-partitions {
> +				ppi_cluster0: interrupt-partition-0 {
> +					affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
> +				};
> +
> +				ppi_cluster1: interrupt-partition-1 {
> +					affinity = <&cpu4 &cpu5>;
> +				};
> +
> +				ppi_cluster2: interrupt-partition-2 {
> +					affinity = <&cpu6 &cpu7>;
> +				};
> +			};
> +		};
> +
> +		sysreg_peric0: syscon@10820000 {
> +			compatible = "google,gs101-peric0-sysreg", "syscon";
> +			reg = <0x10820000 0x10000>;
> +		};
> +
> +		pinctrl_peric0: pinctrl@10840000 {
> +			compatible = "google,gs101-pinctrl";
> +			reg = <0x10840000 0x00001000>;
> +			interrupts = <GIC_SPI 625 IRQ_TYPE_LEVEL_HIGH 0>;
> +		};
> +
> +		usi_uart: usi@10a000c0 {
> +			compatible = "google,gs101-usi",
> +				     "samsung,exynos850-usi";
> +			reg = <0x10a000c0 0x20>;
> +			samsung,sysreg = <&sysreg_peric0 0x1020>;
> +			samsung,mode = <USI_V2_UART>;

vendor properties go to the end, after standard properties, before status.

https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst?h=dt/next&id=0d3a771610d0e155c9aa305f142f84dda5030fae#n122

> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			clocks = <&dummy_clk>, <&dummy_clk>;
> +			clock-names = "pclk", "ipclk";
> +			status = "disabled";
> +
> +			serial_0: serial@10a00000 {
> +				compatible = "google,gs101-uart";
> +				reg = <0x10a00000 0xc0>;
> +				reg-io-width = <4>;
> +				samsung,uart-fifosize = <256>;

Ditto

> +				interrupts = <GIC_SPI 634
> +					      IRQ_TYPE_LEVEL_HIGH 0>;
> +				clocks = <&dummy_clk 0>, <&dummy_clk 0>;
> +				clock-names = "uart", "clk_uart_baud0";
> +				status = "disabled";
> +			};
> +		};


Best regards,
Krzysztof