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

[Jacky Huang <ychuang570808@gmail.com>]

Dear Krzysztof,


Thanks for your review.


On 2023/3/16 下午 03:45, Krzysztof Kozlowski wrote:
> 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.


OK, I will remove it from dtsi, and add to dts if required.


>> +	};
>> +
>> +	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.


OK, I will fix them.


>> +		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.


I will rewrite as:

clk_hxt: clock-hxt {


>
>> +		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)


Yes, it's external to SoC and should be board specific.

I will move this to board dts.


>> +	};
>> +
>> +	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.


OK, I will fix it.


>
>> +		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?


uart0 is on all the way since this SoC booting from the MaskROM boot code,

load arm-trusted-firmware, load bootloader, and finally load linux  kernel.

uart0 is also the Linux console.


>
> Best regards,
> Krzysztof


Best regards,

Jacky Huang