2022-06-08 11:00:29

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

This adds initial device tree support for the
Nuvoton NPCM845 Board Management controller (BMC) SoC family.

The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and
have various peripheral IPs.

Signed-off-by: Tomer Maimon <[email protected]>
---
arch/arm64/boot/dts/Makefile | 1 +
.../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 ++++++++++++++++++
.../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 +++++++
3 files changed, 274 insertions(+)
create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.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/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
new file mode 100644
index 000000000000..97e108c50760
--- /dev/null
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2021 Nuvoton Technology [email protected]
+
+#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-parent = <&gic>;
+
+ /* external reference clock */
+ clk_refclk: clk-refclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <25000000>;
+ clock-output-names = "refclk";
+ };
+
+ /* external reference clock for cpu. float in normal operation */
+ clk_sysbypck: clk-sysbypck {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <1000000000>;
+ clock-output-names = "sysbypck";
+ };
+
+ /* external reference clock for MC. float in normal operation */
+ clk_mcbypck: clk-mcbypck {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <1050000000>;
+ clock-output-names = "mcbypck";
+ };
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ compatible = "simple-bus";
+ interrupt-parent = <&gic>;
+ ranges;
+
+ gcr: gcr@f0800000 {
+ compatible = "nuvoton,npcm845-gcr", "syscon",
+ "simple-mfd";
+ reg = <0x0 0xf0800000 0x0 0x1000>;
+ };
+
+ gic: interrupt-controller@dfff9000 {
+ compatible = "arm,gic-400";
+ reg = <0x0 0xdfff9000 0x0 0x1000>,
+ <0x0 0xdfffa000 0x0 0x2000>,
+ <0x0 0xdfffc000 0x0 0x2000>,
+ <0x0 0xdfffe000 0x0 0x2000>;
+ interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ #address-cells = <0>;
+ ppi-partitions {
+ ppi_cluster0: interrupt-partition-0 {
+ affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
+ };
+ };
+ };
+ };
+
+ ahb {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ compatible = "simple-bus";
+ interrupt-parent = <&gic>;
+ ranges;
+
+ rstc: rstc@f0801000 {
+ compatible = "nuvoton,npcm845-reset";
+ reg = <0x0 0xf0801000 0x0 0x78>;
+ #reset-cells = <2>;
+ nuvoton,sysgcr = <&gcr>;
+ };
+
+ clk: clock-controller@f0801000 {
+ compatible = "nuvoton,npcm845-clk";
+ #clock-cells = <1>;
+ reg = <0x0 0xf0801000 0x0 0x1000>;
+ clock-names = "refclk", "sysbypck", "mcbypck";
+ clocks = <&clk_refclk>, <&clk_sysbypck>, <&clk_mcbypck>;
+ };
+
+ apb {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "simple-bus";
+ interrupt-parent = <&gic>;
+ ranges = <0x0 0x0 0xf0000000 0x00300000>,
+ <0xfff00000 0x0 0xfff00000 0x00016000>;
+
+ timer0: timer@8000 {
+ compatible = "nuvoton,npcm845-timer";
+ interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x8000 0x1C>;
+ clocks = <&clk_refclk>;
+ clock-names = "refclk";
+ };
+
+ serial0: serial@0 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x0 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial1: serial@1000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x1000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial2: serial@2000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x2000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial3: serial@3000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x3000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial4: serial@4000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x4000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial5: serial@5000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x5000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ serial6: serial@6000 {
+ compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart";
+ reg = <0x6000 0x1000>;
+ clocks = <&clk NPCM8XX_CLK_UART>;
+ interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>;
+ reg-shift = <2>;
+ status = "disabled";
+ };
+
+ watchdog0: watchdog@801c {
+ compatible = "nuvoton,npcm845-wdt", "nuvoton,npcm750-wdt";
+ interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x801c 0x4>;
+ status = "disabled";
+ clocks = <&clk_refclk>;
+ syscon = <&gcr>;
+ };
+
+ watchdog1: watchdog@901c {
+ compatible = "nuvoton,npcm845-wdt", "nuvoton,npcm750-wdt";
+ interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x901c 0x4>;
+ status = "disabled";
+ clocks = <&clk_refclk>;
+ syscon = <&gcr>;
+ };
+
+ watchdog2: watchdog@a01c {
+ compatible = "nuvoton,npcm845-wdt", "nuvoton,npcm750-wdt";
+ interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0xa01c 0x4>;
+ status = "disabled";
+ clocks = <&clk_refclk>;
+ syscon = <&gcr>;
+ };
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi
new file mode 100644
index 000000000000..12118b75c0e6
--- /dev/null
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2021 Nuvoton Technology [email protected]
+
+#include "nuvoton-common-npcm8xx.dtsi"
+
+/ {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ clocks = <&clk NPCM8XX_CLK_CPU>;
+ reg = <0x0 0x0>;
+ next-level-cache = <&l2>;
+ enable-method = "psci";
+ };
+
+ cpu1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ clocks = <&clk NPCM8XX_CLK_CPU>;
+ reg = <0x0 0x1>;
+ next-level-cache = <&l2>;
+ enable-method = "psci";
+ };
+
+ cpu2: cpu@2 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ clocks = <&clk NPCM8XX_CLK_CPU>;
+ reg = <0x0 0x2>;
+ next-level-cache = <&l2>;
+ enable-method = "psci";
+ };
+
+ cpu3: cpu@3 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ clocks = <&clk NPCM8XX_CLK_CPU>;
+ reg = <0x0 0x3>;
+ next-level-cache = <&l2>;
+ enable-method = "psci";
+ };
+
+ l2: l2-cache {
+ compatible = "cache";
+ };
+ };
+
+ arm-pmu {
+ compatible = "arm,cortex-a35-pmu";
+ interrupts = <GIC_SPI 242 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
+ };
+
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+ };
+
+ 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)>;
+ };
+};
--
2.33.0


2022-06-08 11:07:35

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

On 08/06/2022 11:56, Tomer Maimon wrote:
> This adds initial device tree support for the
> Nuvoton NPCM845 Board Management controller (BMC) SoC family.
>
> The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and
> have various peripheral IPs.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
> arch/arm64/boot/dts/Makefile | 1 +
> .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 ++++++++++++++++++
> .../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 +++++++
> 3 files changed, 274 insertions(+)
> create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.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/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> new file mode 100644
> index 000000000000..97e108c50760
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2021 Nuvoton Technology [email protected]
> +
> +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-parent = <&gic>;
> +
> + /* external reference clock */
> + clk_refclk: clk-refclk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <25000000>;

Ignored comment.

> + clock-output-names = "refclk";
> + };
> +
> + /* external reference clock for cpu. float in normal operation */
> + clk_sysbypck: clk-sysbypck {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1000000000>;

Ignored comment.

> + clock-output-names = "sysbypck";
> + };
> +
> + /* external reference clock for MC. float in normal operation */
> + clk_mcbypck: clk-mcbypck {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1050000000>;
> + clock-output-names = "mcbypck";
> + };
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + interrupt-parent = <&gic>;
> + ranges;
> +
> + gcr: gcr@f0800000 {

Ignored comment.

> + compatible = "nuvoton,npcm845-gcr", "syscon",
> + "simple-mfd";

This is not a simple-mfd... I see original bindings defined it that way,
but why? I think they should be corrected - remove simple-mfd from the
bindings and DTS.


> + reg = <0x0 0xf0800000 0x0 0x1000>;
> + };
> +
> + gic: interrupt-controller@dfff9000 {
> + compatible = "arm,gic-400";
> + reg = <0x0 0xdfff9000 0x0 0x1000>,
> + <0x0 0xdfffa000 0x0 0x2000>,
> + <0x0 0xdfffc000 0x0 0x2000>,
> + <0x0 0xdfffe000 0x0 0x2000>;
> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + #address-cells = <0>;
> + ppi-partitions {
> + ppi_cluster0: interrupt-partition-0 {
> + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
> + };
> + };
> + };
> + };
> +
> + ahb {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + interrupt-parent = <&gic>;
> + ranges;
> +
> + rstc: rstc@f0801000 {

Ignored comment.

Four comments from v1 ignored in this patch alone.

I'll stop reviewing, it is a waste of my time.

NAK for this change.

Best regards,
Krzysztof

2022-06-09 23:08:23

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

Hi Krzysztof,

Sorry, probably I missed your comments (too many patches to handle at
one time :-))...

On Wed, 8 Jun 2022 at 13:21, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/06/2022 11:56, Tomer Maimon wrote:
> > This adds initial device tree support for the
> > Nuvoton NPCM845 Board Management controller (BMC) SoC family.
> >
> > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and
> > have various peripheral IPs.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
> > arch/arm64/boot/dts/Makefile | 1 +
> > .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 ++++++++++++++++++
> > .../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 +++++++
> > 3 files changed, 274 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.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/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > new file mode 100644
> > index 000000000000..97e108c50760
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2021 Nuvoton Technology [email protected]
> > +
> > +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +/ {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + interrupt-parent = <&gic>;
> > +
> > + /* external reference clock */
> > + clk_refclk: clk-refclk {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <25000000>;
>
> Ignored comment.
Could we use it as a default clock-frequency?
>
> > + clock-output-names = "refclk";
> > + };
> > +
> > + /* external reference clock for cpu. float in normal operation */
> > + clk_sysbypck: clk-sysbypck {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <1000000000>;
>
> Ignored comment.
same as above
>
> > + clock-output-names = "sysbypck";
> > + };
> > +
> > + /* external reference clock for MC. float in normal operation */
> > + clk_mcbypck: clk-mcbypck {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <1050000000>;
same as above
> > + clock-output-names = "mcbypck";
> > + };
> > +
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + compatible = "simple-bus";
> > + interrupt-parent = <&gic>;
> > + ranges;
> > +
> > + gcr: gcr@f0800000 {
I understand it sounds generic but I try to be as much compatible with NPCM7XX
https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L91
>
> Ignored comment.
>
> > + compatible = "nuvoton,npcm845-gcr", "syscon",
> > + "simple-mfd";
>
> This is not a simple-mfd... I see original bindings defined it that way,
> but why? I think they should be corrected - remove simple-mfd from the
> bindings and DTS.
will remove in both places in V3
>
>
> > + reg = <0x0 0xf0800000 0x0 0x1000>;
> > + };
> > +
> > + gic: interrupt-controller@dfff9000 {
> > + compatible = "arm,gic-400";
> > + reg = <0x0 0xdfff9000 0x0 0x1000>,
> > + <0x0 0xdfffa000 0x0 0x2000>,
> > + <0x0 0xdfffc000 0x0 0x2000>,
> > + <0x0 0xdfffe000 0x0 0x2000>;
> > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> > + #interrupt-cells = <3>;
> > + interrupt-controller;
> > + #address-cells = <0>;
> > + ppi-partitions {
> > + ppi_cluster0: interrupt-partition-0 {
> > + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
> > + };
> > + };
> > + };
> > + };
> > +
> > + ahb {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + compatible = "simple-bus";
> > + interrupt-parent = <&gic>;

> > + ranges;
> > +
> > + rstc: rstc@f0801000 {
>
> Ignored comment.
>
I understand it sounds generic but I try to be as much compatible with NPCM7XX
https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L109
> Four comments from v1 ignored in this patch alone.
>
one more comment in V1
"+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ clocks = <&clk NPCM8XX_CLK_CPU>;
+ reg = <0x0 0x0>;
Why do you have two address cells? A bit more complicated and not
necessary, I think."
the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>;
and therefore reg = <0x0 0x0>;

> I'll stop reviewing, it is a waste of my time.
>
> NAK for this change.
>
> Best regards,
> Krzysztof

Again sorry to miss these comments in V1.

Appreciate your time.

Best regards,

Tomer

2022-06-10 09:08:16

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

Hi Tomer,

On Fri, Jun 10, 2022 at 12:30 AM Tomer Maimon <[email protected]> wrote:
> On Wed, 8 Jun 2022 at 13:21, Krzysztof Kozlowski
> <[email protected]> wrote:
> > On 08/06/2022 11:56, Tomer Maimon wrote:
> > > This adds initial device tree support for the
> > > Nuvoton NPCM845 Board Management controller (BMC) SoC family.
> > >
> > > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and
> > > have various peripheral IPs.
> > >
> > > Signed-off-by: Tomer Maimon <[email protected]>

> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > > @@ -0,0 +1,197 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2021 Nuvoton Technology [email protected]
> > > +
> > > +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h>
> > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +#include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +/ {
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > + interrupt-parent = <&gic>;
> > > +
> > > + /* external reference clock */
> > > + clk_refclk: clk-refclk {
> > > + compatible = "fixed-clock";
> > > + #clock-cells = <0>;
> > > + clock-frequency = <25000000>;
> >
> > Ignored comment.
> Could we use it as a default clock-frequency?

If the oscillator is present on the board, and not an SoC builtin, its
clock frequency should be described in the board DTS.
Some clocks may be optional, and left unpopulated.
Others clocks may be fed with different frequencies than the default.

> >
> > > + clock-output-names = "refclk";
> > > + };
> > > +
> > > + /* external reference clock for cpu. float in normal operation */
> > > + clk_sysbypck: clk-sysbypck {
> > > + compatible = "fixed-clock";
> > > + #clock-cells = <0>;
> > > + clock-frequency = <1000000000>;
> >
> > Ignored comment.
> same as above
> >
> > > + clock-output-names = "sysbypck";
> > > + };
> > > +
> > > + /* external reference clock for MC. float in normal operation */
> > > + clk_mcbypck: clk-mcbypck {
> > > + compatible = "fixed-clock";
> > > + #clock-cells = <0>;
> > > + clock-frequency = <1050000000>;
> same as above
> > > + clock-output-names = "mcbypck";
> > > + };

> "+ cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + clocks = <&clk NPCM8XX_CLK_CPU>;
> + reg = <0x0 0x0>;
> Why do you have two address cells? A bit more complicated and not
> necessary, I think."
> the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>;
> and therefore reg = <0x0 0x0>;

These addresses are not addresses on the main memory bus (which
is indeed 64-bit), but on the logical CPU bus.
Now, Documentation/devicetree/bindings/arm/cpus.yaml says you can
have #address-cells = <2> if you have non-zero MPIDR_EL1 high bits.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-06-10 10:31:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

On 10/06/2022 00:29, Tomer Maimon wrote:
>>> + clk_refclk: clk-refclk {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>;
>>> + clock-frequency = <25000000>;
>>
>> Ignored comment.
> Could we use it as a default clock-frequency?

In DTS? If my assumption, that this clock is not on SoC itself, is
correct, then the answer is no, you cannot. The clock physically sits on
the board, so it is defined by board DTS. Feel free to embed in SoC DTSI
most of the clock properties, but the core property - frequency - must
be outside.

>>
>>> + clock-output-names = "refclk";
>>> + };
>>> +
>>> + /* external reference clock for cpu. float in normal operation */
>>> + clk_sysbypck: clk-sysbypck {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>;
>>> + clock-frequency = <1000000000>;
>>
>> Ignored comment.
> same as above
>>
>>> + clock-output-names = "sysbypck";
>>> + };
>>> +
>>> + /* external reference clock for MC. float in normal operation */
>>> + clk_mcbypck: clk-mcbypck {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>;
>>> + clock-frequency = <1050000000>;
> same as above
>>> + clock-output-names = "mcbypck";
>>> + };
>>> +
>>> + soc {
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> + compatible = "simple-bus";
>>> + interrupt-parent = <&gic>;
>>> + ranges;
>>> +
>>> + gcr: gcr@f0800000 {
> I understand it sounds generic but I try to be as much compatible with NPCM7XX
> https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L91

Then fix NPCM7XX. Please do not multiple bad choices because it looks
similar. Fix the other wrong one.

>>
>> Ignored comment.
>>
>>> + compatible = "nuvoton,npcm845-gcr", "syscon",
>>> + "simple-mfd";
>>
>> This is not a simple-mfd... I see original bindings defined it that way,
>> but why? I think they should be corrected - remove simple-mfd from the
>> bindings and DTS.
> will remove in both places in V3
>>
>>
>>> + reg = <0x0 0xf0800000 0x0 0x1000>;
>>> + };
>>> +
>>> + gic: interrupt-controller@dfff9000 {
>>> + compatible = "arm,gic-400";
>>> + reg = <0x0 0xdfff9000 0x0 0x1000>,
>>> + <0x0 0xdfffa000 0x0 0x2000>,
>>> + <0x0 0xdfffc000 0x0 0x2000>,
>>> + <0x0 0xdfffe000 0x0 0x2000>;
>>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>> + #interrupt-cells = <3>;
>>> + interrupt-controller;
>>> + #address-cells = <0>;
>>> + ppi-partitions {
>>> + ppi_cluster0: interrupt-partition-0 {
>>> + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
>>> + };
>>> + };
>>> + };
>>> + };
>>> +
>>> + ahb {
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> + compatible = "simple-bus";
>>> + interrupt-parent = <&gic>;
>
>>> + ranges;
>>> +
>>> + rstc: rstc@f0801000 {
>>
>> Ignored comment.
>>
> I understand it sounds generic but I try to be as much compatible with NPCM7XX
> https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L109

Fix 7xx.


Best regards,
Krzysztof

2022-06-10 10:45:28

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

On 10/06/2022 09:57, Geert Uytterhoeven wrote:
>> "+ cpu0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a35";
>> + clocks = <&clk NPCM8XX_CLK_CPU>;
>> + reg = <0x0 0x0>;
>> Why do you have two address cells? A bit more complicated and not
>> necessary, I think."
>> the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>;
>> and therefore reg = <0x0 0x0>;
>
> These addresses are not addresses on the main memory bus (which
> is indeed 64-bit), but on the logical CPU bus.
> Now, Documentation/devicetree/bindings/arm/cpus.yaml says you can
> have #address-cells = <2> if you have non-zero MPIDR_EL1 high bits.
>

Thanks Tomer and Geert for explanation. OK for me.


Best regards,
Krzysztof