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

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

[Keguang Zhang]

This patchset adds devicetree support for Loongson-1 clock,
including the devicetree binding and the driver.

Keguang Zhang (2):
  dt-bindings: clock: Add binding for Loongson-1 clock driver
  clk: loongson1: Refactor to add devicetree support

 .../bindings/clock/loongson,ls1x-clk.yaml     |  81 ++++
 drivers/clk/Makefile                          |   2 +-
 drivers/clk/clk-loongson1.c                   | 348 ++++++++++++++++++
 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 -
 8 files changed, 430 insertions(+), 274 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


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.34.1

[PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Keguang Zhang]

Add devicetree binding document for the Loongson-1 clock driver.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
 .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml

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..4709c6757f1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
@@ -0,0 +1,81 @@
+# 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:
+  "#clock-cells":
+    const: 0
+
+  compatible:
+    enum:
+      - loongson,ls1b-clk-pll
+      - loongson,ls1b-clk-cpu
+      - loongson,ls1b-clk-ahb
+      - loongson,ls1c-clk-pll
+      - loongson,ls1c-clk-cpu
+      - loongson,ls1c-clk-ahb
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - "#clock-cells"
+  - compatible
+  - clocks
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    clocks {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+
+        xtal: xtal {
+            compatible = "fixed-clock";
+            #clock-cells = <0>;
+            clock-frequency = <33000000>;
+        };
+
+        pll: pll@1fe78030 {
+            compatible = "loongson,ls1b-clk-pll";
+            #clock-cells = <0>;
+            clocks = <&xtal>;
+            reg = <0x1fe78030 0x4>;
+        };
+
+        cpu_clk: cpu_clk@1fe78034 {
+            compatible = "loongson,ls1b-clk-cpu";
+            #clock-cells = <0>;
+            clocks = <&pll>;
+            reg = <0x1fe78034 0x4>;
+        };
+
+        ahb_clk: ahb_clk@1fe78034 {
+            compatible = "loongson,ls1b-clk-ahb";
+            #clock-cells = <0>;
+            clocks = <&pll>;
+            reg = <0x1fe78034 0x4>;
+        };
+
+        apb_clk: apb_clk {
+            compatible = "fixed-factor-clock";
+            #clock-cells = <0>;
+            clocks = <&ahb_clk>;
+            clock-div = <2>;
+            clock-mult = <1>;
+        };
+    };
+...
-- 
2.34.1

[PATCH 2/2] clk: loongson1: Refactor to add devicetree support

[Keguang Zhang]

This patch refactors Loongson-1 clock driver.
- Use CLK_OF_DECLARE() to declare the "early clocks"
  required by of_clk_init()
- Merge clk-loongson1b.c and clk-loongson1c.c into one driver
  because most of the differences between them will be moved to DT
- Add set_rate callback for ls1x_clk_divider
- Update the Kconfig/Makefile accordingly
- Update copyright

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
 drivers/clk/Makefile                   |   2 +-
 drivers/clk/clk-loongson1.c            | 348 +++++++++++++++++++++++++
 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 --
 7 files changed, 349 insertions(+), 274 deletions(-)
 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

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058a25..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
@@ -93,7 +94,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/clk-loongson1.c b/drivers/clk/clk-loongson1.c
new file mode 100644
index 000000000000..5ce23079a590
--- /dev/null
+++ b/drivers/clk/clk-loongson1.c
@@ -0,0 +1,348 @@
+// 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>
+
+struct ls1x_clk_pll {
+	struct clk_hw hw;
+	void __iomem *reg;
+};
+
+#define to_ls1x_clk_pll(p) container_of(p, struct ls1x_clk_pll, hw)
+
+struct ls1x_clk_divider {
+	struct clk_hw hw;
+	void __iomem *reg;
+	u8 shift;
+	u8 width;
+	u8 flags;
+	const struct clk_div_table *table;
+	u8 bypass_shift;
+	u8 bypass_inv;
+	spinlock_t lock;	/* protect access to DIV registers */
+};
+
+#define to_ls1x_clk_divider(_hw) container_of(_hw, struct ls1x_clk_divider, hw)
+
+struct ls1x_clk_div_factor {
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+	const struct clk_div_table *table;
+	u8 bypass_shift;
+	u8 bypass_inv;
+};
+
+static const struct ls1x_clk_div_factor ls1b_clk_cpu_div = {
+	.shift = 20,
+	.width = 4,
+	.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST,
+	.bypass_shift = 8,
+	.bypass_inv = 0,
+};
+
+static const struct ls1x_clk_div_factor ls1c_clk_cpu_div = {
+	.shift = 8,
+	.width = 7,
+	.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST,
+	.bypass_shift = 0,
+	.bypass_inv = 1,
+};
+
+static const struct ls1x_clk_div_factor ls1b_clk_ahb_div = {
+	.shift = 14,
+	.width = 4,
+	.flags = CLK_DIVIDER_ONE_BASED,
+};
+
+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 const struct ls1x_clk_div_factor ls1c_clk_ahb_div = {
+	.shift = 0,
+	.width = 2,
+	.table = ls1c_ahb_div_table,
+	.flags = CLK_DIVIDER_ALLOW_ZERO,
+};
+
+static unsigned long ls1b_pll_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct ls1x_clk_pll *pll = to_ls1x_clk_pll(hw);
+	u32 val, rate;
+
+	val = readl(pll->reg);
+	rate = 12 + (val & GENMASK(5, 0));
+	rate *= parent_rate;
+	rate >>= 1;
+
+	return rate;
+}
+
+static unsigned long ls1c_pll_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct ls1x_clk_pll *pll = to_ls1x_clk_pll(hw);
+	u32 val, rate;
+
+	val = readl(pll->reg);
+	rate = ((val >> 8) & 0xff) + ((val >> 16) & 0xff);
+	rate *= parent_rate;
+	rate >>= 2;
+
+	return rate;
+}
+
+static const struct clk_ops ls1b_pll_clk_ops = {
+	.recalc_rate = ls1b_pll_recalc_rate,
+};
+
+static const struct clk_ops ls1c_pll_clk_ops = {
+	.recalc_rate = ls1c_pll_recalc_rate,
+};
+
+struct clk_hw *__init ls1x_clk_hw_register_pll(struct device *dev,
+					       const char *name,
+					       const char *parent_name,
+					       void __iomem *reg,
+					       const struct clk_ops *ops)
+{
+	struct ls1x_clk_pll *pll;
+	struct clk_hw *hw;
+	struct clk_init_data init = {};
+	int ret;
+
+	/* allocate the clock pll */
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	/* struct ls1x_clk_pll assignments */
+	pll->reg = reg;
+	pll->hw.init = &init;
+
+	/* register the clock */
+	hw = &pll->hw;
+	ret = clk_hw_register(dev, hw);
+	if (ret) {
+		kfree(hw);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
+static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct ls1x_clk_divider *divider = to_ls1x_clk_divider(hw);
+	unsigned int val;
+
+	val = readl(divider->reg) >> divider->shift;
+	val &= clk_div_mask(divider->width);
+
+	return divider_recalc_rate(hw, parent_rate, val, divider->table,
+				   divider->flags, divider->width);
+}
+
+static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *prate)
+{
+	struct ls1x_clk_divider *divider = to_ls1x_clk_divider(hw);
+
+	return divider_round_rate(hw, rate, prate, divider->table,
+				  divider->width, divider->flags);
+}
+
+static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long parent_rate)
+{
+	struct ls1x_clk_divider *divider = to_ls1x_clk_divider(hw);
+	int value;
+	unsigned long flags = 0;
+	u32 val;
+
+	value = divider_get_val(rate, parent_rate, divider->table,
+				divider->width, divider->flags);
+	if (value < 0)
+		return value;
+
+	if (&divider->lock)
+		spin_lock_irqsave(&divider->lock, flags);
+	else
+		__acquire(&divider->lock);
+
+	/* Bypass the clock */
+	val = readl(divider->reg);
+	if (divider->bypass_inv)
+		val &= ~BIT(divider->bypass_shift);
+	else
+		val |= BIT(divider->bypass_shift);
+	writel(val, divider->reg);
+
+	val = readl(divider->reg);
+	val &= ~(clk_div_mask(divider->width) << divider->shift);
+	val |= (u32)value << divider->shift;
+	writel(val, divider->reg);
+
+	/* Restore the clock */
+	val = readl(divider->reg);
+	if (divider->bypass_inv)
+		val |= BIT(divider->bypass_shift);
+	else
+		val &= ~BIT(divider->bypass_shift);
+	writel(val, divider->reg);
+
+	if (&divider->lock)
+		spin_unlock_irqrestore(&divider->lock, flags);
+	else
+		__release(&divider->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,
+};
+
+struct clk_hw *__init ls1x_clk_hw_register_divider(struct device *dev,
+						   const char *name,
+						   const char *parent_name,
+						   unsigned long flags,
+						   void __iomem *reg,
+						   const struct
+						   ls1x_clk_div_factor *factor)
+{
+	struct ls1x_clk_divider *div;
+	struct clk_hw *hw;
+	struct clk_init_data init = {};
+	int ret;
+
+	/* allocate the clock divider */
+	div = kzalloc(sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &ls1x_clk_divider_ops;
+	init.flags = flags;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	if (parent_name)
+		init.num_parents = 1;
+	else
+		init.num_parents = 0;
+
+	/* struct ls1x_clk_divider assignments */
+	div->reg = reg;
+	div->shift = factor->shift;
+	div->width = factor->width;
+	div->flags = factor->flags;
+	div->hw.init = &init;
+	div->table = factor->table;
+	div->bypass_shift = factor->bypass_shift;
+	div->bypass_inv = factor->bypass_inv;
+	spin_lock_init(&div->lock);
+
+	/* register the clock */
+	hw = &div->hw;
+	ret = clk_hw_register(dev, hw);
+	if (ret) {
+		kfree(div);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
+static const struct of_device_id ls1x_pll_match[] __initconst = {
+	{ .compatible = "loongson,ls1b-clk-pll", .data = &ls1b_pll_clk_ops, },
+	{ .compatible = "loongson,ls1c-clk-pll", .data = &ls1c_pll_clk_ops, },
+	{ /* sentinel */ }
+};
+
+static void __init of_ls1x_pll_init(struct device_node *np)
+{
+	struct clk_hw *hw;
+	const char *clk_name = np->name;
+	const char *parent_name;
+	const struct of_device_id *match;
+	void __iomem *reg;
+
+	reg = of_iomap(np, 0);
+	if (!reg) {
+		pr_err("Unable to map PLL register for %pOF\n", np);
+		return;
+	}
+
+	parent_name = of_clk_get_parent_name(np, 0);
+
+	match = of_match_node(ls1x_pll_match, np);
+	if (WARN_ON(!match))
+		return;
+
+	hw = ls1x_clk_hw_register_pll(NULL, clk_name, parent_name,
+				      reg, match->data);
+	if (!IS_ERR(hw))
+		of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
+}
+
+CLK_OF_DECLARE(ls1b_clk_pll, "loongson,ls1b-clk-pll", of_ls1x_pll_init);
+CLK_OF_DECLARE(ls1c_clk_pll, "loongson,ls1c-clk-pll", of_ls1x_pll_init);
+
+static const struct of_device_id ls1x_divider_match[] __initconst = {
+	{ .compatible = "loongson,ls1b-clk-cpu", .data = &ls1b_clk_cpu_div, },
+	{ .compatible = "loongson,ls1b-clk-ahb", .data = &ls1b_clk_ahb_div, },
+	{ .compatible = "loongson,ls1c-clk-cpu", .data = &ls1c_clk_cpu_div, },
+	{ .compatible = "loongson,ls1c-clk-ahb", .data = &ls1c_clk_ahb_div, },
+	{ /* sentinel */ }
+};
+
+static void __init of_ls1x_divider_init(struct device_node *np)
+{
+	struct clk_hw *hw;
+	const char *clk_name = np->name;
+	const char *parent_name;
+	const struct of_device_id *match;
+	void __iomem *reg;
+
+	reg = of_iomap(np, 0);
+	if (!reg) {
+		pr_err("Unable to map DIV register for %pOF\n", np);
+		return;
+	}
+
+	parent_name = of_clk_get_parent_name(np, 0);
+
+	match = of_match_node(ls1x_divider_match, np);
+	if (!match)
+		return;
+
+	hw = ls1x_clk_hw_register_divider(NULL, clk_name, parent_name,
+					  CLK_GET_RATE_NOCACHE, reg,
+					  match->data);
+	if (!IS_ERR(hw))
+		of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
+}
+
+CLK_OF_DECLARE(ls1b_clk_cpu, "loongson,ls1b-clk-cpu", of_ls1x_divider_init);
+CLK_OF_DECLARE(ls1b_clk_ahb, "loongson,ls1b-clk-ahb", of_ls1x_divider_init);
+CLK_OF_DECLARE(ls1c_clk_cpu, "loongson,ls1c-clk-cpu", of_ls1x_divider_init);
+CLK_OF_DECLARE(ls1c_clk_ahb, "loongson,ls1c-clk-ahb", of_ls1x_divider_init);
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

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Krzysztof Kozlowski]

On 13/01/2023 12:07, Keguang Zhang wrote:
> Add devicetree binding document for the Loongson-1 clock driver.

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

Subject: Drop driver, not related to hardware.

> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
>  .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> 
> 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..4709c6757f1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> @@ -0,0 +1,81 @@
> +# 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

Wasn't this already sent?
https://lore.kernel.org/all/20190130194731.GA25716@bogus/
Then this is a v4? Aren't you duplicating efforts (and reviewers efforts)?

> +
> +maintainers:
> +  - Keguang Zhang <keguang.zhang@gmail.com>
> +
> +properties:

compatible is a first property.

> +  "#clock-cells":
> +    const: 0
> +
> +  compatible:
> +    enum:
> +      - loongson,ls1b-clk-pll
> +      - loongson,ls1b-clk-cpu
> +      - loongson,ls1b-clk-ahb
> +      - loongson,ls1c-clk-pll
> +      - loongson,ls1c-clk-cpu
> +      - loongson,ls1c-clk-ahb

Are you registering single clocks? It looks like. No, make a proper
clock controller.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - "#clock-cells"
> +  - compatible
> +  - clocks
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clocks {

No, not really related to the binding.

> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        xtal: xtal {

Incorrect in this context. Missing unit address.

> +            compatible = "fixed-clock";
> +            #clock-cells = <0>;
> +            clock-frequency = <33000000>;
> +        };
> +
> +        pll: pll@1fe78030 {

Node names should be generic, so "clock-controller"
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +            compatible = "loongson,ls1b-clk-pll";
> +            #clock-cells = <0>;
> +            clocks = <&xtal>;
> +            reg = <0x1fe78030 0x4>;

compatible is first property, then reg, then the rest.

> +        };
> +
> +        cpu_clk: cpu_clk@1fe78034 {

No underscores in node names. Anyway this should be gone - make a proper
clock controller.


Best regards,
Krzysztof

Re: [PATCH 2/2] clk: loongson1: Refactor to add devicetree support

[Krzysztof Kozlowski]

On 13/01/2023 12:07, Keguang Zhang wrote:
> This patch refactors Loongson-1 clock driver.
> - Use CLK_OF_DECLARE() to declare the "early clocks"
>   required by of_clk_init()
> - Merge clk-loongson1b.c and clk-loongson1c.c into one driver
>   because most of the differences between them will be moved to DT
> - Add set_rate callback for ls1x_clk_divider
> - Update the Kconfig/Makefile accordingly
> - Update copyright
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
>  drivers/clk/Makefile                   |   2 +-
>  drivers/clk/clk-loongson1.c            | 348 +++++++++++++++++++++++++

No, this is not a refactor. This is removal and re-add. NAK.

One change per commit, this is unreviewable.

>  drivers/clk/loongson1/Makefile         |   4 -
>  drivers/clk/loongson1/clk-loongson1b.c | 118 ---------
>  drivers/clk/loongson1/clk-loongson1c.c |  95 -------

This is not explained at all. You are pushing some crappy vendor code
here instead of merging with upstream code.

Otherwise, explain the drop of directory. Why Loongson should be special?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Rob Herring]

On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote:
> Add devicetree binding document for the Loongson-1 clock driver.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
>  .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034)

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230113110738.1505973-2-keguang.zhang@gmail.com

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

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

pip3 install dtschema --upgrade

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

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Kelvin Cheung]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月13日周五 19:17写道：
>
> On 13/01/2023 12:07, Keguang Zhang wrote:
> > Add devicetree binding document for the Loongson-1 clock driver.
>
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
>
> Subject: Drop driver, not related to hardware.

Wil do.
>
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> >  .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> >
> > 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..4709c6757f1e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> > @@ -0,0 +1,81 @@
> > +# 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
>
> Wasn't this already sent?
> https://lore.kernel.org/all/20190130194731.GA25716@bogus/
> Then this is a v4? Aren't you duplicating efforts (and reviewers efforts)?

Sorry. I didn't notice that patch.
This binding is totally different from that, which goes with the
following driver re-implementation.
>
> > +
> > +maintainers:
> > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > +
> > +properties:
>
> compatible is a first property.

Will do.
>
> > +  "#clock-cells":
> > +    const: 0
> > +
> > +  compatible:
> > +    enum:
> > +      - loongson,ls1b-clk-pll
> > +      - loongson,ls1b-clk-cpu
> > +      - loongson,ls1b-clk-ahb
> > +      - loongson,ls1c-clk-pll
> > +      - loongson,ls1c-clk-cpu
> > +      - loongson,ls1c-clk-ahb
>
> Are you registering single clocks? It looks like. No, make a proper
> clock controller.

This binding contains two types of clock, pll-clk and div-clk.
Should I split the binding to two bindings files?
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +required:
> > +  - "#clock-cells"
> > +  - compatible
> > +  - clocks
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    clocks {
>
> No, not really related to the binding.

Should I remove the "clocks" section?
>
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        xtal: xtal {
>
> Incorrect in this context. Missing unit address.

XTAL doesn't have reg property.
>
> > +            compatible = "fixed-clock";
> > +            #clock-cells = <0>;
> > +            clock-frequency = <33000000>;
> > +        };
> > +
> > +        pll: pll@1fe78030 {
>
> Node names should be generic, so "clock-controller"
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Will change the node name.
>
> > +            compatible = "loongson,ls1b-clk-pll";
> > +            #clock-cells = <0>;
> > +            clocks = <&xtal>;
> > +            reg = <0x1fe78030 0x4>;
>
> compatible is first property, then reg, then the rest.

Will do.
>
> > +        };
> > +
> > +        cpu_clk: cpu_clk@1fe78034 {
>
> No underscores in node names. Anyway this should be gone - make a proper
> clock controller.

Will change the node name.
>
>
> Best regards,
> Krzysztof
>


-- 
Best regards,

Kelvin Cheung

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Krzysztof Kozlowski]

On 17/01/2023 11:31, Kelvin Cheung wrote:
>>> +  "#clock-cells":
>>> +    const: 0
>>> +
>>> +  compatible:
>>> +    enum:
>>> +      - loongson,ls1b-clk-pll
>>> +      - loongson,ls1b-clk-cpu
>>> +      - loongson,ls1b-clk-ahb
>>> +      - loongson,ls1c-clk-pll
>>> +      - loongson,ls1c-clk-cpu
>>> +      - loongson,ls1c-clk-ahb
>>
>> Are you registering single clocks? It looks like. No, make a proper
>> clock controller.
> 
> This binding contains two types of clock, pll-clk and div-clk.
> Should I split the binding to two bindings files?

No, you should register rather one clock controller. Why this have to be
3 separate clock controllers?

>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - "#clock-cells"
>>> +  - compatible
>>> +  - clocks
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    clocks {
>>
>> No, not really related to the binding.
> 
> Should I remove the "clocks" section?

Yes.

>>
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +
>>> +        xtal: xtal {
>>
>> Incorrect in this context. Missing unit address.
> 
> XTAL doesn't have reg property.

Yeah, but DTS is not correct now, is it? If you doubt, build your DTB
with W=1.

>>
>>> +            compatible = "fixed-clock";
>>> +            #clock-cells = <0>;
>>> +            clock-frequency = <33000000>;
>>> +        };
>>> +

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Kelvin Cheung]

Rob Herring <robh@kernel.org> 于2023年1月13日周五 23:26写道：
>
>
> On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote:
> > Add devicetree binding document for the Loongson-1 clock driver.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> >  .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034)

I did notice this warning.
But my situation is the cpu_clk and ahb_clk share the same registers.
May I have your suggestion?
Thanks!
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230113110738.1505973-2-keguang.zhang@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>


-- 
Best regards,

Kelvin Cheung

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Krzysztof Kozlowski]

On 17/01/2023 11:54, Kelvin Cheung wrote:
> Rob Herring <robh@kernel.org> 于2023年1月13日周五 23:26写道：
>>
>>
>> On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote:
>>> Add devicetree binding document for the Loongson-1 clock driver.
>>>
>>> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
>>> ---
>>>  .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
>>>  1 file changed, 81 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml
>>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034)
> 
> I did notice this warning.
> But my situation is the cpu_clk and ahb_clk share the same registers.
> May I have your suggestion?

Don't introduce warnings and errors no matter what. If your code is not
correct, don't submit it, but instead try to fix it.

You got proper solution - use one clock controller, not devices per each
clock. Why Loongson is special here from all other devices in the world?


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Kelvin Cheung]

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月17日周二 18:47写道：
>
> On 17/01/2023 11:31, Kelvin Cheung wrote:
> >>> +  "#clock-cells":
> >>> +    const: 0
> >>> +
> >>> +  compatible:
> >>> +    enum:
> >>> +      - loongson,ls1b-clk-pll
> >>> +      - loongson,ls1b-clk-cpu
> >>> +      - loongson,ls1b-clk-ahb
> >>> +      - loongson,ls1c-clk-pll
> >>> +      - loongson,ls1c-clk-cpu
> >>> +      - loongson,ls1c-clk-ahb
> >>
> >> Are you registering single clocks? It looks like. No, make a proper
> >> clock controller.
> >
> > This binding contains two types of clock, pll-clk and div-clk.
> > Should I split the binding to two bindings files?
>
> No, you should register rather one clock controller. Why this have to be
> 3 separate clock controllers?
>
This sounds like a big change for the driver.
Could you please show me a good example of one clock controller?
Thanks very much!
> >>
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +
> >>> +required:
> >>> +  - "#clock-cells"
> >>> +  - compatible
> >>> +  - clocks
> >>> +  - reg
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    clocks {
> >>
> >> No, not really related to the binding.
> >
> > Should I remove the "clocks" section?
>
> Yes.
>
> >>
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <1>;
> >>> +        ranges;
> >>> +
> >>> +        xtal: xtal {
> >>
> >> Incorrect in this context. Missing unit address.
> >
> > XTAL doesn't have reg property.
>
> Yeah, but DTS is not correct now, is it? If you doubt, build your DTB
> with W=1.
>
No doubt.
I just want to know the right way to declare XTAL.
Could you please show me an example?
Thanks again!
> >>
> >>> +            compatible = "fixed-clock";
> >>> +            #clock-cells = <0>;
> >>> +            clock-frequency = <33000000>;
> >>> +        };
> >>> +
>
> Best regards,
> Krzysztof
>


-- 
Best regards,

Kelvin Cheung

Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

[Krzysztof Kozlowski]

On 18/01/2023 12:16, Kelvin Cheung wrote:
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月17日周二 18:47写道：
>>
>> On 17/01/2023 11:31, Kelvin Cheung wrote:
>>>>> +  "#clock-cells":
>>>>> +    const: 0
>>>>> +
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - loongson,ls1b-clk-pll
>>>>> +      - loongson,ls1b-clk-cpu
>>>>> +      - loongson,ls1b-clk-ahb
>>>>> +      - loongson,ls1c-clk-pll
>>>>> +      - loongson,ls1c-clk-cpu
>>>>> +      - loongson,ls1c-clk-ahb
>>>>
>>>> Are you registering single clocks? It looks like. No, make a proper
>>>> clock controller.
>>>
>>> This binding contains two types of clock, pll-clk and div-clk.
>>> Should I split the binding to two bindings files?
>>
>> No, you should register rather one clock controller. Why this have to be
>> 3 separate clock controllers?
>>
> This sounds like a big change for the driver.
> Could you please show me a good example of one clock controller?

All or almost all the drivers?

> Thanks very much!
>>>>
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +required:
>>>>> +  - "#clock-cells"
>>>>> +  - compatible
>>>>> +  - clocks
>>>>> +  - reg
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    clocks {
>>>>
>>>> No, not really related to the binding.
>>>
>>> Should I remove the "clocks" section?
>>
>> Yes.
>>
>>>>
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <1>;
>>>>> +        ranges;
>>>>> +
>>>>> +        xtal: xtal {
>>>>
>>>> Incorrect in this context. Missing unit address.
>>>
>>> XTAL doesn't have reg property.
>>
>> Yeah, but DTS is not correct now, is it? If you doubt, build your DTB
>> with W=1.
>>
> No doubt.
> I just want to know the right way to declare XTAL.
> Could you please show me an example?

Almost all DTSes?


Best regards,
Krzysztof

Re: [PATCH 2/2] clk: loongson1: Refactor to add devicetree support

[kernel test robot]

Hi Keguang,

I love your patch! Yet something to improve:

[auto build test ERROR on 1b929c02afd37871d5afb9d498426f83432e71c2]

url:    https://github.com/intel-lab-lkp/linux/commits/Keguang-Zhang/dt-bindings-clock-Add-binding-for-Loongson-1-clock-driver/20230113-192017
base:   1b929c02afd37871d5afb9d498426f83432e71c2
patch link:    https://lore.kernel.org/r/20230113110738.1505973-3-keguang.zhang%40gmail.com
patch subject: [PATCH 2/2] clk: loongson1: Refactor to add devicetree support
config: mips-loongson1b_defconfig (https://download.01.org/0day-ci/archive/20230119/202301191914.Rs7JrOPY-lkp@intel.com/config)
compiler: mipsel-linux-gcc (GCC) 12.1.0
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
        # https://github.com/intel-lab-lkp/linux/commit/b753dca857d9c85e25ba410fe013b78c7c73ca40
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Keguang-Zhang/dt-bindings-clock-Add-binding-for-Loongson-1-clock-driver/20230113-192017
        git checkout b753dca857d9c85e25ba410fe013b78c7c73ca40
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/clk/clk-loongson1.c:115:23: warning: no previous prototype for 'ls1x_clk_hw_register_pll' [-Wmissing-prototypes]
     115 | struct clk_hw *__init ls1x_clk_hw_register_pll(struct device *dev,
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/clk/clk-loongson1.c: In function 'ls1x_divider_set_rate':
>> drivers/clk/clk-loongson1.c:186:13: warning: the comparison will always evaluate as 'true' for the address of 'lock' will never be NULL [-Waddress]
     186 |         if (&divider->lock)
         |             ^
   drivers/clk/clk-loongson1.c:28:20: note: 'lock' declared here
      28 |         spinlock_t lock;        /* protect access to DIV registers */
         |                    ^~~~
   drivers/clk/clk-loongson1.c:212:13: warning: the comparison will always evaluate as 'true' for the address of 'lock' will never be NULL [-Waddress]
     212 |         if (&divider->lock)
         |             ^
   drivers/clk/clk-loongson1.c:28:20: note: 'lock' declared here
      28 |         spinlock_t lock;        /* protect access to DIV registers */
         |                    ^~~~
   drivers/clk/clk-loongson1.c: At top level:
>> drivers/clk/clk-loongson1.c:226:23: warning: no previous prototype for 'ls1x_clk_hw_register_divider' [-Wmissing-prototypes]
     226 | struct clk_hw *__init ls1x_clk_hw_register_divider(struct device *dev,
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/clk/clk-loongson1.c:310:34: warning: 'ls1x_divider_match' defined but not used [-Wunused-const-variable=]
     310 | static const struct of_device_id ls1x_divider_match[] __initconst = {
         |                                  ^~~~~~~~~~~~~~~~~~
   drivers/clk/clk-loongson1.c:275:34: warning: 'ls1x_pll_match' defined but not used [-Wunused-const-variable=]
     275 | static const struct of_device_id ls1x_pll_match[] __initconst = {
         |                                  ^~~~~~~~~~~~~~
--
   mipsel-linux-ld: arch/mips/loongson32/common/time.o: in function `plat_time_init':
>> time.c:(.init.text+0x8): undefined reference to `ls1x_clk_init'

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