Introduce Nuvoton Arbel NPCM8XX BMC SoC

Add Nuvoton BMC NPCM845 timer support.
The NPCM845 uses the same timer controller as the NPCM750.

Signed-off-by Tomer Maimon <[email protected]>

---
 drivers/clocksource/timer-npcm7xx.c | 1 +

Add Nuvoton BMC NPCM845 UART support.
The NPCM845 uses the same UART as the NPCM750.

Signed-off-by Tomer Maimon <[email protected]>

Quoted 4 lines · Tomer Maimon

Add Nuvoton BMC NPCM845 UART support.
The NPCM845 uses the same UART as the NPCM750.

Quoted 1 line

Signed-off-by Tomer Maimon <[email protected]>

This one should no longer be needed if the timers are compatible with the old ones and correctly described in the DT.

Arnd

Sorry but Just to clarify, This patch should not be applied and the NPCM8XX UART should use nuvoton,npcm750-uart compatible in the device tree?

Because I thought that in your comment a few weeks ago
https://www.spinics.net/lists/linux-serial/msg48179.html

We need only to modify the compatible string in the device tree as we did in V2 patchset
https://www.spinics.net/lists/arm-kernel/msg986480.html

Add Nuvoton BMC NPCM845 watchdog support. The NPCM845 uses the same watchdog as the NPCM750.

Signed-off-by Tomer Maimon <[email protected]>

Quoted 5 lines · Tomer Maimon

Add Nuvoton BMC NPCM845 watchdog support. The NPCM845 uses the same watchdog as the NPCM750.

Signed-off-by Tomer Maimon <[email protected]>

This one should no longer be needed if the timers are compatible with the old ones and correctly described in the DT.

Arnd

Quoted 3 lines · Arnd Bergmann

Quoted 1 line · Tomer Maimon

Quoted 7 lines

Quoted 4 lines

Add Nuvoton BMC NPCM845 watchdog support. The NPCM845 uses the same watchdog as the NPCM750.

Signed-off-by Tomer Maimon <[email protected]>

This one should no longer be needed if the timers are compatible with the old ones and correctly described in the DT.

by timers do you mean clocks?

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +25 -0

new file mode 100644
index 000000000000..e1f375716bc5
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:

Quoted 41 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +25 -0

new file mode 100644
index 000000000000..e1f375716bc5
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 35

Quoted 10 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +10 -0

+      - description: 25M reference clock
+      - description: CPU reference clock
+      - description: MC reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+      - const: sysbypck
+      - const: mcbypck
+

I asked what is the suffix about and you replied "ck"... ok, so let's make clear. This should be:

items:
- const: ref
- const: sysbyp
- const: mcbyp

or something similar, without the same suffix all over.

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Quoted 6 lines

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +0 -0

new file mode 100644
index 000000000000..229915a254a5

Same comment as before. No changes here...

Best regards,
Krzysztof

Hi Krzysztof,

Thanks for your comments.

Quoted 57 lines · Krzysztof Kozlowski

Quoted 41 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +25 -0

new file mode 100644
index 000000000000..e1f375716bc5
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 35

Quoted 10 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +10 -0

+      - description: 25M reference clock
+      - description: CPU reference clock
+      - description: MC reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+      - const: sysbypck
+      - const: mcbypck
+

I asked what is the suffix about and you replied "ck"... ok, so let's make clear. This should be:

Quoted 6 lines

items:
- const: ref
- const: sysbyp
- const: mcbyp

or something similar, without the same suffix all over.

The clock names are the same clock name in our spec, this why we prefer to leave the clock names as is.

Quoted 9 lines

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Quoted 6 lines

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +0 -0

new file mode 100644
index 000000000000..229915a254a5

Same comment as before. No changes here...

about the comments from V1::
- Krzysztof: Filename - same as bindings, so nuvoton,npcm845-clk.h In NPCM7XX we use the same include file and clock source dt-binding
https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/clock/nuvoton,npcm750-clk.txt
dt-binding include
https://elixir.bootlin.com/linux/v5.19-rc1/source/include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
we prefer to be align with our older BMC version

- Krzysztof: Dual license, same as bindings.
modified in the file * SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
the same license approved in en7523-clk include file and pushed to Linux kernel 5.19 :
https://elixir.bootlin.com/linux/v5.19-rc1/source/include/dt-bindings/clock/en7523-clk.h

Quoted 68 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your comments.

Quoted 62 lines · Krzysztof Kozlowski

Quoted 41 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +25 -0

new file mode 100644
index 000000000000..e1f375716bc5
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 35

Quoted 10 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +10 -0

+      - description: 25M reference clock
+      - description: CPU reference clock
+      - description: MC reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+      - const: sysbypck
+      - const: mcbypck
+

I asked what is the suffix about and you replied "ck"... ok, so let's make clear. This should be:

items:
- const: ref
- const: sysbyp
- const: mcbyp

Quoted 3 lines

Quoted 1 line

or something similar, without the same suffix all over.

The clock names are the same clock name in our spec, this why we prefer to leave the clock names as is.

The naming with useless suffixes does not help. If your spec had "refclk_really_clock_this_is_a_clock" you also would insist on that? It does not make sense.

Quoted 10 lines

Quoted 10 lines

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Quoted 7 lines

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +0 -0

new file mode 100644
index 000000000000..229915a254a5

Same comment as before. No changes here...

Quoted 8 lines

about the comments from V1::
- Krzysztof: Filename - same as bindings, so nuvoton,npcm845-clk.h In NPCM7XX we use the same include file and clock source dt-binding
https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/clock/nuvoton,npcm750-clk.txt
dt-binding include
https://elixir.bootlin.com/linux/v5.19-rc1/source/include/dt-bindings/clock/nuvoton,npcm7xx-clock.h
we prefer to be align with our older BMC version

Older has incorrect name, so do not align to incorrect one. What is the logic behind having header not matching the bindings file? It makes it only more difficult to connect these two.

Quoted 8 lines

- Krzysztof: Dual license, same as bindings.
modified in the file * SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
the same license approved in en7523-clk include file and pushed to Linux kernel 5.19 :
https://elixir.bootlin.com/linux/v5.19-rc1/source/include/dt-bindings/clock/en7523-clk.h

I don't understand this comment at all. I am not commenting about en7523-clk.h. I am commenting about the header here - it should have dual license. What en7523-clk.h has to do with it?

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 74 lines · Krzysztof Kozlowski

Quoted 71 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your comments.

Quoted 57 lines · Krzysztof Kozlowski

Quoted 41 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +25 -0

new file mode 100644
index 000000000000..e1f375716bc5
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 35

Quoted 10 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +10 -0

+      - description: 25M reference clock
+      - description: CPU reference clock
+      - description: MC reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+      - const: sysbypck
+      - const: mcbypck
+

I asked what is the suffix about and you replied "ck"... ok, so let's make clear. This should be:

Quoted 6 lines

items:
- const: ref
- const: sysbyp
- const: mcbyp

or something similar, without the same suffix all over.

The clock names are the same clock name in our spec, this why we prefer to leave the clock names as is.

Quoted 3 lines

The naming with useless suffixes does not help. If your spec had "refclk_really_clock_this_is_a_clock" you also would insist on that? It does not make sense.

Sorry but I don't understand why the clock name cause an issue, we prefer it will be the same as in our spec-clock diagram BTW, the same naming found in NPCM7XX
https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/clock/nuvoton,npcm750-clk.txt#L36

Quoted 4 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 35

Quoted 12 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +12 -0

+  clocks:
+    items:
+      - description: 25M reference clock
+      - description: CPU reference clock
+      - description: MC reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+      - const: sysbypck
+      - const: mcbypck
+

I asked what is the suffix about and you replied "ck"... ok, so let's make clear. This should be:

items:
- const: ref
- const: sysbyp
- const: mcbyp

or something similar, without the same suffix all over.

drivers/clk/clk-npcm8xx.c new file +0 -0

new file mode 100644
index 000000000000..40340c3611b5
@@ -0,0 +1,756 @@

drivers/clk/clk-npcm8xx.c new file +9 -0

+
+#define PLLCON_LOKI    BIT(31)
+#define PLLCON_LOKS    BIT(30)
+#define PLLCON_FBDV    GENMASK(27, 16)
+#define PLLCON_OTDV2   GENMASK(15, 13)
+#define PLLCON_PWDEN   BIT(12)
+#define PLLCON_OTDV1   GENMASK(10, 8)
+#define PLLCON_INDV    GENMASK(5, 0)
+

drivers/clk/clk-npcm8xx.c new file +6 -0

+               return 0;
+       }
+
+       val = readl_relaxed(pll->pllcon);
+
+       indv = FIELD_GET(PLLCON_INDV, val);

Quoting Tomer Maimon (2022-06-08 02:56:10)

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +0 -0

new file mode 100644
index 000000000000..40340c3611b5
@@ -0,0 +1,756 @@

[...]

Quoted 9 lines

drivers/clk/clk-npcm8xx.c new file +9 -0

+
+#define PLLCON_LOKI    BIT(31)
+#define PLLCON_LOKS    BIT(30)
+#define PLLCON_FBDV    GENMASK(27, 16)
+#define PLLCON_OTDV2   GENMASK(15, 13)
+#define PLLCON_PWDEN   BIT(12)
+#define PLLCON_OTDV1   GENMASK(10, 8)
+#define PLLCON_INDV    GENMASK(5, 0)
+

drivers/clk/clk-npcm8xx.c line 48

Quoted 10 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+static unsigned long npcm8xx_clk_pll_recalc_rate(struct clk_hw *hw,
+                                                unsigned long parent_rate)
+{
+       struct npcm8xx_clk_pll *pll = to_npcm8xx_clk_pll(hw);
+       unsigned long fbdv, indv, otdv1, otdv2;
+       unsigned int val;
+       u64 ret;
+
+       if (parent_rate == 0) {
+               pr_debug("%s: parent rate is zero", __func__);

Missing newline.

drivers/clk/clk-npcm8xx.c line 54

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+               return 0;
+       }
+
+       val = readl_relaxed(pll->pllcon);
+
+       indv = FIELD_GET(PLLCON_INDV, val);

Modify nuvoton,npcm750-reset specific name to
general NPCM file name, nuvoton,npcm-reset.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml}     | 2 +-

Quoted 6 lines · Tomer Maimon

Modify nuvoton,npcm750-reset specific name to
general NPCM file name, nuvoton,npcm-reset.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml}     | 2 +-

No. Name from the first compatible should be used.

Best regards,
Krzysztof

Describe syscon property that handles general
control registers(GCR) in Nuvoton BMC NPCM
reset driver.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../devicetree/bindings/reset/nuvoton,npcm-reset.yaml       | 6 ++++++
 1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml +0 -0

index 0998f481578d..c6bbc1589ab9 100644

Describe syscon property that handles general
control registers(GCR) in Nuvoton BMC NPCM
reset driver.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../devicetree/bindings/reset/nuvoton,npcm-reset.yaml       | 6 ++++++
 1 file changed, 6 insertions(+)

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Quoted 2 lines · Tomer Maimon

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Wrong wrapping.

Best regards,
Krzysztof

Hi Krzysztof,

Thanks for your comments

Quoted 1 line · Krzysztof Kozlowski

Quoted 5 lines · Tomer Maimon

Quoted 2 lines

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Wrong wrapping.

it will be very helpful if you could point me what wrong wrapped in the commit message, is it the explanation or the header? or something else?

Quoted 3 lines

Best regards,
Krzysztof

Best regards,

Tomer

Hi Tomer,

Quoted 8 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your comments

Quoted 1 line · Krzysztof Kozlowski

Quoted 8 lines

Quoted 5 lines · Tomer Maimon

Quoted 2 lines

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Wrong wrapping.

it will be very helpful if you could point me what wrong wrapped in the commit message, is it the explanation or the header? or something else?

The commit message body should be wrapped at 72 chars. You can fit more on the first line if you reflow:

Add nuvoton,sysgcr syscon property to the reset node to handle the general control registers.

Quoted 7 lines

Quoted 3 lines

Best regards,
Krzysztof

Best regards,

Tomer

Hi Krzysztof,

Thanks for your comments

Quoted 1 line · Krzysztof Kozlowski

Quoted 5 lines · Tomer Maimon

Quoted 2 lines

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Wrong wrapping.

it will be very helpful if you could point me what wrong wrapped in the commit message, is it the explanation or the header? or something else?

Quoted 7 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your comments

Quoted 1 line · Krzysztof Kozlowski

Quoted 8 lines

Quoted 5 lines · Tomer Maimon

Quoted 2 lines

Add nuvoton,sysgcr syscon property to the reset
node to handle the general control registers.

Wrong wrapping.

it will be very helpful if you could point me what wrong wrapped in the commit message, is it the explanation or the header? or something else?

I pointed you last time. I pointed the exact line, exact rule you need to follow. I pointed it three times already and three times I said wrapping is wrong:
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

"The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch."

Your wrapping is not at 75 columns and it causes the commit to be less readable, without any reason. Please follow Linux kernel coding style/rules.

Best regards,
Krzysztof

Using syscon device tree property instead of
device data to handle the NPCM general control
registers.

Signed-off-by Tomer Maimon <[email protected]>

---
 drivers/reset/reset-npcm.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

drivers/reset/reset-npcm.c +1 -3

index 2ea4d3136e15..312c3b594b8f 100644
@@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev,
 }
 
 static const struct of_device_id npcm_rc_match[] = {
-	{ .compatible = "nuvoton,npcm750-reset",
-		.data = (void *)"nuvoton,npcm750-gcr" },
+	{ .compatible = "nuvoton,npcm750-reset"},
 	{ }
 };
 

@@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc)
 	u32 ipsrst1_bits = 0;
 	u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST;
 	u32 ipsrst3_bits = 0;
-	const char *gcr_dt;

Quoted 4 lines · Tomer Maimon

Using syscon device tree property instead of
device data to handle the NPCM general control
registers.

Again ignored the comment.

Quoted 24 lines

Signed-off-by Tomer Maimon <[email protected]>

---
 drivers/reset/reset-npcm.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

drivers/reset/reset-npcm.c +1 -3

index 2ea4d3136e15..312c3b594b8f 100644
@@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev,
 }
 
 static const struct of_device_id npcm_rc_match[] = {
-     { .compatible = "nuvoton,npcm750-reset",
-             .data = (void *)"nuvoton,npcm750-gcr" },
+     { .compatible = "nuvoton,npcm750-reset"},
      { }
 };
 

@@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc)
      u32 ipsrst1_bits = 0;
      u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST;
      u32 ipsrst3_bits = 0;
-     const char *gcr_dt;

drivers/reset/reset-npcm.c static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) · line 161

Quoted 10 lines

drivers/reset/reset-npcm.c +2 -5

 
-     gcr_dt = (const char *)
-     of_match_device(dev->driver->of_match_table, dev)->data;
-
-     gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt);
+     gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr");
      if (IS_ERR(gcr_regmap)) {
-             dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt);
+             dev_err(&pdev->dev, "Failed to find gcr syscon");
              return PTR_ERR(gcr_regmap);

Comment still ignored.

There is no point in this review if you keep ignoring what we ask to fix.

If something is unclear, ask for clarification. Resending without implementing the comment means that you ignore the review which is waste of my time.

I am sorry, but this is not acceptable.

Best regards,
Krzysztof

Hi Krzysztof

Sorry but I didn't ignore your comment.

For not breaking exciting boards I add the following patch in V2
https://lore.kernel.org/linux-arm-kernel/[email protected]/

Quoted 6 lines · Tomer Maimon

Hi Krzysztof

Sorry but I didn't ignore your comment.

For not breaking exciting boards I add the following patch in V2
https://lore.kernel.org/linux-arm-kernel/[email protected]/

No, it does not solve it.
1. Patchset goes via separate trees (DTS are always separate), so it is not bisectable. One of the branches/trees will have broken DTS.

2. All out of tree DTSes are broken. This is expressed as ABI and - with some reasonable exceptions - you should not break it.
https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/ABI.rst

You have to keep backwards compatibility, so parse/handle both versions of DTS.

Best regards,
Krzysztof

Add binding document and device tree binding constants for Nuvoton BMC NPCM8XX reset controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/reset/nuvoton,npcm-reset.yaml    |  13 +-
 .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 ++++++++++++++++++
 2 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h

Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml +12 -1

index c6bbc1589ab9..93ea81686f58 100644
@@ -9,9 +9,20 @@ title: Nuvoton NPCM Reset controller
 maintainers:
   - Tomer Maimon <[email protected]>
 
+description: |
+  The NPCM reset controller used to reset various set of peripherals. 
+  Please refer to reset.txt in this directory for common reset
+  controller binding usage.
+
+  For list of all valid reset indices see
+    <dt-bindings/reset/nuvoton,npcm7xx-reset.h> for Poleg NPCM7XX SoC,
+    <dt-bindings/reset/nuvoton,npcm8xx-reset.h> for Arbel NPCM8XX SoC.
+
 properties:
   compatible:
-    const: nuvoton,npcm750-reset
+    enum: 
+      - nuvoton,npcm750-reset        # Poleg NPCM7XX SoC
+      - nuvoton,npcm845-reset        # Arbel NPCM8XX SoC
 
   reg:
     maxItems: 1

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +0 -0

new file mode 100644
index 000000000000..5b3b74534b50

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +5 -0

+/*
+ * Copyright (c) 2022 Nuvoton Technology corporation.
+ * Author: Tomer Maimon <[email protected]>
+ */
+

Quoted 42 lines · Tomer Maimon

Add binding document and device tree binding constants for Nuvoton BMC NPCM8XX reset controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/reset/nuvoton,npcm-reset.yaml    |  13 +-
 .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 ++++++++++++++++++
 2 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h

Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml +12 -1

index c6bbc1589ab9..93ea81686f58 100644
@@ -9,9 +9,20 @@ title: Nuvoton NPCM Reset controller
 maintainers:
   - Tomer Maimon <[email protected]>
 
+description: |
+  The NPCM reset controller used to reset various set of peripherals.
+  Please refer to reset.txt in this directory for common reset
+  controller binding usage.
+
+  For list of all valid reset indices see
+    <dt-bindings/reset/nuvoton,npcm7xx-reset.h> for Poleg NPCM7XX SoC,
+    <dt-bindings/reset/nuvoton,npcm8xx-reset.h> for Arbel NPCM8XX SoC.
+
 properties:
   compatible:
-    const: nuvoton,npcm750-reset
+    enum:
+      - nuvoton,npcm750-reset        # Poleg NPCM7XX SoC
+      - nuvoton,npcm845-reset        # Arbel NPCM8XX SoC
 
   reg:
     maxItems: 1

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +0 -0

new file mode 100644
index 000000000000..5b3b74534b50

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h line 1

Quoted 1 line

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +1 -0

@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0 */

Again - ignored comment from v1.

Quoted 5 lines

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +5 -0

+/*
+ * Copyright (c) 2022 Nuvoton Technology corporation.
+ * Author: Tomer Maimon <[email protected]>
+ */
+

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h line 16

Quoted 10 lines

include/dt-bindings/reset/nuvoton,npcm8xx-reset.h new file +10 -0

+#ifndef _DT_BINDINGS_NPCM8XX_RESET_H
+#define _DT_BINDINGS_NPCM8XX_RESET_H
+
+/* represent reset register offset */
+#define NPCM8XX_RESET_IPSRST1                0x20
+#define NPCM8XX_RESET_IPSRST2                0x24
+#define NPCM8XX_RESET_IPSRST3                0x34
+#define NPCM8XX_RESET_IPSRST4                0x74
+
+/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */

Again - ignored comment from v1. My last message was quite clear, wasn't it?

https://lore.kernel.org/all/[email protected]/

You ignored several of previous comments, so:

NAK.

Best regards,
Krzysztof

Hi Krzysztof,

Sorry, but I thought the fix is only to add an explanation to the dt-binding file as was done in V2.

The NPCM8XX binding is done in the same way as the NPCM7XX and both use the same reset driver and use the same reset method in upstreamed NPCM reset driver.

Can you please explain again what you suggest to do?

Quoted 10 lines · Tomer Maimon

Hi Krzysztof,

Sorry, but I thought the fix is only to add an explanation to the dt-binding file as was done in V2.

The NPCM8XX binding is done in the same way as the NPCM7XX and both use the same reset driver and use the same reset method in upstreamed NPCM reset driver.

Can you please explain again what you suggest to do?

If you want abstract IDs, they must be abstract, so not representing hardware registers. Then they start at 1 and are incremented by 1.

Other option is to skip such IDs entirely and use register offsets/addresses directly, like Arnd suggested in linked documents. I think he expressed it clearly, so please read his answers which I linked in previous discussion.

There is no single reason to store register addresses/values/offsets as binding headers. These are not bindings.

Best regards,
Krzysztof

Hi Krzysztof,

Thanks for your clarification.

We can remove the dt-binding file and use numbers in the DTS, appreciate if you can answer few additional questions:
1. Do you suggest adding all NPCM reset values to the NPCM reset document or the reset values should describe in the module documentation that uses it?

2. Some of the NPCM7XX document modules describe the reset value they use from the dt-binding for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61

If we remove the NPCM8XX dt-binding file should we describe the NPCM8XX values in the NPCM-ADC document file?

Quoted 4 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your clarification.

Quoted 5 lines

We can remove the dt-binding file and use numbers in the DTS, appreciate if you can answer few additional questions:
1. Do you suggest adding all NPCM reset values to the NPCM reset document or the reset values should describe in the module documentation that uses it?

What is "NPCM reset document"? Are these reset values anyhow different than interrupts or pins?

Quoted 4 lines

2. Some of the NPCM7XX document modules describe the reset value they use from the dt-binding for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61

This is NPCM750

Quoted 2 lines

If we remove the NPCM8XX dt-binding file should we describe the NPCM8XX values in the NPCM-ADC document file?

What is NPCM-ADC document file? What do you want to describe there? Again - how is it different than interrupts?

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 12 lines · Krzysztof Kozlowski

Quoted 9 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your clarification.

We can remove the dt-binding file and use numbers in the DTS, appreciate if you can answer few additional questions:
1. Do you suggest adding all NPCM reset values to the NPCM reset document or the reset values should describe in the module documentation that uses it?

Quoted 2 lines

What is "NPCM reset document"? Are these reset values anyhow different than interrupts or pins?

No, they represent the same values.

Quoted 7 lines

Quoted 4 lines

2. Some of the NPCM7XX document modules describe the reset value they use from the dt-binding for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61

This is NPCM750

Quoted 5 lines

Quoted 2 lines

If we remove the NPCM8XX dt-binding file should we describe the NPCM8XX values in the NPCM-ADC document file?

What is NPCM-ADC document file? What do you want to describe there? Again - how is it different than interrupts?

It is not different from the interrupts.
I will remove the dt-binding reset include file, the reset property will use numbers and not macro's.

Quoted 16 lines · Tomer Maimon

Hi Krzysztof,

Quoted 12 lines · Krzysztof Kozlowski

Quoted 9 lines · Tomer Maimon

Hi Krzysztof,

Thanks for your clarification.

We can remove the dt-binding file and use numbers in the DTS, appreciate if you can answer few additional questions:
1. Do you suggest adding all NPCM reset values to the NPCM reset document or the reset values should describe in the module documentation that uses it?

Quoted 3 lines

Quoted 2 lines

What is "NPCM reset document"? Are these reset values anyhow different than interrupts or pins?

No, they represent the same values.

We do not document in the bindings actual pin or interrupt numbers...

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

2. Some of the NPCM7XX document modules describe the reset value they use from the dt-binding for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61

This is NPCM750

Quoted 2 lines

If we remove the NPCM8XX dt-binding file should we describe the NPCM8XX values in the NPCM-ADC document file?

Quoted 5 lines

Quoted 2 lines

What is NPCM-ADC document file? What do you want to describe there? Again - how is it different than interrupts?

It is not different from the interrupts.
I will remove the dt-binding reset include file, the reset property will use numbers and not macro's.

I have no clue what are you referring now... This is NPCM8xx and it has no binding header with reset values. What to remove then?

Best regards,
Krzysztof

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

What is NPCM-ADC document file? What do you want to describe there? Again - how is it different than interrupts?

It is not different from the interrupts.
I will remove the dt-binding reset include file, the reset property will use numbers and not macro's.

I have no clue what are you referring now... This is NPCM8xx and it has no binding header with reset values. What to remove then?

I refer nuvoton,npcm8xx-reset.h file, we don't need it.

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

arch/arm64/boot/dts/Makefile +1 -0

index 1ba04e31a438..7b107fa7414b 100644
@@ -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

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +7 -0

new file mode 100644
index 000000000000..97e108c50760
@@ -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>
+

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +9 -0

+		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";

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +19 -0

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

Quoted 44 lines · Tomer Maimon

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

arch/arm64/boot/dts/Makefile +1 -0

index 1ba04e31a438..7b107fa7414b 100644
@@ -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

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +11 -0

new file mode 100644
index 000000000000..97e108c50760
@@ -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>;

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 17

Quoted 6 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +6 -0

+
+     /* external reference clock */
+     clk_refclk: clk-refclk {
+             compatible = "fixed-clock";
+             #clock-cells = <0>;
+             clock-frequency = <25000000>;

Ignored comment.

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 25

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

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

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 33

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

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

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 44

Quoted 11 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +11 -0

+             clock-output-names = "mcbypck";
+     };
+
+     soc {
+             #address-cells = <2>;
+             #size-cells = <2>;
+             compatible = "simple-bus";
+             interrupt-parent = <&gic>;
+             ranges;
+
+             gcr: gcr@f0800000 {

Ignored comment.

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 46

Quoted 2 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +2 -0

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

Quoted 28 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +26 -0

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

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 75

Quoted 3 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +3 -0

+             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

Quoted 23 lines · Krzysztof Kozlowski

Quoted 22 lines · Tomer Maimon

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/nuvoton/nuvoton-common-npcm8xx.dtsi new file +11 -0

@@ -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>;

Quoted 8 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 17

Quoted 6 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +6 -0

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

Quoted 11 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 25

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

+             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

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 33

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

+             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

Quoted 2 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 35

Quoted 2 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +2 -0

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

Quoted 9 lines

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

Quoted 6 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 17

Quoted 4 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +4 -0

+     clk_refclk: clk-refclk {
+             compatible = "fixed-clock";
+             #clock-cells = <0>;
+             clock-frequency = <25000000>;

Ignored comment.

Could we use it as a default clock-frequency?

Quoted 11 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 25

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

+             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

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 33

Quoted 9 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +8 -0

+             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

Quoted 11 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 44

Quoted 11 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +11 -0

+             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

Quoted 9 lines

Ignored comment.

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 46

Quoted 2 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +2 -0

+                     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

Quoted 28 lines

Quoted 28 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +26 -0

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

Quoted 6 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi line 75

Quoted 3 lines

arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi new file +3 -0

+             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