[PATCH v23 0/3] Introduce Nuvoton Arbel NPCM8XX BMC SoC

[PATCH v23 0/3] 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.

for your note:
 1. dt-bindings clock modification started from v22 since the upstream npcm8xx 
    clock driver haven't merged yet and requires dt binding update according to 
    the new npcm8xx clock driver.

 2. 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:
 - Rob Herring: https://www.spinics.net/lists/devicetree/msg663403.html
 - Krzysztof Kozlowski: https://www.spinics.net/lists/devicetree/msg665206.html

Changes since version 22:
 - Modify commit message to explain broken ABI in dt-binding
 - Using regmap parenet regmap memory therefore remove use of npcm8xx rst-clock patch.
 - Leave npcm7xx rst node as is
 
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 (3):
  dt-bindings: clock: npcm845: Add reference 25m clock property
  arm64: dts: nuvoton: npcm8xx: add reference 25m clock
  clk: npcm8xx: add clock controller

 .../bindings/clock/nuvoton,npcm845-clk.yaml   |  12 +
 .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi   |   2 +
 .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts  |   6 +
 drivers/clk/Kconfig                           |   8 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-npcm8xx.c                     | 509 ++++++++++++++++++
 6 files changed, 538 insertions(+)
 create mode 100644 drivers/clk/clk-npcm8xx.c

-- 
2.34.1

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

[Tomer Maimon]

The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding
clock property.

The new required clock property does not break the NPCM8XX clock ABI
since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla.

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

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
index b901ca13cd25..7060891d0c32 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 reference clock
+
+  clock-names:
+    items:
+      - const: refclk
+
   '#clock-cells':
     const: 1
     description:
@@ -30,6 +38,8 @@ properties:
 required:
   - compatible
   - reg
+  - clocks
+  - clock-names
   - '#clock-cells'
 
 additionalProperties: false
@@ -44,6 +54,8 @@ examples:
             compatible = "nuvoton,npcm845-clk";
             reg = <0x0 0xf0801000 0x0 0x1000>;
             #clock-cells = <1>;
+            clocks = <&refclk>;
+            clock-names = "refclk";
         };
     };
 ...
-- 
2.34.1

[PATCH v23 2/3] arm64: dts: nuvoton: npcm8xx: add reference 25m clock

[Tomer Maimon]

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

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

diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
index ecd171b2feba..f10027c14e63 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 {
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 v23 3/3] 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/Kconfig       |   8 +
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-npcm8xx.c | 509 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 518 insertions(+)
 create mode 100644 drivers/clk/clk-npcm8xx.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 50af5fc7f570..a324678f3e12 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -334,6 +334,14 @@ config COMMON_CLK_LOCHNAGAR
 	  This driver supports the clocking features of the Cirrus Logic
 	  Lochnagar audio development board.
 
+config COMMON_CLK_NPCM8XX
+	tristate "Clock driver for the NPCM8XX SoC Family"
+	depends on ARCH_NPCM || COMPILE_TEST
+	help
+	  This driver supports the clocks on the Nuvoton BMC NPCM8XX SoC Family,
+	  all the clocks are initialized by the bootloader, so this driver
+	  allows only reading of current settings directly from the hardware.
+
 config COMMON_CLK_LOONGSON2
 	bool "Clock driver for Loongson-2 SoC"
 	depends on LOONGARCH || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 14fa8d4ecc1f..584fc82061f8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARCH_MILBEAUT_M10V)	+= clk-milbeaut.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
 obj-$(CONFIG_ARCH_NPCM7XX)	    	+= clk-npcm7xx.o
+obj-$(CONFIG_COMMON_CLK_NPCM8XX)	+= clk-npcm8xx.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
 obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
new file mode 100644
index 000000000000..eacb579d30af
--- /dev/null
+++ b/drivers/clk/clk-npcm8xx.c
@@ -0,0 +1,509 @@
+// 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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
+	struct clk_hw_onecell_data *npcm8xx_clk_data;
+	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_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(clk_regmap))
+		return dev_err_probe(dev, PTR_ERR(clk_regmap), "Can't register clock map\n");
+
+	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 v23 0/3] Introduce Nuvoton Arbel NPCM8XX BMC SoC

[Krzysztof Kozlowski]

On 31/01/2024 19:26, Tomer Maimon wrote:
> 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.
> 
> for your note:
>  1. dt-bindings clock modification started from v22 since the upstream npcm8xx 
>     clock driver haven't merged yet and requires dt binding update according to 
>     the new npcm8xx clock driver.
> 
>  2. 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:
>  - Rob Herring: https://www.spinics.net/lists/devicetree/msg663403.html
>  - Krzysztof Kozlowski: https://www.spinics.net/lists/devicetree/msg665206.html

Use lore links which are way much more helpful.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 31/01/2024 19:26, Tomer Maimon wrote:
> The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding
> clock property.
> 
> The new required clock property does not break the NPCM8XX clock ABI
> since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Stephen Boyd]

Quoting Tomer Maimon (2024-01-31 10:26:53)
> diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> new file mode 100644
> index 000000000000..eacb579d30af
> --- /dev/null
> +++ b/drivers/clk/clk-npcm8xx.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NPCM8xx Clock Generator
> + * All the clocks are initialized by the bootloader, so this driver allows only
[...]
> +
> +/* 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"

Please inline all these string #defines to the place they're used.

> +
> +static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;
[..]
> +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;

Is it necessary to check? Can't it be set unconditionally?

> +       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_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 device_node *parent_np = of_get_parent(pdev->dev.of_node);

The parent of this device is not a syscon.

> +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> +       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_node_to_regmap(parent_np);
> +       of_node_put(parent_np);

Is there another binding update that is going to move this node to be a
child of the syscon?

		gcr: system-controller@f0800000 {
                        compatible = "nuvoton,npcm845-gcr", "syscon";
                        reg = <0x0 0xf0800000 0x0 0x1000>;
                };

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

[Stephen Boyd]

Quoting Tomer Maimon (2024-01-31 10:26:51)
> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> index b901ca13cd25..7060891d0c32 100644
> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> @@ -44,6 +54,8 @@ examples:
>              compatible = "nuvoton,npcm845-clk";
>              reg = <0x0 0xf0801000 0x0 0x1000>;
>              #clock-cells = <1>;
> +            clocks = <&refclk>;
> +            clock-names = "refclk";

The driver seems to want this to be a child of the mfd syscon. Is that
right?

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2024-01-31 10:26:53)
> > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > new file mode 100644
> > index 000000000000..eacb579d30af
> > --- /dev/null
> > +++ b/drivers/clk/clk-npcm8xx.c
> > @@ -0,0 +1,509 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NPCM8xx Clock Generator
> > + * All the clocks are initialized by the bootloader, so this driver allows only
> [...]
> > +
> > +/* 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"
>
> Please inline all these string #defines to the place they're used.
The version V21 you mention using define only when the definition is
used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?
>
> > +
> > +static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;
> [..]
> > +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;
>
> Is it necessary to check? Can't it be set unconditionally?
Will remove
>
> > +       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_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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
>
> The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock
in general is syscon, this is why we will modify the DTS so the clock
and the reset will be under syscon father node
                sysctrl: system-controller@f0801000 {
                        compatible = "syscon", "simple-mfd";
                        reg = <0x0 0xf0801000 0x0 0x1000>;

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

                        clk: clock-controller {
                                compatible = "nuvoton,npcm845-clk";
                                #clock-cells = <1>;
                                clocks = <&refclk>;
                                clock-names = "refclk";
                        };
                };
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
>
> > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > +       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_node_to_regmap(parent_np);
> > +       of_node_put(parent_np);
>
> Is there another binding update that is going to move this node to be a
> child of the syscon?
>
>                 gcr: system-controller@f0800000 {
>                         compatible = "nuvoton,npcm845-gcr", "syscon";
>                         reg = <0x0 0xf0800000 0x0 0x1000>;
>                 };
No, sorry but I'm not going to use the GCR node the handle the clock
and reset modules, the GCR has different memory space.
the clock driver will have the following device tree
               sysctrl: system-controller@f0801000 {
                        compatible = "syscon", "simple-mfd";
                        reg = <0x0 0xf0801000 0x0 0x1000>;

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

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

Stephen appreciate it if you could speed this upstream and apply it in
this kernel version.
hope the clarification is understood and if it is fine I can send V24 this week.

Thanks,

Tomer

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

[Tomer Maimon]

Hi Stephen,

The plan is for both the clock and reset will be under only the memory
region handle like
                sysctrl: system-controller@f0801000 {
                        compatible = "syscon", "simple-mfd";
                        reg = <0x0 0xf0801000 0x0 0x1000>;

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

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

is it problematic?

But this commit is not related to it.

Thanks,

Tomer

On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2024-01-31 10:26:51)
> > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > index b901ca13cd25..7060891d0c32 100644
> > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > @@ -44,6 +54,8 @@ examples:
> >              compatible = "nuvoton,npcm845-clk";
> >              reg = <0x0 0xf0801000 0x0 0x1000>;
> >              #clock-cells = <1>;
> > +            clocks = <&refclk>;
> > +            clock-names = "refclk";
>
> The driver seems to want this to be a child of the mfd syscon. Is that
> right?

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Stephen Boyd]

Quoting Tomer Maimon (2024-02-25 10:00:35)
> Hi Stephen,
> 
> On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > new file mode 100644
> > > index 000000000000..eacb579d30af
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-npcm8xx.c
> > > @@ -0,0 +1,509 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Nuvoton NPCM8xx Clock Generator
> > > + * All the clocks are initialized by the bootloader, so this driver allows only
> > [...]
> > > +
> > > +/* 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"
> >
> > Please inline all these string #defines to the place they're used.
> The version V21 you mention using define only when the definition is
> used more than once
> https://www.spinics.net/lists/kernel/msg5045826.html
> Should I remove all the string definitions and add the string to the array?

If it's a clk name for a clk registered in this file it should be
inlined. Is that the case for everything besides refclk? And even refclk
could be inlined so that we don't have to jump to the definition of a
string.

> > > +
> > > +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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
> >
> > The parent of this device is not a syscon.
> Once I have registered the map that handles both reset and the clock
> in general is syscon, this is why we will modify the DTS so the clock
> and the reset will be under syscon father node
>                 sysctrl: system-controller@f0801000 {
>                         compatible = "syscon", "simple-mfd";
>                         reg = <0x0 0xf0801000 0x0 0x1000>;
> 
>                         rstc: reset-controller {
>                                 compatible = "nuvoton,npcm845-reset";
>                                 reg = <0x0 0xf0801000 0x0 0xC4>;
>                                 #reset-cells = <2>;
>                                 nuvoton,sysgcr = <&gcr>;
>                         };
> 
>                         clk: clock-controller {
>                                 compatible = "nuvoton,npcm845-clk";
>                                 #clock-cells = <1>;
>                                 clocks = <&refclk>;
>                                 clock-names = "refclk";
>                         };
>                 };
> You can see other drivers that using the same method like
> https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml

You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
then to describe the child nodes.

Socionext may not be the best example to follow. I generally try to
avoid syscon and simply put #reset-cells and #clock-cells in the node
for the device. You can use the auxiliary bus to register drivers for
clk and reset and put them into the resepective driver directories.
Avoid syscon means random drivers can't reach into the device with a
regmap handle and read/write registers that they're not supposed to.

> >
> > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > +       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_node_to_regmap(parent_np);
> > > +       of_node_put(parent_np);
> >
> > Is there another binding update that is going to move this node to be a
> > child of the syscon?
> >
> >                 gcr: system-controller@f0800000 {
> >                         compatible = "nuvoton,npcm845-gcr", "syscon";
> >                         reg = <0x0 0xf0800000 0x0 0x1000>;
> >                 };
> No, sorry but I'm not going to use the GCR node the handle the clock
> and reset modules, the GCR has different memory space.
> the clock driver will have the following device tree

What does the reset driver use the CGR node for? The driver looks like
it's using it to control USB phy resets.

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

This isn't a valid reg property for a child node like this.

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

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

Thanks for your reply.

On Thu, 29 Feb 2024 at 00:48, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2024-02-25 10:00:35)
> > Hi Stephen,
> >
> > On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > > new file mode 100644
> > > > index 000000000000..eacb579d30af
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-npcm8xx.c
> > > > @@ -0,0 +1,509 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Nuvoton NPCM8xx Clock Generator
> > > > + * All the clocks are initialized by the bootloader, so this driver allows only
> > > [...]
> > > > +
> > > > +/* 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"
> > >
> > > Please inline all these string #defines to the place they're used.
> > The version V21 you mention using define only when the definition is
> > used more than once
> > https://www.spinics.net/lists/kernel/msg5045826.html
> > Should I remove all the string definitions and add the string to the array?
>
> If it's a clk name for a clk registered in this file it should be
> inlined. Is that the case for everything besides refclk? And even refclk
> could be inlined so that we don't have to jump to the definition of a
> string.
I will add the string in the clock arrays and remove all the string definitions.
>
> > > > +
> > > > +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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > >
> > > The parent of this device is not a syscon.
> > Once I have registered the map that handles both reset and the clock
> > in general is syscon, this is why we will modify the DTS so the clock
> > and the reset will be under syscon father node
> >                 sysctrl: system-controller@f0801000 {
> >                         compatible = "syscon", "simple-mfd";
> >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> >
> >                         rstc: reset-controller {
> >                                 compatible = "nuvoton,npcm845-reset";
> >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> >                                 #reset-cells = <2>;
> >                                 nuvoton,sysgcr = <&gcr>;
> >                         };
> >
> >                         clk: clock-controller {
> >                                 compatible = "nuvoton,npcm845-clk";
> >                                 #clock-cells = <1>;
> >                                 clocks = <&refclk>;
> >                                 clock-names = "refclk";
> >                         };
> >                 };
> > You can see other drivers that using the same method like
> > https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
>
> You will need a similar file like
> Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
> then to describe the child nodes.
I can do it.
>
> Socionext may not be the best example to follow. I generally try to
> avoid syscon and simply put #reset-cells and #clock-cells in the node
If I remove syscon I can't use syscon_node_to_regmap function, What
should I use If I remove syscon? auxiliary bus? something else?
> for the device. You can use the auxiliary bus to register drivers for
> clk and reset and put them into the resepective driver directories.
I little bit confused, what is an auxiliary bus to register drivers,
can you provide me an example?
> Avoid syscon means random drivers can't reach into the device with a
> regmap handle and read/write registers that they're not supposed to.
Indeed, but the drivers could use the reset and clock memory map only
if the module is also a child node.

Please let me know what is your preferred way to handle it:
1. stick with syscon and upstream-defined documentation for the rst clk syscon.
2. avoid syscon and use an auxiliary bus, appreciate if you could give
me an example of how it should be done.
3. Avoid sycon and handle it differently.
>
> > >
> > > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > > +       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_node_to_regmap(parent_np);
> > > > +       of_node_put(parent_np);
> > >
> > > Is there another binding update that is going to move this node to be a
> > > child of the syscon?
> > >
> > >                 gcr: system-controller@f0800000 {
> > >                         compatible = "nuvoton,npcm845-gcr", "syscon";
> > >                         reg = <0x0 0xf0800000 0x0 0x1000>;
> > >                 };
> > No, sorry but I'm not going to use the GCR node the handle the clock
> > and reset modules, the GCR has different memory space.
> > the clock driver will have the following device tree
>
> What does the reset driver use the CGR node for? The driver looks like
> it's using it to control USB phy resets.
Yes, the USB PHY reset is handled through the GCR registers.
>
> >                sysctrl: system-controller@f0801000 {
> >                         compatible = "syscon", "simple-mfd";
> >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> >
> >                         rstc: reset-controller {
> >                                 compatible = "nuvoton,npcm845-reset";
> >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
>
> This isn't a valid reg property for a child node like this.
O.K.
>
> >                                 #reset-cells = <2>;
> >                                 nuvoton,sysgcr = <&gcr>;
> >                         };
> >
> >                         clk: clock-controller {
> >                                 compatible = "nuvoton,npcm845-clk";
> >                                 #clock-cells = <1>;
> >                                 clocks = <&refclk>;
> >                                 clock-names = "refclk";
> >                         };
> >                 };

Appreciate your guidance!

Thanks,

Tomer

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

Appreciate it if you could reply to my email afew days ago, It is
really important to us to move this driver to upstream.

Thanks,

Tomer

On Thu, 29 Feb 2024 at 23:29, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Hi Stephen,
>
> Thanks for your reply.
>
> On Thu, 29 Feb 2024 at 00:48, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2024-02-25 10:00:35)
> > > Hi Stephen,
> > >
> > > On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > > > new file mode 100644
> > > > > index 000000000000..eacb579d30af
> > > > > --- /dev/null
> > > > > +++ b/drivers/clk/clk-npcm8xx.c
> > > > > @@ -0,0 +1,509 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Nuvoton NPCM8xx Clock Generator
> > > > > + * All the clocks are initialized by the bootloader, so this driver allows only
> > > > [...]
> > > > > +
> > > > > +/* 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"
> > > >
> > > > Please inline all these string #defines to the place they're used.
> > > The version V21 you mention using define only when the definition is
> > > used more than once
> > > https://www.spinics.net/lists/kernel/msg5045826.html
> > > Should I remove all the string definitions and add the string to the array?
> >
> > If it's a clk name for a clk registered in this file it should be
> > inlined. Is that the case for everything besides refclk? And even refclk
> > could be inlined so that we don't have to jump to the definition of a
> > string.
> I will add the string in the clock arrays and remove all the string definitions.
> >
> > > > > +
> > > > > +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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > > >
> > > > The parent of this device is not a syscon.
> > > Once I have registered the map that handles both reset and the clock
> > > in general is syscon, this is why we will modify the DTS so the clock
> > > and the reset will be under syscon father node
> > >                 sysctrl: system-controller@f0801000 {
> > >                         compatible = "syscon", "simple-mfd";
> > >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> > >
> > >                         rstc: reset-controller {
> > >                                 compatible = "nuvoton,npcm845-reset";
> > >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> > >                                 #reset-cells = <2>;
> > >                                 nuvoton,sysgcr = <&gcr>;
> > >                         };
> > >
> > >                         clk: clock-controller {
> > >                                 compatible = "nuvoton,npcm845-clk";
> > >                                 #clock-cells = <1>;
> > >                                 clocks = <&refclk>;
> > >                                 clock-names = "refclk";
> > >                         };
> > >                 };
> > > You can see other drivers that using the same method like
> > > https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
> >
> > You will need a similar file like
> > Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
> > then to describe the child nodes.
> I can do it.
> >
> > Socionext may not be the best example to follow. I generally try to
> > avoid syscon and simply put #reset-cells and #clock-cells in the node
> If I remove syscon I can't use syscon_node_to_regmap function, What
> should I use If I remove syscon? auxiliary bus? something else?
> > for the device. You can use the auxiliary bus to register drivers for
> > clk and reset and put them into the resepective driver directories.
> I little bit confused, what is an auxiliary bus to register drivers,
> can you provide me an example?
> > Avoid syscon means random drivers can't reach into the device with a
> > regmap handle and read/write registers that they're not supposed to.
> Indeed, but the drivers could use the reset and clock memory map only
> if the module is also a child node.
>
> Please let me know what is your preferred way to handle it:
> 1. stick with syscon and upstream-defined documentation for the rst clk syscon.
> 2. avoid syscon and use an auxiliary bus, appreciate if you could give
> me an example of how it should be done.
> 3. Avoid sycon and handle it differently.
> >
> > > >
> > > > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > > > +       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_node_to_regmap(parent_np);
> > > > > +       of_node_put(parent_np);
> > > >
> > > > Is there another binding update that is going to move this node to be a
> > > > child of the syscon?
> > > >
> > > >                 gcr: system-controller@f0800000 {
> > > >                         compatible = "nuvoton,npcm845-gcr", "syscon";
> > > >                         reg = <0x0 0xf0800000 0x0 0x1000>;
> > > >                 };
> > > No, sorry but I'm not going to use the GCR node the handle the clock
> > > and reset modules, the GCR has different memory space.
> > > the clock driver will have the following device tree
> >
> > What does the reset driver use the CGR node for? The driver looks like
> > it's using it to control USB phy resets.
> Yes, the USB PHY reset is handled through the GCR registers.
> >
> > >                sysctrl: system-controller@f0801000 {
> > >                         compatible = "syscon", "simple-mfd";
> > >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> > >
> > >                         rstc: reset-controller {
> > >                                 compatible = "nuvoton,npcm845-reset";
> > >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> >
> > This isn't a valid reg property for a child node like this.
> O.K.
> >
> > >                                 #reset-cells = <2>;
> > >                                 nuvoton,sysgcr = <&gcr>;
> > >                         };
> > >
> > >                         clk: clock-controller {
> > >                                 compatible = "nuvoton,npcm845-clk";
> > >                                 #clock-cells = <1>;
> > >                                 clocks = <&refclk>;
> > >                                 clock-names = "refclk";
> > >                         };
> > >                 };
>
> Appreciate your guidance!
>
> Thanks,
>
> Tomer

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

Kind remainder, appreciate if you can reply about the comment that
been sent few weeks ago.

Thanks,

Tomer

On Tue, 5 Mar 2024 at 17:59, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Hi Stephen,
>
> Appreciate it if you could reply to my email afew days ago, It is
> really important to us to move this driver to upstream.
>
> Thanks,
>
> Tomer
>
> On Thu, 29 Feb 2024 at 23:29, Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Hi Stephen,
> >
> > Thanks for your reply.
> >
> > On Thu, 29 Feb 2024 at 00:48, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Tomer Maimon (2024-02-25 10:00:35)
> > > > Hi Stephen,
> > > >
> > > > On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > > > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..eacb579d30af
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/clk/clk-npcm8xx.c
> > > > > > @@ -0,0 +1,509 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Nuvoton NPCM8xx Clock Generator
> > > > > > + * All the clocks are initialized by the bootloader, so this driver allows only
> > > > > [...]
> > > > > > +
> > > > > > +/* 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"
> > > > >
> > > > > Please inline all these string #defines to the place they're used.
> > > > The version V21 you mention using define only when the definition is
> > > > used more than once
> > > > https://www.spinics.net/lists/kernel/msg5045826.html
> > > > Should I remove all the string definitions and add the string to the array?
> > >
> > > If it's a clk name for a clk registered in this file it should be
> > > inlined. Is that the case for everything besides refclk? And even refclk
> > > could be inlined so that we don't have to jump to the definition of a
> > > string.
> > I will add the string in the clock arrays and remove all the string definitions.
> > >
> > > > > > +
> > > > > > +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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > > > >
> > > > > The parent of this device is not a syscon.
> > > > Once I have registered the map that handles both reset and the clock
> > > > in general is syscon, this is why we will modify the DTS so the clock
> > > > and the reset will be under syscon father node
> > > >                 sysctrl: system-controller@f0801000 {
> > > >                         compatible = "syscon", "simple-mfd";
> > > >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> > > >
> > > >                         rstc: reset-controller {
> > > >                                 compatible = "nuvoton,npcm845-reset";
> > > >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> > > >                                 #reset-cells = <2>;
> > > >                                 nuvoton,sysgcr = <&gcr>;
> > > >                         };
> > > >
> > > >                         clk: clock-controller {
> > > >                                 compatible = "nuvoton,npcm845-clk";
> > > >                                 #clock-cells = <1>;
> > > >                                 clocks = <&refclk>;
> > > >                                 clock-names = "refclk";
> > > >                         };
> > > >                 };
> > > > You can see other drivers that using the same method like
> > > > https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
> > >
> > > You will need a similar file like
> > > Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
> > > then to describe the child nodes.
> > I can do it.
> > >
> > > Socionext may not be the best example to follow. I generally try to
> > > avoid syscon and simply put #reset-cells and #clock-cells in the node
> > If I remove syscon I can't use syscon_node_to_regmap function, What
> > should I use If I remove syscon? auxiliary bus? something else?
> > > for the device. You can use the auxiliary bus to register drivers for
> > > clk and reset and put them into the resepective driver directories.
> > I little bit confused, what is an auxiliary bus to register drivers,
> > can you provide me an example?
> > > Avoid syscon means random drivers can't reach into the device with a
> > > regmap handle and read/write registers that they're not supposed to.
> > Indeed, but the drivers could use the reset and clock memory map only
> > if the module is also a child node.
> >
> > Please let me know what is your preferred way to handle it:
> > 1. stick with syscon and upstream-defined documentation for the rst clk syscon.
> > 2. avoid syscon and use an auxiliary bus, appreciate if you could give
> > me an example of how it should be done.
> > 3. Avoid sycon and handle it differently.
> > >
> > > > >
> > > > > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > > > > +       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_node_to_regmap(parent_np);
> > > > > > +       of_node_put(parent_np);
> > > > >
> > > > > Is there another binding update that is going to move this node to be a
> > > > > child of the syscon?
> > > > >
> > > > >                 gcr: system-controller@f0800000 {
> > > > >                         compatible = "nuvoton,npcm845-gcr", "syscon";
> > > > >                         reg = <0x0 0xf0800000 0x0 0x1000>;
> > > > >                 };
> > > > No, sorry but I'm not going to use the GCR node the handle the clock
> > > > and reset modules, the GCR has different memory space.
> > > > the clock driver will have the following device tree
> > >
> > > What does the reset driver use the CGR node for? The driver looks like
> > > it's using it to control USB phy resets.
> > Yes, the USB PHY reset is handled through the GCR registers.
> > >
> > > >                sysctrl: system-controller@f0801000 {
> > > >                         compatible = "syscon", "simple-mfd";
> > > >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> > > >
> > > >                         rstc: reset-controller {
> > > >                                 compatible = "nuvoton,npcm845-reset";
> > > >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> > >
> > > This isn't a valid reg property for a child node like this.
> > O.K.
> > >
> > > >                                 #reset-cells = <2>;
> > > >                                 nuvoton,sysgcr = <&gcr>;
> > > >                         };
> > > >
> > > >                         clk: clock-controller {
> > > >                                 compatible = "nuvoton,npcm845-clk";
> > > >                                 #clock-cells = <1>;
> > > >                                 clocks = <&refclk>;
> > > >                                 clock-names = "refclk";
> > > >                         };
> > > >                 };
> >
> > Appreciate your guidance!
> >
> > Thanks,
> >
> > Tomer

Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Stephen Boyd]

Quoting Tomer Maimon (2024-02-29 13:29:46)
> Hi Stephen,
> 
> Thanks for your reply.
> 
> On Thu, 29 Feb 2024 at 00:48, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2024-02-25 10:00:35)
> > > Hi Stephen,
> > >
> > > On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > > > +
> > > > > +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 device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > > >
> > > > The parent of this device is not a syscon.
> > > Once I have registered the map that handles both reset and the clock
> > > in general is syscon, this is why we will modify the DTS so the clock
> > > and the reset will be under syscon father node
> > >                 sysctrl: system-controller@f0801000 {
> > >                         compatible = "syscon", "simple-mfd";
> > >                         reg = <0x0 0xf0801000 0x0 0x1000>;
> > >
> > >                         rstc: reset-controller {
> > >                                 compatible = "nuvoton,npcm845-reset";
> > >                                 reg = <0x0 0xf0801000 0x0 0xC4>;
> > >                                 #reset-cells = <2>;
> > >                                 nuvoton,sysgcr = <&gcr>;
> > >                         };
> > >
> > >                         clk: clock-controller {
> > >                                 compatible = "nuvoton,npcm845-clk";
> > >                                 #clock-cells = <1>;
> > >                                 clocks = <&refclk>;
> > >                                 clock-names = "refclk";
> > >                         };
> > >                 };
> > > You can see other drivers that using the same method like
> > > https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
> >
> > You will need a similar file like
> > Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
> > then to describe the child nodes.
> I can do it.
> >
> > Socionext may not be the best example to follow. I generally try to
> > avoid syscon and simply put #reset-cells and #clock-cells in the node
> If I remove syscon I can't use syscon_node_to_regmap function, What
> should I use If I remove syscon? auxiliary bus? something else?

You should use auxiliary bus. You can make a regmap in the parent
driver and pass that to the child auxiliary devices still.

> > for the device. You can use the auxiliary bus to register drivers for
> > clk and reset and put them into the resepective driver directories.
> I little bit confused, what is an auxiliary bus to register drivers,
> can you provide me an example?

$ git grep -l auxiliary_ -- drivers/clk/
drivers/clk/microchip/clk-mpfs.c
drivers/clk/starfive/clk-starfive-jh7110-sys.c

You can decide to make either the clk or the reset driver the "main"
driver that registers the other auxiliary devices. Either way the DT
binding has a single node instead of one per logical driver in the
kernel.

> > Avoid syscon means random drivers can't reach into the device with a
> > regmap handle and read/write registers that they're not supposed to.
> Indeed, but the drivers could use the reset and clock memory map only
> if the module is also a child node.
> 
> Please let me know what is your preferred way to handle it:
> 1. stick with syscon and upstream-defined documentation for the rst clk syscon.
> 2. avoid syscon and use an auxiliary bus, appreciate if you could give
> me an example of how it should be done.
> 3. Avoid sycon and handle it differently.

I prefer 2