Add the initial device tree files for Nuvoton MA35D1 Soc.
Signed-off-by: Jacky Huang <[email protected]>
---
arch/arm64/boot/dts/Makefile | 1 +
arch/arm64/boot/dts/nuvoton/Makefile | 2 +
arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts | 24 +++++
arch/arm64/boot/dts/nuvoton/ma35d1.dtsi | 120 +++++++++++++++++++++
4 files changed, 147 insertions(+)
create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile
create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index 1ba04e31a438..7b107fa7414b 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -19,6 +19,7 @@ subdir-y += lg
subdir-y += marvell
subdir-y += mediatek
subdir-y += microchip
+subdir-y += nuvoton
subdir-y += nvidia
subdir-y += qcom
subdir-y += realtek
diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
new file mode 100644
index 000000000000..e1e0c466bf5e
--- /dev/null
+++ b/arch/arm64/boot/dts/nuvoton/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-evb.dtb
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
new file mode 100644
index 000000000000..95f0facb0476
--- /dev/null
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Device Tree Source for MA35D1 Evaluation Board (EVB)
+ *
+ * Copyright (C) 2022 Nuvoton Technology Corp.
+ */
+
+/dts-v1/;
+#include "ma35d1.dtsi"
+
+/ {
+ model = "Nuvoton MA35D1-EVB";
+ compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1";
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ memory@80000000 {
+ device_type = "memory";
+ reg = <0x0 0x80000000 0x0 0x10000000>;
+ };
+};
+
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
new file mode 100644
index 000000000000..7212f8de6906
--- /dev/null
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Copyright (c) 2022 Nuvoton Technology Corp.
+ */
+
+#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>
+
+/ {
+ compatible = "nuvoton,ma35d1";
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ cpu-map {
+ cluster0 {
+ core0 {
+ cpu = <&cpu0>;
+ };
+ core1 {
+ cpu = <&cpu1>;
+ };
+ };
+ };
+
+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <0x0>;
+ enable-method = "psci";
+ next-level-cache = <&L2_0>;
+ };
+
+ cpu1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <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";
+ };
+
+ hxt_24m: hxt_24mhz {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ clock-output-names = "HXT_24MHz";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>;
+ clock-frequency = <12000000>;
+ };
+
+ sys: system-controller@40460000 {
+ compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
+ reg = <0x0 0x40460000 0x0 0x400>;
+ };
+
+ reset: reset-controller {
+ compatible = "nuvoton,ma35d1-reset";
+ nuvoton,ma35d1-sys = <&sys>;
+ #reset-cells = <1>;
+ };
+
+ clk: clock-controller@40460200 {
+ compatible = "nuvoton,ma35d1-clk";
+ reg = <0x0 0x40460200 0x0 0x100>;
+ #clock-cells = <1>;
+ clocks = <&hxt_24m>;
+ clock-names = "HXT_24MHz";
+ assigned-clocks = <&clk CAPLL>,
+ <&clk DDRPLL>,
+ <&clk APLL>,
+ <&clk EPLL>,
+ <&clk VPLL>;
+ assigned-clock-rates = <1000000000>,
+ <266000000>,
+ <180000000>,
+ <500000000>,
+ <102000000>;
+ nuvoton,clk-pll-mode = <0 1 0 0 0>;
+ };
+
+ gic: interrupt-controller@50801000 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ reg = <0x0 0x50801000 0x0 0x1000>,
+ <0x0 0x50802000 0x0 0x2000>,
+ <0x0 0x50804000 0x0 0x2000>,
+ <0x0 0x50806000 0x0 0x2000>;
+ interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
+ IRQ_TYPE_LEVEL_HIGH)>;
+ };
+};
--
2.30.2
On 2022/5/10 下午 03:01, Arnd Bergmann wrote:
> On Tue, May 10, 2022 at 5:25 AM Jacky Huang <[email protected]> wrote:
>> Add the initial device tree files for Nuvoton MA35D1 Soc.
>>
>> Signed-off-by: Jacky Huang <[email protected]>
>> ---
>> +
>> +/ {
>> + model = "Nuvoton MA35D1-EVB";
>> + compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1";
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
> Something seems to be missing here: you set the console to the serial0
> alias, but that is not defined anywhere, and the ma35d1.dtsi file does not
> appear to define any UART at all. Are you still missing the driver for this?
>
> Please add a more detailed description in the changelog text above that
> explains what kind of SoC this is (maybe a link to the product web page,
> if there is one), and a status of how complete the support is: which drivers
> are already merged, and which ones are still being worked on?
>
> Arnd
Hi Arnd,
The serial driver is ready
(https://github.com/OpenNuvoton/MA35D1_linux-5.4.y/blob/master/drivers/tty/serial/ma35d1_serial.c),
but we have to review the coding style and porting it from Linux 5.4.y
to 5.18.
In the next patch version, I will added a brief introduction about
MA35D1 in the cover-letter [PATCH 0/5].
Thanks for your review.
Sincerely,
Jacky Huang
On Tue, May 10, 2022 at 5:25 AM Jacky Huang <[email protected]> wrote:
>
> Add the initial device tree files for Nuvoton MA35D1 Soc.
>
> Signed-off-by: Jacky Huang <[email protected]>
> ---
> +
> +/ {
> + model = "Nuvoton MA35D1-EVB";
> + compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1";
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
Something seems to be missing here: you set the console to the serial0
alias, but that is not defined anywhere, and the ma35d1.dtsi file does not
appear to define any UART at all. Are you still missing the driver for this?
Please add a more detailed description in the changelog text above that
explains what kind of SoC this is (maybe a link to the product web page,
if there is one), and a status of how complete the support is: which drivers
are already merged, and which ones are still being worked on?
Arnd
On 10/05/2022 05:25, Jacky Huang wrote:
> Add the initial device tree files for Nuvoton MA35D1 Soc.
>
> Signed-off-by: Jacky Huang <[email protected]>
> ---
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/nuvoton/Makefile | 2 +
> arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts | 24 +++++
> arch/arm64/boot/dts/nuvoton/ma35d1.dtsi | 120 +++++++++++++++++++++
> 4 files changed, 147 insertions(+)
> create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile
> create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
> create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 1ba04e31a438..7b107fa7414b 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -19,6 +19,7 @@ subdir-y += lg
> subdir-y += marvell
> subdir-y += mediatek
> subdir-y += microchip
> +subdir-y += nuvoton
> subdir-y += nvidia
> subdir-y += qcom
> subdir-y += realtek
> diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
> new file mode 100644
> index 000000000000..e1e0c466bf5e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-evb.dtb
> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
> new file mode 100644
> index 000000000000..95f0facb0476
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Device Tree Source for MA35D1 Evaluation Board (EVB)
> + *
> + * Copyright (C) 2022 Nuvoton Technology Corp.
> + */
> +
> +/dts-v1/;
> +#include "ma35d1.dtsi"
> +
> +/ {
> + model = "Nuvoton MA35D1-EVB";
> + compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1";
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + memory@80000000 {
> + device_type = "memory";
> + reg = <0x0 0x80000000 0x0 0x10000000>;
> + };
> +};
> +
.git/rebase-apply/patch:60: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> new file mode 100644
> index 000000000000..7212f8de6906
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (c) 2022 Nuvoton Technology Corp.
> + */
> +
> +#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>
> +
> +/ {
> + compatible = "nuvoton,ma35d1";
> + interrupt-parent = <&gic>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&cpu0>;
> + };
> + core1 {
> + cpu = <&cpu1>;
> + };
> + };
> + };
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0>;
> + enable-method = "psci";
> + next-level-cache = <&L2_0>;
> + };
> +
> + cpu1: cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <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";
> + };
> +
> + hxt_24m: hxt_24mhz {
No underscores in node name. Generic node names, so "clock-X" or
"clock-some-suffix"
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24000000>;
This does not look like property of SoC. Where is this clock defined? In
the SoC or on the board?
> + clock-output-names = "HXT_24MHz";
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>;
> + clock-frequency = <12000000>;
> + };
> +
> + sys: system-controller@40460000 {
> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
Why is this a simple-mfd if there are no children here? What do you want
to instantiate here?
Where is the nuvoton,ma35d1-sys compatible documented?
> + reg = <0x0 0x40460000 0x0 0x400>;
> + };
> +
> + reset: reset-controller {
> + compatible = "nuvoton,ma35d1-reset";
Also not documented.
> + nuvoton,ma35d1-sys = <&sys>;
> + #reset-cells = <1>;
> + };
> +
> + clk: clock-controller@40460200 {
> + compatible = "nuvoton,ma35d1-clk";
> + reg = <0x0 0x40460200 0x0 0x100>;
> + #clock-cells = <1>;
> + clocks = <&hxt_24m>;
> + clock-names = "HXT_24MHz";
Please test your DTS with make dtbs_check.
Don't send DTS which does not pass the checks. It is unnecessary use of
reviewers time when the same job can be done by automated tools.
Best regards,
Krzysztof
On 2022/5/12 下午 10:10, Krzysztof Kozlowski wrote:
> On 10/05/2022 05:25, Jacky Huang wrote:
>> Add the initial device tree files for Nuvoton MA35D1 Soc.
>>
>> Signed-off-by: Jacky Huang <[email protected]>
>> ---
>> arch/arm64/boot/dts/Makefile | 1 +
>> arch/arm64/boot/dts/nuvoton/Makefile | 2 +
>> arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts | 24 +++++
>> arch/arm64/boot/dts/nuvoton/ma35d1.dtsi | 120 +++++++++++++++++++++
>> 4 files changed, 147 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile
>> create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
>> create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
>> index 1ba04e31a438..7b107fa7414b 100644
>> --- a/arch/arm64/boot/dts/Makefile
>> +++ b/arch/arm64/boot/dts/Makefile
>> @@ -19,6 +19,7 @@ subdir-y += lg
>> subdir-y += marvell
>> subdir-y += mediatek
>> subdir-y += microchip
>> +subdir-y += nuvoton
>> subdir-y += nvidia
>> subdir-y += qcom
>> subdir-y += realtek
>> diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
>> new file mode 100644
>> index 000000000000..e1e0c466bf5e
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/nuvoton/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-evb.dtb
>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
>> new file mode 100644
>> index 000000000000..95f0facb0476
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Device Tree Source for MA35D1 Evaluation Board (EVB)
>> + *
>> + * Copyright (C) 2022 Nuvoton Technology Corp.
>> + */
>> +
>> +/dts-v1/;
>> +#include "ma35d1.dtsi"
>> +
>> +/ {
>> + model = "Nuvoton MA35D1-EVB";
>> + compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1";
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +
>> + memory@80000000 {
>> + device_type = "memory";
>> + reg = <0x0 0x80000000 0x0 0x10000000>;
>> + };
>> +};
>> +
>
> .git/rebase-apply/patch:60: new blank line at EOF.
>
> +
>
> warning: 1 line adds whitespace errors.
>
I will fix it.
>
>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>> new file mode 100644
>> index 000000000000..7212f8de6906
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>> @@ -0,0 +1,120 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Copyright (c) 2022 Nuvoton Technology Corp.
>> + */
>> +
>> +#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>
>> +
>> +/ {
>> + compatible = "nuvoton,ma35d1";
>> + interrupt-parent = <&gic>;
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + cpus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + cpu-map {
>> + cluster0 {
>> + core0 {
>> + cpu = <&cpu0>;
>> + };
>> + core1 {
>> + cpu = <&cpu1>;
>> + };
>> + };
>> + };
>> +
>> + cpu0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a35";
>> + reg = <0x0>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_0>;
>> + };
>> +
>> + cpu1: cpu@1 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a35";
>> + reg = <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";
>> + };
>> +
>> + hxt_24m: hxt_24mhz {
> No underscores in node name. Generic node names, so "clock-X" or
> "clock-some-suffix"
OK, I will modify it as
hxt-24m: hxt-24mhz
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <24000000>;
> This does not look like property of SoC. Where is this clock defined? In
> the SoC or on the board?
It's an external crystal on the board.
I add this node, because it's the clock source of clock controller.
It always present on all ma35d1 boards.
clk: clock-controller@40460200 {
compatible = "nuvoton,ma35d1-clk";
reg = <0x0 0x40460200 0x0 0x100>;
#clock-cells = <1>;
clocks = <&hxt_24m>;
clock-names = "HXT_24MHz";
...
>> + clock-output-names = "HXT_24MHz";
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>;
>> + clock-frequency = <12000000>;
>> + };
>> +
>> + sys: system-controller@40460000 {
>> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> Why is this a simple-mfd if there are no children here? What do you want
> to instantiate here?
It's not a device, but a set of registers for system level control.
I want to provide a register base mapping for other devices to access
system control registers.
> Where is the nuvoton,ma35d1-sys compatible documented?
OK, I will add the compatible document in next version.
>> + reg = <0x0 0x40460000 0x0 0x400>;
>> + };
>> +
>> + reset: reset-controller {
>> + compatible = "nuvoton,ma35d1-reset";
> Also not documented.
I will also add the document for it.
>
>> + nuvoton,ma35d1-sys = <&sys>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + clk: clock-controller@40460200 {
>> + compatible = "nuvoton,ma35d1-clk";
>> + reg = <0x0 0x40460200 0x0 0x100>;
>> + #clock-cells = <1>;
>> + clocks = <&hxt_24m>;
>> + clock-names = "HXT_24MHz";
> Please test your DTS with make dtbs_check.
>
> Don't send DTS which does not pass the checks. It is unnecessary use of
> reviewers time when the same job can be done by automated tools.
>
> Best regards,
> Krzysztof
Yes, I read the "writing-schema.rst" and know how to do now.
Thank you.
Sincerely,
Jacky Huang
On 13/05/2022 08:48, Jacky Huang wrote:
>>> +
>>> + hxt_24m: hxt_24mhz {
>> No underscores in node name. Generic node names, so "clock-X" or
>> "clock-some-suffix"
>
> OK, I will modify it as
> hxt-24m: hxt-24mhz
No, it is not a generic node name. Please read my reply again.
>
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>;
>>> + clock-frequency = <24000000>;
>> This does not look like property of SoC. Where is this clock defined? In
>> the SoC or on the board?
>
> It's an external crystal on the board.
> I add this node, because it's the clock source of clock controller.
> It always present on all ma35d1 boards.
>
> clk: clock-controller@40460200 {
> compatible = "nuvoton,ma35d1-clk";
> reg = <0x0 0x40460200 0x0 0x100>;
> #clock-cells = <1>;
> clocks = <&hxt_24m>;
> clock-names = "HXT_24MHz";
> ...
>
>>> + clock-output-names = "HXT_24MHz";
>>> + };
>>> +
>>> + timer {
>>> + compatible = "arm,armv8-timer";
>>> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
>>> + IRQ_TYPE_LEVEL_LOW)>,
>>> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
>>> + IRQ_TYPE_LEVEL_LOW)>,
>>> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
>>> + IRQ_TYPE_LEVEL_LOW)>,
>>> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
>>> + IRQ_TYPE_LEVEL_LOW)>;
>>> + clock-frequency = <12000000>;
>>> + };
>>> +
>>> + sys: system-controller@40460000 {
>>> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>> Why is this a simple-mfd if there are no children here? What do you want
>> to instantiate here?
>
> It's not a device, but a set of registers for system level control.
> I want to provide a register base mapping for other devices to access
> system control registers.
This does not explain why you need simple-mfd. simple-mfd is not for
providing a register base mapping for other devices.
>
>> Where is the nuvoton,ma35d1-sys compatible documented?
>
> OK, I will add the compatible document in next version.
>
>
>>> + reg = <0x0 0x40460000 0x0 0x400>;
>>> + };
>>> +
>>> + reset: reset-controller {
>>> + compatible = "nuvoton,ma35d1-reset";
>> Also not documented.
>
> I will also add the document for it.
All of these should fail on checkpatch which points that you either did
not run it or ignored the result.
Please run checkpatch on all your submissions to Linux kernel and be
sure that there is no warning or error.
Best regards,
Krzysztof
On 2022/5/13 下午 02:57, Krzysztof Kozlowski wrote:
> On 13/05/2022 08:48, Jacky Huang wrote:
>>>> +
>>>> + hxt_24m: hxt_24mhz {
>>> No underscores in node name. Generic node names, so "clock-X" or
>>> "clock-some-suffix"
>> OK, I will modify it as
>> hxt-24m: hxt-24mhz
> No, it is not a generic node name. Please read my reply again.
I would modify it as
clock-hxt: clock-hspd-ext-crystal
>
>>>> + compatible = "fixed-clock";
>>>> + #clock-cells = <0>;
>>>> + clock-frequency = <24000000>;
>>> This does not look like property of SoC. Where is this clock defined? In
>>> the SoC or on the board?
>> It's an external crystal on the board.
>> I add this node, because it's the clock source of clock controller.
>> It always present on all ma35d1 boards.
>>
>> clk: clock-controller@40460200 {
>> compatible = "nuvoton,ma35d1-clk";
>> reg = <0x0 0x40460200 0x0 0x100>;
>> #clock-cells = <1>;
>> clocks = <&hxt_24m>;
>> clock-names = "HXT_24MHz";
>> ...
>>
>>>> + clock-output-names = "HXT_24MHz";
>>>> + };
>>>> +
>>>> + timer {
>>>> + compatible = "arm,armv8-timer";
>>>> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
>>>> + IRQ_TYPE_LEVEL_LOW)>,
>>>> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
>>>> + IRQ_TYPE_LEVEL_LOW)>,
>>>> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
>>>> + IRQ_TYPE_LEVEL_LOW)>,
>>>> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
>>>> + IRQ_TYPE_LEVEL_LOW)>;
>>>> + clock-frequency = <12000000>;
>>>> + };
>>>> +
>>>> + sys: system-controller@40460000 {
>>>> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>>> Why is this a simple-mfd if there are no children here? What do you want
>>> to instantiate here?
>> It's not a device, but a set of registers for system level control.
>> I want to provide a register base mapping for other devices to access
>> system control registers.
> This does not explain why you need simple-mfd. simple-mfd is not for
> providing a register base mapping for other devices.
OK, I will remove the "simple-mfd" from sys node.
>>> Where is the nuvoton,ma35d1-sys compatible documented?
>> OK, I will add the compatible document in next version.
>>
>>
>>>> + reg = <0x0 0x40460000 0x0 0x400>;
>>>> + };
>>>> +
>>>> + reset: reset-controller {
>>>> + compatible = "nuvoton,ma35d1-reset";
>>> Also not documented.
>> I will also add the document for it.
> All of these should fail on checkpatch which points that you either did
> not run it or ignored the result.
>
> Please run checkpatch on all your submissions to Linux kernel and be
> sure that there is no warning or error.
>
>
>
> Best regards,
> Krzysztof
Sure, thank you for reminding.
Sincerely,.
Jacky Huang
On 15/05/2022 07:53, Jacky Huang wrote:
>
> On 2022/5/13 下午 02:57, Krzysztof Kozlowski wrote:
>> On 13/05/2022 08:48, Jacky Huang wrote:
>>>>> +
>>>>> + hxt_24m: hxt_24mhz {
>>>> No underscores in node name. Generic node names, so "clock-X" or
>>>> "clock-some-suffix"
>>> OK, I will modify it as
>>> hxt-24m: hxt-24mhz
>> No, it is not a generic node name. Please read my reply again.
>
> I would modify it as
>
> clock-hxt: clock-hspd-ext-crystal
>
>
>>
>>>>> + compatible = "fixed-clock";
>>>>> + #clock-cells = <0>;
>>>>> + clock-frequency = <24000000>;
>>>> This does not look like property of SoC. Where is this clock defined? In
>>>> the SoC or on the board?
>>> It's an external crystal on the board.
>>> I add this node, because it's the clock source of clock controller.
>>> It always present on all ma35d1 boards.
>>>
Then such clock is not a property of a SoC, but a board. Feel free to
simplify DTS by storing most of the clock node in DTSI, but frequency
should be defined by each board.
Best regards,
Krzysztof