Messages in this thread | | | Date | Sat, 18 Mar 2023 14:07:40 +0800 | Subject | Re: [PATCH 11/15] arm64: dts: nuvoton: Add initial ma35d1 device tree | From | Jacky Huang <> |
| |
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
| |