[PATCH v11 0/4] Nuvoton WPCM450 clock and reset driver

[PATCH v11 0/4] Nuvoton WPCM450 clock and reset driver

[Jonathan Neuschäfer]

This series adds support for the clock and reset controller in the Nuvoton
WPCM450 SoC. This means that the clock rates for peripherals will be calculated
automatically based on the clock tree as it was preconfigured by the bootloader.
The 24 MHz dummy clock, that is currently in the devicetree, is no longer needed.
Somewhat unfortunately, this also means that there is a breaking change once
the devicetree starts relying on the clock driver, but I find it acceptable in
this case, because WPCM450 is still at a somewhat early stage.

v11:
- Improved description in "ARM: dts: wpcm450: Remove clock-output-names
  from reference clock node"
- some minor format differences due to switching to B4

v10:
- A small tweak (using selected instead of extending an already-long
  default line) in Kconfig, for better robustness

v9:
- Various improvements to the driver
- No longer use global clock names (and the clock-output-names property)
  to refer to the reference clock, but instead rely on a phandle reference

v8:
- https://lore.kernel.org/lkml/20230428190226.1304326-1-j.neuschaefer@gmx.net/
- Use %pe throughout the driver

v7:
- Simplified the error handling, by largely removing resource
  deallocation, which:
  - was already incomplete
  - would only happen in a case when the system is in pretty bad state
    because the clock driver didn't initialize correctly (in other
    words, the clock driver isn't optional enough that complex error
    handling really pays off)

v6:
- Dropped all patches except the clock binding and the clock driver, because
  they have mostly been merged
- Minor correction to how RESET_SIMPLE is selected

v5:
- Dropped patch 2 (watchdog: npcm: Enable clock if provided), which
  was since merged upstream
- Added patch 2 (clocksource: timer-npcm7xx: Enable timer 1 clock before use) again,
  because I wasn't able to find it in linux-next
- Switched the driver to using struct clk_parent_data
- Rebased on 6.1-rc3

v4:
- Leave WDT clock running during after restart handler
- Fix reset controller initialization
- Dropped patch 2/7 (clocksource: timer-npcm7xx: Enable timer 1 clock before use),
  as it was applied by Daniel Lezcano

v3:
- https://lore.kernel.org/lkml/20220508194333.2170161-1-j.neuschaefer@gmx.net/
- Changed "refclk" string to "ref"
- Fixed some dead code in the driver
- Added clk_prepare_enable call to the watchdog restart handler
- Added a few review tags

v2:
- https://lore.kernel.org/lkml/20220429172030.398011-1-j.neuschaefer@gmx.net/
- various small improvements

v1:
- https://lore.kernel.org/lkml/20220422183012.444674-1-j.neuschaefer@gmx.net/

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
Jonathan Neuschäfer (4):
      dt-bindings: clock: Add Nuvoton WPCM450 clock/reset controller
      ARM: dts: wpcm450: Remove clock-output-names from reference clock node
      clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver
      ARM: dts: wpcm450: Switch clocks to clock controller

 .../bindings/clock/nuvoton,wpcm450-clk.yaml        |  65 ++++
 arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi     |  23 +-
 drivers/clk/Makefile                               |   2 +-
 drivers/clk/nuvoton/Kconfig                        |  10 +-
 drivers/clk/nuvoton/Makefile                       |   1 +
 drivers/clk/nuvoton/clk-wpcm450.c                  | 372 +++++++++++++++++++++
 include/dt-bindings/clock/nuvoton,wpcm450-clk.h    |  67 ++++
 7 files changed, 525 insertions(+), 15 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240330-wpcm-clk-222a37f59cfb

Best regards,
--
Jonathan Neuschäfer <j.neuschaefer@gmx.net>

[PATCH v11 1/4] dt-bindings: clock: Add Nuvoton WPCM450 clock/reset controller

[Jonathan Neuschäfer]

The Nuvoton WPCM450 SoC has a combined clock and reset controller.
Add a devicetree binding for it, as well as definitions for the bit
numbers used by it.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v10-v11:
- no changes

v9:
- Remove clock-output-names in example, because it's now unnecessary due
  to driver improvements

v5-v8:
- no changes

v4:
- https://lore.kernel.org/lkml/20220610072141.347795-4-j.neuschaefer@gmx.net/
- Add R-b tag

v3:
- Change clock-output-names and clock-names from "refclk" to "ref", suggested
  by Krzysztof Kozlowski

v2:
- https://lore.kernel.org/lkml/20220429172030.398011-5-j.neuschaefer@gmx.net/
- Various improvements, suggested by Krzysztof Kozlowski

v1:
- https://lore.kernel.org/lkml/20220422183012.444674-5-j.neuschaefer@gmx.net/
---
 .../bindings/clock/nuvoton,wpcm450-clk.yaml        | 65 +++++++++++++++++++++
 include/dt-bindings/clock/nuvoton,wpcm450-clk.h    | 67 ++++++++++++++++++++++
 2 files changed, 132 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
new file mode 100644
index 00000000000000..93521cf68a040f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nuvoton,wpcm450-clk.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,wpcm450-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton WPCM450 clock controller
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description:
+  The clock controller of the Nuvoton WPCM450 SoC supplies clocks and resets to
+  the rest of the chip.
+
+properties:
+  compatible:
+    const: nuvoton,wpcm450-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Reference clock oscillator (should be 48 MHz)
+
+  clock-names:
+    items:
+      - const: ref
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    const: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    refclk: clock-48mhz {
+      /* 48 MHz reference oscillator */
+      compatible = "fixed-clock";
+      clock-frequency = <48000000>;
+      #clock-cells = <0>;
+    };
+
+    clk: clock-controller@b0000200 {
+      reg = <0xb0000200 0x100>;
+      compatible = "nuvoton,wpcm450-clk";
+      clocks = <&refclk>;
+      clock-names = "ref";
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/nuvoton,wpcm450-clk.h b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
new file mode 100644
index 00000000000000..86e1c895921b71
--- /dev/null
+++ b/include/dt-bindings/clock/nuvoton,wpcm450-clk.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
+#define _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H
+
+/* Clocks based on CLKEN bits */
+#define WPCM450_CLK_FIU            0
+#define WPCM450_CLK_XBUS           1
+#define WPCM450_CLK_KCS            2
+#define WPCM450_CLK_SHM            4
+#define WPCM450_CLK_USB1           5
+#define WPCM450_CLK_EMC0           6
+#define WPCM450_CLK_EMC1           7
+#define WPCM450_CLK_USB0           8
+#define WPCM450_CLK_PECI           9
+#define WPCM450_CLK_AES           10
+#define WPCM450_CLK_UART0         11
+#define WPCM450_CLK_UART1         12
+#define WPCM450_CLK_SMB2          13
+#define WPCM450_CLK_SMB3          14
+#define WPCM450_CLK_SMB4          15
+#define WPCM450_CLK_SMB5          16
+#define WPCM450_CLK_HUART         17
+#define WPCM450_CLK_PWM           18
+#define WPCM450_CLK_TIMER0        19
+#define WPCM450_CLK_TIMER1        20
+#define WPCM450_CLK_TIMER2        21
+#define WPCM450_CLK_TIMER3        22
+#define WPCM450_CLK_TIMER4        23
+#define WPCM450_CLK_MFT0          24
+#define WPCM450_CLK_MFT1          25
+#define WPCM450_CLK_WDT           26
+#define WPCM450_CLK_ADC           27
+#define WPCM450_CLK_SDIO          28
+#define WPCM450_CLK_SSPI          29
+#define WPCM450_CLK_SMB0          30
+#define WPCM450_CLK_SMB1          31
+
+/* Other clocks */
+#define WPCM450_CLK_USBPHY        32
+
+#define WPCM450_NUM_CLKS          33
+
+/* Resets based on IPSRST bits */
+#define WPCM450_RESET_FIU          0
+#define WPCM450_RESET_EMC0         6
+#define WPCM450_RESET_EMC1         7
+#define WPCM450_RESET_USB0         8
+#define WPCM450_RESET_USB1         9
+#define WPCM450_RESET_AES_PECI    10
+#define WPCM450_RESET_UART        11
+#define WPCM450_RESET_MC          12
+#define WPCM450_RESET_SMB2        13
+#define WPCM450_RESET_SMB3        14
+#define WPCM450_RESET_SMB4        15
+#define WPCM450_RESET_SMB5        16
+#define WPCM450_RESET_PWM         18
+#define WPCM450_RESET_TIMER       19
+#define WPCM450_RESET_ADC         27
+#define WPCM450_RESET_SDIO        28
+#define WPCM450_RESET_SSPI        29
+#define WPCM450_RESET_SMB0        30
+#define WPCM450_RESET_SMB1        31
+
+#define WPCM450_NUM_RESETS        32
+
+#endif /* _DT_BINDINGS_CLOCK_NUVOTON_WPCM450_CLK_H */

--
2.43.0

[PATCH v11 2/4] ARM: dts: wpcm450: Remove clock-output-names from reference clock node

[Jonathan Neuschäfer]

A previous version of the as-yet unmerged clk-wpcm450 driver[1] used the
output name, but in the current iteration it doesn't rely on it anymore.
Thus it can be removed from the devicetree.

[1]: Added in "clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver"

Fixes: 362e8be2ec04a6 ("ARM: dts: wpcm450: Add clock controller node")
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v11:
- Specify since when clock-output-names is unnecessary

v10:
- no changes

v9:
- New patch
---
 arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
index 6e1f0f164cb4f5..9dfdd8f67319d3 100644
--- a/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
+++ b/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
@@ -40,7 +40,6 @@ clk24m: clock-24mhz {
 	refclk: clock-48mhz {
 		/* 48 MHz reference oscillator */
 		compatible = "fixed-clock";
-		clock-output-names = "ref";
 		clock-frequency = <48000000>;
 		#clock-cells = <0>;
 	};

--
2.43.0

[PATCH v11 3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver

[Jonathan Neuschäfer]

This driver implements the following features w.r.t. the clock and reset
controller in the WPCM450 SoC:

- It calculates the rates for all clocks managed by the clock controller
- It leaves the clock tree mostly unchanged, except that it enables/
  disables clock gates based on usage.
- It exposes the reset lines managed by the controller using the
  Generic Reset Controller subsystem

NOTE: If the driver and the corresponding devicetree node are present,
      the driver will disable "unused" clocks. This is problem until
      the clock relations are properly declared in the devicetree (in a
      later patch). Until then, the clk_ignore_unused kernel parameter
      can be used as a workaround.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---

I have considered converting this driver to a platform driver instead of
using CLK_OF_DECLARE, because platform drivers are generally the way
forward. However, the timer-npcm7xx driver used on the same platform
requires is initialized with TIMER_OF_DECLARE and thus requires the
clocks to be available earlier than a platform driver can provide them.

v11:
- no changes

v10:
- select RESET_{CONTROLLER,SIMPLE} from CLK_WPCM450 instead of messing with the 'default' statement

v9:
- Apply comments made by Stephen Boyd
- Move to drivers/clk/nuvoton/ directory
- Update SPDX license identifier from GPL-2.0 to GPL-2.0-only
- Rename clk_np variable to np
- Use of_clk_hw_register
- Refer to clock parents by .fw_name

v8:
- https://lore.kernel.org/lkml/20230428190226.1304326-3-j.neuschaefer@gmx.net/
- Use %pe format specifier throughout the driver, as suggested by Philipp Zabel
- Add Joel's R-b

v7:
- https://lore.kernel.org/lkml/20230422220240.322572-3-j.neuschaefer@gmx.net/
- Simplify error handling by not deallocating resources

v6:
- Enable RESET_SIMPLE based on ARCH_WPCM450, not ARCH_NPCM, as suggested by Tomer Maimon

v5:
- https://lore.kernel.org/lkml/20221104161850.2889894-6-j.neuschaefer@gmx.net/
- Switch to using clk_parent_data

v4:
- Fix reset controller initialization

v3:
- Change reference clock name from "refclk" to "ref"
- Remove unused variable in return path of wpcm450_clk_register_pll
- Remove unused divisor tables

v2:
- no changes
---
 drivers/clk/Makefile              |   2 +-
 drivers/clk/nuvoton/Kconfig       |  10 +-
 drivers/clk/nuvoton/Makefile      |   1 +
 drivers/clk/nuvoton/clk-wpcm450.c | 372 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 383 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 14fa8d4ecc1fbe..cdeb2ecf3a8e99 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -107,7 +107,7 @@ endif
 obj-y					+= mstar/
 obj-y					+= mvebu/
 obj-$(CONFIG_ARCH_MXS)			+= mxs/
-obj-$(CONFIG_ARCH_MA35)			+= nuvoton/
+obj-y					+= nuvoton/
 obj-$(CONFIG_COMMON_CLK_NXP)		+= nxp/
 obj-$(CONFIG_COMMON_CLK_PISTACHIO)	+= pistachio/
 obj-$(CONFIG_COMMON_CLK_PXA)		+= pxa/
diff --git a/drivers/clk/nuvoton/Kconfig b/drivers/clk/nuvoton/Kconfig
index fe4b7f62f46704..908881654b2e91 100644
--- a/drivers/clk/nuvoton/Kconfig
+++ b/drivers/clk/nuvoton/Kconfig
@@ -3,7 +3,7 @@

 config COMMON_CLK_NUVOTON
 	bool "Nuvoton clock controller common support"
-	depends on ARCH_MA35 || COMPILE_TEST
+	depends on ARCH_MA35 || ARCH_NPCM || COMPILE_TEST
 	default y
 	help
 	  Say y here to enable common clock controller for Nuvoton platforms.
@@ -16,4 +16,12 @@ config CLK_MA35D1
 	help
 	  Build the clock controller driver for MA35D1 SoC.

+config CLK_WPCM450
+	bool "Nuvoton WPCM450 clock/reset controller support"
+	default y
+	select RESET_CONTROLLER
+	select RESET_SIMPLE
+	help
+	  Build the clock and reset controller driver for the WPCM450 SoC.
+
 endif
diff --git a/drivers/clk/nuvoton/Makefile b/drivers/clk/nuvoton/Makefile
index c3c59dd9f2aaab..b130f0d3889ca0 100644
--- a/drivers/clk/nuvoton/Makefile
+++ b/drivers/clk/nuvoton/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_CLK_MA35D1) += clk-ma35d1.o
 obj-$(CONFIG_CLK_MA35D1) += clk-ma35d1-divider.o
 obj-$(CONFIG_CLK_MA35D1) += clk-ma35d1-pll.o
+obj-$(CONFIG_CLK_WPCM450) += clk-wpcm450.o
diff --git a/drivers/clk/nuvoton/clk-wpcm450.c b/drivers/clk/nuvoton/clk-wpcm450.c
new file mode 100644
index 00000000000000..9100c4b8a56483
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-wpcm450.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Nuvoton WPCM450 clock and reset controller driver.
+ *
+ * Copyright (C) 2022 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/reset-controller.h>
+#include <linux/reset/reset-simple.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
+
+struct wpcm450_clk_pll {
+	struct clk_hw hw;
+	void __iomem *pllcon;
+	u8 flags;
+};
+
+#define to_wpcm450_clk_pll(_hw) container_of(_hw, struct wpcm450_clk_pll, hw)
+
+#define PLLCON_FBDV	GENMASK(24, 16)
+#define PLLCON_PRST	BIT(13)
+#define PLLCON_PWDEN	BIT(12)
+#define PLLCON_OTDV	GENMASK(10, 8)
+#define PLLCON_INDV	GENMASK(5, 0)
+
+static unsigned long wpcm450_clk_pll_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct wpcm450_clk_pll *pll = to_wpcm450_clk_pll(hw);
+	unsigned long fbdv, indv, otdv;
+	u64 rate;
+	u32 pllcon;
+
+	if (parent_rate == 0)
+		return 0;
+
+	pllcon = readl_relaxed(pll->pllcon);
+
+	indv = FIELD_GET(PLLCON_INDV, pllcon) + 1;
+	fbdv = FIELD_GET(PLLCON_FBDV, pllcon) + 1;
+	otdv = FIELD_GET(PLLCON_OTDV, pllcon) + 1;
+
+	rate = (u64)parent_rate * fbdv;
+	do_div(rate, indv * otdv);
+
+	return rate;
+}
+
+static int wpcm450_clk_pll_is_enabled(struct clk_hw *hw)
+{
+	struct wpcm450_clk_pll *pll = to_wpcm450_clk_pll(hw);
+	u32 pllcon;
+
+	pllcon = readl_relaxed(pll->pllcon);
+
+	return !(pllcon & PLLCON_PRST);
+}
+
+static void wpcm450_clk_pll_disable(struct clk_hw *hw)
+{
+	struct wpcm450_clk_pll *pll = to_wpcm450_clk_pll(hw);
+	u32 pllcon;
+
+	pllcon = readl_relaxed(pll->pllcon);
+	pllcon |= PLLCON_PRST | PLLCON_PWDEN;
+	writel(pllcon, pll->pllcon);
+}
+
+static const struct clk_ops wpcm450_clk_pll_ops = {
+	.recalc_rate = wpcm450_clk_pll_recalc_rate,
+	.is_enabled = wpcm450_clk_pll_is_enabled,
+	.disable = wpcm450_clk_pll_disable
+};
+
+static struct clk_hw *
+wpcm450_clk_register_pll(struct device_node *np, void __iomem *pllcon, const char *name,
+			 const struct clk_parent_data *parent, unsigned long flags)
+{
+	struct wpcm450_clk_pll *pll;
+	struct clk_init_data init = {};
+	int ret;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &wpcm450_clk_pll_ops;
+	init.parent_data = parent;
+	init.num_parents = 1;
+	init.flags = flags;
+
+	pll->pllcon = pllcon;
+	pll->hw.init = &init;
+
+	ret = of_clk_hw_register(np, &pll->hw);
+	if (ret) {
+		kfree(pll);
+		return ERR_PTR(ret);
+	}
+
+	return &pll->hw;
+}
+
+#define REG_CLKEN	0x00
+#define REG_CLKSEL	0x04
+#define REG_CLKDIV	0x08
+#define REG_PLLCON0	0x0c
+#define REG_PLLCON1	0x10
+#define REG_PMCON	0x14
+#define REG_IRQWAKECON	0x18
+#define REG_IRQWAKEFLAG	0x1c
+#define REG_IPSRST	0x20
+
+struct wpcm450_pll_data {
+	const char *name;
+	struct clk_parent_data parent;
+	unsigned int reg;
+	unsigned long flags;
+};
+
+static const struct wpcm450_pll_data pll_data[] = {
+	{ "pll0", { .fw_name = "ref" }, REG_PLLCON0, 0 },
+	{ "pll1", { .fw_name = "ref" }, REG_PLLCON1, 0 },
+};
+
+struct wpcm450_clksel_data {
+	const char *name;
+	const struct clk_parent_data *parents;
+	unsigned int num_parents;
+	const u32 *table;
+	int shift;
+	int width;
+	int index;
+	unsigned long flags;
+};
+
+static const u32 parent_table[] = { 0, 1, 2 };
+
+static const struct clk_parent_data default_parents[] = {
+	{ .name = "pll0" },
+	{ .name = "pll1" },
+	{ .name = "ref" },
+};
+
+static const struct clk_parent_data huart_parents[] = {
+	{ .fw_name = "ref" },
+	{ .name = "refdiv2" },
+};
+
+static const struct wpcm450_clksel_data clksel_data[] = {
+	{ "cpusel", default_parents, ARRAY_SIZE(default_parents),
+		parent_table, 0, 2, -1, CLK_IS_CRITICAL },
+	{ "clkout", default_parents, ARRAY_SIZE(default_parents),
+		parent_table, 2, 2, -1, 0 },
+	{ "usbphy", default_parents, ARRAY_SIZE(default_parents),
+		parent_table, 6, 2, -1, 0 },
+	{ "uartsel", default_parents, ARRAY_SIZE(default_parents),
+		parent_table, 8, 2, WPCM450_CLK_USBPHY, 0 },
+	{ "huartsel", huart_parents, ARRAY_SIZE(huart_parents),
+		parent_table, 10, 1, -1, 0 },
+};
+
+static const struct clk_div_table div_fixed2[] = {
+	{ .val = 0, .div = 2 },
+	{ }
+};
+
+struct wpcm450_clkdiv_data {
+	const char *name;
+	struct clk_parent_data parent;
+	int div_flags;
+	const struct clk_div_table *table;
+	int shift;
+	int width;
+	unsigned long flags;
+};
+
+static struct wpcm450_clkdiv_data clkdiv_data_early[] = {
+	{ "refdiv2", { .name = "ref" }, 0, div_fixed2, 0, 0 },
+};
+
+static const struct wpcm450_clkdiv_data clkdiv_data[] = {
+	{ "cpu", { .name = "cpusel" }, 0, div_fixed2, 0, 0, CLK_IS_CRITICAL },
+	{ "adcdiv", { .name = "ref" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 28, 2, 0 },
+	{ "apb", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 26, 2, 0 },
+	{ "ahb", { .name = "cpu" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 24, 2, 0 },
+	{ "uart", { .name = "uartsel" }, 0, NULL, 16, 4, 0 },
+	{ "ahb3", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 8, 2, 0 },
+};
+
+struct wpcm450_clken_data {
+	const char *name;
+	struct clk_parent_data parent;
+	int bitnum;
+	unsigned long flags;
+};
+
+static const struct wpcm450_clken_data clken_data[] = {
+	{ "fiu", { .name = "ahb3" }, WPCM450_CLK_FIU, 0 },
+	{ "xbus", { .name = "ahb3" }, WPCM450_CLK_XBUS, 0 },
+	{ "kcs", { .name = "apb" }, WPCM450_CLK_KCS, 0 },
+	{ "shm", { .name = "ahb3" }, WPCM450_CLK_SHM, 0 },
+	{ "usb1", { .name = "ahb" }, WPCM450_CLK_USB1, 0 },
+	{ "emc0", { .name = "ahb" }, WPCM450_CLK_EMC0, 0 },
+	{ "emc1", { .name = "ahb" }, WPCM450_CLK_EMC1, 0 },
+	{ "usb0", { .name = "ahb" }, WPCM450_CLK_USB0, 0 },
+	{ "peci", { .name = "apb" }, WPCM450_CLK_PECI, 0 },
+	{ "aes", { .name = "apb" }, WPCM450_CLK_AES, 0 },
+	{ "uart0", { .name = "uart" }, WPCM450_CLK_UART0, 0 },
+	{ "uart1", { .name = "uart" }, WPCM450_CLK_UART1, 0 },
+	{ "smb2", { .name = "apb" }, WPCM450_CLK_SMB2, 0 },
+	{ "smb3", { .name = "apb" }, WPCM450_CLK_SMB3, 0 },
+	{ "smb4", { .name = "apb" }, WPCM450_CLK_SMB4, 0 },
+	{ "smb5", { .name = "apb" }, WPCM450_CLK_SMB5, 0 },
+	{ "huart", { .name = "huartsel" }, WPCM450_CLK_HUART, 0 },
+	{ "pwm", { .name = "apb" }, WPCM450_CLK_PWM, 0 },
+	{ "timer0", { .name = "refdiv2" }, WPCM450_CLK_TIMER0, 0 },
+	{ "timer1", { .name = "refdiv2" }, WPCM450_CLK_TIMER1, 0 },
+	{ "timer2", { .name = "refdiv2" }, WPCM450_CLK_TIMER2, 0 },
+	{ "timer3", { .name = "refdiv2" }, WPCM450_CLK_TIMER3, 0 },
+	{ "timer4", { .name = "refdiv2" }, WPCM450_CLK_TIMER4, 0 },
+	{ "mft0", { .name = "apb" }, WPCM450_CLK_MFT0, 0 },
+	{ "mft1", { .name = "apb" }, WPCM450_CLK_MFT1, 0 },
+	{ "wdt", { .name = "refdiv2" }, WPCM450_CLK_WDT, 0 },
+	{ "adc", { .name = "adcdiv" }, WPCM450_CLK_ADC, 0 },
+	{ "sdio", { .name = "ahb" }, WPCM450_CLK_SDIO, 0 },
+	{ "sspi", { .name = "apb" }, WPCM450_CLK_SSPI, 0 },
+	{ "smb0", { .name = "apb" }, WPCM450_CLK_SMB0, 0 },
+	{ "smb1", { .name = "apb" }, WPCM450_CLK_SMB1, 0 },
+};
+
+static DEFINE_SPINLOCK(wpcm450_clk_lock);
+
+/*
+ * NOTE: Error handling is very rudimentary here. If the clock driver initial-
+ * ization fails, the system is probably in bigger trouble than what is caused
+ * by a few leaked resources.
+ */
+
+static void __init wpcm450_clk_init(struct device_node *np)
+{
+	struct clk_hw_onecell_data *clk_data;
+	static struct clk_hw **hws;
+	static struct clk_hw *hw;
+	void __iomem *clk_base;
+	int i, ret;
+	struct reset_simple_data *reset;
+
+	clk_base = of_iomap(np, 0);
+	if (!clk_base) {
+		pr_err("%pOFP: failed to map registers\n", np);
+		of_node_put(np);
+		return;
+	}
+	of_node_put(np);
+
+	clk_data = kzalloc(struct_size(clk_data, hws, WPCM450_NUM_CLKS), GFP_KERNEL);
+	if (!clk_data)
+		return;
+
+	clk_data->num = WPCM450_NUM_CLKS;
+	hws = clk_data->hws;
+
+	for (i = 0; i < WPCM450_NUM_CLKS; i++)
+		hws[i] = ERR_PTR(-ENOENT);
+
+	/* PLLs */
+	for (i = 0; i < ARRAY_SIZE(pll_data); i++) {
+		const struct wpcm450_pll_data *data = &pll_data[i];
+
+		hw = wpcm450_clk_register_pll(np, clk_base + data->reg, data->name,
+					      &data->parent, data->flags);
+		if (IS_ERR(hw)) {
+			pr_info("Failed to register PLL: %pe\n", hw);
+			return;
+		}
+	}
+
+	/* Early divisors (REF/2) */
+	for (i = 0; i < ARRAY_SIZE(clkdiv_data_early); i++) {
+		const struct wpcm450_clkdiv_data *data = &clkdiv_data_early[i];
+
+		hw = clk_hw_register_divider_table_parent_data(NULL, data->name, &data->parent,
+							       data->flags, clk_base + REG_CLKDIV,
+							       data->shift, data->width,
+							       data->div_flags, data->table,
+							       &wpcm450_clk_lock);
+		if (IS_ERR(hw)) {
+			pr_err("Failed to register div table: %pe\n", hw);
+			return;
+		}
+	}
+
+	/* Selects/muxes */
+	for (i = 0; i < ARRAY_SIZE(clksel_data); i++) {
+		const struct wpcm450_clksel_data *data = &clksel_data[i];
+
+		hw = clk_hw_register_mux_parent_data(NULL, data->name, data->parents,
+						     data->num_parents, data->flags,
+						     clk_base + REG_CLKSEL, data->shift,
+						     data->width, 0,
+						     &wpcm450_clk_lock);
+		if (IS_ERR(hw)) {
+			pr_err("Failed to register mux: %pe\n", hw);
+			return;
+		}
+		if (data->index >= 0)
+			clk_data->hws[data->index] = hw;
+	}
+
+	/* Divisors */
+	for (i = 0; i < ARRAY_SIZE(clkdiv_data); i++) {
+		const struct wpcm450_clkdiv_data *data = &clkdiv_data[i];
+
+		hw = clk_hw_register_divider_table_parent_data(NULL, data->name, &data->parent,
+							       data->flags, clk_base + REG_CLKDIV,
+							       data->shift, data->width,
+							       data->div_flags, data->table,
+							       &wpcm450_clk_lock);
+		if (IS_ERR(hw)) {
+			pr_err("Failed to register divider: %pe\n", hw);
+			return;
+		}
+	}
+
+	/* Enables/gates */
+	for (i = 0; i < ARRAY_SIZE(clken_data); i++) {
+		const struct wpcm450_clken_data *data = &clken_data[i];
+
+		hw = clk_hw_register_gate_parent_data(NULL, data->name, &data->parent, data->flags,
+						      clk_base + REG_CLKEN, data->bitnum,
+						      data->flags, &wpcm450_clk_lock);
+		if (IS_ERR(hw)) {
+			pr_err("Failed to register gate: %pe\n", hw);
+			return;
+		}
+		clk_data->hws[data->bitnum] = hw;
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
+	if (ret)
+		pr_err("Failed to add DT provider: %pe\n", ERR_PTR(ret));
+
+	/* Reset controller */
+	reset = kzalloc(sizeof(*reset), GFP_KERNEL);
+	if (!reset)
+		return;
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.nr_resets = WPCM450_NUM_RESETS;
+	reset->rcdev.ops = &reset_simple_ops;
+	reset->rcdev.of_node = np;
+	reset->membase = clk_base + REG_IPSRST;
+	ret = reset_controller_register(&reset->rcdev);
+	if (ret)
+		pr_err("Failed to register reset controller: %pe\n", ERR_PTR(ret));
+
+	of_node_put(np);
+}
+
+CLK_OF_DECLARE(wpcm450_clk_init, "nuvoton,wpcm450-clk", wpcm450_clk_init);

--
2.43.0

Re: [PATCH v11 3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver

[Stephen Boyd]

Quoting Jonathan Neuschäfer (2024-04-01 07:06:32)
> This driver implements the following features w.r.t. the clock and reset
> controller in the WPCM450 SoC:
> 
> - It calculates the rates for all clocks managed by the clock controller
> - It leaves the clock tree mostly unchanged, except that it enables/
>   disables clock gates based on usage.
> - It exposes the reset lines managed by the controller using the
>   Generic Reset Controller subsystem
> 
> NOTE: If the driver and the corresponding devicetree node are present,
>       the driver will disable "unused" clocks. This is problem until
>       the clock relations are properly declared in the devicetree (in a
>       later patch). Until then, the clk_ignore_unused kernel parameter
>       can be used as a workaround.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> 
> I have considered converting this driver to a platform driver instead of
> using CLK_OF_DECLARE, because platform drivers are generally the way
> forward. However, the timer-npcm7xx driver used on the same platform
> requires is initialized with TIMER_OF_DECLARE and thus requires the
> clocks to be available earlier than a platform driver can provide them.

In that case you can use CLK_OF_DECLARE_DRIVER(), register the clks
needed for the timer driver to probe, and then put the rest of the clk
registration in a normal platform driver.

> diff --git a/drivers/clk/nuvoton/clk-wpcm450.c b/drivers/clk/nuvoton/clk-wpcm450.c
> new file mode 100644
> index 00000000000000..9100c4b8a56483
> --- /dev/null
> +++ b/drivers/clk/nuvoton/clk-wpcm450.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Nuvoton WPCM450 clock and reset controller driver.
> + *
> + * Copyright (C) 2022 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Isn't KBUILD_MODNAME an option already for dynamic debug?

> +
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset/reset-simple.h>
> +#include <linux/slab.h>
> +
[...]
> +
> +static const struct clk_parent_data default_parents[] = {
> +       { .name = "pll0" },
> +       { .name = "pll1" },
> +       { .name = "ref" },
> +};
> +
> +static const struct clk_parent_data huart_parents[] = {
> +       { .fw_name = "ref" },
> +       { .name = "refdiv2" },

Please remove all .name elements and use indexes or direct pointers.

> +};
> +
> +static const struct wpcm450_clksel_data clksel_data[] = {
> +       { "cpusel", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 0, 2, -1, CLK_IS_CRITICAL },
> +       { "clkout", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 2, 2, -1, 0 },
> +       { "usbphy", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 6, 2, -1, 0 },
> +       { "uartsel", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 8, 2, WPCM450_CLK_USBPHY, 0 },
> +       { "huartsel", huart_parents, ARRAY_SIZE(huart_parents),
> +               parent_table, 10, 1, -1, 0 },
> +};
> +
> +static const struct clk_div_table div_fixed2[] = {
> +       { .val = 0, .div = 2 },
> +       { }
> +};
> +
> +struct wpcm450_clkdiv_data {
> +       const char *name;
> +       struct clk_parent_data parent;
> +       int div_flags;
> +       const struct clk_div_table *table;
> +       int shift;
> +       int width;
> +       unsigned long flags;
> +};
> +
> +static struct wpcm450_clkdiv_data clkdiv_data_early[] = {
> +       { "refdiv2", { .name = "ref" }, 0, div_fixed2, 0, 0 },
> +};
> +
> +static const struct wpcm450_clkdiv_data clkdiv_data[] = {
> +       { "cpu", { .name = "cpusel" }, 0, div_fixed2, 0, 0, CLK_IS_CRITICAL },
> +       { "adcdiv", { .name = "ref" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 28, 2, 0 },
> +       { "apb", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 26, 2, 0 },
> +       { "ahb", { .name = "cpu" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 24, 2, 0 },
> +       { "uart", { .name = "uartsel" }, 0, NULL, 16, 4, 0 },
> +       { "ahb3", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 8, 2, 0 },
> +};
> +
> +struct wpcm450_clken_data {
> +       const char *name;
> +       struct clk_parent_data parent;
> +       int bitnum;
> +       unsigned long flags;
> +};
> +
> +static const struct wpcm450_clken_data clken_data[] = {
> +       { "fiu", { .name = "ahb3" }, WPCM450_CLK_FIU, 0 },

This actually is  { .index = 0, .name = "ahb3" } and that is a bad
combination. struct clk_parent_data should only have .name as a fallback
when there's an old binding out there that doesn't have the 'clocks'
property for the clk provider node. There shouldn't be any .name
property because this is new code and a new binding.

> +       { "xbus", { .name = "ahb3" }, WPCM450_CLK_XBUS, 0 },
> +       { "kcs", { .name = "apb" }, WPCM450_CLK_KCS, 0 },
> +       { "shm", { .name = "ahb3" }, WPCM450_CLK_SHM, 0 },
> +       { "usb1", { .name = "ahb" }, WPCM450_CLK_USB1, 0 },
> +       { "emc0", { .name = "ahb" }, WPCM450_CLK_EMC0, 0 },
> +       { "emc1", { .name = "ahb" }, WPCM450_CLK_EMC1, 0 },
> +       { "usb0", { .name = "ahb" }, WPCM450_CLK_USB0, 0 },
> +       { "peci", { .name = "apb" }, WPCM450_CLK_PECI, 0 },
> +       { "aes", { .name = "apb" }, WPCM450_CLK_AES, 0 },
> +       { "uart0", { .name = "uart" }, WPCM450_CLK_UART0, 0 },
> +       { "uart1", { .name = "uart" }, WPCM450_CLK_UART1, 0 },
> +       { "smb2", { .name = "apb" }, WPCM450_CLK_SMB2, 0 },
> +       { "smb3", { .name = "apb" }, WPCM450_CLK_SMB3, 0 },
> +       { "smb4", { .name = "apb" }, WPCM450_CLK_SMB4, 0 },
> +       { "smb5", { .name = "apb" }, WPCM450_CLK_SMB5, 0 },
> +       { "huart", { .name = "huartsel" }, WPCM450_CLK_HUART, 0 },
> +       { "pwm", { .name = "apb" }, WPCM450_CLK_PWM, 0 },
> +       { "timer0", { .name = "refdiv2" }, WPCM450_CLK_TIMER0, 0 },
> +       { "timer1", { .name = "refdiv2" }, WPCM450_CLK_TIMER1, 0 },
> +       { "timer2", { .name = "refdiv2" }, WPCM450_CLK_TIMER2, 0 },
> +       { "timer3", { .name = "refdiv2" }, WPCM450_CLK_TIMER3, 0 },
> +       { "timer4", { .name = "refdiv2" }, WPCM450_CLK_TIMER4, 0 },
> +       { "mft0", { .name = "apb" }, WPCM450_CLK_MFT0, 0 },
> +       { "mft1", { .name = "apb" }, WPCM450_CLK_MFT1, 0 },
> +       { "wdt", { .name = "refdiv2" }, WPCM450_CLK_WDT, 0 },
> +       { "adc", { .name = "adcdiv" }, WPCM450_CLK_ADC, 0 },
> +       { "sdio", { .name = "ahb" }, WPCM450_CLK_SDIO, 0 },
> +       { "sspi", { .name = "apb" }, WPCM450_CLK_SSPI, 0 },
> +       { "smb0", { .name = "apb" }, WPCM450_CLK_SMB0, 0 },
> +       { "smb1", { .name = "apb" }, WPCM450_CLK_SMB1, 0 },
> +};
> +
> +static DEFINE_SPINLOCK(wpcm450_clk_lock);
> +
> +/*
> + * NOTE: Error handling is very rudimentary here. If the clock driver initial-
> + * ization fails, the system is probably in bigger trouble than what is caused

Don't break words across lines with hyphens.

> + * by a few leaked resources.
> + */
> +
> +static void __init wpcm450_clk_init(struct device_node *np)
> +{
> +       struct clk_hw_onecell_data *clk_data;
> +       static struct clk_hw **hws;
> +       static struct clk_hw *hw;
> +       void __iomem *clk_base;
> +       int i, ret;
> +       struct reset_simple_data *reset;
> +
> +       clk_base = of_iomap(np, 0);
> +       if (!clk_base) {
> +               pr_err("%pOFP: failed to map registers\n", np);
> +               of_node_put(np);
> +               return;
> +       }
> +       of_node_put(np);

The 'np' is used later when registering PLLs. You can only put the node
after it's no longer used. Also, you never got the node with
of_node_get(), so putting it here actually causes an underflow on the
refcount. Just remove all the get/puts instead.

> +
> +       clk_data = kzalloc(struct_size(clk_data, hws, WPCM450_NUM_CLKS), GFP_KERNEL);
> +       if (!clk_data)
> +               return;
> +
> +       clk_data->num = WPCM450_NUM_CLKS;
[...]
> +       /* Reset controller */
> +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> +       if (!reset)
> +               return;
> +       reset->rcdev.owner = THIS_MODULE;
> +       reset->rcdev.nr_resets = WPCM450_NUM_RESETS;
> +       reset->rcdev.ops = &reset_simple_ops;
> +       reset->rcdev.of_node = np;
> +       reset->membase = clk_base + REG_IPSRST;
> +       ret = reset_controller_register(&reset->rcdev);
> +       if (ret)
> +               pr_err("Failed to register reset controller: %pe\n", ERR_PTR(ret));

It would be nicer to register this device as an auxiliary device with a
single API call and then have all the resets exist in that file
instead of this file. The driver would be put in drivers/reset/ as well.

> +
> +       of_node_put(np);

Drop this of_node_put()

Re: [PATCH v11 3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 7732 bytes --]

Hi,

On Fri, Apr 12, 2024 at 12:25:05AM -0700, Stephen Boyd wrote:
> Quoting Jonathan Neuschäfer (2024-04-01 07:06:32)
> > This driver implements the following features w.r.t. the clock and reset
> > controller in the WPCM450 SoC:
> > 
> > - It calculates the rates for all clocks managed by the clock controller
> > - It leaves the clock tree mostly unchanged, except that it enables/
> >   disables clock gates based on usage.
> > - It exposes the reset lines managed by the controller using the
> >   Generic Reset Controller subsystem
> > 
> > NOTE: If the driver and the corresponding devicetree node are present,
> >       the driver will disable "unused" clocks. This is problem until
> >       the clock relations are properly declared in the devicetree (in a
> >       later patch). Until then, the clk_ignore_unused kernel parameter
> >       can be used as a workaround.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > ---
> > 
> > I have considered converting this driver to a platform driver instead of
> > using CLK_OF_DECLARE, because platform drivers are generally the way
> > forward. However, the timer-npcm7xx driver used on the same platform
> > requires is initialized with TIMER_OF_DECLARE and thus requires the
> > clocks to be available earlier than a platform driver can provide them.
> 
> In that case you can use CLK_OF_DECLARE_DRIVER(), register the clks
> needed for the timer driver to probe, and then put the rest of the clk
> registration in a normal platform driver.

I'll give it a try. I'm not sure how to make it work correctly without
calling (devm_)of_clk_add_hw_provider twice, though (once for the early
clock, timer0; once for the rest).

Another (probably simpler) approach seems be to declare a fixed-clock or
fixed-factor-clock in the DT, and use that in the timer:

	refclk_div2: clock-div2 {
		compatible = "fixed-factor-clock";
		clocks = <&refclk>;
		#clock-cells = <0>;
		clock-mult = <1>;
		clock-div = <2>;
	};

	timer0: timer@b8001000 {
		compatible = "nuvoton,wpcm450-timer";
		interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
		reg = <0xb8001000 0x1c>;
		clocks = <&refclk_div2>;
	};

... and additionally to mark the timer clocks as critical in
clk-wpcm450.c, so they don't get disabled for being "unused".


> > diff --git a/drivers/clk/nuvoton/clk-wpcm450.c b/drivers/clk/nuvoton/clk-wpcm450.c
> > new file mode 100644
> > index 00000000000000..9100c4b8a56483
> > --- /dev/null
> > +++ b/drivers/clk/nuvoton/clk-wpcm450.c
> > @@ -0,0 +1,372 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Nuvoton WPCM450 clock and reset controller driver.
> > + *
> > + * Copyright (C) 2022 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Isn't KBUILD_MODNAME an option already for dynamic debug?

Indeed, it's the +m option.

My motivation for setting pr_fmt in the first place should become
obsolete with the move towards a platform driver anyway, because then I
can use dev_err() etc. I'll remove the #define.

> 
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/reset/reset-simple.h>
> > +#include <linux/slab.h>
> > +
> [...]
> > +
> > +static const struct clk_parent_data default_parents[] = {
> > +       { .name = "pll0" },
> > +       { .name = "pll1" },
> > +       { .name = "ref" },
> > +};
> > +
> > +static const struct clk_parent_data huart_parents[] = {
> > +       { .fw_name = "ref" },
> > +       { .name = "refdiv2" },
> 
> Please remove all .name elements and use indexes or direct pointers.

Will do.

What I'm not yet sure about, is which of these is better:

 1. Having two kinds of indexes, 1. for internal use in the driver,
    identifying all clocks, 2. public as part of the devicetree binding
    ABI (defined in include/dt-bindings/clock/nuvoton,wpcm450-clk.h)
 2. Unifying the two and giving every clock a public index
 3. Using the same number space, but only providing public definitions
    (in the binding) for clocks that can be used outside the clock
    controller.

Option 3 sounds fairly reasonable.

> > +static const struct wpcm450_clken_data clken_data[] = {
> > +       { "fiu", { .name = "ahb3" }, WPCM450_CLK_FIU, 0 },
> 
> This actually is  { .index = 0, .name = "ahb3" } and that is a bad
> combination. struct clk_parent_data should only have .name as a fallback
> when there's an old binding out there that doesn't have the 'clocks'
> property for the clk provider node. There shouldn't be any .name
> property because this is new code and a new binding.

I'll try switching to .index or .hw instead for the references to
internal clocks.


[...]
> > +/*
> > + * NOTE: Error handling is very rudimentary here. If the clock driver initial-
> > + * ization fails, the system is probably in bigger trouble than what is caused
> 
> Don't break words across lines with hyphens.

Good point.

(Due to the switch to a platform driver, this comment will probably
become obsolete anyway.)

> > + * by a few leaked resources.
> > + */
> > +
> > +static void __init wpcm450_clk_init(struct device_node *np)
> > +{
> > +       struct clk_hw_onecell_data *clk_data;
> > +       static struct clk_hw **hws;
> > +       static struct clk_hw *hw;
> > +       void __iomem *clk_base;
> > +       int i, ret;
> > +       struct reset_simple_data *reset;
> > +
> > +       clk_base = of_iomap(np, 0);
> > +       if (!clk_base) {
> > +               pr_err("%pOFP: failed to map registers\n", np);
> > +               of_node_put(np);
> > +               return;
> > +       }
> > +       of_node_put(np);
> 
> The 'np' is used later when registering PLLs. You can only put the node
> after it's no longer used. Also, you never got the node with
> of_node_get(), so putting it here actually causes an underflow on the
> refcount. Just remove all the get/puts instead.

That simplifies it, thanks for the hint!

> > +
> > +       clk_data = kzalloc(struct_size(clk_data, hws, WPCM450_NUM_CLKS), GFP_KERNEL);
> > +       if (!clk_data)
> > +               return;
> > +
> > +       clk_data->num = WPCM450_NUM_CLKS;
> [...]
> > +       /* Reset controller */
> > +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> > +       if (!reset)
> > +               return;
> > +       reset->rcdev.owner = THIS_MODULE;
> > +       reset->rcdev.nr_resets = WPCM450_NUM_RESETS;
> > +       reset->rcdev.ops = &reset_simple_ops;
> > +       reset->rcdev.of_node = np;
> > +       reset->membase = clk_base + REG_IPSRST;
> > +       ret = reset_controller_register(&reset->rcdev);
> > +       if (ret)
> > +               pr_err("Failed to register reset controller: %pe\n", ERR_PTR(ret));
> 
> It would be nicer to register this device as an auxiliary device with a
> single API call and then have all the resets exist in that file
> instead of this file. The driver would be put in drivers/reset/ as well.

Not sure I'd move ten lines to a whole new file, but moving them to a
separate function definitely makes sense, I'll do that.

> 
> > +
> > +       of_node_put(np);
> 
> Drop this of_node_put()

Ok.


Thanks for your detailed review!

I'll send the next revision after testing my changes on the relevant
hardware (which I currently don't have with me).

-jn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v11 4/4] ARM: dts: wpcm450: Switch clocks to clock controller

[Jonathan Neuschäfer]

This change is incompatible with older kernels because it requires the
clock controller driver, but I think that's acceptable because WPCM450
support is generally still in an early phase.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

It's probably best to delay merging of this patch until after the driver
is merged; I'm including it here for review, and in case someone wants
to set up a shared branch between the clock and devicetree parts.

v11:
- no changes

v10:
- Reintroducing this patch as part of the clock/reset controller series
---
 arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
index 9dfdd8f67319d3..7e3ea8b31151b3 100644
--- a/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
+++ b/arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi
@@ -2,6 +2,7 @@
 // Copyright 2021 Jonathan Neuschäfer

 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/clock/nuvoton,wpcm450-clk.h>

 / {
 	compatible = "nuvoton,wpcm450";
@@ -30,13 +31,6 @@ cpu@0 {
 		};
 	};

-	clk24m: clock-24mhz {
-		/* 24 MHz dummy clock */
-		compatible = "fixed-clock";
-		clock-frequency = <24000000>;
-		#clock-cells = <0>;
-	};
-
 	refclk: clock-48mhz {
 		/* 48 MHz reference oscillator */
 		compatible = "fixed-clock";
@@ -70,7 +64,7 @@ serial0: serial@b8000000 {
 			reg = <0xb8000000 0x20>;
 			reg-shift = <2>;
 			interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&clk24m>;
+			clocks = <&clk WPCM450_CLK_UART0>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&bsp_pins>;
 			status = "disabled";
@@ -81,7 +75,7 @@ serial1: serial@b8000100 {
 			reg = <0xb8000100 0x20>;
 			reg-shift = <2>;
 			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&clk24m>;
+			clocks = <&clk WPCM450_CLK_UART1>;
 			status = "disabled";
 		};

@@ -89,14 +83,18 @@ timer0: timer@b8001000 {
 			compatible = "nuvoton,wpcm450-timer";
 			interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xb8001000 0x1c>;
-			clocks = <&clk24m>;
+			clocks = <&clk WPCM450_CLK_TIMER0>,
+				 <&clk WPCM450_CLK_TIMER1>,
+				 <&clk WPCM450_CLK_TIMER2>,
+				 <&clk WPCM450_CLK_TIMER3>,
+				 <&clk WPCM450_CLK_TIMER4>;
 		};

 		watchdog0: watchdog@b800101c {
 			compatible = "nuvoton,wpcm450-wdt";
 			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0xb800101c 0x4>;
-			clocks = <&clk24m>;
+			clocks = <&clk WPCM450_CLK_WDT>;
 		};

 		aic: interrupt-controller@b8002000 {
@@ -480,7 +478,7 @@ fiu: spi-controller@c8000000 {
 			#size-cells = <0>;
 			reg = <0xc8000000 0x1000>, <0xc0000000 0x4000000>;
 			reg-names = "control", "memory";
-			clocks = <&clk 0>;
+			clocks = <&clk WPCM450_CLK_FIU>;
 			nuvoton,shm = <&shm>;
 			status = "disabled";
 		};

--
2.43.0