[PATCH v3 0/4] Devicetree support for Loongson-1 clock

[PATCH v3 0/4] Devicetree support for Loongson-1 clock

[Keguang Zhang]

Removes the old Loongson-1 clock driver and related code
mainly because of no DT support and outdated implementation.

Then, re-implement it to solve the above issues,
along with the devicetree binding document.

Changelog
V2 -> V3: Add 'reg' property into the 'required' field
          Delete the unnecessary property 'clock-names'
          Use the same license as binding document for the header file.
          Add MODULE_AUTHOR and MODULE_DESCRIPTION info
          Add patch "MIPS: loongson32: Update the clock initialization" into this series
	  Add Acked-by tag from Stephen Boyd and Thomas Bogendoerfer
V1 -> V2: Change to one clock controller (suggested by Krzysztof Kozlowski)
          Add clock-related dt-binding header file
          Fix the warning of dt_binding_check
          Split the driver removal to a separate patch
          Implement one clock controller instead of single clocks
          (suggested by Krzysztof Kozlowski)

Keguang Zhang (4):
  dt-bindings: clock: Add Loongson-1 clock
  clk: loongson1: Remove the outdated driver
  clk: loongson1: Re-implement the clock driver
  MIPS: loongson32: Update the clock initialization

 .../bindings/clock/loongson,ls1x-clk.yaml     |  45 +++
 .../include/asm/mach-loongson32/platform.h    |   1 -
 arch/mips/loongson32/common/time.c            |   3 +-
 drivers/clk/Makefile                          |   2 +-
 drivers/clk/clk-loongson1.c                   | 301 ++++++++++++++++++
 drivers/clk/loongson1/Makefile                |   4 -
 drivers/clk/loongson1/clk-loongson1b.c        | 118 -------
 drivers/clk/loongson1/clk-loongson1c.c        |  95 ------
 drivers/clk/loongson1/clk.c                   |  41 ---
 drivers/clk/loongson1/clk.h                   |  15 -
 include/dt-bindings/clock/loongson,ls1x-clk.h |  19 ++
 11 files changed, 368 insertions(+), 276 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
 create mode 100644 drivers/clk/clk-loongson1.c
 delete mode 100644 drivers/clk/loongson1/Makefile
 delete mode 100644 drivers/clk/loongson1/clk-loongson1b.c
 delete mode 100644 drivers/clk/loongson1/clk-loongson1c.c
 delete mode 100644 drivers/clk/loongson1/clk.c
 delete mode 100644 drivers/clk/loongson1/clk.h
 create mode 100644 include/dt-bindings/clock/loongson,ls1x-clk.h


base-commit: 6f173737e1b5670c200329677e821cce1d3d755e
-- 
2.34.1

[PATCH v3 1/4] dt-bindings: clock: Add Loongson-1 clock

[Keguang Zhang]

Add devicetree binding document and related header file
for the Loongson-1 clock.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: Add 'reg' property into the 'required' field
          Delete the unnecessary property 'clock-names'
          Use the same license as binding document for the header file.
V1 -> V2: Change to one clock controller (suggested by Krzysztof Kozlowski)
          Add clock-related dt-binding header file
          Fix the warning of dt_binding_check
---
 .../bindings/clock/loongson,ls1x-clk.yaml     | 45 +++++++++++++++++++
 include/dt-bindings/clock/loongson,ls1x-clk.h | 19 ++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
 create mode 100644 include/dt-bindings/clock/loongson,ls1x-clk.h

diff --git a/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
new file mode 100644
index 000000000000..01561a0f35d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/loongson,ls1x-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-1 Clock Controller
+
+maintainers:
+  - Keguang Zhang <keguang.zhang@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls1b-clk
+      - loongson,ls1c-clk
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    clkc: clock-controller@1fe78030 {
+        compatible = "loongson,ls1b-clk";
+        reg = <0x1fe78030 0x8>;
+
+        clocks = <&xtal>;
+        #clock-cells = <1>;
+    };
+
+...
diff --git a/include/dt-bindings/clock/loongson,ls1x-clk.h b/include/dt-bindings/clock/loongson,ls1x-clk.h
new file mode 100644
index 000000000000..d400e9ac6002
--- /dev/null
+++ b/include/dt-bindings/clock/loongson,ls1x-clk.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Loongson-1 clock tree IDs
+ *
+ * Copyright (C) 2023 Keguang Zhang <keguang.zhang@gmail.com>
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_LS1X_CLK_H__
+#define __DT_BINDINGS_CLOCK_LS1X_CLK_H__
+
+#define LS1X_CLKID_PLL	0
+#define LS1X_CLKID_CPU	1
+#define LS1X_CLKID_DC	2
+#define LS1X_CLKID_AHB	3
+#define LS1X_CLKID_APB	4
+
+#define CLK_NR_CLKS	(LS1X_CLKID_APB + 1)
+
+#endif /* __DT_BINDINGS_CLOCK_LS1X_CLK_H__ */
-- 
2.34.1

Re: [PATCH v3 1/4] dt-bindings: clock: Add Loongson-1 clock

[Krzysztof Kozlowski]

On 16/03/2023 11:47, Keguang Zhang wrote:
> Add devicetree binding document and related header file
> for the Loongson-1 clock.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>


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

Best regards,
Krzysztof

[PATCH v3 2/4] clk: loongson1: Remove the outdated driver

[Keguang Zhang]

Remove the outdated driver due to the following aspects.
- no DT support
- duplicate code across LS1B and LS1C
- does not fit into the current clock framework

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: None
V1 -> V2: Split the driver removal to a separate patch
---
 drivers/clk/Makefile                   |   1 -
 drivers/clk/loongson1/Makefile         |   4 -
 drivers/clk/loongson1/clk-loongson1b.c | 118 -------------------------
 drivers/clk/loongson1/clk-loongson1c.c |  95 --------------------
 drivers/clk/loongson1/clk.c            |  41 ---------
 drivers/clk/loongson1/clk.h            |  15 ----
 6 files changed, 274 deletions(-)
 delete mode 100644 drivers/clk/loongson1/Makefile
 delete mode 100644 drivers/clk/loongson1/clk-loongson1b.c
 delete mode 100644 drivers/clk/loongson1/clk-loongson1c.c
 delete mode 100644 drivers/clk/loongson1/clk.c
 delete mode 100644 drivers/clk/loongson1/clk.h

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058a25..b7b2c6d64636 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -93,7 +93,6 @@ obj-y					+= imx/
 obj-y					+= ingenic/
 obj-$(CONFIG_ARCH_K3)			+= keystone/
 obj-$(CONFIG_ARCH_KEYSTONE)		+= keystone/
-obj-$(CONFIG_MACH_LOONGSON32)		+= loongson1/
 obj-y					+= mediatek/
 obj-$(CONFIG_ARCH_MESON)		+= meson/
 obj-y					+= microchip/
diff --git a/drivers/clk/loongson1/Makefile b/drivers/clk/loongson1/Makefile
deleted file mode 100644
index 251d0fe9dcd1..000000000000
--- a/drivers/clk/loongson1/Makefile
+++ /dev/null
@@ -1,4 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-only
-obj-y				+= clk.o
-obj-$(CONFIG_LOONGSON1_LS1B)	+= clk-loongson1b.o
-obj-$(CONFIG_LOONGSON1_LS1C)	+= clk-loongson1c.o
diff --git a/drivers/clk/loongson1/clk-loongson1b.c b/drivers/clk/loongson1/clk-loongson1b.c
deleted file mode 100644
index 13a2ca23a159..000000000000
--- a/drivers/clk/loongson1/clk-loongson1b.c
+++ /dev/null
@@ -1,118 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (c) 2012-2016 Zhang, Keguang <keguang.zhang@gmail.com>
- */
-
-#include <linux/clkdev.h>
-#include <linux/clk-provider.h>
-#include <linux/io.h>
-#include <linux/err.h>
-
-#include <loongson1.h>
-#include "clk.h"
-
-#define OSC		(33 * 1000000)
-#define DIV_APB		2
-
-static DEFINE_SPINLOCK(_lock);
-
-static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
-					  unsigned long parent_rate)
-{
-	u32 pll, rate;
-
-	pll = __raw_readl(LS1X_CLK_PLL_FREQ);
-	rate = 12 + (pll & GENMASK(5, 0));
-	rate *= OSC;
-	rate >>= 1;
-
-	return rate;
-}
-
-static const struct clk_ops ls1x_pll_clk_ops = {
-	.recalc_rate = ls1x_pll_recalc_rate,
-};
-
-static const char *const cpu_parents[] = { "cpu_clk_div", "osc_clk", };
-static const char *const ahb_parents[] = { "ahb_clk_div", "osc_clk", };
-static const char *const dc_parents[] = { "dc_clk_div", "osc_clk", };
-
-void __init ls1x_clk_init(void)
-{
-	struct clk_hw *hw;
-
-	hw = clk_hw_register_fixed_rate(NULL, "osc_clk", NULL, 0, OSC);
-	clk_hw_register_clkdev(hw, "osc_clk", NULL);
-
-	/* clock derived from 33 MHz OSC clk */
-	hw = clk_hw_register_pll(NULL, "pll_clk", "osc_clk",
-				 &ls1x_pll_clk_ops, 0);
-	clk_hw_register_clkdev(hw, "pll_clk", NULL);
-
-	/* clock derived from PLL clk */
-	/*                                 _____
-	 *         _______________________|     |
-	 * OSC ___/                       | MUX |___ CPU CLK
-	 *        \___ PLL ___ CPU DIV ___|     |
-	 *                                |_____|
-	 */
-	hw = clk_hw_register_divider(NULL, "cpu_clk_div", "pll_clk",
-				   CLK_GET_RATE_NOCACHE, LS1X_CLK_PLL_DIV,
-				   DIV_CPU_SHIFT, DIV_CPU_WIDTH,
-				   CLK_DIVIDER_ONE_BASED |
-				   CLK_DIVIDER_ROUND_CLOSEST, &_lock);
-	clk_hw_register_clkdev(hw, "cpu_clk_div", NULL);
-	hw = clk_hw_register_mux(NULL, "cpu_clk", cpu_parents,
-			       ARRAY_SIZE(cpu_parents),
-			       CLK_SET_RATE_NO_REPARENT, LS1X_CLK_PLL_DIV,
-			       BYPASS_CPU_SHIFT, BYPASS_CPU_WIDTH, 0, &_lock);
-	clk_hw_register_clkdev(hw, "cpu_clk", NULL);
-
-	/*                                 _____
-	 *         _______________________|     |
-	 * OSC ___/                       | MUX |___ DC  CLK
-	 *        \___ PLL ___ DC  DIV ___|     |
-	 *                                |_____|
-	 */
-	hw = clk_hw_register_divider(NULL, "dc_clk_div", "pll_clk",
-				   0, LS1X_CLK_PLL_DIV, DIV_DC_SHIFT,
-				   DIV_DC_WIDTH, CLK_DIVIDER_ONE_BASED, &_lock);
-	clk_hw_register_clkdev(hw, "dc_clk_div", NULL);
-	hw = clk_hw_register_mux(NULL, "dc_clk", dc_parents,
-			       ARRAY_SIZE(dc_parents),
-			       CLK_SET_RATE_NO_REPARENT, LS1X_CLK_PLL_DIV,
-			       BYPASS_DC_SHIFT, BYPASS_DC_WIDTH, 0, &_lock);
-	clk_hw_register_clkdev(hw, "dc_clk", NULL);
-
-	/*                                 _____
-	 *         _______________________|     |
-	 * OSC ___/                       | MUX |___ DDR CLK
-	 *        \___ PLL ___ DDR DIV ___|     |
-	 *                                |_____|
-	 */
-	hw = clk_hw_register_divider(NULL, "ahb_clk_div", "pll_clk",
-				   0, LS1X_CLK_PLL_DIV, DIV_DDR_SHIFT,
-				   DIV_DDR_WIDTH, CLK_DIVIDER_ONE_BASED,
-				   &_lock);
-	clk_hw_register_clkdev(hw, "ahb_clk_div", NULL);
-	hw = clk_hw_register_mux(NULL, "ahb_clk", ahb_parents,
-			       ARRAY_SIZE(ahb_parents),
-			       CLK_SET_RATE_NO_REPARENT, LS1X_CLK_PLL_DIV,
-			       BYPASS_DDR_SHIFT, BYPASS_DDR_WIDTH, 0, &_lock);
-	clk_hw_register_clkdev(hw, "ahb_clk", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-dma", NULL);
-	clk_hw_register_clkdev(hw, "stmmaceth", NULL);
-
-	/* clock derived from AHB clk */
-	/* APB clk is always half of the AHB clk */
-	hw = clk_hw_register_fixed_factor(NULL, "apb_clk", "ahb_clk", 0, 1,
-					DIV_APB);
-	clk_hw_register_clkdev(hw, "apb_clk", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-ac97", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-i2c", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-nand", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-pwmtimer", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-spi", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-wdt", NULL);
-	clk_hw_register_clkdev(hw, "serial8250", NULL);
-}
diff --git a/drivers/clk/loongson1/clk-loongson1c.c b/drivers/clk/loongson1/clk-loongson1c.c
deleted file mode 100644
index 1ebf740380ef..000000000000
--- a/drivers/clk/loongson1/clk-loongson1c.c
+++ /dev/null
@@ -1,95 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (c) 2016 Yang Ling <gnaygnil@gmail.com>
- */
-
-#include <linux/clkdev.h>
-#include <linux/clk-provider.h>
-#include <linux/io.h>
-
-#include <loongson1.h>
-#include "clk.h"
-
-#define OSC		(24 * 1000000)
-#define DIV_APB		1
-
-static DEFINE_SPINLOCK(_lock);
-
-static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
-					  unsigned long parent_rate)
-{
-	u32 pll, rate;
-
-	pll = __raw_readl(LS1X_CLK_PLL_FREQ);
-	rate = ((pll >> 8) & 0xff) + ((pll >> 16) & 0xff);
-	rate *= OSC;
-	rate >>= 2;
-
-	return rate;
-}
-
-static const struct clk_ops ls1x_pll_clk_ops = {
-	.recalc_rate = ls1x_pll_recalc_rate,
-};
-
-static const struct clk_div_table ahb_div_table[] = {
-	[0] = { .val = 0, .div = 2 },
-	[1] = { .val = 1, .div = 4 },
-	[2] = { .val = 2, .div = 3 },
-	[3] = { .val = 3, .div = 3 },
-	[4] = { /* sentinel */ }
-};
-
-void __init ls1x_clk_init(void)
-{
-	struct clk_hw *hw;
-
-	hw = clk_hw_register_fixed_rate(NULL, "osc_clk", NULL, 0, OSC);
-	clk_hw_register_clkdev(hw, "osc_clk", NULL);
-
-	/* clock derived from 24 MHz OSC clk */
-	hw = clk_hw_register_pll(NULL, "pll_clk", "osc_clk",
-				&ls1x_pll_clk_ops, 0);
-	clk_hw_register_clkdev(hw, "pll_clk", NULL);
-
-	hw = clk_hw_register_divider(NULL, "cpu_clk_div", "pll_clk",
-				   CLK_GET_RATE_NOCACHE, LS1X_CLK_PLL_DIV,
-				   DIV_CPU_SHIFT, DIV_CPU_WIDTH,
-				   CLK_DIVIDER_ONE_BASED |
-				   CLK_DIVIDER_ROUND_CLOSEST, &_lock);
-	clk_hw_register_clkdev(hw, "cpu_clk_div", NULL);
-	hw = clk_hw_register_fixed_factor(NULL, "cpu_clk", "cpu_clk_div",
-					0, 1, 1);
-	clk_hw_register_clkdev(hw, "cpu_clk", NULL);
-
-	hw = clk_hw_register_divider(NULL, "dc_clk_div", "pll_clk",
-				   0, LS1X_CLK_PLL_DIV, DIV_DC_SHIFT,
-				   DIV_DC_WIDTH, CLK_DIVIDER_ONE_BASED, &_lock);
-	clk_hw_register_clkdev(hw, "dc_clk_div", NULL);
-	hw = clk_hw_register_fixed_factor(NULL, "dc_clk", "dc_clk_div",
-					0, 1, 1);
-	clk_hw_register_clkdev(hw, "dc_clk", NULL);
-
-	hw = clk_hw_register_divider_table(NULL, "ahb_clk_div", "cpu_clk_div",
-				0, LS1X_CLK_PLL_FREQ, DIV_DDR_SHIFT,
-				DIV_DDR_WIDTH, CLK_DIVIDER_ALLOW_ZERO,
-				ahb_div_table, &_lock);
-	clk_hw_register_clkdev(hw, "ahb_clk_div", NULL);
-	hw = clk_hw_register_fixed_factor(NULL, "ahb_clk", "ahb_clk_div",
-					0, 1, 1);
-	clk_hw_register_clkdev(hw, "ahb_clk", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-dma", NULL);
-	clk_hw_register_clkdev(hw, "stmmaceth", NULL);
-
-	/* clock derived from AHB clk */
-	hw = clk_hw_register_fixed_factor(NULL, "apb_clk", "ahb_clk", 0, 1,
-					DIV_APB);
-	clk_hw_register_clkdev(hw, "apb_clk", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-ac97", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-i2c", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-nand", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-pwmtimer", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-spi", NULL);
-	clk_hw_register_clkdev(hw, "ls1x-wdt", NULL);
-	clk_hw_register_clkdev(hw, "serial8250", NULL);
-}
diff --git a/drivers/clk/loongson1/clk.c b/drivers/clk/loongson1/clk.c
deleted file mode 100644
index f336a3126d31..000000000000
--- a/drivers/clk/loongson1/clk.c
+++ /dev/null
@@ -1,41 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (c) 2012-2016 Zhang, Keguang <keguang.zhang@gmail.com>
- */
-
-#include <linux/clk-provider.h>
-#include <linux/slab.h>
-
-#include "clk.h"
-
-struct clk_hw *__init clk_hw_register_pll(struct device *dev,
-					  const char *name,
-					  const char *parent_name,
-					  const struct clk_ops *ops,
-					  unsigned long flags)
-{
-	int ret;
-	struct clk_hw *hw;
-	struct clk_init_data init;
-
-	/* allocate the divider */
-	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
-	if (!hw)
-		return ERR_PTR(-ENOMEM);
-
-	init.name = name;
-	init.ops = ops;
-	init.flags = flags;
-	init.parent_names = parent_name ? &parent_name : NULL;
-	init.num_parents = parent_name ? 1 : 0;
-	hw->init = &init;
-
-	/* register the clock */
-	ret = clk_hw_register(dev, hw);
-	if (ret) {
-		kfree(hw);
-		hw = ERR_PTR(ret);
-	}
-
-	return hw;
-}
diff --git a/drivers/clk/loongson1/clk.h b/drivers/clk/loongson1/clk.h
deleted file mode 100644
index 124642302b12..000000000000
--- a/drivers/clk/loongson1/clk.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Copyright (c) 2012-2016 Zhang, Keguang <keguang.zhang@gmail.com>
- */
-
-#ifndef __LOONGSON1_CLK_H
-#define __LOONGSON1_CLK_H
-
-struct clk_hw *clk_hw_register_pll(struct device *dev,
-				   const char *name,
-				   const char *parent_name,
-				   const struct clk_ops *ops,
-				   unsigned long flags);
-
-#endif /* __LOONGSON1_CLK_H */
-- 
2.34.1

[PATCH v3 3/4] clk: loongson1: Re-implement the clock driver

[Keguang Zhang]

Re-implement the clock driver for Loongson-1 to
add devicetree support and fit into the clock framework.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: Add MODULE_AUTHOR and MODULE_DESCRIPTION info
V1 -> V2: Implement one clock controller instead of single clocks
          (suggested by Krzysztof Kozlowski)
---
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-loongson1.c | 301 ++++++++++++++++++++++++++++++++++++
 2 files changed, 302 insertions(+)
 create mode 100644 drivers/clk/clk-loongson1.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b7b2c6d64636..417bc27ab6e8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_COMMON_CLK_K210)		+= clk-k210.o
 obj-$(CONFIG_LMK04832)			+= clk-lmk04832.o
 obj-$(CONFIG_COMMON_CLK_LAN966X)	+= clk-lan966x.o
 obj-$(CONFIG_COMMON_CLK_LOCHNAGAR)	+= clk-lochnagar.o
+obj-$(CONFIG_MACH_LOONGSON32)		+= clk-loongson1.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
 obj-$(CONFIG_COMMON_CLK_MAX9485)	+= clk-max9485.o
 obj-$(CONFIG_ARCH_MILBEAUT_M10V)	+= clk-milbeaut.o
diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c
new file mode 100644
index 000000000000..4fda55c67d8d
--- /dev/null
+++ b/drivers/clk/clk-loongson1.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Clock driver for Loongson-1 SoC
+ *
+ * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/clock/loongson,ls1x-clk.h>
+
+/* Loongson 1 Clock Register Definitions */
+#define CLK_PLL_FREQ		0x0
+#define CLK_PLL_DIV		0x4
+
+static DEFINE_SPINLOCK(ls1x_clk_div_lock);
+
+struct ls1x_clk_pll_data {
+	u32 fixed;
+	u8 shift;
+	u8 int_shift;
+	u8 int_width;
+	u8 frac_shift;
+	u8 frac_width;
+};
+
+struct ls1x_clk_div_data {
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+	const struct clk_div_table *table;
+	u8 bypass_shift;
+	u8 bypass_inv;
+	spinlock_t *lock;	/* protect access to DIV registers */
+};
+
+struct ls1x_clk {
+	void __iomem *reg;
+	unsigned int offset;
+	struct clk_hw hw;
+	void *data;
+};
+
+#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw)
+
+static inline unsigned long ls1x_pll_rate_part(unsigned int val,
+					       unsigned int shift,
+					       unsigned int width)
+{
+	return (val & GENMASK(shift + width, shift)) >> shift;
+}
+
+static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
+	const struct ls1x_clk_pll_data *d = ls1x_clk->data;
+	u32 val, rate;
+
+	val = readl(ls1x_clk->reg);
+	rate = d->fixed;
+	rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width);
+	if (d->frac_width)
+		rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width);
+	rate *= parent_rate;
+	rate >>= d->shift;
+
+	return rate;
+}
+
+static const struct clk_ops ls1x_pll_clk_ops = {
+	.recalc_rate = ls1x_pll_recalc_rate,
+};
+
+static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
+	const struct ls1x_clk_div_data *d = ls1x_clk->data;
+	unsigned int val;
+
+	val = readl(ls1x_clk->reg) >> d->shift;
+	val &= clk_div_mask(d->width);
+
+	return divider_recalc_rate(hw, parent_rate, val, d->table,
+				   d->flags, d->width);
+}
+
+static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *prate)
+{
+	struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
+	const struct ls1x_clk_div_data *d = ls1x_clk->data;
+
+	return divider_round_rate(hw, rate, prate, d->table,
+				  d->width, d->flags);
+}
+
+static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long parent_rate)
+{
+	struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
+	const struct ls1x_clk_div_data *d = ls1x_clk->data;
+	int val, div_val;
+	unsigned long flags = 0;
+
+	div_val = divider_get_val(rate, parent_rate, d->table,
+				  d->width, d->flags);
+	if (div_val < 0)
+		return div_val;
+
+	if (d->lock)
+		spin_lock_irqsave(d->lock, flags);
+	else
+		__acquire(d->lock);
+
+	/* Bypass the clock */
+	val = readl(ls1x_clk->reg);
+	if (d->bypass_inv)
+		val &= ~BIT(d->bypass_shift);
+	else
+		val |= BIT(d->bypass_shift);
+	writel(val, ls1x_clk->reg);
+
+	val = readl(ls1x_clk->reg);
+	val &= ~(clk_div_mask(d->width) << d->shift);
+	val |= (u32)div_val << d->shift;
+	writel(val, ls1x_clk->reg);
+
+	/* Restore the clock */
+	val = readl(ls1x_clk->reg);
+	if (d->bypass_inv)
+		val |= BIT(d->bypass_shift);
+	else
+		val &= ~BIT(d->bypass_shift);
+	writel(val, ls1x_clk->reg);
+
+	if (d->lock)
+		spin_unlock_irqrestore(d->lock, flags);
+	else
+		__release(d->lock);
+
+	return 0;
+}
+
+static const struct clk_ops ls1x_clk_divider_ops = {
+	.recalc_rate = ls1x_divider_recalc_rate,
+	.round_rate = ls1x_divider_round_rate,
+	.set_rate = ls1x_divider_set_rate,
+};
+
+#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift,			\
+		     f_shift, f_width, i_shift, i_width)		\
+struct ls1x_clk _name = {						\
+	.offset = (_offset),						\
+	.data = &(struct ls1x_clk_pll_data) {				\
+		.fixed = (_fixed),					\
+		.shift = (_shift),					\
+		.int_shift = (i_shift),					\
+		.int_width = (i_width),					\
+		.frac_shift = (f_shift),				\
+		.frac_width = (f_width),				\
+	},								\
+	.hw.init = &(struct clk_init_data) {				\
+		.name = #_name,						\
+		.ops = &ls1x_pll_clk_ops,				\
+		.parent_data = &(const struct clk_parent_data) {	\
+			.fw_name = "xtal",				\
+			.name = "xtal",					\
+			.index = -1,					\
+		},							\
+		.num_parents = 1,					\
+	},								\
+}
+
+#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width,		\
+		     _table, _bypass_shift, _bypass_inv, _flags)	\
+struct ls1x_clk _name = {						\
+	.offset = (_offset),						\
+	.data = &(struct ls1x_clk_div_data){				\
+		.shift = (_shift),					\
+		.width = (_width),					\
+		.table = (_table),					\
+		.flags = (_flags),					\
+		.bypass_shift = (_bypass_shift),			\
+		.bypass_inv = (_bypass_inv),				\
+		.lock = &ls1x_clk_div_lock,				\
+	},								\
+	.hw.init = &(struct clk_init_data) {				\
+		.name = #_name,						\
+		.ops = &ls1x_clk_divider_ops,				\
+		.parent_hws = (const struct clk_hw *[]) { _pname },	\
+		.num_parents = 1,					\
+		.flags = CLK_GET_RATE_NOCACHE,				\
+	},								\
+}
+
+static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0);
+static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV,
+		    20, 4, NULL, 8, 0,
+		    CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
+static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV,
+		    26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED);
+static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV,
+		    14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED);
+static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1,
+			CLK_SET_RATE_PARENT);
+
+static struct clk_hw_onecell_data ls1b_clk_hw_data = {
+	.hws = {
+		[LS1X_CLKID_PLL] = &ls1b_clk_pll.hw,
+		[LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw,
+		[LS1X_CLKID_DC] = &ls1b_clk_dc.hw,
+		[LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw,
+		[LS1X_CLKID_APB] = &ls1b_clk_apb.hw,
+		[CLK_NR_CLKS] = NULL,
+	},
+	.num = CLK_NR_CLKS,
+};
+
+static const struct clk_div_table ls1c_ahb_div_table[] = {
+	[0] = { .val = 0, .div = 2 },
+	[1] = { .val = 1, .div = 4 },
+	[2] = { .val = 2, .div = 3 },
+	[3] = { .val = 3, .div = 3 },
+	[4] = { /* sentinel */ }
+};
+
+static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8);
+static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV,
+		    8, 7, NULL, 0, 1,
+		    CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
+static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV,
+		    24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED);
+static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ,
+		    0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO);
+static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1,
+			CLK_SET_RATE_PARENT);
+
+static struct clk_hw_onecell_data ls1c_clk_hw_data = {
+	.hws = {
+		[LS1X_CLKID_PLL] = &ls1c_clk_pll.hw,
+		[LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw,
+		[LS1X_CLKID_DC] = &ls1c_clk_dc.hw,
+		[LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw,
+		[LS1X_CLKID_APB] = &ls1c_clk_apb.hw,
+		[CLK_NR_CLKS] = NULL,
+	},
+	.num = CLK_NR_CLKS,
+};
+
+static void __init ls1x_clk_init(struct device_node *np,
+				 struct clk_hw_onecell_data *hw_data)
+{
+	struct ls1x_clk *ls1x_clk;
+	void __iomem *reg;
+	int i, ret;
+
+	reg = of_iomap(np, 0);
+	if (!reg) {
+		pr_err("Unable to map base for %pOF\n", np);
+		return;
+	}
+
+	for (i = 0; i < CLK_NR_CLKS; i++) {
+		/* array might be sparse */
+		if (!hw_data->hws[i])
+			continue;
+
+		if (i != LS1X_CLKID_APB) {
+			ls1x_clk = to_ls1x_clk(hw_data->hws[i]);
+			ls1x_clk->reg = reg + ls1x_clk->offset;
+		}
+
+		ret = of_clk_hw_register(np, hw_data->hws[i]);
+		if (ret)
+			return;
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data);
+	if (ret)
+		pr_err("Failed to register %pOF\n", np);
+}
+
+static void __init ls1b_clk_init(struct device_node *np)
+{
+	return ls1x_clk_init(np, &ls1b_clk_hw_data);
+}
+
+static void __init ls1c_clk_init(struct device_node *np)
+{
+	return ls1x_clk_init(np, &ls1c_clk_hw_data);
+}
+
+CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init);
+CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init);
+
+MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
+MODULE_DESCRIPTION("Loongson1 clock driver");
-- 
2.34.1

Re: [PATCH v3 3/4] clk: loongson1: Re-implement the clock driver

[kernel test robot]

Hi Keguang,

I love your patch! Yet something to improve:

[auto build test ERROR on 6f173737e1b5670c200329677e821cce1d3d755e]

url:    https://github.com/intel-lab-lkp/linux/commits/Keguang-Zhang/dt-bindings-clock-Add-Loongson-1-clock/20230316-185026
base:   6f173737e1b5670c200329677e821cce1d3d755e
patch link:    https://lore.kernel.org/r/20230316104707.236034-4-keguang.zhang%40gmail.com
patch subject: [PATCH v3 3/4] clk: loongson1: Re-implement the clock driver
config: mips-loongson1c_defconfig (https://download.01.org/0day-ci/archive/20230318/202303181358.BXLJVMkh-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mipsel-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/de00eab744ddc82edb1853048dd5d50aa8220115
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Keguang-Zhang/dt-bindings-clock-Add-Loongson-1-clock/20230316-185026
        git checkout de00eab744ddc82edb1853048dd5d50aa8220115
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303181358.BXLJVMkh-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/clk/clk-loongson1.c:300:15: error: expected parameter declarator
   MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
                 ^
>> drivers/clk/clk-loongson1.c:300:15: error: expected ')'
   drivers/clk/clk-loongson1.c:300:14: note: to match this '('
   MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
                ^
>> drivers/clk/clk-loongson1.c:300:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
   MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
   ^
   int
>> drivers/clk/clk-loongson1.c:300:14: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
   MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
                ^
                                                          void
   drivers/clk/clk-loongson1.c:301:20: error: expected parameter declarator
   MODULE_DESCRIPTION("Loongson1 clock driver");
                      ^
   drivers/clk/clk-loongson1.c:301:20: error: expected ')'
   drivers/clk/clk-loongson1.c:301:19: note: to match this '('
   MODULE_DESCRIPTION("Loongson1 clock driver");
                     ^
   drivers/clk/clk-loongson1.c:301:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
   MODULE_DESCRIPTION("Loongson1 clock driver");
   ^
   int
   drivers/clk/clk-loongson1.c:301:19: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
   MODULE_DESCRIPTION("Loongson1 clock driver");
                     ^
                                              void
   8 errors generated.


vim +300 drivers/clk/clk-loongson1.c

   299	
 > 300	MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v3 3/4] clk: loongson1: Re-implement the clock driver

[Stephen Boyd]

Quoting Keguang Zhang (2023-03-16 03:47:06)
> diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c
> new file mode 100644
> index 000000000000..4fda55c67d8d
> --- /dev/null
> +++ b/drivers/clk/clk-loongson1.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Clock driver for Loongson-1 SoC
> + *
> + * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>

Need some more includes here, for container_of() and GENMASK(), readl(),
etc.

> +
> +#include <dt-bindings/clock/loongson,ls1x-clk.h>
> +
> +/* Loongson 1 Clock Register Definitions */
> +#define CLK_PLL_FREQ           0x0
> +#define CLK_PLL_DIV            0x4
> +
> +static DEFINE_SPINLOCK(ls1x_clk_div_lock);
> +

Needs include.

> +struct ls1x_clk_pll_data {
> +       u32 fixed;
> +       u8 shift;
> +       u8 int_shift;
> +       u8 int_width;
> +       u8 frac_shift;
> +       u8 frac_width;
> +};
> +
> +struct ls1x_clk_div_data {
> +       u8 shift;
> +       u8 width;
> +       unsigned long flags;
> +       const struct clk_div_table *table;
> +       u8 bypass_shift;
> +       u8 bypass_inv;
> +       spinlock_t *lock;       /* protect access to DIV registers */
> +};
> +
> +struct ls1x_clk {
> +       void __iomem *reg;
> +       unsigned int offset;
> +       struct clk_hw hw;
> +       void *data;
> +};
> +
> +#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw)
> +
> +static inline unsigned long ls1x_pll_rate_part(unsigned int val,

return a u32?

> +                                              unsigned int shift,
> +                                              unsigned int width)
> +{
> +       return (val & GENMASK(shift + width, shift)) >> shift;
> +}
> +
> +static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
> +                                         unsigned long parent_rate)
> +{
> +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> +       const struct ls1x_clk_pll_data *d = ls1x_clk->data;
> +       u32 val, rate;
> +
> +       val = readl(ls1x_clk->reg);
> +       rate = d->fixed;
> +       rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width);
> +       if (d->frac_width)
> +               rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width);
> +       rate *= parent_rate;
> +       rate >>= d->shift;
> +
> +       return rate;
> +}
> +
> +static const struct clk_ops ls1x_pll_clk_ops = {
> +       .recalc_rate = ls1x_pll_recalc_rate,
> +};
> +
> +static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw,
> +                                             unsigned long parent_rate)
> +{
> +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> +       unsigned int val;
> +
> +       val = readl(ls1x_clk->reg) >> d->shift;
> +       val &= clk_div_mask(d->width);
> +
> +       return divider_recalc_rate(hw, parent_rate, val, d->table,
> +                                  d->flags, d->width);
> +}
> +
> +static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                   unsigned long *prate)
> +{
> +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> +
> +       return divider_round_rate(hw, rate, prate, d->table,
> +                                 d->width, d->flags);
> +}
> +
> +static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long parent_rate)
> +{
> +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> +       int val, div_val;
> +       unsigned long flags = 0;
> +
> +       div_val = divider_get_val(rate, parent_rate, d->table,
> +                                 d->width, d->flags);
> +       if (div_val < 0)
> +               return div_val;
> +
> +       if (d->lock)
> +               spin_lock_irqsave(d->lock, flags);
> +       else
> +               __acquire(d->lock);
> +
> +       /* Bypass the clock */
> +       val = readl(ls1x_clk->reg);
> +       if (d->bypass_inv)
> +               val &= ~BIT(d->bypass_shift);
> +       else
> +               val |= BIT(d->bypass_shift);
> +       writel(val, ls1x_clk->reg);
> +
> +       val = readl(ls1x_clk->reg);
> +       val &= ~(clk_div_mask(d->width) << d->shift);
> +       val |= (u32)div_val << d->shift;
> +       writel(val, ls1x_clk->reg);
> +
> +       /* Restore the clock */
> +       val = readl(ls1x_clk->reg);
> +       if (d->bypass_inv)
> +               val |= BIT(d->bypass_shift);
> +       else
> +               val &= ~BIT(d->bypass_shift);
> +       writel(val, ls1x_clk->reg);
> +
> +       if (d->lock)
> +               spin_unlock_irqrestore(d->lock, flags);
> +       else
> +               __release(d->lock);

Is there a case where there isn't a lock? It would be easier to read if
this always had a lock and it wasn't optional.

> +
> +       return 0;
> +}
> +
> +static const struct clk_ops ls1x_clk_divider_ops = {
> +       .recalc_rate = ls1x_divider_recalc_rate,
> +       .round_rate = ls1x_divider_round_rate,
> +       .set_rate = ls1x_divider_set_rate,
> +};
> +
> +#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift,                   \
> +                    f_shift, f_width, i_shift, i_width)                \
> +struct ls1x_clk _name = {                                              \
> +       .offset = (_offset),                                            \
> +       .data = &(struct ls1x_clk_pll_data) {                           \
> +               .fixed = (_fixed),                                      \
> +               .shift = (_shift),                                      \
> +               .int_shift = (i_shift),                                 \
> +               .int_width = (i_width),                                 \
> +               .frac_shift = (f_shift),                                \
> +               .frac_width = (f_width),                                \
> +       },                                                              \
> +       .hw.init = &(struct clk_init_data) {                            \
> +               .name = #_name,                                         \
> +               .ops = &ls1x_pll_clk_ops,                               \
> +               .parent_data = &(const struct clk_parent_data) {        \
> +                       .fw_name = "xtal",                              \
> +                       .name = "xtal",                                 \
> +                       .index = -1,                                    \
> +               },                                                      \
> +               .num_parents = 1,                                       \
> +       },                                                              \
> +}
> +
> +#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width,           \
> +                    _table, _bypass_shift, _bypass_inv, _flags)        \
> +struct ls1x_clk _name = {                                              \
> +       .offset = (_offset),                                            \
> +       .data = &(struct ls1x_clk_div_data){                            \
> +               .shift = (_shift),                                      \
> +               .width = (_width),                                      \
> +               .table = (_table),                                      \
> +               .flags = (_flags),                                      \
> +               .bypass_shift = (_bypass_shift),                        \
> +               .bypass_inv = (_bypass_inv),                            \
> +               .lock = &ls1x_clk_div_lock,                             \
> +       },                                                              \
> +       .hw.init = &(struct clk_init_data) {                            \

Can be const.

> +               .name = #_name,                                         \
> +               .ops = &ls1x_clk_divider_ops,                           \
> +               .parent_hws = (const struct clk_hw *[]) { _pname },     \
> +               .num_parents = 1,                                       \
> +               .flags = CLK_GET_RATE_NOCACHE,                          \
> +       },                                                              \
> +}
> +
> +static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0);
> +static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> +                   20, 4, NULL, 8, 0,
> +                   CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
> +static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> +                   26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED);
> +static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> +                   14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED);
> +static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1,
> +                       CLK_SET_RATE_PARENT);
> +
> +static struct clk_hw_onecell_data ls1b_clk_hw_data = {
> +       .hws = {
> +               [LS1X_CLKID_PLL] = &ls1b_clk_pll.hw,
> +               [LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw,
> +               [LS1X_CLKID_DC] = &ls1b_clk_dc.hw,
> +               [LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw,
> +               [LS1X_CLKID_APB] = &ls1b_clk_apb.hw,
> +               [CLK_NR_CLKS] = NULL,

Do you need a CLK_NR_CLKS sentinel entry?

> +       },
> +       .num = CLK_NR_CLKS,
> +};
> +
> +static const struct clk_div_table ls1c_ahb_div_table[] = {
> +       [0] = { .val = 0, .div = 2 },
> +       [1] = { .val = 1, .div = 4 },
> +       [2] = { .val = 2, .div = 3 },
> +       [3] = { .val = 3, .div = 3 },
> +       [4] = { /* sentinel */ }
> +};
> +
> +static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8);
> +static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV,
> +                   8, 7, NULL, 0, 1,
> +                   CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
> +static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV,
> +                   24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED);
> +static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ,
> +                   0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO);
> +static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1,
> +                       CLK_SET_RATE_PARENT);
> +
> +static struct clk_hw_onecell_data ls1c_clk_hw_data = {
> +       .hws = {
> +               [LS1X_CLKID_PLL] = &ls1c_clk_pll.hw,
> +               [LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw,
> +               [LS1X_CLKID_DC] = &ls1c_clk_dc.hw,
> +               [LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw,
> +               [LS1X_CLKID_APB] = &ls1c_clk_apb.hw,
> +               [CLK_NR_CLKS] = NULL,
> +       },
> +       .num = CLK_NR_CLKS,
> +};
> +
> +static void __init ls1x_clk_init(struct device_node *np,
> +                                struct clk_hw_onecell_data *hw_data)
> +{
> +       struct ls1x_clk *ls1x_clk;
> +       void __iomem *reg;
> +       int i, ret;
> +
> +       reg = of_iomap(np, 0);
> +       if (!reg) {
> +               pr_err("Unable to map base for %pOF\n", np);

Needs include.

> +               return;
> +       }
> +
> +       for (i = 0; i < CLK_NR_CLKS; i++) {
> +               /* array might be sparse */
> +               if (!hw_data->hws[i])
> +                       continue;
> +
> +               if (i != LS1X_CLKID_APB) {
> +                       ls1x_clk = to_ls1x_clk(hw_data->hws[i]);
> +                       ls1x_clk->reg = reg + ls1x_clk->offset;
> +               }
> +
> +               ret = of_clk_hw_register(np, hw_data->hws[i]);
> +               if (ret)
> +                       return;

unmap memory on failure? and unregister clks?

> +       }
> +
> +       ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data);
> +       if (ret)
> +               pr_err("Failed to register %pOF\n", np);

unmap memory on failure? And unregister clks?

> +}
> +
> +static void __init ls1b_clk_init(struct device_node *np)
> +{
> +       return ls1x_clk_init(np, &ls1b_clk_hw_data);
> +}
> +
> +static void __init ls1c_clk_init(struct device_node *np)
> +{
> +       return ls1x_clk_init(np, &ls1c_clk_hw_data);
> +}
> +
> +CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init);
> +CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init);

Any reason these can't be platform device drivers?

> +
> +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
> +MODULE_DESCRIPTION("Loongson1 clock driver");

It's not a module. So these are useless macros. Drop them?

Re: [PATCH v3 3/4] clk: loongson1: Re-implement the clock driver

[Keguang Zhang]

On Tue, Mar 21, 2023 at 4:06 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Keguang Zhang (2023-03-16 03:47:06)
> > diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c
> > new file mode 100644
> > index 000000000000..4fda55c67d8d
> > --- /dev/null
> > +++ b/drivers/clk/clk-loongson1.c
> > @@ -0,0 +1,301 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Clock driver for Loongson-1 SoC
> > + *
> > + * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
>
> Need some more includes here, for container_of() and GENMASK(), readl(),
> etc.
>
Will do.

> > +
> > +#include <dt-bindings/clock/loongson,ls1x-clk.h>
> > +
> > +/* Loongson 1 Clock Register Definitions */
> > +#define CLK_PLL_FREQ           0x0
> > +#define CLK_PLL_DIV            0x4
> > +
> > +static DEFINE_SPINLOCK(ls1x_clk_div_lock);
> > +
>
> Needs include.
>
Will do.

> > +struct ls1x_clk_pll_data {
> > +       u32 fixed;
> > +       u8 shift;
> > +       u8 int_shift;
> > +       u8 int_width;
> > +       u8 frac_shift;
> > +       u8 frac_width;
> > +};
> > +
> > +struct ls1x_clk_div_data {
> > +       u8 shift;
> > +       u8 width;
> > +       unsigned long flags;
> > +       const struct clk_div_table *table;
> > +       u8 bypass_shift;
> > +       u8 bypass_inv;
> > +       spinlock_t *lock;       /* protect access to DIV registers */
> > +};
> > +
> > +struct ls1x_clk {
> > +       void __iomem *reg;
> > +       unsigned int offset;
> > +       struct clk_hw hw;
> > +       void *data;
> > +};
> > +
> > +#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw)
> > +
> > +static inline unsigned long ls1x_pll_rate_part(unsigned int val,
>
> return a u32?
>
Yes.

> > +                                              unsigned int shift,
> > +                                              unsigned int width)
> > +{
> > +       return (val & GENMASK(shift + width, shift)) >> shift;
> > +}
> > +
> > +static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
> > +                                         unsigned long parent_rate)
> > +{
> > +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> > +       const struct ls1x_clk_pll_data *d = ls1x_clk->data;
> > +       u32 val, rate;
> > +
> > +       val = readl(ls1x_clk->reg);
> > +       rate = d->fixed;
> > +       rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width);
> > +       if (d->frac_width)
> > +               rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width);
> > +       rate *= parent_rate;
> > +       rate >>= d->shift;
> > +
> > +       return rate;
> > +}
> > +
> > +static const struct clk_ops ls1x_pll_clk_ops = {
> > +       .recalc_rate = ls1x_pll_recalc_rate,
> > +};
> > +
> > +static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw,
> > +                                             unsigned long parent_rate)
> > +{
> > +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> > +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> > +       unsigned int val;
> > +
> > +       val = readl(ls1x_clk->reg) >> d->shift;
> > +       val &= clk_div_mask(d->width);
> > +
> > +       return divider_recalc_rate(hw, parent_rate, val, d->table,
> > +                                  d->flags, d->width);
> > +}
> > +
> > +static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                   unsigned long *prate)
> > +{
> > +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> > +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> > +
> > +       return divider_round_rate(hw, rate, prate, d->table,
> > +                                 d->width, d->flags);
> > +}
> > +
> > +static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                                unsigned long parent_rate)
> > +{
> > +       struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw);
> > +       const struct ls1x_clk_div_data *d = ls1x_clk->data;
> > +       int val, div_val;
> > +       unsigned long flags = 0;
> > +
> > +       div_val = divider_get_val(rate, parent_rate, d->table,
> > +                                 d->width, d->flags);
> > +       if (div_val < 0)
> > +               return div_val;
> > +
> > +       if (d->lock)
> > +               spin_lock_irqsave(d->lock, flags);
> > +       else
> > +               __acquire(d->lock);
> > +
> > +       /* Bypass the clock */
> > +       val = readl(ls1x_clk->reg);
> > +       if (d->bypass_inv)
> > +               val &= ~BIT(d->bypass_shift);
> > +       else
> > +               val |= BIT(d->bypass_shift);
> > +       writel(val, ls1x_clk->reg);
> > +
> > +       val = readl(ls1x_clk->reg);
> > +       val &= ~(clk_div_mask(d->width) << d->shift);
> > +       val |= (u32)div_val << d->shift;
> > +       writel(val, ls1x_clk->reg);
> > +
> > +       /* Restore the clock */
> > +       val = readl(ls1x_clk->reg);
> > +       if (d->bypass_inv)
> > +               val |= BIT(d->bypass_shift);
> > +       else
> > +               val &= ~BIT(d->bypass_shift);
> > +       writel(val, ls1x_clk->reg);
> > +
> > +       if (d->lock)
> > +               spin_unlock_irqrestore(d->lock, flags);
> > +       else
> > +               __release(d->lock);
>
> Is there a case where there isn't a lock? It would be easier to read if
> this always had a lock and it wasn't optional.
>
Indeed. The lock always exists in this driver.
Will remove the unnecessary __acquire()/__release().

> > +
> > +       return 0;
> > +}
> > +
> > +static const struct clk_ops ls1x_clk_divider_ops = {
> > +       .recalc_rate = ls1x_divider_recalc_rate,
> > +       .round_rate = ls1x_divider_round_rate,
> > +       .set_rate = ls1x_divider_set_rate,
> > +};
> > +
> > +#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift,                   \
> > +                    f_shift, f_width, i_shift, i_width)                \
> > +struct ls1x_clk _name = {                                              \
> > +       .offset = (_offset),                                            \
> > +       .data = &(struct ls1x_clk_pll_data) {                           \
> > +               .fixed = (_fixed),                                      \
> > +               .shift = (_shift),                                      \
> > +               .int_shift = (i_shift),                                 \
> > +               .int_width = (i_width),                                 \
> > +               .frac_shift = (f_shift),                                \
> > +               .frac_width = (f_width),                                \
> > +       },                                                              \
> > +       .hw.init = &(struct clk_init_data) {                            \
> > +               .name = #_name,                                         \
> > +               .ops = &ls1x_pll_clk_ops,                               \
> > +               .parent_data = &(const struct clk_parent_data) {        \
> > +                       .fw_name = "xtal",                              \
> > +                       .name = "xtal",                                 \
> > +                       .index = -1,                                    \
> > +               },                                                      \
> > +               .num_parents = 1,                                       \
> > +       },                                                              \
> > +}
> > +
> > +#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width,           \
> > +                    _table, _bypass_shift, _bypass_inv, _flags)        \
> > +struct ls1x_clk _name = {                                              \
> > +       .offset = (_offset),                                            \
> > +       .data = &(struct ls1x_clk_div_data){                            \
> > +               .shift = (_shift),                                      \
> > +               .width = (_width),                                      \
> > +               .table = (_table),                                      \
> > +               .flags = (_flags),                                      \
> > +               .bypass_shift = (_bypass_shift),                        \
> > +               .bypass_inv = (_bypass_inv),                            \
> > +               .lock = &ls1x_clk_div_lock,                             \
> > +       },                                                              \
> > +       .hw.init = &(struct clk_init_data) {                            \
>
> Can be const.
>
Will do.

> > +               .name = #_name,                                         \
> > +               .ops = &ls1x_clk_divider_ops,                           \
> > +               .parent_hws = (const struct clk_hw *[]) { _pname },     \
> > +               .num_parents = 1,                                       \
> > +               .flags = CLK_GET_RATE_NOCACHE,                          \
> > +       },                                                              \
> > +}
> > +
> > +static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0);
> > +static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> > +                   20, 4, NULL, 8, 0,
> > +                   CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
> > +static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> > +                   26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED);
> > +static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV,
> > +                   14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED);
> > +static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1,
> > +                       CLK_SET_RATE_PARENT);
> > +
> > +static struct clk_hw_onecell_data ls1b_clk_hw_data = {
> > +       .hws = {
> > +               [LS1X_CLKID_PLL] = &ls1b_clk_pll.hw,
> > +               [LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw,
> > +               [LS1X_CLKID_DC] = &ls1b_clk_dc.hw,
> > +               [LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw,
> > +               [LS1X_CLKID_APB] = &ls1b_clk_apb.hw,
> > +               [CLK_NR_CLKS] = NULL,
>
> Do you need a CLK_NR_CLKS sentinel entry?
>
Not necessary.
Will remove.

> > +       },
> > +       .num = CLK_NR_CLKS,
> > +};
> > +
> > +static const struct clk_div_table ls1c_ahb_div_table[] = {
> > +       [0] = { .val = 0, .div = 2 },
> > +       [1] = { .val = 1, .div = 4 },
> > +       [2] = { .val = 2, .div = 3 },
> > +       [3] = { .val = 3, .div = 3 },
> > +       [4] = { /* sentinel */ }
> > +};
> > +
> > +static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8);
> > +static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV,
> > +                   8, 7, NULL, 0, 1,
> > +                   CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST);
> > +static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV,
> > +                   24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED);
> > +static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ,
> > +                   0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO);
> > +static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1,
> > +                       CLK_SET_RATE_PARENT);
> > +
> > +static struct clk_hw_onecell_data ls1c_clk_hw_data = {
> > +       .hws = {
> > +               [LS1X_CLKID_PLL] = &ls1c_clk_pll.hw,
> > +               [LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw,
> > +               [LS1X_CLKID_DC] = &ls1c_clk_dc.hw,
> > +               [LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw,
> > +               [LS1X_CLKID_APB] = &ls1c_clk_apb.hw,
> > +               [CLK_NR_CLKS] = NULL,
> > +       },
> > +       .num = CLK_NR_CLKS,
> > +};
> > +
> > +static void __init ls1x_clk_init(struct device_node *np,
> > +                                struct clk_hw_onecell_data *hw_data)
> > +{
> > +       struct ls1x_clk *ls1x_clk;
> > +       void __iomem *reg;
> > +       int i, ret;
> > +
> > +       reg = of_iomap(np, 0);
> > +       if (!reg) {
> > +               pr_err("Unable to map base for %pOF\n", np);
>
> Needs include.
>
Will do.

> > +               return;
> > +       }
> > +
> > +       for (i = 0; i < CLK_NR_CLKS; i++) {
> > +               /* array might be sparse */
> > +               if (!hw_data->hws[i])
> > +                       continue;
> > +
> > +               if (i != LS1X_CLKID_APB) {
> > +                       ls1x_clk = to_ls1x_clk(hw_data->hws[i]);
> > +                       ls1x_clk->reg = reg + ls1x_clk->offset;
> > +               }
> > +
> > +               ret = of_clk_hw_register(np, hw_data->hws[i]);
> > +               if (ret)
> > +                       return;
>
> unmap memory on failure? and unregister clks?
>
Will add the error handling.

> > +       }
> > +
> > +       ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data);
> > +       if (ret)
> > +               pr_err("Failed to register %pOF\n", np);
>
> unmap memory on failure? And unregister clks?
>
Will add the error handling.

> > +}
> > +
> > +static void __init ls1b_clk_init(struct device_node *np)
> > +{
> > +       return ls1x_clk_init(np, &ls1b_clk_hw_data);
> > +}
> > +
> > +static void __init ls1c_clk_init(struct device_node *np)
> > +{
> > +       return ls1x_clk_init(np, &ls1c_clk_hw_data);
> > +}
> > +
> > +CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init);
> > +CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init);
>
> Any reason these can't be platform device drivers?
>
The Loongson1 system uses performance counter as the default clocksource.
Then, mips_hpt_frequency should be figured out when doing plat_time_init(),
just before calling mips_clockevent_init() and init_mips_clocksource().
And mips_hpt_frequency depends on CPU clock.
So these clocks should be initialized as early as possible.
It's too late for being a platform device driver.

> > +
> > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
> > +MODULE_DESCRIPTION("Loongson1 clock driver");
>
> It's not a module. So these are useless macros. Drop them?

Yes. I realized these macros will never be called.
Will drop.


--
Best regards,

Keguang Zhang

[PATCH v3 4/4] MIPS: loongson32: Update the clock initialization

[Keguang Zhang]

The Loongson-1 clock driver is under re-implementation
to add DT support. As a result, ls1x_clk_init() will be dropped soon.
Therefore, call of_clk_init() for clock initialization instead.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
V2 -> V3: Add this patch to "Devicetree support for Loongson-1 clock" series
	  Add Acked-by tag from Stephen Boyd and Thomas Bogendoerfer
V1 -> V2: None
---
 arch/mips/include/asm/mach-loongson32/platform.h | 1 -
 arch/mips/loongson32/common/time.c               | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/mach-loongson32/platform.h b/arch/mips/include/asm/mach-loongson32/platform.h
index 86e1a6aab4e5..2cdcfb5f6012 100644
--- a/arch/mips/include/asm/mach-loongson32/platform.h
+++ b/arch/mips/include/asm/mach-loongson32/platform.h
@@ -20,7 +20,6 @@ extern struct platform_device ls1x_gpio1_pdev;
 extern struct platform_device ls1x_rtc_pdev;
 extern struct platform_device ls1x_wdt_pdev;
 
-void __init ls1x_clk_init(void);
 void __init ls1x_rtc_set_extclk(struct platform_device *pdev);
 void __init ls1x_serial_set_uartclk(struct platform_device *pdev);
 
diff --git a/arch/mips/loongson32/common/time.c b/arch/mips/loongson32/common/time.c
index 459b15c96d3b..965c04aa56fd 100644
--- a/arch/mips/loongson32/common/time.c
+++ b/arch/mips/loongson32/common/time.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/of_clk.h>
 #include <linux/interrupt.h>
 #include <linux/sizes.h>
 #include <asm/time.h>
@@ -211,7 +212,7 @@ void __init plat_time_init(void)
 	struct clk *clk = NULL;
 
 	/* initialize LS1X clocks */
-	ls1x_clk_init();
+	of_clk_init(NULL);
 
 #ifdef CONFIG_CEVT_CSRC_LS1X
 	/* setup LS1X PWM timer */
-- 
2.34.1