Re: [PATCH 11/15] arm64: dts: nuvoton: Add initial ma35d1 device tree

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

On 15/03/2023 08:28, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Add initial device tree support for Nuvoton ma35d1 SoC, including
> cpu, clock, reset, and serial controllers.
> Add reference boards som-256m and iot-512m.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  arch/arm64/boot/dts/nuvoton/Makefile          |   2 +
>  .../boot/dts/nuvoton/ma35d1-iot-512m.dts      |  24 ++
>  .../boot/dts/nuvoton/ma35d1-som-256m.dts      |  23 ++
>  arch/arm64/boot/dts/nuvoton/ma35d1.dtsi       | 272 ++++++++++++++++++
>  4 files changed, 321 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
>  create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
>  create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> 
> diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
> index a99dab90472a..c11ab4eac9c7 100644
> --- a/arch/arm64/boot/dts/nuvoton/Makefile
> +++ b/arch/arm64/boot/dts/nuvoton/Makefile
> @@ -1,2 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  dtb-$(CONFIG_ARCH_NPCM) += nuvoton-npcm845-evb.dtb
> +dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-iot-512m.dtb
> +dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-som-256m.dtb
> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
> new file mode 100644
> index 000000000000..dffcaef1e6d8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Nuvoton Technology Corp.
> + * Author: Shan-Chun Hung <schung@nuvoton.com>
> + *         Jacky huang <ychuang3@nuvoton.com>
> + */
> +
> +/dts-v1/;
> +#include "ma35d1.dtsi"
> +
> +/ {
> +	model = "Nuvoton MA35D1-IoT";
> +	compatible = "nuvoton,ma35d1-iot", "nuvoton,ma35d1";
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	mem: memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x00000000 0x80000000 0 0x20000000>; /* 512M DRAM */
> +	};
> +};
> +
> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
> new file mode 100644
> index 000000000000..3e6c3d5469ac
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Nuvoton Technology Corp.
> + * Author: Shan-Chun Hung <schung@nuvoton.com>
> + *         Jacky huang <ychuang3@nuvoton.com>
> + */
> +
> +/dts-v1/;
> +#include "ma35d1.dtsi"
> +
> +/ {
> +	model = "Nuvoton MA35D1-SOM";
> +	compatible = "nuvoton,ma35d1-som", "nuvoton,ma35d1";
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	mem: memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x00000000 0x80000000 0 0x10000000>; /* 256M DRAM */
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> new file mode 100644
> index 000000000000..8c855f6b330a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Nuvoton Technology Corp.
> + * Author: Shan-Chun Hung <schung@nuvoton.com>
> + *         Jacky huang <ychuang3@nuvoton.com>
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
> +
> +/ {
> +	compatible = "nuvoton,ma35d1";
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +		serial4 = &uart4;
> +		serial5 = &uart5;
> +		serial6 = &uart6;
> +		serial7 = &uart7;
> +		serial8 = &uart8;
> +		serial9 = &uart9;
> +		serial10 = &uart10;
> +		serial11 = &uart11;
> +		serial12 = &uart12;
> +		serial13 = &uart13;
> +		serial14 = &uart14;
> +		serial15 = &uart15;
> +		serial16 = &uart16;

Aliases of interfaces coming out of SoC are properties of boards, not
SoC DTSI.

> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;


Blank line.

> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a35";
> +			reg = <0x0 0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};

Between every node as well.

> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a35";
> +			reg = <0x0 0x1>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +		L2_0: l2-cache0 {
> +			compatible = "cache";
> +			cache-level = <2>;
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +	clk_hxt: clock_hxt {

No underscores in node names.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "clk_hxt";

This looks like a property of boards, not SoC. Are you sure the clock
physically is in every SoC? If so, why it is not part of clock
controller? (before you start explaining what is this, have in mind that
I am pretty sure I know what is this, so rather answer the questions)

> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
> +			      IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
> +			      IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
> +			      IRQ_TYPE_LEVEL_LOW)>, /* Virtual */
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
> +			      IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */
> +		clock-frequency = <12000000>;
> +		interrupt-parent = <&gic>;
> +	};
> +
> +	sys: system-management@40460000 {
> +		compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> +		reg = <0x0 0x40460000 0x0 0x200>;
> +
> +		reset: reset-controller {
> +			compatible = "nuvoton,ma35d1-reset";
> +			regmap = <&sys>;
> +			#reset-cells = <1>;
> +		};
> +	};
> +
> +	clk: clock-controller@40460200 {
> +		compatible = "nuvoton,ma35d1-clk", "syscon";
> +		reg = <0x00000000 0x40460200 0x0 0x100>;
> +		#clock-cells = <1>;
> +		clocks = <&clk_hxt>;
> +		clock-names = "clk_hxt";
> +		assigned-clocks = <&clk CAPLL>,
> +				  <&clk DDRPLL>,
> +				  <&clk APLL>,
> +				  <&clk EPLL>,
> +				  <&clk VPLL>;
> +		assigned-clock-rates = <800000000>,
> +				       <266000000>,
> +				       <180000000>,
> +				       <500000000>,
> +				       <102000000>;
> +		nuvoton,pll-mode = <0>, <1>, <0>, <0>, <0>;
> +		nuvoton,sys = <&sys>;
> +	};
> +
> +	gic: interrupt-controller@50801000 {
> +		compatible = "arm,gic-400";
> +		#interrupt-cells = <3>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		reg =   <0x0 0x50801000 0 0x1000>, /* GICD */
> +			<0x0 0x50802000 0 0x2000>, /* GICC */
> +			<0x0 0x50804000 0 0x2000>, /* GICH */
> +			<0x0 0x50806000 0 0x2000>; /* GICV */

reg is second property.

> +		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
> +			      IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	uart0:serial@40700000 {
> +		compatible = "nuvoton,ma35d1-uart";
> +		reg = <0x0 0x40700000 0x0 0x100>;
> +		interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clk UART0_GATE>;
> +		status = "okay";

Why? Drop the line... or convert it to disabled. Otherwise, why every
SoC has serial0 enabled? Is it used internally?


Best regards,
Krzysztof