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

[Marc Zyngier <maz@kernel.org>]

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 <semen.protsenko@linaro.org>
> ---
>  .../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.
-- 
Jazz is not dead. It just smells funny...