| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c902606e-8a1b-6673-02c7-7beea5477795@gmail.com>
Date: Sat, 18 Mar 2023 14:07:40 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
lee@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
p.zabel@...gutronix.de, gregkh@...uxfoundation.org,
jirislaby@...nel.org
Cc: devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
schung@...oton.com, Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH 11/15] arm64: dts: nuvoton: Add initial ma35d1 device tree
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@...oton.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@...oton.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@...oton.com>
>> + * Jacky huang <ychuang3@...oton.com>
>> + */
>> +
>> +/dts-v1/;
>> +#include "ma35d1.dtsi"
>> +
>> +/ {
>> + model = "Nuvoton MA35D1-IoT";
>> + compatible = "nuvoton,ma35d1-iot", "nuvoton,ma35d1";
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +
>> + mem: memory@...00000 {
>> + 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@...oton.com>
>> + * Jacky huang <ychuang3@...oton.com>
>> + */
>> +
>> +/dts-v1/;
>> +#include "ma35d1.dtsi"
>> +
>> +/ {
>> + model = "Nuvoton MA35D1-SOM";
>> + compatible = "nuvoton,ma35d1-som", "nuvoton,ma35d1";
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +
>> + mem: memory@...00000 {
>> + 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@...oton.com>
>> + * Jacky huang <ychuang3@...oton.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@...60000 {
>> + 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@...60200 {
>> + 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@...01000 {
>> + 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@...00000 {
>> + 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
Powered by blists - more mailing lists