[PATCH v22 0/8] Introduce Nuvoton Arbel NPCM8XX BMC SoC

[PATCH v22 0/8] Introduce Nuvoton Arbel NPCM8XX BMC SoC

[Tomer Maimon]

This patchset adds clock support for the Nuvoton 
Arbel NPCM8XX Board Management controller (BMC) SoC family.

This patchset cover letter is based from the initial support for NPCM8xx BMC to
keep tracking the version history.

all the other initial support patches had been applied to Linux kernel 6.0.

This patchset was tested on the Arbel NPCM8XX evaluation board.

Addressed comments from:
 - Stephen Boyd: https://www.spinics.net/lists/linux-clk/msg93838.html

Changes since version 21:
 - Since using regmap instead of ioremap replace reg to syscon 
   property in dt-bindings and dts.
 - Add reference clock property to the dt-bindings and dts.
 - Using .index instead of .name in clk_parent_data structures.
 - Using string where any macros are used once.

Changes since version 20:
 - Using regmap instead of ioremap.
   the clock and reset modules are sharing the same memory region 
   and cause failure when using devm_platform_ioremap_resource
   function, this version uses regmap to handle shared 
   reset and clock memory region, in case it is approved I will
   modify the reset driver to use the regmap as well.
 - Using clk_hw instead of clk_parent_data structre.
 - Divider clock definition to one line

Changes since version 19:
 - Remove unnecessary free command.
 - Defining pr_fmt().
 - Using dev_err_probe.
 - Return zero in the end of the probe function.

Changes since version 18:
 - NPCM8XX clock driver did not changed from version 18 only build and tested under kernel 6.6-rc1.

Changes since version 17:
 - NPCM8XX clock driver did not changed from version 17 only build and tested under kernel 6.5-rc3.

Changes since version 16:
 - NPCM8XX clock driver
	- Using devm_kzalloc instead kzalloc.
	- Remove unnecessary parenthesis.
	- Modify incorrect spelling.

Changes since version 15:
 - NPCM8XX clock driver
	- Remove unused regs parameter from npcm8xx_pll_data structure.
	- Using index and clk_hw parameters to set the clock parent in the clock structures.

Changes since version 14:
 - NPCM8XX clock driver
	- Remove unnecessary register definitions.
	- Remove the internal reference clock, instead use the external DT reference clock.
	- rearrange the driver.
	- using .names parameter in DT to define clock (refclk).

Changes since version 13:
 - NPCM8XX clock driver
	- Remove unnecessary definitions and add module.h define
	- Use in clk_parent_data struct.fw_name and .name.
	- Add module_exit function.
	- Add const to divider clock names.
	- Add MODULE_DESCRIPTION and MODULE_LICENSE

Changes since version 12:
 - NPCM8XX clock driver
	- Use clk_parent_data in mux and div clock structure.
	- Add const to mux tables.
	- Using devm_clk_hw_register_fixed_rate function.
	- use only .name clk_parent_data instead .name and .fw_name.
	- Modify mask values in mux clocks. 

Changes since version 11:
 - NPCM8XX clock driver
	- Modify Kconfig help.
	- Modify loop variable to unsigned int.

Changes since version 11:
 - NPCM8XX clock driver
	- Modify Kconfig help.
	- Modify loop variable to unsigned int.

Changes since version 10:
 - NPCM8XX clock driver
	- Fix const warning.

Changes since version 9:
 - NPCM8XX clock driver
	- Move configuration place.
	- Using clk_parent_data instead of parent_name
	- using devm_ioremap instead of ioremap. deeply sorry, I know we had
	 a long discussion on what should the driver use, from other examples 
	 (also in other clock drivers) I see the combination of 
	 platform_get_resource and devm_ioremap are commonly used and it answer
	 the reset and clock needs.

Changes since version 8:
 - NPCM8XX clock driver
	- Move configuration place.
	- Add space before and aftre '{' '}'.
	- Handle devm_of_clk_add_hw_provider function error.

Changes since version 7:
 - NPCM8XX clock driver
	- The clock and reset registers using the same memory region, 
	  due to it the clock driver should claim the ioremap directly 
	  without checking the memory region.

Changes since version 5:
 - NPCM8XX clock driver
	- Remove refclk if devm_of_clk_add_hw_provider function failed.

Changes since version 4:
 - NPCM8XX clock driver
	- Use the same quote in the dt-binding file.

Changes since version 3:
 - NPCM8XX clock driver
	- Rename NPCM8xx clock dt-binding header file.
	- Remove unused structures.
	- Improve Handling the clocks registration.

Changes since version 2:
 - NPCM8XX clock driver
	- Add debug new line.
	- Add 25M fixed rate clock.
	- Remove unused clocks and clock name from dt-binding.

Changes since version 1:
 - NPCM8XX clock driver
	- Modify dt-binding.
	- Remove unsed definition and include.
	- Include alphabetically.
	- Use clock devm.


Tomer Maimon (8):
  dt-bindings: clock: npcm845: Add reference 25m clock  property
  arm64: dts: nuvoton: npcm8xx: add refernace clock
  arm: dts: nuvoton: npcm7xx: modify rst syscon node
  dt-bindings: soc: nuvoton: add binding for clock and reset registers
  arm64: dts: nuvoton: npcm8xx: add clock reset syscon node
  dt-bindings: clock: npcm845: replace reg with syscon property
  arm64: dts: nuvoton: npcm8xx: replace reg with syscon property
  clk: npcm8xx: add clock controller

 .../bindings/clock/nuvoton,npcm845-clk.yaml   |  38 +-
 .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     |  40 ++
 .../dts/nuvoton/nuvoton-common-npcm7xx.dtsi   |   4 +-
 .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi   |  19 +-
 .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts  |   6 +
 drivers/clk/clk-npcm8xx.c                     | 510 ++++++++++++++++++
 6 files changed, 597 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
 create mode 100644 drivers/clk/clk-npcm8xx.c

-- 
2.34.1

[PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Tomer Maimon]

The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
refclk property.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
index b901ca13cd25..0b642bfce292 100644
--- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
+++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
@@ -21,6 +21,14 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    items:
+      - description: 25Mhz referance clock
+
+  clock-names:
+    items:
+      - const: refclk
+
   '#clock-cells':
     const: 1
     description:
@@ -30,12 +38,20 @@ properties:
 required:
   - compatible
   - reg
+  - clocks
+  - clock-names
   - '#clock-cells'
 
 additionalProperties: false
 
 examples:
   - |
+    refclk: refclk-25mhz {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-frequency = <25000000>;
+    }; 
+  
     ahb {
         #address-cells = <2>;
         #size-cells = <2>;
-- 
2.34.1

[PATCH v22 2/8] arm64: dts: nuvoton: npcm8xx: add refernace clock

[Tomer Maimon]

Add 25Mhz reference clock since the reference clock in not a part of
Nuvoton BMC NPCM8XX SoC.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 10 ++++++----
 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts    |  6 ++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
index ecd171b2feba..9c4df91031e7 100644
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
@@ -58,6 +58,8 @@ clk: clock-controller@f0801000 {
 			compatible = "nuvoton,npcm845-clk";
 			#clock-cells = <1>;
 			reg = <0x0 0xf0801000 0x0 0x1000>;
+			clocks = <&refclk>;
+			clock-names = "refclk";
 		};
 
 		apb {
@@ -81,7 +83,7 @@ timer0: timer@8000 {
 				compatible = "nuvoton,npcm845-timer";
 				interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <0x8000 0x1C>;
-				clocks = <&clk NPCM8XX_CLK_REFCLK>;
+				clocks = <&refclk>;
 				clock-names = "refclk";
 			};
 
@@ -153,7 +155,7 @@ watchdog0: watchdog@801c {
 				interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <0x801c 0x4>;
 				status = "disabled";
-				clocks = <&clk NPCM8XX_CLK_REFCLK>;
+				clocks = <&refclk>;
 				syscon = <&gcr>;
 			};
 
@@ -162,7 +164,7 @@ watchdog1: watchdog@901c {
 				interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <0x901c 0x4>;
 				status = "disabled";
-				clocks = <&clk NPCM8XX_CLK_REFCLK>;
+				clocks = <&refclk>;
 				syscon = <&gcr>;
 			};
 
@@ -171,7 +173,7 @@ watchdog2: watchdog@a01c {
 				interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <0xa01c 0x4>;
 				status = "disabled";
-				clocks = <&clk NPCM8XX_CLK_REFCLK>;
+				clocks = <&refclk>;
 				syscon = <&gcr>;
 			};
 		};
diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
index a5ab2bc0f835..722a46d78d23 100644
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
@@ -19,6 +19,12 @@ chosen {
 	memory {
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	refclk: refclk-25mhz {
+		compatible = "fixed-clock";
+		clock-frequency = <25000000>;
+		#clock-cells = <0>;
+	};
 };
 
 &serial0 {
-- 
2.34.1

[PATCH v22 3/8] arm: dts: nuvoton: npcm7xx: modify rst syscon node

[Tomer Maimon]

rst node name and compatible property modified since clock and reset are
handled in the same memory region.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
index 868454ae6bde..f72c5a03d04c 100644
--- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
+++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
@@ -93,8 +93,8 @@ gcr: gcr@800000 {
 			reg = <0x800000 0x1000>;
 		};
 
-		rst: rst@801000 {
-			compatible = "nuvoton,npcm750-rst", "syscon", "simple-mfd";
+		clk_rst: syscon@801000 {
+			compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
 			reg = <0x801000 0x6C>;
 		};
 	};
-- 
2.34.1

[PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Tomer Maimon]

A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
retrieve SoC model and version information.

This patch adds a binding to describe this node.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml

diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
new file mode 100644
index 000000000000..dfec64a8eb26
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clock and reset registers block in Nuvoton SoCs
+
+maintainers:
+  - Tomer Maimon <tmaimon77@gmail.com>
+
+description:
+  The clock and reset registers are a registers block in Nuvoton SoCs that 
+  handle both reset and clock functionality.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - nuvoton,npcm750-clk-rst
+          - nuvoton,npcm845-clk-rst
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties:
+  type: object
+
+examples:
+  - |
+    clk_rst: syscon@801000 {
+      compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
+      reg = <0x801000 0x6C>;
+    };
-- 
2.34.1

[PATCH v22 5/8] arm64: dts: nuvoton: npcm8xx: add clock reset syscon node

[Tomer Maimon]

Add clock reset syscon node to handle reset and clock registers
controllers.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
index 9c4df91031e7..7d5956e2c9f3 100644
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
@@ -22,6 +22,11 @@ gcr: system-controller@f0800000 {
 			reg = <0x0 0xf0800000 0x0 0x1000>;
 		};
 
+		clk_rst: syscon@f0801000 {
+			compatible = "nuvoton,npcm845-clk-rst", "syscon", "simple-mfd";
+			reg = <0x0 0xf0801000 0x0 0xC4>;
+		};
+
 		gic: interrupt-controller@dfff9000 {
 			compatible = "arm,gic-400";
 			reg = <0x0 0xdfff9000 0x0 0x1000>,
-- 
2.34.1

[PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Tomer Maimon]

Replace reg with syscon property since the clock registers handle the
reset registers as well.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
index 0b642bfce292..c6bf05c163b4 100644
--- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
+++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
@@ -18,8 +18,9 @@ properties:
     enum:
       - nuvoton,npcm845-clk
 
-  reg:
-    maxItems: 1
+  nuvoton,sysclk:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to access clock registers.
 
   clocks:
     items:
@@ -37,7 +38,7 @@ properties:
 
 required:
   - compatible
-  - reg
+  - nuvoton,sysclk
   - clocks
   - clock-names
   - '#clock-cells'
@@ -52,14 +53,11 @@ examples:
         clock-frequency = <25000000>;
     }; 
   
-    ahb {
-        #address-cells = <2>;
-        #size-cells = <2>;
-
-        clock-controller@f0801000 {
-            compatible = "nuvoton,npcm845-clk";
-            reg = <0x0 0xf0801000 0x0 0x1000>;
-            #clock-cells = <1>;
-        };
+    clk: clock-controller {
+        compatible = "nuvoton,npcm845-clk";
+        nuvoton,sysclk = <&clk_rst>;
+        #clock-cells = <1>;
+        clocks = <&refclk>;
+        clock-names = "refclk";
     };
 ...
-- 
2.34.1

[PATCH v22 7/8] arm64: dts: nuvoton: npcm8xx: replace reg with syscon property

[Tomer Maimon]

Replace reg with syscon property since the clock registers handle the
reset registers as well.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
index 7d5956e2c9f3..5cc0efdbb3c7 100644
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
@@ -59,10 +59,10 @@ rstc: reset-controller@f0801000 {
 			nuvoton,sysgcr = <&gcr>;
 		};
 
-		clk: clock-controller@f0801000 {
+		clk: clock-controller {
 			compatible = "nuvoton,npcm845-clk";
 			#clock-cells = <1>;
-			reg = <0x0 0xf0801000 0x0 0x1000>;
+			nuvoton,sysclk = <&clk_rst>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 		};
-- 
2.34.1

[PATCH v22 8/8] clk: npcm8xx: add clock controller

[Tomer Maimon]

Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller which
generates and supplies clocks to all modules within the BMC.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
Acked-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-npcm8xx.c | 510 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 510 insertions(+)
 create mode 100644 drivers/clk/clk-npcm8xx.c

diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
new file mode 100644
index 000000000000..35123a460acd
--- /dev/null
+++ b/drivers/clk/clk-npcm8xx.c
@@ -0,0 +1,510 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NPCM8xx Clock Generator
+ * All the clocks are initialized by the bootloader, so this driver allows only
+ * reading of current settings directly from the hardware.
+ *
+ * Copyright (C) 2020 Nuvoton Technologies
+ * Author: Tomer Maimon <tomer.maimon@nuvoton.com>
+ */
+
+#define pr_fmt(fmt) "npcm8xx_clk: " fmt
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#include <dt-bindings/clock/nuvoton,npcm845-clk.h>
+
+/* npcm8xx clock registers*/
+#define NPCM8XX_CLKSEL		0x04
+#define NPCM8XX_CLKDIV1		0x08
+#define NPCM8XX_CLKDIV2		0x2C
+#define NPCM8XX_CLKDIV3		0x58
+#define NPCM8XX_CLKDIV4		0x7C
+#define NPCM8XX_PLLCON0		0x0C
+#define NPCM8XX_PLLCON1		0x10
+#define NPCM8XX_PLLCON2		0x54
+#define NPCM8XX_PLLCONG		0x60
+#define NPCM8XX_THRTL_CNT	0xC0
+
+#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)
+
+struct npcm8xx_clk {
+	struct regmap	*clk_regmap;
+	unsigned int	offset;
+	const char	*name;
+	const u32	*table;
+	u32		mask;
+	u8		shift;
+	unsigned long	width;
+	unsigned long	flags;
+	struct clk_hw	hw;
+};
+
+#define to_npcm8xx_clk(_hw) container_of(_hw, struct npcm8xx_clk, hw)
+
+struct npcm8xx_clk_pll_data {
+	const char *name;
+	struct clk_parent_data parent;
+	unsigned int reg;
+	unsigned long flags;
+	struct clk_hw hw;
+};
+
+struct npcm8xx_clk_div_data {
+	u32 reg;
+	u8 shift;
+	u8 width;
+	const char *name;
+	const struct clk_hw *parent_hw;
+	unsigned long clk_divider_flags;
+	unsigned long flags;
+	int onecell_idx;
+	struct clk_hw hw;
+};
+
+struct npcm8xx_clk_mux_data {
+	u8 shift;
+	u32 mask;
+	const u32 *table;
+	const char *name;
+	const struct clk_parent_data *parent_data;
+	u8 num_parents;
+	unsigned long flags;
+	struct clk_hw hw;
+};
+
+/* external clock definition */
+#define NPCM8XX_CLK_S_REFCLK	"refclk"
+
+/* pll definition */
+#define NPCM8XX_CLK_S_PLL0	"pll0"
+#define NPCM8XX_CLK_S_PLL1	"pll1"
+#define NPCM8XX_CLK_S_PLL2	"pll2"
+#define NPCM8XX_CLK_S_PLL_GFX	"pll_gfx"
+
+/* early divider definition */
+#define NPCM8XX_CLK_S_PLL2_DIV2		"pll2_div2"
+#define NPCM8XX_CLK_S_PLL_GFX_DIV2	"pll_gfx_div2"
+#define NPCM8XX_CLK_S_PLL1_DIV2		"pll1_div2"
+
+/* mux definition */
+#define NPCM8XX_CLK_S_CPU_MUX     "cpu_mux"
+
+/* div definition */
+#define NPCM8XX_CLK_S_TH          "th"
+#define NPCM8XX_CLK_S_AXI         "axi"
+
+static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;
+static struct npcm8xx_clk_pll_data npcm8xx_pll_clks[] = {
+	{ NPCM8XX_CLK_S_PLL0, { .index = 0 }, NPCM8XX_PLLCON0, 0 },
+	{ NPCM8XX_CLK_S_PLL1, { .index = 0 }, NPCM8XX_PLLCON1, 0 },
+	{ NPCM8XX_CLK_S_PLL2, { .index = 0 }, NPCM8XX_PLLCON2, 0 },
+	{ NPCM8XX_CLK_S_PLL_GFX, { .index = 0 }, NPCM8XX_PLLCONG, 0 },
+};
+
+static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
+static const struct clk_parent_data cpuck_mux_parents[] = {
+	{ .hw = &npcm8xx_pll_clks[0].hw },
+	{ .hw = &npcm8xx_pll_clks[1].hw },
+	{ .index = 0 },
+	{ .hw = &npcm8xx_pll_clks[2].hw }
+};
+
+static const u32 pixcksel_mux_table[] = { 0, 2 };
+static const struct clk_parent_data pixcksel_mux_parents[] = {
+	{ .hw = &npcm8xx_pll_clks[3].hw },
+	{ .index = 0 }
+};
+
+static const u32 default_mux_table[] = { 0, 1, 2, 3 };
+static const struct clk_parent_data default_mux_parents[] = {
+	{ .hw = &npcm8xx_pll_clks[0].hw },
+	{ .hw = &npcm8xx_pll_clks[1].hw },
+	{ .index = 0 },
+	{ .hw = &hw_pll2_div2 }
+};
+
+static const u32 sucksel_mux_table[] = { 2, 3 };
+static const struct clk_parent_data sucksel_mux_parents[] = {
+	{ .index = 0 },
+	{ .hw = &hw_pll2_div2 }
+};
+
+static const u32 mccksel_mux_table[] = { 0, 2 };
+static const struct clk_parent_data mccksel_mux_parents[] = {
+	{ .hw = &hw_pll1_div2 },
+	{ .index = 0 }
+};
+
+static const u32 clkoutsel_mux_table[] = { 0, 1, 2, 3, 4 };
+static const struct clk_parent_data clkoutsel_mux_parents[] = {
+	{ .hw = &npcm8xx_pll_clks[0].hw },
+	{ .hw = &npcm8xx_pll_clks[1].hw },
+	{ .index = 0 },
+	{ .hw = &hw_gfx_div2 },
+	{ .hw = &hw_pll2_div2 }
+};
+
+static const u32 gfxmsel_mux_table[] = { 2, 3 };
+static const struct clk_parent_data gfxmsel_mux_parents[] = {
+	{ .index = 0 },
+	{ .hw = &npcm8xx_pll_clks[2].hw }
+};
+
+static const u32 dvcssel_mux_table[] = { 2, 3 };
+static const struct clk_parent_data dvcssel_mux_parents[] = {
+	{ .index = 0 },
+	{ .hw = &npcm8xx_pll_clks[2].hw }
+};
+
+static const u32 default3_mux_table[] = { 0, 1, 2 };
+static const struct clk_parent_data default3_mux_parents[] = {
+	{ .hw = &npcm8xx_pll_clks[0].hw },
+	{ .hw = &npcm8xx_pll_clks[1].hw },
+	{ .index = 0 }
+};
+
+static struct npcm8xx_clk_mux_data npcm8xx_muxes[] = {
+	{ 0, 7, cpuck_mux_table, NPCM8XX_CLK_S_CPU_MUX, cpuck_mux_parents,
+		ARRAY_SIZE(cpuck_mux_parents), CLK_IS_CRITICAL },
+	{ 4, 3, pixcksel_mux_table, "gfx_pixel_mux", pixcksel_mux_parents,
+		ARRAY_SIZE(pixcksel_mux_parents), 0 },
+	{ 6, 3, default_mux_table, "sd_mux", default_mux_parents,
+		ARRAY_SIZE(default_mux_parents), 0 },
+	{ 8, 3, default_mux_table, "uart_mux", default_mux_parents,
+		ARRAY_SIZE(default_mux_parents), 0 },
+	{ 10, 3, sucksel_mux_table, "serial_usb_mux", sucksel_mux_parents,
+		ARRAY_SIZE(sucksel_mux_parents), 0 },
+	{ 12, 3, mccksel_mux_table, "mc_mux", mccksel_mux_parents,
+		ARRAY_SIZE(mccksel_mux_parents), 0 },
+	{ 14, 3, default_mux_table, "adc_mux", default_mux_parents,
+		ARRAY_SIZE(default_mux_parents), 0 },
+	{ 16, 3, default_mux_table, "gfx_mux", default_mux_parents,
+		ARRAY_SIZE(default_mux_parents), 0 },
+	{ 18, 7, clkoutsel_mux_table, "clkout_mux", clkoutsel_mux_parents,
+		ARRAY_SIZE(clkoutsel_mux_parents), 0 },
+	{ 21, 3, gfxmsel_mux_table, "gfxm_mux", gfxmsel_mux_parents,
+		ARRAY_SIZE(gfxmsel_mux_parents), 0 },
+	{ 23, 3, dvcssel_mux_table, "dvc_mux", dvcssel_mux_parents,
+		ARRAY_SIZE(dvcssel_mux_parents), 0 },
+	{ 25, 3, default3_mux_table, "rg_mux", default3_mux_parents,
+		ARRAY_SIZE(default3_mux_parents), 0 },
+	{ 27, 3, default3_mux_table, "rcp_mux", default3_mux_parents,
+		ARRAY_SIZE(default3_mux_parents), 0 },
+};
+
+static struct npcm8xx_clk_div_data npcm8xx_pre_divs[] = {
+	{ NPCM8XX_CLKDIV1, 21, 5, "pre_adc", &npcm8xx_muxes[6].hw, CLK_DIVIDER_READ_ONLY, 0, -1 },
+	{ NPCM8XX_CLKDIV1, 26, 2, "ahb", &hw_pre_clk, CLK_DIVIDER_READ_ONLY, CLK_IS_CRITICAL, NPCM8XX_CLK_AHB },
+};
+
+/* configurable dividers: */
+static struct npcm8xx_clk_div_data npcm8xx_divs[] = {
+	{ NPCM8XX_CLKDIV1, 28, 3, "adc", &npcm8xx_pre_divs[0].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_ADC },
+	{ NPCM8XX_CLKDIV1, 16, 5, "uart", &npcm8xx_muxes[3].hw, 0, 0, NPCM8XX_CLK_UART },
+	{ NPCM8XX_CLKDIV1, 11, 5, "mmc", &npcm8xx_muxes[2].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_MMC },
+	{ NPCM8XX_CLKDIV1, 6, 5, "spi3", &npcm8xx_pre_divs[1].hw, 0, 0, NPCM8XX_CLK_SPI3 },
+	{ NPCM8XX_CLKDIV1, 2, 4, "pci", &npcm8xx_muxes[7].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_PCI },
+
+	{ NPCM8XX_CLKDIV2, 30, 2, "apb4", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_APB4 },
+	{ NPCM8XX_CLKDIV2, 28, 2, "apb3", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_APB3 },
+	{ NPCM8XX_CLKDIV2, 26, 2, "apb2", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_APB2 },
+	{ NPCM8XX_CLKDIV2, 24, 2, "apb1", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_APB1 },
+	{ NPCM8XX_CLKDIV2, 22, 2, "apb5", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_APB5 },
+	{ NPCM8XX_CLKDIV2, 16, 5, "clkout", &npcm8xx_muxes[8].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_CLKOUT },
+	{ NPCM8XX_CLKDIV2, 13, 3, "gfx", &npcm8xx_muxes[7].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_GFX },
+	{ NPCM8XX_CLKDIV2, 8, 5, "usb_bridge", &npcm8xx_muxes[4].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SU },
+	{ NPCM8XX_CLKDIV2, 4, 4, "usb_host", &npcm8xx_muxes[4].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SU48 },
+	{ NPCM8XX_CLKDIV2, 0, 4, "sdhc", &npcm8xx_muxes[2].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SDHC },
+
+	{ NPCM8XX_CLKDIV3, 16, 8, "spi1", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SPI1 },
+	{ NPCM8XX_CLKDIV3, 11, 5, "uart2", &npcm8xx_muxes[3].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_UART2 },
+	{ NPCM8XX_CLKDIV3, 6, 5, "spi0", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SPI0 },
+	{ NPCM8XX_CLKDIV3, 1, 5, "spix", &npcm8xx_pre_divs[1].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SPIX },
+
+	{ NPCM8XX_CLKDIV4, 28, 4, "rg", &npcm8xx_muxes[11].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_RG },
+	{ NPCM8XX_CLKDIV4, 12, 4, "rcp", &npcm8xx_muxes[12].hw, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_RCP },
+
+	{ NPCM8XX_THRTL_CNT, 0, 2, NPCM8XX_CLK_S_TH, &npcm8xx_muxes[0].hw, CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_TH },
+};
+
+static struct clk_hw *
+npcm8xx_clk_register(struct device *dev, const char *name,
+		     struct regmap *clk_regmap, unsigned int offset,
+		     unsigned long flags, const struct clk_ops *npcm8xx_clk_ops,
+		     const struct clk_parent_data *parent_data,
+		     const struct clk_hw *parent_hw, u8 num_parents,
+		     u8 shift, u32 mask, unsigned long width,
+		     const u32 *table, unsigned long clk_flags)
+{
+	struct npcm8xx_clk *clk;
+	struct clk_init_data init = {};
+	int ret;
+
+	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = npcm8xx_clk_ops;
+	init.parent_data = parent_data;
+	init.parent_hws = parent_hw ? &parent_hw : NULL;
+	init.num_parents = num_parents;
+	init.flags = flags;
+
+	clk->clk_regmap = clk_regmap;
+	clk->hw.init = &init;
+	clk->offset = offset;
+	clk->shift = shift;
+	clk->mask = mask;
+	clk->width = width;
+	clk->table = table;
+	clk->flags = clk_flags;
+
+	ret = devm_clk_hw_register(dev, &clk->hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &clk->hw;
+}
+
+static unsigned long npcm8xx_clk_pll_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct npcm8xx_clk *pll = to_npcm8xx_clk(hw);
+	unsigned long fbdv, indv, otdv1, otdv2;
+	unsigned int val;
+	u64 ret;
+
+	if (parent_rate == 0) {
+		pr_debug("%s: parent rate is zero\n", __func__);
+		return 0;
+	}
+
+	regmap_read(pll->clk_regmap, pll->offset, &val);
+
+	indv = FIELD_GET(PLLCON_INDV, val);
+	fbdv = FIELD_GET(PLLCON_FBDV, val);
+	otdv1 = FIELD_GET(PLLCON_OTDV1, val);
+	otdv2 = FIELD_GET(PLLCON_OTDV2, val);
+
+	ret = (u64)parent_rate * fbdv;
+	do_div(ret, indv * otdv1 * otdv2);
+
+	return ret;
+}
+
+static const struct clk_ops npcm8xx_clk_pll_ops = {
+	.recalc_rate = npcm8xx_clk_pll_recalc_rate,
+};
+
+static u8 npcm8xx_clk_mux_get_parent(struct clk_hw *hw)
+{
+	struct npcm8xx_clk *mux = to_npcm8xx_clk(hw);
+	u32 val;
+
+	regmap_read(mux->clk_regmap, mux->offset, &val);
+	val = val >> mux->shift;
+	val &= mux->mask;
+
+	return clk_mux_val_to_index(hw, mux->table, mux->flags, val);
+}
+
+static const struct clk_ops npcm8xx_clk_mux_ops = {
+	.get_parent = npcm8xx_clk_mux_get_parent,
+};
+
+static unsigned long npcm8xx_clk_div_get_parent(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct npcm8xx_clk *div = to_npcm8xx_clk(hw);
+	unsigned int val;
+
+	regmap_read(div->clk_regmap, div->offset, &val);
+	val = val >> div->shift;
+	val &= clk_div_mask(div->width);
+
+	return divider_recalc_rate(hw, parent_rate, val, NULL, div->flags,
+				   div->width);
+}
+
+static const struct clk_ops npcm8xx_clk_div_ops = {
+	.recalc_rate = npcm8xx_clk_div_get_parent,
+};
+
+static int npcm8xx_clk_probe(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *npcm8xx_clk_data;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct regmap *clk_regmap;
+	struct clk_hw *hw;
+	unsigned int i;
+
+	npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+							 NPCM8XX_NUM_CLOCKS),
+					GFP_KERNEL);
+	if (!npcm8xx_clk_data)
+		return -ENOMEM;
+
+	clk_regmap = syscon_regmap_lookup_by_phandle(np, "nuvoton,sysclk");
+	if (IS_ERR(clk_regmap)) {
+		dev_err(&pdev->dev, "Failed to find nuvoton,sysclk\n");
+		return PTR_ERR(clk_regmap);
+	}
+
+	npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+
+	for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+		npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	/* Register plls */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_pll_clks); i++) {
+		struct npcm8xx_clk_pll_data *pll_clk = &npcm8xx_pll_clks[i];
+
+		hw = npcm8xx_clk_register(dev, pll_clk->name, clk_regmap,
+					  pll_clk->reg, pll_clk->flags,
+					  &npcm8xx_clk_pll_ops, &pll_clk->parent,
+					  NULL, 1, 0, 0, 0, NULL, 0);
+		if (IS_ERR(hw))
+			return dev_err_probe(dev, PTR_ERR(hw), "Can't register pll\n");
+		pll_clk->hw = *hw;
+	}
+
+	/* Register fixed dividers */
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL1_DIV2,
+					       NPCM8XX_CLK_S_PLL1, 0, 1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register fixed div\n");
+	hw_pll1_div2 = *hw;
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL2_DIV2,
+					       NPCM8XX_CLK_S_PLL2, 0, 1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register pll2 div2\n");
+	hw_pll2_div2 = *hw;
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL_GFX_DIV2,
+					       NPCM8XX_CLK_S_PLL_GFX, 0, 1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register gfx div2\n");
+	hw_gfx_div2 = *hw;
+
+	/* Register muxes */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+		struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+		hw = npcm8xx_clk_register(dev, mux_data->name, clk_regmap,
+					  NPCM8XX_CLKSEL, mux_data->flags,
+					  &npcm8xx_clk_mux_ops,
+					  mux_data->parent_data, NULL,
+					  mux_data->num_parents,
+					  mux_data->shift, mux_data->mask, 0,
+					  mux_data->table, 0);
+		if (IS_ERR(hw))
+			return dev_err_probe(dev, PTR_ERR(hw), "Can't register mux\n");
+		mux_data->hw = *hw;
+	}
+
+	hw = devm_clk_hw_register_fixed_factor(dev, "pre_clk",
+					       NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register pre clk div2\n");
+	hw_pre_clk = *hw;
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_AXI,
+					       NPCM8XX_CLK_S_TH, 0, 1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register axi div2\n");
+	npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = hw;
+
+	hw = devm_clk_hw_register_fixed_factor(dev, "atb", NPCM8XX_CLK_S_AXI, 0,
+					       1, 2);
+	if (IS_ERR(hw))
+		return dev_err_probe(dev, PTR_ERR(hw), "Can't register atb div2\n");
+	npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = hw;
+
+	/* Register clock pre dividers specified in npcm8xx_divs */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_pre_divs); i++) {
+		struct npcm8xx_clk_div_data *div_data = &npcm8xx_pre_divs[i];
+
+		hw = npcm8xx_clk_register(dev, div_data->name, clk_regmap,
+					  div_data->reg, div_data->flags,
+					  &npcm8xx_clk_div_ops, NULL,
+					  div_data->parent_hw, 1,
+					  div_data->shift, 0, div_data->width,
+					  NULL, div_data->clk_divider_flags);
+		if (IS_ERR(hw))
+			return dev_err_probe(dev, PTR_ERR(hw), "Can't register pre div table\n");
+		div_data->hw = *hw;
+
+		if (div_data->onecell_idx >= 0)
+			npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+	}
+
+	/* Register clock dividers specified in npcm8xx_divs */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
+		struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
+
+		hw = npcm8xx_clk_register(dev, div_data->name, clk_regmap,
+					  div_data->reg, div_data->flags,
+					  &npcm8xx_clk_div_ops, NULL,
+					  div_data->parent_hw, 1,
+					  div_data->shift, 0, div_data->width,
+					  NULL, div_data->clk_divider_flags);
+		if (IS_ERR(hw))
+			return dev_err_probe(dev, PTR_ERR(hw), "Can't register div table\n");
+
+		if (div_data->onecell_idx >= 0)
+			npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   npcm8xx_clk_data);
+}
+
+static const struct of_device_id npcm8xx_clk_dt_ids[] = {
+	{ .compatible = "nuvoton,npcm845-clk", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, npcm8xx_clk_dt_ids);
+
+static struct platform_driver npcm8xx_clk_driver = {
+	.probe  = npcm8xx_clk_probe,
+	.driver = {
+		.name = "npcm8xx_clk",
+		.of_match_table = npcm8xx_clk_dt_ids,
+	},
+};
+
+static int __init npcm8xx_clk_driver_init(void)
+{
+	return platform_driver_register(&npcm8xx_clk_driver);
+}
+arch_initcall(npcm8xx_clk_driver_init);
+
+static void __exit npcm8xx_clk_exit(void)
+{
+	platform_driver_unregister(&npcm8xx_clk_driver);
+}
+module_exit(npcm8xx_clk_exit);
+
+MODULE_DESCRIPTION("Clock driver for Nuvoton NPCM8XX BMC SoC");
+MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
+MODULE_LICENSE("GPL v2");
+
-- 
2.34.1

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Rob Herring]

On Mon, 08 Jan 2024 15:54:14 +0200, Tomer Maimon wrote:
> The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
> refclk property.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clocks' is a required property
	from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clock-names' is a required property
	from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240108135421.684263-2-tmaimon77@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Tomer Maimon]

Hi Rob,

Thanks for your comment.

On Mon, 8 Jan 2024 at 23:09, Rob Herring <robh@kernel.org> wrote:
>
>
> On Mon, 08 Jan 2024 15:54:14 +0200, Tomer Maimon wrote:
> > The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
> > refclk property.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clocks' is a required property
>         from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clock-names' is a required property
>         from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240108135421.684263-2-tmaimon77@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

probably I missed adding the clock and clock-names to the example
node, will be fixed next version

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Rob Herring]

On Mon, Jan 08, 2024 at 03:54:14PM +0200, Tomer Maimon wrote:
> The NPCM8XX clock driver uses 25Mhz external clock, therefor adding

therefore

> refclk property.

'refclk' is not a property.

> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> index b901ca13cd25..0b642bfce292 100644
> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> @@ -21,6 +21,14 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    items:
> +      - description: 25Mhz referance clock

reference

> +
> +  clock-names:
> +    items:
> +      - const: refclk
> +
>    '#clock-cells':
>      const: 1
>      description:
> @@ -30,12 +38,20 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - clocks
> +  - clock-names

New required properties are an ABI break. That's fine if you explain why 
that's okay in the commit msg.


>    - '#clock-cells'
>  
>  additionalProperties: false
>  
>  examples:
>    - |
> +    refclk: refclk-25mhz {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-frequency = <25000000>;
> +    }; 

Examples don't need to show providers.

> +  
>      ahb {
>          #address-cells = <2>;
>          #size-cells = <2>;
> -- 
> 2.34.1
> 

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Tomer Maimon]

Hi Rob,

Thanks for your comment.

On Tue, 9 Jan 2024 at 19:08, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jan 08, 2024 at 03:54:14PM +0200, Tomer Maimon wrote:
> > The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
>
> therefore
>
> > refclk property.
>
> 'refclk' is not a property.
>
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > index b901ca13cd25..0b642bfce292 100644
> > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > @@ -21,6 +21,14 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > +  clocks:
> > +    items:
> > +      - description: 25Mhz referance clock
>
> reference
>
> > +
> > +  clock-names:
> > +    items:
> > +      - const: refclk
> > +
> >    '#clock-cells':
> >      const: 1
> >      description:
> > @@ -30,12 +38,20 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > +  - clocks
> > +  - clock-names
>
> New required properties are an ABI break. That's fine if you explain why
> that's okay in the commit msg.
What do you mean?
Could I add the new required properties to the required list?
>
>
> >    - '#clock-cells'
> >
> >  additionalProperties: false
> >
> >  examples:
> >    - |
> > +    refclk: refclk-25mhz {
> > +        compatible = "fixed-clock";
> > +        #clock-cells = <0>;
> > +        clock-frequency = <25000000>;
> > +    };
>
> Examples don't need to show providers.
>
> > +
> >      ahb {
> >          #address-cells = <2>;
> >          #size-cells = <2>;
> > --
> > 2.34.1
> >

Best regards,

Tomer

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Krzysztof Kozlowski]

On 10/01/2024 14:47, Tomer Maimon wrote:
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: refclk
>>> +
>>>    '#clock-cells':
>>>      const: 1
>>>      description:
>>> @@ -30,12 +38,20 @@ properties:
>>>  required:
>>>    - compatible
>>>    - reg
>>> +  - clocks
>>> +  - clock-names
>>
>> New required properties are an ABI break. That's fine if you explain why
>> that's okay in the commit msg.
> What do you mean?

I think it was clear. Which part is not clear?

> Could I add the new required properties to the required list?

You just did, didn't you? And received feedback that you are breaking
the ABI.

Best regards,
Krzysztof

Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Krzysztof Kozlowski]

On 08/01/2024 14:54, Tomer Maimon wrote:
> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> retrieve SoC model and version information.
> 

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> This patch adds a binding to describe this node.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---

How possibly could it be v22 if there is:
1. No changelog
2. No previous submissions
?

NAK, it's something completely new without any explanation.

Limited review follows.


>  .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> new file mode 100644
> index 000000000000..dfec64a8eb26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock and reset registers block in Nuvoton SoCs

This is vague. Any block? All blocks? Your SoC has only one block? I
doubt, although possible.

Anyway, clocks go to clock directory, not to soc! We've been here and
you already received that feedback.


> +
> +maintainers:
> +  - Tomer Maimon <tmaimon77@gmail.com>
> +
> +description:
> +  The clock and reset registers are a registers block in Nuvoton SoCs that 
> +  handle both reset and clock functionality.

That's still vague. Say something useful.

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nuvoton,npcm750-clk-rst
> +          - nuvoton,npcm845-clk-rst
> +      - const: syscon
> +      - const: simple-mfd

No, it's not a syscon and not a simple-mfd. You just said it is clock
provider and reset controller. Thus missing clock cells and reset cells.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties:
> +  type: object

No, instead:
additionalProperties: false

> +
> +examples:
> +  - |
> +    clk_rst: syscon@801000 {

Suddenly a syscon?

Drop unused label.

> +      compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
> +      reg = <0x801000 0x6C>;

Only lowercase hex.

You just sent some v22 of something new, making all the mistakes from
the past submissions for which you received feedback.
> +    };

Best regards,
Krzysztof

Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Krzysztof Kozlowski]

On 08/01/2024 14:54, Tomer Maimon wrote:
> Replace reg with syscon property since the clock registers handle the
> reset registers as well.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> index 0b642bfce292..c6bf05c163b4 100644
> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> @@ -18,8 +18,9 @@ properties:
>      enum:
>        - nuvoton,npcm845-clk
>  
> -  reg:
> -    maxItems: 1
> +  nuvoton,sysclk:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to access clock registers.

NAK. Not explained, not justified, not reasonable, breaking ABI.

Best regards,
Krzysztof

Re: [PATCH v22 7/8] arm64: dts: nuvoton: npcm8xx: replace reg with syscon property

[Krzysztof Kozlowski]

On 08/01/2024 14:54, Tomer Maimon wrote:
> Replace reg with syscon property since the clock registers handle the
> reset registers as well.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---

NAK for the same reasons as previous patch.

Best regards,
Krzysztof

Re: [PATCH v22 3/8] arm: dts: nuvoton: npcm7xx: modify rst syscon node

[Krzysztof Kozlowski]

On 08/01/2024 14:54, Tomer Maimon wrote:
> rst node name and compatible property modified since clock and reset are
> handled in the same memory region.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> index 868454ae6bde..f72c5a03d04c 100644
> --- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> +++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> @@ -93,8 +93,8 @@ gcr: gcr@800000 {
>  			reg = <0x800000 0x1000>;
>  		};
>  
> -		rst: rst@801000 {
> -			compatible = "nuvoton,npcm750-rst", "syscon", "simple-mfd";
> +		clk_rst: syscon@801000 {
> +			compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";

NAK. This breakes the users, is not justified, is not explained.

Best regards,
Krzysztof

Re: [PATCH v22 5/8] arm64: dts: nuvoton: npcm8xx: add clock reset syscon node

[Krzysztof Kozlowski]

On 08/01/2024 14:54, Tomer Maimon wrote:
> Add clock reset syscon node to handle reset and clock registers
> controllers.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> index 9c4df91031e7..7d5956e2c9f3 100644
> --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> @@ -22,6 +22,11 @@ gcr: system-controller@f0800000 {
>  			reg = <0x0 0xf0800000 0x0 0x1000>;
>  		};
>  
> +		clk_rst: syscon@f0801000 {
> +			compatible = "nuvoton,npcm845-clk-rst", "syscon", "simple-mfd";

This is not a simple-mfd. No children,

> +			reg = <0x0 0xf0801000 0x0 0xC4>;

Use lowercase hex. Please store this feedback in your checklist and do
not repeat the same mistakes in further submissions.

Best regards,
Krzysztof

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2024-01-10 12:54:14)
> On 10/01/2024 14:47, Tomer Maimon wrote:
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: refclk
> >>> +
> >>>    '#clock-cells':
> >>>      const: 1
> >>>      description:
> >>> @@ -30,12 +38,20 @@ properties:
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> +  - clocks
> >>> +  - clock-names
> >>
> >> New required properties are an ABI break. That's fine if you explain why
> >> that's okay in the commit msg.
> > What do you mean?
> 
> I think it was clear. Which part is not clear?
> 
> > Could I add the new required properties to the required list?
> 
> You just did, didn't you? And received feedback that you are breaking
> the ABI.
> 

It's fine to break the ABI as long as the commit message explains that
the driver isn't merged yet.

Re: [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

[Tomer Maimon]

Hi Stephen,

On Wed, 10 Jan 2024 at 23:46, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Krzysztof Kozlowski (2024-01-10 12:54:14)
> > On 10/01/2024 14:47, Tomer Maimon wrote:
> > >>> +
> > >>> +  clock-names:
> > >>> +    items:
> > >>> +      - const: refclk
> > >>> +
> > >>>    '#clock-cells':
> > >>>      const: 1
> > >>>      description:
> > >>> @@ -30,12 +38,20 @@ properties:
> > >>>  required:
> > >>>    - compatible
> > >>>    - reg
> > >>> +  - clocks
> > >>> +  - clock-names
> > >>
> > >> New required properties are an ABI break. That's fine if you explain why
> > >> that's okay in the commit msg.
> > > What do you mean?
> >
> > I think it was clear. Which part is not clear?
> >
> > > Could I add the new required properties to the required list?
> >
> > You just did, didn't you? And received feedback that you are breaking
> > the ABI.
> >
>
> It's fine to break the ABI as long as the commit message explains that
> the driver isn't merged yet.

Thanks for your clarification.

Best regards,

Tomer

Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comments.

Sorry for the long explanation but I think it is necessary.

In the NPCM8XX SoC, the reset and the clock register modules are
scrambled in the same memory register region.
The NPCM8XX Clock driver is still in the upstream process (for a long
time) but the NPCM8XX reset driver is already upstreamed.

One of the main comments in the NPCM8XX Clock driver upstream process
is that the clock register is mixed with the reset register and
therefore we can't map (ioremap) the clock register
region because is already mapped by the reset module, therefore we
decided to use an external syscon to handle the clock and the reset
registers driver.

 I highly appreciate your guidance on this topic.

On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> > will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> > NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> > retrieve SoC model and version information.
> >
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> > This patch adds a binding to describe this node.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
>
> How possibly could it be v22 if there is:
> 1. No changelog
> 2. No previous submissions
> ?
Should the dt-binding and dts patches be a part of the clock patch set
(this is why it's V22) or should I open a new patch set?
>
> NAK, it's something completely new without any explanation.
>
> Limited review follows.
>
>
> >  .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > new file mode 100644
> > index 000000000000..dfec64a8eb26
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > @@ -0,0 +1,40 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Clock and reset registers block in Nuvoton SoCs
>
> This is vague. Any block? All blocks? Your SoC has only one block? I
> doubt, although possible.
>
> Anyway, clocks go to clock directory, not to soc! We've been here and
> you already received that feedback.
Since one region handles the reset and the clock registers shouldn't I
add the dt-binding to the SoC like the GCR and not to the clock
directory?
https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
>
>
> > +
> > +maintainers:
> > +  - Tomer Maimon <tmaimon77@gmail.com>
> > +
> > +description:
> > +  The clock and reset registers are a registers block in Nuvoton SoCs that
> > +  handle both reset and clock functionality.
>
> That's still vague. Say something useful.
Will describe more
>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - nuvoton,npcm750-clk-rst
> > +          - nuvoton,npcm845-clk-rst
> > +      - const: syscon
> > +      - const: simple-mfd
>
> No, it's not a syscon and not a simple-mfd. You just said it is clock
Yes, I understand the syscon node represents a register region
containing a set of miscellaneous registers, but as explain above it
is quite the case here.
I will remove the simple-mfd.
> provider and reset controller. Thus missing clock cells and reset cells.
The reset cell and clock cell found at the clock and reset dt-binding,
is it enough?
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties:
> > +  type: object
>
> No, instead:
> additionalProperties: false
O.K.
>
> > +
> > +examples:
> > +  - |
> > +    clk_rst: syscon@801000 {
I should use syscon no? if no what should I use?
>
> Suddenly a syscon?
>
> Drop unused label.
>
> > +      compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
> > +      reg = <0x801000 0x6C>;
>
> Only lowercase hex.
>
> You just sent some v22 of something new, making all the mistakes from
> the past submissions for which you received feedback.
> > +    };
>
> Best regards,
> Krzysztof
>

Thanks a lot!

Tomer

Re: [PATCH v22 5/8] arm64: dts: nuvoton: npcm8xx: add clock reset syscon node

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comments.

On Wed, 10 Jan 2024 at 23:01, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > Add clock reset syscon node to handle reset and clock registers
> > controllers.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > index 9c4df91031e7..7d5956e2c9f3 100644
> > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > @@ -22,6 +22,11 @@ gcr: system-controller@f0800000 {
> >                       reg = <0x0 0xf0800000 0x0 0x1000>;
> >               };
> >
> > +             clk_rst: syscon@f0801000 {
> > +                     compatible = "nuvoton,npcm845-clk-rst", "syscon", "simple-mfd";
>
> This is not a simple-mfd. No children,
>
> > +                     reg = <0x0 0xf0801000 0x0 0xC4>;
>
> Use lowercase hex. Please store this feedback in your checklist and do
> not repeat the same mistakes in further submissions.
>
> Best regards,
> Krzysztof
>
will be fixed in the next version.

Best regards,

Tomer

Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Tomer Maimon]

Hi Krzysztof,

As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add
binding for clock and reset registers mail.

In the NPCM8XX SoC, the reset and the clock register modules are
scrambled in the same memory register region.
The NPCM8XX Clock driver is still in the upstream process (for a long
time) but the NPCM8XX reset driver is already upstreamed.

On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > Replace reg with syscon property since the clock registers handle the
> > reset registers as well.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > index 0b642bfce292..c6bf05c163b4 100644
> > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > @@ -18,8 +18,9 @@ properties:
> >      enum:
> >        - nuvoton,npcm845-clk
> >
> > -  reg:
> > -    maxItems: 1
> > +  nuvoton,sysclk:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to access clock registers.
>
> NAK. Not explained, not justified, not reasonable, breaking ABI.
Should I explain more in the commit message or/and the nuvoton,sysclk property?
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v22 7/8] arm64: dts: nuvoton: npcm8xx: replace reg with syscon property

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comment.

On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > Replace reg with syscon property since the clock registers handle the
> > reset registers as well.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
>
> NAK for the same reasons as previous patch.
Will explain more in the commit message
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v22 3/8] arm: dts: nuvoton: npcm7xx: modify rst syscon node

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comment.

On Wed, 10 Jan 2024 at 23:01, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > rst node name and compatible property modified since clock and reset are
> > handled in the same memory region.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> > index 868454ae6bde..f72c5a03d04c 100644
> > --- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> > +++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
> > @@ -93,8 +93,8 @@ gcr: gcr@800000 {
> >                       reg = <0x800000 0x1000>;
> >               };
> >
> > -             rst: rst@801000 {
> > -                     compatible = "nuvoton,npcm750-rst", "syscon", "simple-mfd";
> > +             clk_rst: syscon@801000 {
> > +                     compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
>
> NAK. This breakes the users, is not justified, is not explained.
Sorry, I didn't understand, which user it is breaking? there isn't a
device tree node that uses the rst node.
Should I explain it better in the commit message?
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v22 3/8] arm: dts: nuvoton: npcm7xx: modify rst syscon node

[Krzysztof Kozlowski]

On 16/01/2024 20:51, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> Thanks for your comment.
> 
> On Wed, 10 Jan 2024 at 23:01, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>> rst node name and compatible property modified since clock and reset are
>>> handled in the same memory region.
>>>
>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>> ---
>>>  arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
>>> index 868454ae6bde..f72c5a03d04c 100644
>>> --- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
>>> +++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
>>> @@ -93,8 +93,8 @@ gcr: gcr@800000 {
>>>                       reg = <0x800000 0x1000>;
>>>               };
>>>
>>> -             rst: rst@801000 {
>>> -                     compatible = "nuvoton,npcm750-rst", "syscon", "simple-mfd";
>>> +             clk_rst: syscon@801000 {
>>> +                     compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
>>
>> NAK. This breakes the users, is not justified, is not explained.
> Sorry, I didn't understand, which user it is breaking? there isn't a
> device tree node that uses the rst node.

Any user of this DTS.

> Should I explain it better in the commit message?

I doubt that you can find proper reason, because "I want different name"
is not valid. It's just compatible, it cannot be changed just because
you add new property.

Best regards,
Krzysztof

Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Krzysztof Kozlowski]

On 16/01/2024 20:02, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> Thanks for your comments.
> 
> Sorry for the long explanation but I think it is necessary.
> 
> In the NPCM8XX SoC, the reset and the clock register modules are
> scrambled in the same memory register region.
> The NPCM8XX Clock driver is still in the upstream process (for a long
> time) but the NPCM8XX reset driver is already upstreamed.
> 
> One of the main comments in the NPCM8XX Clock driver upstream process
> is that the clock register is mixed with the reset register and
> therefore we can't map (ioremap) the clock register
> region because is already mapped by the reset module, therefore we
> decided to use an external syscon to handle the clock and the reset
> registers driver.
> 
>  I highly appreciate your guidance on this topic.

Linux deals with it easily, that's why we have regmaps. What's the
problem exactly?

> 
> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
>>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
>>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
>>> retrieve SoC model and version information.
>>>
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>>> This patch adds a binding to describe this node.
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>>
>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>> ---
>>
>> How possibly could it be v22 if there is:
>> 1. No changelog
>> 2. No previous submissions
>> ?
> Should the dt-binding and dts patches be a part of the clock patch set
> (this is why it's V22) or should I open a new patch set?

You should explain what is happening here. That's why you have changelog
for.

>>
>> NAK, it's something completely new without any explanation.
>>
>> Limited review follows.
>>
>>
>>>  .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>> new file mode 100644
>>> index 000000000000..dfec64a8eb26
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>> @@ -0,0 +1,40 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Clock and reset registers block in Nuvoton SoCs
>>
>> This is vague. Any block? All blocks? Your SoC has only one block? I
>> doubt, although possible.
>>
>> Anyway, clocks go to clock directory, not to soc! We've been here and
>> you already received that feedback.
> Since one region handles the reset and the clock registers shouldn't I
> add the dt-binding to the SoC like the GCR and not to the clock

No, soc is not a dumping ground..

> directory?
> https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml

Choose the main feature of the block - either clock controller or reset
controller - and put it there.

>>
>>
>>> +
>>> +maintainers:
>>> +  - Tomer Maimon <tmaimon77@gmail.com>
>>> +
>>> +description:
>>> +  The clock and reset registers are a registers block in Nuvoton SoCs that
>>> +  handle both reset and clock functionality.
>>
>> That's still vague. Say something useful.
> Will describe more
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - nuvoton,npcm750-clk-rst
>>> +          - nuvoton,npcm845-clk-rst
>>> +      - const: syscon
>>> +      - const: simple-mfd
>>
>> No, it's not a syscon and not a simple-mfd. You just said it is clock
> Yes, I understand the syscon node represents a register region
> containing a set of miscellaneous registers, but as explain above it
> is quite the case here.

Nothing in this patch was telling this.

> I will remove the simple-mfd.
>> provider and reset controller. Thus missing clock cells and reset cells.
> The reset cell and clock cell found at the clock and reset dt-binding,
> is it enough?

This is the reset and clock binding, isn't it? You called it (your title):
"Clock and reset registers block in Nuvoton SoCs"




Best regards,
Krzysztof

Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Krzysztof Kozlowski]

On 16/01/2024 20:37, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add
> binding for clock and reset registers mail.
> 
> In the NPCM8XX SoC, the reset and the clock register modules are
> scrambled in the same memory register region.
> The NPCM8XX Clock driver is still in the upstream process (for a long
> time) but the NPCM8XX reset driver is already upstreamed.

First of all, the drivers itself don't matter here, we talk about
bindings. I assume though they were going together, so that's why you
mentioned driver... but just to clarify: we talk here only about drivers.

If reset bindings were accepted, then why they aren't referenced?

If clock bindings were not accepted, then what is this patch and this
file about?

> 
> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>> Replace reg with syscon property since the clock registers handle the
>>> reset registers as well.
>>>
>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>> ---
>>>  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>> index 0b642bfce292..c6bf05c163b4 100644
>>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>> @@ -18,8 +18,9 @@ properties:
>>>      enum:
>>>        - nuvoton,npcm845-clk
>>>
>>> -  reg:
>>> -    maxItems: 1
>>> +  nuvoton,sysclk:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: phandle to access clock registers.
>>
>> NAK. Not explained, not justified, not reasonable, breaking ABI.
> Should I explain more in the commit message or/and the nuvoton,sysclk property?

Let's try to explain here first. I really do not understand why do you
change this binding. Your device did not change, so your binding should
not. Now, if you say "but I changed drivers", then it does not matter.
Bindings do not change because you did something in the drivers, in
general. At least they should not.

Best regards,
Krzysztof

Re: [PATCH v22 7/8] arm64: dts: nuvoton: npcm8xx: replace reg with syscon property

[Krzysztof Kozlowski]

On 16/01/2024 20:39, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> Thanks for your comment.
> 
> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>> Replace reg with syscon property since the clock registers handle the
>>> reset registers as well.
>>>
>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>> ---
>>
>> NAK for the same reasons as previous patch.
> Will explain more in the commit message

No, this wasn't even tested. Build your code with W=1 and fix all
warnings first. But anyway this is not the way to go.

Best regards,
Krzysztof

Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comment

On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 16/01/2024 20:02, Tomer Maimon wrote:
> > Hi Krzysztof,
> >
> > Thanks for your comments.
> >
> > Sorry for the long explanation but I think it is necessary.
> >
> > In the NPCM8XX SoC, the reset and the clock register modules are
> > scrambled in the same memory register region.
> > The NPCM8XX Clock driver is still in the upstream process (for a long
> > time) but the NPCM8XX reset driver is already upstreamed.
> >
> > One of the main comments in the NPCM8XX Clock driver upstream process
> > is that the clock register is mixed with the reset register and
> > therefore we can't map (ioremap) the clock register
> > region because is already mapped by the reset module, therefore we
> > decided to use an external syscon to handle the clock and the reset
> > registers driver.
> >
> >  I highly appreciate your guidance on this topic.
>
> Linux deals with it easily, that's why we have regmaps. What's the
> problem exactly?
This is exactly what is done in the submitted clock driver.
>
> >
> > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 08/01/2024 14:54, Tomer Maimon wrote:
> >>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> >>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> >>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> >>> retrieve SoC model and version information.
> >>>
> >>
> >> A nit, subject: drop second/last, redundant "bindings". The
> >> "dt-bindings" prefix is already stating that these are bindings.
> >> See also:
> >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >>
> >>> This patch adds a binding to describe this node.
> >>
> >> Please do not use "This commit/patch/change", but imperative mood. See
> >> longer explanation here:
> >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> >>
> >>>
> >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> >>> ---
> >>
> >> How possibly could it be v22 if there is:
> >> 1. No changelog
> >> 2. No previous submissions
> >> ?
> > Should the dt-binding and dts patches be a part of the clock patch set
> > (this is why it's V22) or should I open a new patch set?
>
> You should explain what is happening here. That's why you have changelog
> for.
Will explain it better in the cover letter in the change log.
>
> >>
> >> NAK, it's something completely new without any explanation.
> >>
> >> Limited review follows.
> >>
> >>
> >>>  .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
> >>>  1 file changed, 40 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>> new file mode 100644
> >>> index 000000000000..dfec64a8eb26
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>> @@ -0,0 +1,40 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Clock and reset registers block in Nuvoton SoCs
> >>
> >> This is vague. Any block? All blocks? Your SoC has only one block? I
> >> doubt, although possible.
> >>
> >> Anyway, clocks go to clock directory, not to soc! We've been here and
> >> you already received that feedback.
> > Since one region handles the reset and the clock registers shouldn't I
> > add the dt-binding to the SoC like the GCR and not to the clock
>
> No, soc is not a dumping ground..
Maybe I do not need to add dt binding document for the clock and reset
syscon but handle the registers as follows in the dtsi.

                sysctrl: system-controller@f0801000 {
                        compatible = "syscon", "simple-mfd";
                        reg = <0x0 0xf0801000 0x0 0x1000>;

                        rstc: reset-controller {
                                compatible = "nuvoton,npcm845-reset";
                                #reset-cells = <2>;
                                nuvoton,sysgcr = <&gcr>;
                        };

                        clk: clock-controller {
                                compatible = "nuvoton,npcm845-clk";
                                #clock-cells = <1>;
                                clocks = <&refclk>;
                                clock-names = "refclk";
                        };
                };

is it acceptable?
>
> > directory?
> > https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
>
> Choose the main feature of the block - either clock controller or reset
> controller - and put it there.
>
> >>
> >>
> >>> +
> >>> +maintainers:
> >>> +  - Tomer Maimon <tmaimon77@gmail.com>
> >>> +
> >>> +description:
> >>> +  The clock and reset registers are a registers block in Nuvoton SoCs that
> >>> +  handle both reset and clock functionality.
> >>
> >> That's still vague. Say something useful.
> > Will describe more
> >>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - nuvoton,npcm750-clk-rst
> >>> +          - nuvoton,npcm845-clk-rst
> >>> +      - const: syscon
> >>> +      - const: simple-mfd
> >>
> >> No, it's not a syscon and not a simple-mfd. You just said it is clock
> > Yes, I understand the syscon node represents a register region
> > containing a set of miscellaneous registers, but as explain above it
> > is quite the case here.
>
> Nothing in this patch was telling this.
>
> > I will remove the simple-mfd.
> >> provider and reset controller. Thus missing clock cells and reset cells.
> > The reset cell and clock cell found at the clock and reset dt-binding,
> > is it enough?
>
> This is the reset and clock binding, isn't it? You called it (your title):
> "Clock and reset registers block in Nuvoton SoCs"
>
>
>
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Tomer Maimon]

Hi Krzysztof,

Thanks for your comments.

On Tue, 16 Jan 2024 at 22:40, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 16/01/2024 20:37, Tomer Maimon wrote:
> > Hi Krzysztof,
> >
> > As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add
> > binding for clock and reset registers mail.
> >
> > In the NPCM8XX SoC, the reset and the clock register modules are
> > scrambled in the same memory register region.
> > The NPCM8XX Clock driver is still in the upstream process (for a long
> > time) but the NPCM8XX reset driver is already upstreamed.
>
> First of all, the drivers itself don't matter here, we talk about
> bindings. I assume though they were going together, so that's why you
> mentioned driver... but just to clarify: we talk here only about drivers.
>
> If reset bindings were accepted, then why they aren't referenced?
>
> If clock bindings were not accepted, then what is this patch and this
> file about?
>
> >
> > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 08/01/2024 14:54, Tomer Maimon wrote:
> >>> Replace reg with syscon property since the clock registers handle the
> >>> reset registers as well.
> >>>
> >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> >>> ---
> >>>  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
> >>>  1 file changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> >>> index 0b642bfce292..c6bf05c163b4 100644
> >>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> >>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> >>> @@ -18,8 +18,9 @@ properties:
> >>>      enum:
> >>>        - nuvoton,npcm845-clk
> >>>
> >>> -  reg:
> >>> -    maxItems: 1
> >>> +  nuvoton,sysclk:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: phandle to access clock registers.
> >>
> >> NAK. Not explained, not justified, not reasonable, breaking ABI.
> > Should I explain more in the commit message or/and the nuvoton,sysclk property?
>
> Let's try to explain here first. I really do not understand why do you
> change this binding. Your device did not change, so your binding should
> not. Now, if you say "but I changed drivers", then it does not matter.
> Bindings do not change because you did something in the drivers, in
> general. At least they should not.
The confusion here is because the clock binding was upstreamed but the
clock driver has not upstreamed yet.
The clock driver will use regmap and ioremap so reg property is not
needed, should I remove it? or leave it?
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property

[Krzysztof Kozlowski]

On 22/01/2024 18:26, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> Thanks for your comments.
> 
> On Tue, 16 Jan 2024 at 22:40, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 16/01/2024 20:37, Tomer Maimon wrote:
>>> Hi Krzysztof,
>>>
>>> As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add
>>> binding for clock and reset registers mail.
>>>
>>> In the NPCM8XX SoC, the reset and the clock register modules are
>>> scrambled in the same memory register region.
>>> The NPCM8XX Clock driver is still in the upstream process (for a long
>>> time) but the NPCM8XX reset driver is already upstreamed.
>>
>> First of all, the drivers itself don't matter here, we talk about
>> bindings. I assume though they were going together, so that's why you
>> mentioned driver... but just to clarify: we talk here only about drivers.
>>
>> If reset bindings were accepted, then why they aren't referenced?
>>
>> If clock bindings were not accepted, then what is this patch and this
>> file about?
>>
>>>
>>> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>>>> Replace reg with syscon property since the clock registers handle the
>>>>> reset registers as well.
>>>>>
>>>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>>>> ---
>>>>>  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 22 +++++++++----------
>>>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>> index 0b642bfce292..c6bf05c163b4 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>> @@ -18,8 +18,9 @@ properties:
>>>>>      enum:
>>>>>        - nuvoton,npcm845-clk
>>>>>
>>>>> -  reg:
>>>>> -    maxItems: 1
>>>>> +  nuvoton,sysclk:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description: phandle to access clock registers.
>>>>
>>>> NAK. Not explained, not justified, not reasonable, breaking ABI.
>>> Should I explain more in the commit message or/and the nuvoton,sysclk property?
>>
>> Let's try to explain here first. I really do not understand why do you
>> change this binding. Your device did not change, so your binding should
>> not. Now, if you say "but I changed drivers", then it does not matter.
>> Bindings do not change because you did something in the drivers, in
>> general. At least they should not.
> The confusion here is because the clock binding was upstreamed but the
> clock driver has not upstreamed yet.

So what does it mean? Being upstreamed independently does not mean
something is wrong, so I do not see any confusion. There is absolutely
no confusion in upstreaming binding before upstreaming driver.

> The clock driver will use regmap and ioremap so reg property is not
> needed, should I remove it? or leave it?

Just because you upstream drivers? You should not change bindings just
because you figured out that you will implement something that or other
way. Please describe the hardware in the binding, not the driver.


Best regards,
Krzysztof

Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Tomer Maimon]

Hi Krzysztof,


On Mon, 22 Jan 2024 at 19:14, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Hi Krzysztof,
>
> Thanks for your comment
>
> On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 16/01/2024 20:02, Tomer Maimon wrote:
> > > Hi Krzysztof,
> > >
> > > Thanks for your comments.
> > >
> > > Sorry for the long explanation but I think it is necessary.
> > >
> > > In the NPCM8XX SoC, the reset and the clock register modules are
> > > scrambled in the same memory register region.
> > > The NPCM8XX Clock driver is still in the upstream process (for a long
> > > time) but the NPCM8XX reset driver is already upstreamed.
> > >
> > > One of the main comments in the NPCM8XX Clock driver upstream process
> > > is that the clock register is mixed with the reset register and
> > > therefore we can't map (ioremap) the clock register
> > > region because is already mapped by the reset module, therefore we
> > > decided to use an external syscon to handle the clock and the reset
> > > registers driver.
> > >
> > >  I highly appreciate your guidance on this topic.
> >
> > Linux deals with it easily, that's why we have regmaps. What's the
> > problem exactly?
> This is exactly what is done in the submitted clock driver.
> >
> > >
> > > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 08/01/2024 14:54, Tomer Maimon wrote:
> > >>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> > >>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> > >>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> > >>> retrieve SoC model and version information.
> > >>>
> > >>
> > >> A nit, subject: drop second/last, redundant "bindings". The
> > >> "dt-bindings" prefix is already stating that these are bindings.
> > >> See also:
> > >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> > >>
> > >>> This patch adds a binding to describe this node.
> > >>
> > >> Please do not use "This commit/patch/change", but imperative mood. See
> > >> longer explanation here:
> > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > >>
> > >>>
> > >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > >>> ---
> > >>
> > >> How possibly could it be v22 if there is:
> > >> 1. No changelog
> > >> 2. No previous submissions
> > >> ?
> > > Should the dt-binding and dts patches be a part of the clock patch set
> > > (this is why it's V22) or should I open a new patch set?
> >
> > You should explain what is happening here. That's why you have changelog
> > for.
> Will explain it better in the cover letter in the change log.
> >
> > >>
> > >> NAK, it's something completely new without any explanation.
> > >>
> > >> Limited review follows.
> > >>
> > >>
> > >>>  .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml     | 40 +++++++++++++++++++
> > >>>  1 file changed, 40 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..dfec64a8eb26
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>> @@ -0,0 +1,40 @@
> > >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: Clock and reset registers block in Nuvoton SoCs
> > >>
> > >> This is vague. Any block? All blocks? Your SoC has only one block? I
> > >> doubt, although possible.
> > >>
> > >> Anyway, clocks go to clock directory, not to soc! We've been here and
> > >> you already received that feedback.
> > > Since one region handles the reset and the clock registers shouldn't I
> > > add the dt-binding to the SoC like the GCR and not to the clock
> >
> > No, soc is not a dumping ground..
> Maybe I do not need to add dt binding document for the clock and reset
> syscon but handle the registers as follows in the dtsi.
>
>                 sysctrl: system-controller@f0801000 {
>                         compatible = "syscon", "simple-mfd";
>                         reg = <0x0 0xf0801000 0x0 0x1000>;
>
>                         rstc: reset-controller {
>                                 compatible = "nuvoton,npcm845-reset";
>                                 #reset-cells = <2>;
>                                 nuvoton,sysgcr = <&gcr>;
>                         };
>
>                         clk: clock-controller {
>                                 compatible = "nuvoton,npcm845-clk";
>                                 #clock-cells = <1>;
>                                 clocks = <&refclk>;
>                                 clock-names = "refclk";
>                         };
>                 };
>
> is it acceptable?
Appreciate for your advice on the question above.
> >
> > > directory?
> > > https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
> >
> > Choose the main feature of the block - either clock controller or reset
> > controller - and put it there.
> >
> > >>
> > >>
> > >>> +
> > >>> +maintainers:
> > >>> +  - Tomer Maimon <tmaimon77@gmail.com>
> > >>> +
> > >>> +description:
> > >>> +  The clock and reset registers are a registers block in Nuvoton SoCs that
> > >>> +  handle both reset and clock functionality.
> > >>
> > >> That's still vague. Say something useful.
> > > Will describe more
> > >>
> > >>> +
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    items:
> > >>> +      - enum:
> > >>> +          - nuvoton,npcm750-clk-rst
> > >>> +          - nuvoton,npcm845-clk-rst
> > >>> +      - const: syscon
> > >>> +      - const: simple-mfd
> > >>
> > >> No, it's not a syscon and not a simple-mfd. You just said it is clock
> > > Yes, I understand the syscon node represents a register region
> > > containing a set of miscellaneous registers, but as explain above it
> > > is quite the case here.
> >
> > Nothing in this patch was telling this.
> >
> > > I will remove the simple-mfd.
> > >> provider and reset controller. Thus missing clock cells and reset cells.
> > > The reset cell and clock cell found at the clock and reset dt-binding,
> > > is it enough?
> >
> > This is the reset and clock binding, isn't it? You called it (your title):
> > "Clock and reset registers block in Nuvoton SoCs"
> >
> >
> >
> >
> > Best regards,
> > Krzysztof
> >
>
> Best regards,
>
> Tomer

Best regards,

Tomer