[PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Yinbo Zhu]

The Loongson-2 boot clock was used to spi and lio peripheral and
this patch was to add boot clock index number.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 include/dt-bindings/clock/loongson,ls2k-clk.h | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/dt-bindings/clock/loongson,ls2k-clk.h b/include/dt-bindings/clock/loongson,ls2k-clk.h
index db1e27e792ff1..e86804365e506 100644
--- a/include/dt-bindings/clock/loongson,ls2k-clk.h
+++ b/include/dt-bindings/clock/loongson,ls2k-clk.h
@@ -13,17 +13,18 @@
 #define LOONGSON2_DC_PLL				3
 #define LOONGSON2_PIX0_PLL				4
 #define LOONGSON2_PIX1_PLL				5
-#define LOONGSON2_NODE_CLK				6
-#define LOONGSON2_HDA_CLK				7
-#define LOONGSON2_GPU_CLK				8
-#define LOONGSON2_DDR_CLK				9
-#define LOONGSON2_GMAC_CLK				10
-#define LOONGSON2_DC_CLK				11
-#define LOONGSON2_APB_CLK				12
-#define LOONGSON2_USB_CLK				13
-#define LOONGSON2_SATA_CLK				14
-#define LOONGSON2_PIX0_CLK				15
-#define LOONGSON2_PIX1_CLK				16
-#define LOONGSON2_CLK_END				17
+#define LOONGSON2_BOOT_CLK				6
+#define LOONGSON2_NODE_CLK				7
+#define LOONGSON2_HDA_CLK				8
+#define LOONGSON2_GPU_CLK				9
+#define LOONGSON2_DDR_CLK				10
+#define LOONGSON2_GMAC_CLK				11
+#define LOONGSON2_DC_CLK				12
+#define LOONGSON2_APB_CLK				13
+#define LOONGSON2_USB_CLK				14
+#define LOONGSON2_SATA_CLK				15
+#define LOONGSON2_PIX0_CLK				16
+#define LOONGSON2_PIX1_CLK				17
+#define LOONGSON2_CLK_END				18
 
 #endif
-- 
2.31.1

[PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Yinbo Zhu]

This driver provides support for clock controller on Loongson-2 SoC,
the Loongson-2 SoC uses a 100MHz clock as the PLL reference clock,
there are five independent PLLs inside, each of which PLL can
provide up to three sets of frequency dependent clock outputs.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v13:
		1. Add the Loongson-2 boot clk support.
Change in v12:
		1. Change driver register way that use platform driver register.
Change in v11:
		1. Remove the repetitive COMMON_CLK config in loongarch Kconfig.
		2. Fixup the help paragraph descprition in COMMON_CLK_LOONGSON2.
		3. Drop the <linux/clkdev.h>.
		4. Drop the cast in clock driver.
		5. Use div_u64() replace do_div().
		6. Inline this function for loongson2_check_clk_hws.
		7. Fixup the comma deposition on the loongson2_check_clk_hws.
		8. Drop the loongson2_obtain_fixed_clk_hw and rework this driver
		   use clk_parent_data and the 'fw_name'.
Change in v10:
		1. Detach of_clk_init to another patch. 
Change in v9:
		1. Add all history changelog information.
Change in v8:
		1. Remove the flag "CLK_IS_BASIC".
Change in v7:
		1. Adjust position alphabetically in Kconfig and Makefile.
		2. Add static for loongson2_pll_base.
		3. Move other file-scope variables in probe.
Change in v6:
		1. NO change, but other patch in this series of patches has
		   changes.
Change in v5:
		1. Replace loongson2 with Loongson-2 in commit info.
		2. Replace Loongson2 with Loongson-2 in binding and
		   Kconfig file.
		3. Replace soc with SoC.
Change in v4:
		1. Fixup clock-names that replace "xxx-clk" with "xxx".
Change in v3:
		1. NO change, but other patch in this series of patches has
		   changes.
Change in v2:
		1. Update the include filename.
		2. Change string from refclk/REFCLK to ref/REF.

 MAINTAINERS                 |   1 +
 drivers/clk/Kconfig         |   9 +
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-loongson2.c | 355 ++++++++++++++++++++++++++++++++++++
 4 files changed, 366 insertions(+)
 create mode 100644 drivers/clk/clk-loongson2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f3053..2334623052875 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12121,6 +12121,7 @@ M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-clk@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/loongson,ls2k-clk.yaml
+F:	drivers/clk/clk-loongson2.c
 F:	include/dt-bindings/clock/loongson,ls2k-clk.h
 
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index b6c5bf69a2b2c..27c5fa02916a6 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -325,6 +325,15 @@ config COMMON_CLK_LOCHNAGAR
 	  This driver supports the clocking features of the Cirrus Logic
 	  Lochnagar audio development board.
 
+config COMMON_CLK_LOONGSON2
+	bool "Clock driver for Loongson-2 SoC"
+	depends on COMMON_CLK && OF
+	help
+          This driver provides support for clock controller on Loongson-2 SoC.
+          The clock controller can generates and supplies clock to various
+          peripherals within the SoC.
+          Say Y here to support Loongson-2 SoC clock driver.
+
 config COMMON_CLK_NXP
 	def_bool COMMON_CLK && (ARCH_LPC18XX || ARCH_LPC32XX)
 	select REGMAP_MMIO if ARCH_LPC32XX
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058a256..b298c5dabc1a4 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_COMMON_CLK_LOONGSON2)	+= clk-loongson2.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-loongson2.c b/drivers/clk/clk-loongson2.c
new file mode 100644
index 0000000000000..c24334b19ea70
--- /dev/null
+++ b/drivers/clk/clk-loongson2.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Yinbo Zhu <zhuyinbo@loongson.cn>
+ * Copyright (C) 2022-2023 Loongson Technology Corporation Limited
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/loongson,ls2k-clk.h>
+
+#define LOONGSON2_PLL_MULT_SHIFT		32
+#define LOONGSON2_PLL_MULT_WIDTH		10
+#define LOONGSON2_PLL_DIV_SHIFT			26
+#define LOONGSON2_PLL_DIV_WIDTH			6
+#define LOONGSON2_APB_FREQSCALE_SHIFT		20
+#define LOONGSON2_APB_FREQSCALE_WIDTH		3
+#define LOONGSON2_USB_FREQSCALE_SHIFT		16
+#define LOONGSON2_USB_FREQSCALE_WIDTH		3
+#define LOONGSON2_SATA_FREQSCALE_SHIFT		12
+#define LOONGSON2_SATA_FREQSCALE_WIDTH		3
+#define LOONGSON2_BOOT_FREQSCALE_SHIFT		8
+#define LOONGSON2_BOOT_FREQSCALE_WIDTH		3
+
+static void __iomem *loongson2_pll_base;
+
+static const struct clk_parent_data pdata[] = {
+	{ .fw_name = "ref_100m", .name = "ref_clk", },
+};
+
+static struct clk_hw *loongson2_clk_register(struct device_node *np,
+					  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.num_parents = 1;
+
+	if (!parent_name)
+		init.parent_data = pdata;
+	else
+		init.parent_names = &parent_name;
+
+	hw->init = &init;
+
+	/* register the clock */
+	ret = of_clk_hw_register(np, hw);
+	if (ret) {
+		kfree(hw);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
+static unsigned long loongson2_calc_pll_rate(int offset, unsigned long rate)
+{
+	u64 val;
+	u32 mult = 1, div = 1;
+
+	val = readq(loongson2_pll_base + offset);
+
+	mult = (val >> LOONGSON2_PLL_MULT_SHIFT) &
+			clk_div_mask(LOONGSON2_PLL_MULT_WIDTH);
+	div = (val >> LOONGSON2_PLL_DIV_SHIFT) &
+			clk_div_mask(LOONGSON2_PLL_DIV_WIDTH);
+
+	return div_u64((u64)rate * mult, div);
+}
+
+static unsigned long loongson2_node_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_pll_rate(0x0, parent_rate);
+}
+
+static const struct clk_ops loongson2_node_clk_ops = {
+	.recalc_rate = loongson2_node_recalc_rate,
+};
+
+static unsigned long loongson2_ddr_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_pll_rate(0x10, parent_rate);
+}
+
+static const struct clk_ops loongson2_ddr_clk_ops = {
+	.recalc_rate = loongson2_ddr_recalc_rate,
+};
+
+static unsigned long loongson2_dc_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_pll_rate(0x20, parent_rate);
+}
+
+static const struct clk_ops loongson2_dc_clk_ops = {
+	.recalc_rate = loongson2_dc_recalc_rate,
+};
+
+static unsigned long loongson2_pix0_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_pll_rate(0x30, parent_rate);
+}
+
+static const struct clk_ops loongson2_pix0_clk_ops = {
+	.recalc_rate = loongson2_pix0_recalc_rate,
+};
+
+static unsigned long loongson2_pix1_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_pll_rate(0x40, parent_rate);
+}
+
+static const struct clk_ops loongson2_pix1_clk_ops = {
+	.recalc_rate = loongson2_pix1_recalc_rate,
+};
+
+static unsigned long loongson2_calc_rate(unsigned long rate,
+					 int shift, int width)
+{
+	u64 val;
+	u32 mult;
+
+	val = readq(loongson2_pll_base + 0x50);
+
+	mult = (val >> shift) & clk_div_mask(width);
+
+	return div_u64((u64)rate * (mult + 1), 8);
+}
+
+static unsigned long loongson2_boot_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_rate(parent_rate,
+				   LOONGSON2_BOOT_FREQSCALE_SHIFT,
+				   LOONGSON2_BOOT_FREQSCALE_WIDTH);
+}
+
+static const struct clk_ops loongson2_boot_clk_ops = {
+	.recalc_rate = loongson2_boot_recalc_rate,
+};
+
+static unsigned long loongson2_apb_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_rate(parent_rate,
+				   LOONGSON2_APB_FREQSCALE_SHIFT,
+				   LOONGSON2_APB_FREQSCALE_WIDTH);
+}
+
+static const struct clk_ops loongson2_apb_clk_ops = {
+	.recalc_rate = loongson2_apb_recalc_rate,
+};
+
+static unsigned long loongson2_usb_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_rate(parent_rate,
+				   LOONGSON2_USB_FREQSCALE_SHIFT,
+				   LOONGSON2_USB_FREQSCALE_WIDTH);
+}
+
+static const struct clk_ops loongson2_usb_clk_ops = {
+	.recalc_rate = loongson2_usb_recalc_rate,
+};
+
+static unsigned long loongson2_sata_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return loongson2_calc_rate(parent_rate,
+				   LOONGSON2_SATA_FREQSCALE_SHIFT,
+				   LOONGSON2_SATA_FREQSCALE_WIDTH);
+}
+
+static const struct clk_ops loongson2_sata_clk_ops = {
+	.recalc_rate = loongson2_sata_recalc_rate,
+};
+
+static inline void loongson2_check_clk_hws(struct clk_hw *clks[], unsigned int count)
+{
+	unsigned int i;
+
+	for (i = 0; i < count; i++)
+		if (IS_ERR(clks[i]))
+			pr_err("Loongson2 clk %u: register failed with %ld\n",
+				i, PTR_ERR(clks[i]));
+}
+
+static void loongson2_clocks_init(struct device_node *np)
+{
+	struct clk_hw **hws;
+	struct clk_hw_onecell_data *clk_hw_data;
+	spinlock_t loongson2_clk_lock;
+
+	loongson2_pll_base = of_iomap(np, 0);
+
+	if (!loongson2_pll_base) {
+		pr_err("clk: unable to map loongson2 clk registers\n");
+		return;
+	}
+
+	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws, LOONGSON2_CLK_END),
+					GFP_KERNEL);
+	if (WARN_ON(!clk_hw_data))
+		goto err;
+
+	clk_hw_data->num = LOONGSON2_CLK_END;
+	hws = clk_hw_data->hws;
+
+	hws[LOONGSON2_NODE_PLL] = loongson2_clk_register(np, "node_pll",
+						NULL,
+						&loongson2_node_clk_ops, 0);
+
+	hws[LOONGSON2_DDR_PLL] = loongson2_clk_register(np, "ddr_pll",
+						NULL,
+						&loongson2_ddr_clk_ops, 0);
+
+	hws[LOONGSON2_DC_PLL] = loongson2_clk_register(np, "dc_pll",
+						NULL,
+						&loongson2_dc_clk_ops, 0);
+
+	hws[LOONGSON2_PIX0_PLL] = loongson2_clk_register(np, "pix0_pll",
+						NULL,
+						&loongson2_pix0_clk_ops, 0);
+
+	hws[LOONGSON2_PIX1_PLL] = loongson2_clk_register(np, "pix1_pll",
+						NULL,
+						&loongson2_pix1_clk_ops, 0);
+
+	hws[LOONGSON2_BOOT_CLK] = loongson2_clk_register(np, "boot",
+						NULL,
+						&loongson2_boot_clk_ops, 0);
+
+	hws[LOONGSON2_NODE_CLK] = clk_hw_register_divider(NULL, "node",
+						"node_pll", 0,
+						loongson2_pll_base + 0x8, 0,
+						6, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	/*
+	 * The hda clk divisor in the upper 32bits and the clk-prodiver
+	 * layer code doesn't support 64bit io operation thus a conversion
+	 * is required that subtract shift by 32 and add 4byte to the hda
+	 * address
+	 */
+	hws[LOONGSON2_HDA_CLK] = clk_hw_register_divider(NULL, "hda",
+						"ddr_pll", 0,
+						loongson2_pll_base + 0x22, 12,
+						7, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_GPU_CLK] = clk_hw_register_divider(NULL, "gpu",
+						"ddr_pll", 0,
+						loongson2_pll_base + 0x18, 22,
+						6, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_DDR_CLK] = clk_hw_register_divider(NULL, "ddr",
+						"ddr_pll", 0,
+						loongson2_pll_base + 0x18, 0,
+						6, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_GMAC_CLK] = clk_hw_register_divider(NULL, "gmac",
+						"dc_pll", 0,
+						loongson2_pll_base + 0x28, 22,
+						6, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_DC_CLK] = clk_hw_register_divider(NULL, "dc",
+						"dc_pll", 0,
+						loongson2_pll_base + 0x28, 0,
+						6, CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_APB_CLK] = loongson2_clk_register(NULL, "apb",
+						"gmac",
+						&loongson2_apb_clk_ops, 0);
+
+	hws[LOONGSON2_USB_CLK] = loongson2_clk_register(NULL, "usb",
+						"gmac",
+						&loongson2_usb_clk_ops, 0);
+
+	hws[LOONGSON2_SATA_CLK] = loongson2_clk_register(NULL, "sata",
+						"gmac",
+						&loongson2_sata_clk_ops, 0);
+
+	hws[LOONGSON2_PIX0_CLK] = clk_hw_register_divider(NULL, "pix0",
+						"pix0_pll", 0,
+						loongson2_pll_base + 0x38, 0, 6,
+						CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	hws[LOONGSON2_PIX1_CLK] = clk_hw_register_divider(NULL, "pix1",
+						"pix1_pll", 0,
+						loongson2_pll_base + 0x48, 0, 6,
+						CLK_DIVIDER_ONE_BASED,
+						&loongson2_clk_lock);
+
+	loongson2_check_clk_hws(hws, LOONGSON2_CLK_END);
+
+	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
+
+err:
+	iounmap(loongson2_pll_base);
+}
+
+static const struct of_device_id loongson2_clk_match_table[] = {
+	{ .compatible = "loongson,ls2k-clk" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, loongson2_clk_match_table);
+
+static int loongson2_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	loongson2_clocks_init(np);
+
+	return 0;
+}
+
+static struct platform_driver loongson2_clk_driver = {
+	.probe	= loongson2_clk_probe,
+	.driver	= {
+		.name	= "loongson2-clk",
+		.of_match_table	= loongson2_clk_match_table,
+	},
+};
+module_platform_driver(loongson2_clk_driver);
+
+MODULE_DESCRIPTION("Loongson2 clock driver");
+MODULE_LICENSE("GPL");
-- 
2.31.1

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Krzysztof Kozlowski]

On 07/03/2023 12:50, Yinbo Zhu wrote:
> The Loongson-2 boot clock was used to spi and lio peripheral and
> this patch was to add boot clock index number.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---

This is v13? Where is the changelog then?


>  include/dt-bindings/clock/loongson,ls2k-clk.h | 25 ++++++++++---------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/include/dt-bindings/clock/loongson,ls2k-clk.h b/include/dt-bindings/clock/loongson,ls2k-clk.h
> index db1e27e792ff1..e86804365e506 100644
> --- a/include/dt-bindings/clock/loongson,ls2k-clk.h
> +++ b/include/dt-bindings/clock/loongson,ls2k-clk.h
> @@ -13,17 +13,18 @@
>  #define LOONGSON2_DC_PLL				3
>  #define LOONGSON2_PIX0_PLL				4
>  #define LOONGSON2_PIX1_PLL				5
> -#define LOONGSON2_NODE_CLK				6
> -#define LOONGSON2_HDA_CLK				7
> -#define LOONGSON2_GPU_CLK				8
> -#define LOONGSON2_DDR_CLK				9
> -#define LOONGSON2_GMAC_CLK				10
> -#define LOONGSON2_DC_CLK				11
> -#define LOONGSON2_APB_CLK				12
> -#define LOONGSON2_USB_CLK				13
> -#define LOONGSON2_SATA_CLK				14
> -#define LOONGSON2_PIX0_CLK				15
> -#define LOONGSON2_PIX1_CLK				16
> -#define LOONGSON2_CLK_END				17
> +#define LOONGSON2_BOOT_CLK				6

That's an ABI break and commit msg does not explain it.

> +#define LOONGSON2_NODE_CLK				7



Best regards,
Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[zhuyinbo]

在 2023/3/7 下午8:47, Krzysztof Kozlowski 写道:
> On 07/03/2023 12:50, Yinbo Zhu wrote:
>> The Loongson-2 boot clock was used to spi and lio peripheral and
>> this patch was to add boot clock index number.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
> This is v13? Where is the changelog then?

in fact, this is a new patch(v1),   but another clock driver patch in 
this series had send as v13 and need depend on

this patch so set current patch as v13.

>
>
>>   include/dt-bindings/clock/loongson,ls2k-clk.h | 25 ++++++++++---------
>>   1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/dt-bindings/clock/loongson,ls2k-clk.h b/include/dt-bindings/clock/loongson,ls2k-clk.h
>> index db1e27e792ff1..e86804365e506 100644
>> --- a/include/dt-bindings/clock/loongson,ls2k-clk.h
>> +++ b/include/dt-bindings/clock/loongson,ls2k-clk.h
>> @@ -13,17 +13,18 @@
>>   #define LOONGSON2_DC_PLL				3
>>   #define LOONGSON2_PIX0_PLL				4
>>   #define LOONGSON2_PIX1_PLL				5
>> -#define LOONGSON2_NODE_CLK				6
>> -#define LOONGSON2_HDA_CLK				7
>> -#define LOONGSON2_GPU_CLK				8
>> -#define LOONGSON2_DDR_CLK				9
>> -#define LOONGSON2_GMAC_CLK				10
>> -#define LOONGSON2_DC_CLK				11
>> -#define LOONGSON2_APB_CLK				12
>> -#define LOONGSON2_USB_CLK				13
>> -#define LOONGSON2_SATA_CLK				14
>> -#define LOONGSON2_PIX0_CLK				15
>> -#define LOONGSON2_PIX1_CLK				16
>> -#define LOONGSON2_CLK_END				17
>> +#define LOONGSON2_BOOT_CLK				6
> That's an ABI break and commit msg does not explain it.
you meaning is that need add a explanation in commit msg that why

LOONGSON2_BOOT_CLK was added after LOONGSON2_PIX1_PLL and not add it in ending, right?

>
>> +#define LOONGSON2_NODE_CLK				7
>
>
> Best regards,
> Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Krzysztof Kozlowski]

On 08/03/2023 02:35, zhuyinbo wrote:
> 
> 在 2023/3/7 下午8:47, Krzysztof Kozlowski 写道:
>> On 07/03/2023 12:50, Yinbo Zhu wrote:
>>> The Loongson-2 boot clock was used to spi and lio peripheral and
>>> this patch was to add boot clock index number.
>>>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> ---
>> This is v13? Where is the changelog then?
> 
> in fact, this is a new patch(v1),   but another clock driver patch in 
> this series had send as v13 and need depend on
> 
> this patch so set current patch as v13.

This should be explained in changelog.

> 
>>
>>
>>>   include/dt-bindings/clock/loongson,ls2k-clk.h | 25 ++++++++++---------
>>>   1 file changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/dt-bindings/clock/loongson,ls2k-clk.h b/include/dt-bindings/clock/loongson,ls2k-clk.h
>>> index db1e27e792ff1..e86804365e506 100644
>>> --- a/include/dt-bindings/clock/loongson,ls2k-clk.h
>>> +++ b/include/dt-bindings/clock/loongson,ls2k-clk.h
>>> @@ -13,17 +13,18 @@
>>>   #define LOONGSON2_DC_PLL				3
>>>   #define LOONGSON2_PIX0_PLL				4
>>>   #define LOONGSON2_PIX1_PLL				5
>>> -#define LOONGSON2_NODE_CLK				6
>>> -#define LOONGSON2_HDA_CLK				7
>>> -#define LOONGSON2_GPU_CLK				8
>>> -#define LOONGSON2_DDR_CLK				9
>>> -#define LOONGSON2_GMAC_CLK				10
>>> -#define LOONGSON2_DC_CLK				11
>>> -#define LOONGSON2_APB_CLK				12
>>> -#define LOONGSON2_USB_CLK				13
>>> -#define LOONGSON2_SATA_CLK				14
>>> -#define LOONGSON2_PIX0_CLK				15
>>> -#define LOONGSON2_PIX1_CLK				16
>>> -#define LOONGSON2_CLK_END				17
>>> +#define LOONGSON2_BOOT_CLK				6
>> That's an ABI break and commit msg does not explain it.
> you meaning is that need add a explanation in commit msg that why

You need good explanation to break the ABI. I don't understand the
commit msg, but anyway I could not find there justification for ABI
break. If you do not have good justification, don't break the ABI,



Best regards,
Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[zhuyinbo]

在 2023/3/8 下午4:37, Krzysztof Kozlowski 写道:
> On 08/03/2023 02:35, zhuyinbo wrote:
>> 在 2023/3/7 下午8:47, Krzysztof Kozlowski 写道:
>>> On 07/03/2023 12:50, Yinbo Zhu wrote:
>>>> The Loongson-2 boot clock was used to spi and lio peripheral and
>>>> this patch was to add boot clock index number.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> ---
>>> This is v13? Where is the changelog then?
>> in fact, this is a new patch(v1),   but another clock driver patch in
>> this series had send as v13 and need depend on
>>
>> this patch so set current patch as v13.
> This should be explained in changelog.

okay I got it , and I whether need resend a v14 patch that in order to  
add this explain in changelog ?

>
>>>
>>>>    include/dt-bindings/clock/loongson,ls2k-clk.h | 25 ++++++++++---------
>>>>    1 file changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/dt-bindings/clock/loongson,ls2k-clk.h b/include/dt-bindings/clock/loongson,ls2k-clk.h
>>>> index db1e27e792ff1..e86804365e506 100644
>>>> --- a/include/dt-bindings/clock/loongson,ls2k-clk.h
>>>> +++ b/include/dt-bindings/clock/loongson,ls2k-clk.h
>>>> @@ -13,17 +13,18 @@
>>>>    #define LOONGSON2_DC_PLL				3
>>>>    #define LOONGSON2_PIX0_PLL				4
>>>>    #define LOONGSON2_PIX1_PLL				5
>>>> -#define LOONGSON2_NODE_CLK				6
>>>> -#define LOONGSON2_HDA_CLK				7
>>>> -#define LOONGSON2_GPU_CLK				8
>>>> -#define LOONGSON2_DDR_CLK				9
>>>> -#define LOONGSON2_GMAC_CLK				10
>>>> -#define LOONGSON2_DC_CLK				11
>>>> -#define LOONGSON2_APB_CLK				12
>>>> -#define LOONGSON2_USB_CLK				13
>>>> -#define LOONGSON2_SATA_CLK				14
>>>> -#define LOONGSON2_PIX0_CLK				15
>>>> -#define LOONGSON2_PIX1_CLK				16
>>>> -#define LOONGSON2_CLK_END				17
>>>> +#define LOONGSON2_BOOT_CLK				6
>>> That's an ABI break and commit msg does not explain it.
>> you meaning is that need add a explanation in commit msg that why
> You need good explanation to break the ABI. I don't understand the
> commit msg, but anyway I could not find there justification for ABI
> break. If you do not have good justification, don't break the ABI,

The commit msg is the patch commit  log,  and I maybe not got it about 
break the ABI.  You said about "break the ABI"

is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the 
LOONGSON2_BOOT_CLK was placed

after LOONGSON2_PIX1_PLL that is due to their clock parent is same.     
and I whether need add this explanation

in patch commit log description?

>
>
>
> Best regards,
> Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Krzysztof Kozlowski]

On 08/03/2023 10:24, zhuyinbo wrote:
>>>> That's an ABI break and commit msg does not explain it.
>>> you meaning is that need add a explanation in commit msg that why
>> You need good explanation to break the ABI. I don't understand the
>> commit msg, but anyway I could not find there justification for ABI
>> break. If you do not have good justification, don't break the ABI,
> 
> The commit msg is the patch commit  log,  and I maybe not got it about 
> break the ABI.  You said about "break the ABI"
> 
> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the 
> LOONGSON2_BOOT_CLK was placed
> 
> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.     
> and I whether need add this explanation
> 
> in patch commit log description?

Unfortunately I do not understand single thing from this.

Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[kernel test robot]

Hi Yinbo,

I love your patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on robh/for-next linus/master v6.3-rc1 next-20230308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
        git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
        # 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 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/202303082037.QPfBP64A-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
      79 |         val = readq(loongson2_pll_base + offset);
         |               ^~~~~
         |               readl
   cc1: some warnings being treated as errors


vim +79 drivers/clk/clk-loongson2.c

    73	
    74	static unsigned long loongson2_calc_pll_rate(int offset, unsigned long rate)
    75	{
    76		u64 val;
    77		u32 mult = 1, div = 1;
    78	
  > 79		val = readq(loongson2_pll_base + offset);
    80	
    81		mult = (val >> LOONGSON2_PLL_MULT_SHIFT) &
    82				clk_div_mask(LOONGSON2_PLL_MULT_WIDTH);
    83		div = (val >> LOONGSON2_PLL_DIV_SHIFT) &
    84				clk_div_mask(LOONGSON2_PLL_DIV_WIDTH);
    85	
    86		return div_u64((u64)rate * mult, div);
    87	}
    88	

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

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[zhuyinbo]

在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
> On 08/03/2023 10:24, zhuyinbo wrote:
>>>>> That's an ABI break and commit msg does not explain it.
>>>> you meaning is that need add a explanation in commit msg that why
>>> You need good explanation to break the ABI. I don't understand the
>>> commit msg, but anyway I could not find there justification for ABI
>>> break. If you do not have good justification, don't break the ABI,
>> The commit msg is the patch commit  log,  and I maybe not got it about
>> break the ABI.  You said about "break the ABI"
>>
>> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
>> LOONGSON2_BOOT_CLK was placed
>>
>> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
>> and I whether need add this explanation
>>
>> in patch commit log description?
> Unfortunately I do not understand single thing from this.
>
> Best regards,
> Krzysztof

The patch commit log description is patch desription.  as follows:


commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
Author: Yinbo Zhu <zhuyinbo@loongson.cn>
Date:   Tue Mar 7 17:18:32 2023 +0800

     dt-bindings: clock: add loongson-2 boot clock index

     The Loongson-2 boot clock was used to spi and lio peripheral and
     this patch was to add boot clock index number.


and your advice is "That's an ABI break and commit msg does not explain it."

I got it  from your advice that was to add a explanation about 
LOONGSON2_BOOT_CLK's

location issue in patch description, right?

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[zhuyinbo]

在 2023/3/8 下午8:16, kernel test robot 写道:
> Hi Yinbo,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on clk/clk-next]
> [also build test ERROR on robh/for-next linus/master v6.3-rc1 next-20230308]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
> patch link:    https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support
> config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>          git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>          # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
>        79 |         val = readq(loongson2_pll_base + offset);
>           |               ^~~~~
>           |               readl
>     cc1: some warnings being treated as errors

The CONFIG_64BIT not enabled in your config file, I will add a depend on 
"CONFIG_64BIT" in my clock driver to fix this compile error.

>
>
> vim +79 drivers/clk/clk-loongson2.c
>
>      73	
>      74	static unsigned long loongson2_calc_pll_rate(int offset, unsigned long rate)
>      75	{
>      76		u64 val;
>      77		u32 mult = 1, div = 1;
>      78	
>    > 79		val = readq(loongson2_pll_base + offset);
>      80	
>      81		mult = (val >> LOONGSON2_PLL_MULT_SHIFT) &
>      82				clk_div_mask(LOONGSON2_PLL_MULT_WIDTH);
>      83		div = (val >> LOONGSON2_PLL_DIV_SHIFT) &
>      84				clk_div_mask(LOONGSON2_PLL_DIV_WIDTH);
>      85	
>      86		return div_u64((u64)rate * mult, div);
>      87	}
>      88	
>

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[zhuyinbo]

在 2023/3/9 上午10:58, zhuyinbo 写道:
>
> 在 2023/3/8 下午8:16, kernel test robot 写道:
>> Hi Yinbo,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on clk/clk-next]
>> [also build test ERROR on robh/for-next linus/master v6.3-rc1 
>> next-20230308]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url: 
>> https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git 
>> clk-next
>> patch link: 
>> https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
>> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock 
>> controller driver support
>> config: mips-allyesconfig 
>> (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
>> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>>          git remote add linux-review 
>> https://github.com/intel-lab-lkp/linux
>>          git fetch --no-tags linux-review 
>> Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>          git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>>          # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>>
>> All errors (new ones prefixed by >>):
>>
>>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of 
>>>> function 'readq'; did you mean 'readl'? 
>>>> [-Werror=implicit-function-declaration]
>>        79 |         val = readq(loongson2_pll_base + offset);
>>           |               ^~~~~
>>           |               readl
>>     cc1: some warnings being treated as errors
>
> The CONFIG_64BIT not enabled in your config file, I will add a depend 
> on "CONFIG_64BIT" in my clock driver to fix this compile error.
My clock is for LoongArch platform, The LOONGARCH had select 
"CONFIG_64BIT", I will add a depend on "LOONGARCH" in my clock driver to 
fix this compile error.
>
>>
>>
>> vim +79 drivers/clk/clk-loongson2.c
>>
>>      73
>>      74    static unsigned long loongson2_calc_pll_rate(int offset, 
>> unsigned long rate)
>>      75    {
>>      76        u64 val;
>>      77        u32 mult = 1, div = 1;
>>      78
>>    > 79        val = readq(loongson2_pll_base + offset);
>>      80
>>      81        mult = (val >> LOONGSON2_PLL_MULT_SHIFT) &
>>      82                clk_div_mask(LOONGSON2_PLL_MULT_WIDTH);
>>      83        div = (val >> LOONGSON2_PLL_DIV_SHIFT) &
>>      84                clk_div_mask(LOONGSON2_PLL_DIV_WIDTH);
>>      85
>>      86        return div_u64((u64)rate * mult, div);
>>      87    }
>>      88
>>

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Krzysztof Kozlowski]

On 09/03/2023 03:58, zhuyinbo wrote:
> 
> 在 2023/3/8 下午8:16, kernel test robot 写道:
>> Hi Yinbo,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on clk/clk-next]
>> [also build test ERROR on robh/for-next linus/master v6.3-rc1 next-20230308]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
>> patch link:    https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
>> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support
>> config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
>> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>>          git fetch --no-tags linux-review Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>          git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>>          # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>>
>> All errors (new ones prefixed by >>):
>>
>>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
>>        79 |         val = readq(loongson2_pll_base + offset);
>>           |               ^~~~~
>>           |               readl
>>     cc1: some warnings being treated as errors
> 
> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
> "CONFIG_64BIT" in my clock driver to fix this compile error.

No, that's not how it should be fixed. Fix your code instead.



Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Krzysztof Kozlowski]

On 09/03/2023 04:18, zhuyinbo wrote:
> 
> 在 2023/3/9 上午10:58, zhuyinbo 写道:
>>
>> 在 2023/3/8 下午8:16, kernel test robot 写道:
>>> Hi Yinbo,
>>>
>>> I love your patch! Yet something to improve:
>>>
>>> [auto build test ERROR on clk/clk-next]
>>> [also build test ERROR on robh/for-next linus/master v6.3-rc1 
>>> next-20230308]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url: 
>>> https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git 
>>> clk-next
>>> patch link: 
>>> https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
>>> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock 
>>> controller driver support
>>> config: mips-allyesconfig 
>>> (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
>>> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>>>          git remote add linux-review 
>>> https://github.com/intel-lab-lkp/linux
>>>          git fetch --no-tags linux-review 
>>> Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>>          git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>>>          # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of 
>>>>> function 'readq'; did you mean 'readl'? 
>>>>> [-Werror=implicit-function-declaration]
>>>        79 |         val = readq(loongson2_pll_base + offset);
>>>           |               ^~~~~
>>>           |               readl
>>>     cc1: some warnings being treated as errors
>>
>> The CONFIG_64BIT not enabled in your config file, I will add a depend 
>> on "CONFIG_64BIT" in my clock driver to fix this compile error.
> My clock is for LoongArch platform, The LOONGARCH had select 
> "CONFIG_64BIT", I will add a depend on "LOONGARCH" in my clock driver to 
> fix this compile error.

No. Fix your code instead.

Best regards,
Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Krzysztof Kozlowski]

On 09/03/2023 02:43, zhuyinbo wrote:
> 
> 在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
>> On 08/03/2023 10:24, zhuyinbo wrote:
>>>>>> That's an ABI break and commit msg does not explain it.
>>>>> you meaning is that need add a explanation in commit msg that why
>>>> You need good explanation to break the ABI. I don't understand the
>>>> commit msg, but anyway I could not find there justification for ABI
>>>> break. If you do not have good justification, don't break the ABI,
>>> The commit msg is the patch commit  log,  and I maybe not got it about
>>> break the ABI.  You said about "break the ABI"
>>>
>>> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
>>> LOONGSON2_BOOT_CLK was placed
>>>
>>> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
>>> and I whether need add this explanation
>>>
>>> in patch commit log description?
>> Unfortunately I do not understand single thing from this.
>>
>> Best regards,
>> Krzysztof
> 
> The patch commit log description is patch desription.  as follows:
> 
> 
> commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
> Author: Yinbo Zhu <zhuyinbo@loongson.cn>
> Date:   Tue Mar 7 17:18:32 2023 +0800
> 
>      dt-bindings: clock: add loongson-2 boot clock index
> 
>      The Loongson-2 boot clock was used to spi and lio peripheral and
>      this patch was to add boot clock index number.

I cannot understand this either.

> 
> 
> and your advice is "That's an ABI break and commit msg does not explain it."
> 
> I got it  from your advice that was to add a explanation about 
> LOONGSON2_BOOT_CLK's
> 
> location issue in patch description, right?

ABI break needs justification, why do you think it is fine or who
is/isn't affected etc. Your commit msg does not explain why ABI break is
okay. It doesn't even explain to me why you need it.


Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[zhuyinbo]

在 2023/3/9 下午2:09, Krzysztof Kozlowski 写道:
> On 09/03/2023 04:18, zhuyinbo wrote:
>> 在 2023/3/9 上午10:58, zhuyinbo 写道:
>>> 在 2023/3/8 下午8:16, kernel test robot 写道:
>>>> Hi Yinbo,
>>>>
>>>> I love your patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on clk/clk-next]
>>>> [also build test ERROR on robh/for-next linus/master v6.3-rc1
>>>> next-20230308]
>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>
>>>> url:
>>>> https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
>>>> clk-next
>>>> patch link:
>>>> https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
>>>> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock
>>>> controller driver support
>>>> config: mips-allyesconfig
>>>> (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
>>>> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>>>>           git remote add linux-review
>>>> https://github.com/intel-lab-lkp/linux
>>>>           git fetch --no-tags linux-review
>>>> Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>>>           git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>>>>           # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>>>>
>>>> All errors (new ones prefixed by >>):
>>>>
>>>>      drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of
>>>>>> function 'readq'; did you mean 'readl'?
>>>>>> [-Werror=implicit-function-declaration]
>>>>         79 |         val = readq(loongson2_pll_base + offset);
>>>>            |               ^~~~~
>>>>            |               readl
>>>>      cc1: some warnings being treated as errors
>>> The CONFIG_64BIT not enabled in your config file, I will add a depend
>>> on "CONFIG_64BIT" in my clock driver to fix this compile error.
>> My clock is for LoongArch platform, The LOONGARCH had select
>> "CONFIG_64BIT", I will add a depend on "LOONGARCH" in my clock driver to
>> fix this compile error.
> No. Fix your code instead.

but the readq that ask CONFIG_64BIT is enabled,  definition in 
include/asm-generic/io.h

so need enable CONFIG_64BIT for my clk driver.


#ifdef CONFIG_64BIT
#ifndef readq
#define readq readq
static inline u64 readq(const volatile void __iomem *addr)
{
         u64 val;

         __io_br();
         val = __le64_to_cpu(__raw_readq(addr));
         __io_ar();
         return val;
}
#endif
#endif /* CONFIG_64BIT */

>
> Best regards,
> Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Krzysztof Kozlowski]

On 09/03/2023 07:27, zhuyinbo wrote:
> 
> 在 2023/3/9 下午2:09, Krzysztof Kozlowski 写道:
>> On 09/03/2023 04:18, zhuyinbo wrote:
>>> 在 2023/3/9 上午10:58, zhuyinbo 写道:
>>>> 在 2023/3/8 下午8:16, kernel test robot 写道:
>>>>> Hi Yinbo,
>>>>>
>>>>> I love your patch! Yet something to improve:
>>>>>
>>>>> [auto build test ERROR on clk/clk-next]
>>>>> [also build test ERROR on robh/for-next linus/master v6.3-rc1
>>>>> next-20230308]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url:
>>>>> https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
>>>>> clk-next
>>>>> patch link:
>>>>> https://lore.kernel.org/r/20230307115022.12846-2-zhuyinbo%40loongson.cn
>>>>> patch subject: [PATCH v13 2/2] clk: clk-loongson2: add clock
>>>>> controller driver support
>>>>> config: mips-allyesconfig
>>>>> (https://download.01.org/0day-ci/archive/20230308/202303082037.QPfBP64A-lkp@intel.com/config)
>>>>> compiler: mips-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/391d6fc63ac65f5456e4755c9dd85232a6296285
>>>>>           git remote add linux-review
>>>>> https://github.com/intel-lab-lkp/linux
>>>>>           git fetch --no-tags linux-review
>>>>> Yinbo-Zhu/clk-clk-loongson2-add-clock-controller-driver-support/20230307-195252
>>>>>           git checkout 391d6fc63ac65f5456e4755c9dd85232a6296285
>>>>>           # 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 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/202303082037.QPfBP64A-lkp@intel.com/
>>>>>
>>>>> All errors (new ones prefixed by >>):
>>>>>
>>>>>      drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of
>>>>>>> function 'readq'; did you mean 'readl'?
>>>>>>> [-Werror=implicit-function-declaration]
>>>>>         79 |         val = readq(loongson2_pll_base + offset);
>>>>>            |               ^~~~~
>>>>>            |               readl
>>>>>      cc1: some warnings being treated as errors
>>>> The CONFIG_64BIT not enabled in your config file, I will add a depend
>>>> on "CONFIG_64BIT" in my clock driver to fix this compile error.
>>> My clock is for LoongArch platform, The LOONGARCH had select
>>> "CONFIG_64BIT", I will add a depend on "LOONGARCH" in my clock driver to
>>> fix this compile error.
>> No. Fix your code instead.
> 
> but the readq that ask CONFIG_64BIT is enabled,  definition in 
> include/asm-generic/io.h
> 
> so need enable CONFIG_64BIT for my clk driver.

Ah, you are right, the error was not about the cast but missing readq.
Yeah, you should depend on 64BIT. Not on Loongarch because the code
should compile test on other archs.

Best regards,
Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[zhuyinbo]

在 2023/3/9 下午2:25, Krzysztof Kozlowski 写道:
> On 09/03/2023 02:43, zhuyinbo wrote:
>> 在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
>>> On 08/03/2023 10:24, zhuyinbo wrote:
>>>>>>> That's an ABI break and commit msg does not explain it.
>>>>>> you meaning is that need add a explanation in commit msg that why
>>>>> You need good explanation to break the ABI. I don't understand the
>>>>> commit msg, but anyway I could not find there justification for ABI
>>>>> break. If you do not have good justification, don't break the ABI,
>>>> The commit msg is the patch commit  log,  and I maybe not got it about
>>>> break the ABI.  You said about "break the ABI"
>>>>
>>>> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
>>>> LOONGSON2_BOOT_CLK was placed
>>>>
>>>> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
>>>> and I whether need add this explanation
>>>>
>>>> in patch commit log description?
>>> Unfortunately I do not understand single thing from this.
>>>
>>> Best regards,
>>> Krzysztof
>> The patch commit log description is patch desription.  as follows:
>>
>>
>> commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
>> Author: Yinbo Zhu <zhuyinbo@loongson.cn>
>> Date:   Tue Mar 7 17:18:32 2023 +0800
>>
>>       dt-bindings: clock: add loongson-2 boot clock index
>>
>>       The Loongson-2 boot clock was used to spi and lio peripheral and
>>       this patch was to add boot clock index number.
> I cannot understand this either.
I will rework commit msg .
>
>>
>> and your advice is "That's an ABI break and commit msg does not explain it."
>>
>> I got it  from your advice that was to add a explanation about
>> LOONGSON2_BOOT_CLK's
>>
>> location issue in patch description, right?
> ABI break needs justification, why do you think it is fine or who
> is/isn't affected etc. Your commit msg does not explain why ABI break is
> okay. It doesn't even explain to me why you need it.
  #define LOONGSON2_DC_PLL                               3
  #define LOONGSON2_PIX0_PLL                             4
  #define LOONGSON2_PIX1_PLL                             5
-#define LOONGSON2_NODE_CLK                             6
-#define LOONGSON2_HDA_CLK                              7
-#define LOONGSON2_GPU_CLK                              8
-#define LOONGSON2_DDR_CLK                              9
-#define LOONGSON2_GMAC_CLK                             10
-#define LOONGSON2_DC_CLK                               11
-#define LOONGSON2_APB_CLK                              12
-#define LOONGSON2_USB_CLK                              13
-#define LOONGSON2_SATA_CLK                             14
-#define LOONGSON2_PIX0_CLK                             15
-#define LOONGSON2_PIX1_CLK                             16
-#define LOONGSON2_CLK_END                              17
+#define LOONGSON2_BOOT_CLK                             6
+#define LOONGSON2_NODE_CLK                             7

after add my patch, if dts still use above macro and not cause any 
issue. but

if dts not use macro rather than use original clk number index that will 
cause a uncorrect clk,

eg.

-#define LOONGSON2_NODE_CLK                             6

+#define LOONGSON2_NODE_CLK                             7

  this issue is that what you said about  "ABI break",  isn't it ?


About your advice and question and I will use following description as 
patch  commit msg,  what do you think?


dt-bindings: clock: add loongson-2 boot clock index

The spi need to use boot clock and this patch is to add a boot clock 
index about  LOONGSON2_BOOT_CLK

and the LOONGSON2_BOOT_CLK was placed in after LOONGSON2_PIX1_PLL that 
due to

LOONGSON2_PIX1_PLL,  LOONGSON2_PIX0_PLL , LOONGSON2_DC_PLL and 
LOONGSON2_BOOT_CLK

has same parent clock.  In addition, the Loongson  code of the community 
is still in the development stage,

so this patch modification will  not cause uncorrect clk quote issue at 
present.

>
>
> Best regards,
> Krzysztof

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[Krzysztof Kozlowski]

On 09/03/2023 13:44, zhuyinbo wrote:
> 
> 在 2023/3/9 下午2:25, Krzysztof Kozlowski 写道:
>> On 09/03/2023 02:43, zhuyinbo wrote:
>>> 在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
>>>> On 08/03/2023 10:24, zhuyinbo wrote:
>>>>>>>> That's an ABI break and commit msg does not explain it.
>>>>>>> you meaning is that need add a explanation in commit msg that why
>>>>>> You need good explanation to break the ABI. I don't understand the
>>>>>> commit msg, but anyway I could not find there justification for ABI
>>>>>> break. If you do not have good justification, don't break the ABI,
>>>>> The commit msg is the patch commit  log,  and I maybe not got it about
>>>>> break the ABI.  You said about "break the ABI"
>>>>>
>>>>> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
>>>>> LOONGSON2_BOOT_CLK was placed
>>>>>
>>>>> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
>>>>> and I whether need add this explanation
>>>>>
>>>>> in patch commit log description?
>>>> Unfortunately I do not understand single thing from this.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>> The patch commit log description is patch desription.  as follows:
>>>
>>>
>>> commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
>>> Author: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> Date:   Tue Mar 7 17:18:32 2023 +0800
>>>
>>>       dt-bindings: clock: add loongson-2 boot clock index
>>>
>>>       The Loongson-2 boot clock was used to spi and lio peripheral and
>>>       this patch was to add boot clock index number.
>> I cannot understand this either.
> I will rework commit msg .
>>
>>>
>>> and your advice is "That's an ABI break and commit msg does not explain it."
>>>
>>> I got it  from your advice that was to add a explanation about
>>> LOONGSON2_BOOT_CLK's
>>>
>>> location issue in patch description, right?
>> ABI break needs justification, why do you think it is fine or who
>> is/isn't affected etc. Your commit msg does not explain why ABI break is
>> okay. It doesn't even explain to me why you need it.
>   #define LOONGSON2_DC_PLL                               3
>   #define LOONGSON2_PIX0_PLL                             4
>   #define LOONGSON2_PIX1_PLL                             5
> -#define LOONGSON2_NODE_CLK                             6
> -#define LOONGSON2_HDA_CLK                              7
> -#define LOONGSON2_GPU_CLK                              8
> -#define LOONGSON2_DDR_CLK                              9
> -#define LOONGSON2_GMAC_CLK                             10
> -#define LOONGSON2_DC_CLK                               11
> -#define LOONGSON2_APB_CLK                              12
> -#define LOONGSON2_USB_CLK                              13
> -#define LOONGSON2_SATA_CLK                             14
> -#define LOONGSON2_PIX0_CLK                             15
> -#define LOONGSON2_PIX1_CLK                             16
> -#define LOONGSON2_CLK_END                              17
> +#define LOONGSON2_BOOT_CLK                             6
> +#define LOONGSON2_NODE_CLK                             7
> 
> after add my patch, if dts still use above macro and not cause any 
> issue. but
> 
> if dts not use macro rather than use original clk number index that will 
> cause a uncorrect clk,
> 
> eg.
> 
> -#define LOONGSON2_NODE_CLK                             6
> 
> +#define LOONGSON2_NODE_CLK                             7
> 
>   this issue is that what you said about  "ABI break",  isn't it ?
> 
> 
> About your advice and question and I will use following description as 
> patch  commit msg,  what do you think?
> 
> 
> dt-bindings: clock: add loongson-2 boot clock index
> 
> The spi need to use boot clock and this patch is to add a boot clock 
> index about  LOONGSON2_BOOT_CLK
> 
> and the LOONGSON2_BOOT_CLK was placed in after LOONGSON2_PIX1_PLL that 
> due to
> 
> LOONGSON2_PIX1_PLL,  LOONGSON2_PIX0_PLL , LOONGSON2_DC_PLL and 
> LOONGSON2_BOOT_CLK
> 
> has same parent clock.  In addition, the Loongson  code of the community 
> is still in the development stage,
> 
> so this patch modification will  not cause uncorrect clk quote issue at 
> present.

So the reason is same parent clock...? That's not much. These are IDs
and parent clock do not matter. Drop the ID change.

Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Stephen Boyd]

Quoting zhuyinbo (2023-03-08 18:58:02)
> 
> 在 2023/3/8 下午8:16, kernel test robot 写道:
> > Hi Yinbo,
> >
[...]
> >
> >     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
> >>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
> >        79 |         val = readq(loongson2_pll_base + offset);
> >           |               ^~~~~
> >           |               readl
> >     cc1: some warnings being treated as errors
> 
> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
> "CONFIG_64BIT" in my clock driver to fix this compile error.

Do you need to use readq() here? Can you read two 32-bit registers with
readl() and put them together for a 64-bit number?

Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

[zhuyinbo]

在 2023/3/10 上午12:12, Krzysztof Kozlowski 写道:
> On 09/03/2023 13:44, zhuyinbo wrote:
>> 在 2023/3/9 下午2:25, Krzysztof Kozlowski 写道:
>>> On 09/03/2023 02:43, zhuyinbo wrote:
>>>> 在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
>>>>> On 08/03/2023 10:24, zhuyinbo wrote:
>>>>>>>>> That's an ABI break and commit msg does not explain it.
>>>>>>>> you meaning is that need add a explanation in commit msg that why
>>>>>>> You need good explanation to break the ABI. I don't understand the
>>>>>>> commit msg, but anyway I could not find there justification for ABI
>>>>>>> break. If you do not have good justification, don't break the ABI,
>>>>>> The commit msg is the patch commit  log,  and I maybe not got it about
>>>>>> break the ABI.  You said about "break the ABI"
>>>>>>
>>>>>> is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
>>>>>> LOONGSON2_BOOT_CLK was placed
>>>>>>
>>>>>> after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
>>>>>> and I whether need add this explanation
>>>>>>
>>>>>> in patch commit log description?
>>>>> Unfortunately I do not understand single thing from this.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>> The patch commit log description is patch desription.  as follows:
>>>>
>>>>
>>>> commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
>>>> Author: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> Date:   Tue Mar 7 17:18:32 2023 +0800
>>>>
>>>>        dt-bindings: clock: add loongson-2 boot clock index
>>>>
>>>>        The Loongson-2 boot clock was used to spi and lio peripheral and
>>>>        this patch was to add boot clock index number.
>>> I cannot understand this either.
>> I will rework commit msg .
>>>> and your advice is "That's an ABI break and commit msg does not explain it."
>>>>
>>>> I got it  from your advice that was to add a explanation about
>>>> LOONGSON2_BOOT_CLK's
>>>>
>>>> location issue in patch description, right?
>>> ABI break needs justification, why do you think it is fine or who
>>> is/isn't affected etc. Your commit msg does not explain why ABI break is
>>> okay. It doesn't even explain to me why you need it.
>>    #define LOONGSON2_DC_PLL                               3
>>    #define LOONGSON2_PIX0_PLL                             4
>>    #define LOONGSON2_PIX1_PLL                             5
>> -#define LOONGSON2_NODE_CLK                             6
>> -#define LOONGSON2_HDA_CLK                              7
>> -#define LOONGSON2_GPU_CLK                              8
>> -#define LOONGSON2_DDR_CLK                              9
>> -#define LOONGSON2_GMAC_CLK                             10
>> -#define LOONGSON2_DC_CLK                               11
>> -#define LOONGSON2_APB_CLK                              12
>> -#define LOONGSON2_USB_CLK                              13
>> -#define LOONGSON2_SATA_CLK                             14
>> -#define LOONGSON2_PIX0_CLK                             15
>> -#define LOONGSON2_PIX1_CLK                             16
>> -#define LOONGSON2_CLK_END                              17
>> +#define LOONGSON2_BOOT_CLK                             6
>> +#define LOONGSON2_NODE_CLK                             7
>>
>> after add my patch, if dts still use above macro and not cause any
>> issue. but
>>
>> if dts not use macro rather than use original clk number index that will
>> cause a uncorrect clk,
>>
>> eg.
>>
>> -#define LOONGSON2_NODE_CLK                             6
>>
>> +#define LOONGSON2_NODE_CLK                             7
>>
>>    this issue is that what you said about  "ABI break",  isn't it ?
>>
>>
>> About your advice and question and I will use following description as
>> patch  commit msg,  what do you think?
>>
>>
>> dt-bindings: clock: add loongson-2 boot clock index
>>
>> The spi need to use boot clock and this patch is to add a boot clock
>> index about  LOONGSON2_BOOT_CLK
>>
>> and the LOONGSON2_BOOT_CLK was placed in after LOONGSON2_PIX1_PLL that
>> due to
>>
>> LOONGSON2_PIX1_PLL,  LOONGSON2_PIX0_PLL , LOONGSON2_DC_PLL and
>> LOONGSON2_BOOT_CLK
>>
>> has same parent clock.  In addition, the Loongson  code of the community
>> is still in the development stage,
>>
>> so this patch modification will  not cause uncorrect clk quote issue at
>> present.
> So the reason is same parent clock...? That's not much. These are IDs
> and parent clock do not matter. Drop the ID change.

okay,  I will add this marcro  LOONGSON2_BOOT_CLK in ending.


Thanks.

>
> Best regards,
> Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Krzysztof Kozlowski]

On 10/03/2023 00:47, Stephen Boyd wrote:
> Quoting zhuyinbo (2023-03-08 18:58:02)
>>
>> 在 2023/3/8 下午8:16, kernel test robot 写道:
>>> Hi Yinbo,
>>>
> [...]
>>>
>>>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
>>>        79 |         val = readq(loongson2_pll_base + offset);
>>>           |               ^~~~~
>>>           |               readl
>>>     cc1: some warnings being treated as errors
>>
>> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
>> "CONFIG_64BIT" in my clock driver to fix this compile error.
> 
> Do you need to use readq() here? Can you read two 32-bit registers with
> readl() and put them together for a 64-bit number?

If the platform supports 64-bit reads and these are actually one
register, then readq makes sense - code is more readable, smaller, more
efficient.

Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2023-03-10 00:42:47)
> On 10/03/2023 00:47, Stephen Boyd wrote:
> > Quoting zhuyinbo (2023-03-08 18:58:02)
> >>
> >> 在 2023/3/8 下午8:16, kernel test robot 写道:
> >>> Hi Yinbo,
> >>>
> > [...]
> >>>
> >>>     drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
> >>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
> >>>        79 |         val = readq(loongson2_pll_base + offset);
> >>>           |               ^~~~~
> >>>           |               readl
> >>>     cc1: some warnings being treated as errors
> >>
> >> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
> >> "CONFIG_64BIT" in my clock driver to fix this compile error.
> > 
> > Do you need to use readq() here? Can you read two 32-bit registers with
> > readl() and put them together for a 64-bit number?
> 
> If the platform supports 64-bit reads and these are actually one
> register, then readq makes sense - code is more readable, smaller, more
> efficient.
> 

Please read the section in Documentation/driver-api/device-io.rst about
hi_lo_readq() and <linux/io-64-nonatomic-lo-hi.h>. We shouldn't need to
restrict the driver to CONFIG_64BIT. Instead, include one of these
header files to get the IO access primitives.

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[zhuyinbo]

在 2023/3/14 上午2:20, Stephen Boyd 写道:
> Quoting Krzysztof Kozlowski (2023-03-10 00:42:47)
>> On 10/03/2023 00:47, Stephen Boyd wrote:
>>> Quoting zhuyinbo (2023-03-08 18:58:02)
>>>> 在 2023/3/8 下午8:16, kernel test robot 写道:
>>>>> Hi Yinbo,
>>>>>
>>> [...]
>>>>>      drivers/clk/clk-loongson2.c: In function 'loongson2_calc_pll_rate':
>>>>>>> drivers/clk/clk-loongson2.c:79:15: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
>>>>>         79 |         val = readq(loongson2_pll_base + offset);
>>>>>            |               ^~~~~
>>>>>            |               readl
>>>>>      cc1: some warnings being treated as errors
>>>> The CONFIG_64BIT not enabled in your config file, I will add a depend on
>>>> "CONFIG_64BIT" in my clock driver to fix this compile error.
>>> Do you need to use readq() here? Can you read two 32-bit registers with
>>> readl() and put them together for a 64-bit number?
>> If the platform supports 64-bit reads and these are actually one
>> register, then readq makes sense - code is more readable, smaller, more
>> efficient.
>>
> Please read the section in Documentation/driver-api/device-io.rst about
> hi_lo_readq() and <linux/io-64-nonatomic-lo-hi.h>. We shouldn't need to
> restrict the driver to CONFIG_64BIT. Instead, include one of these
> header files to get the IO access primitives.

okay, I got it.

Thanks your advice!

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Krzysztof Kozlowski]

On 13/03/2023 19:20, Stephen Boyd wrote:
>>>> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
>>>> "CONFIG_64BIT" in my clock driver to fix this compile error.
>>>
>>> Do you need to use readq() here? Can you read two 32-bit registers with
>>> readl() and put them together for a 64-bit number?
>>
>> If the platform supports 64-bit reads and these are actually one
>> register, then readq makes sense - code is more readable, smaller, more
>> efficient.
>>
> 
> Please read the section in Documentation/driver-api/device-io.rst about
> hi_lo_readq() and <linux/io-64-nonatomic-lo-hi.h>. We shouldn't need to
> restrict the driver to CONFIG_64BIT. Instead, include one of these
> header files to get the IO access primitives.

These primitives are for 32bit access. Quoting: "on 32-bit
architectures". What's the point of them if the code *will never* run on
32-bit? It will be a fake choice of linux/io-64-nonatomic-lo-hi.h or
linux/io-64-nonatomic-hi-lo.h misleading users to think this was tested
on 32-bit.

Best regards,
Krzysztof

Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2023-03-13 23:49:40)
> On 13/03/2023 19:20, Stephen Boyd wrote:
> >>>> The CONFIG_64BIT not enabled in your config file, I will add a depend on 
> >>>> "CONFIG_64BIT" in my clock driver to fix this compile error.
> >>>
> >>> Do you need to use readq() here? Can you read two 32-bit registers with
> >>> readl() and put them together for a 64-bit number?
> >>
> >> If the platform supports 64-bit reads and these are actually one
> >> register, then readq makes sense - code is more readable, smaller, more
> >> efficient.
> >>
> > 
> > Please read the section in Documentation/driver-api/device-io.rst about
> > hi_lo_readq() and <linux/io-64-nonatomic-lo-hi.h>. We shouldn't need to
> > restrict the driver to CONFIG_64BIT. Instead, include one of these
> > header files to get the IO access primitives.
> 
> These primitives are for 32bit access. Quoting: "on 32-bit
> architectures". What's the point of them if the code *will never* run on
> 32-bit?

They're there to make drivers portable.

> It will be a fake choice of linux/io-64-nonatomic-lo-hi.h or
> linux/io-64-nonatomic-hi-lo.h misleading users to think this was tested
> on 32-bit.
> 

I don't think anyone is really going to care that it hasn't been tested.
It's not like the Linux kernel driver is the source of truth for
integrating IP blocks into different architectures. If it's wrong
someone will fix it when they try to use the hardware on 32-bit systems.

Can the register handle being read/written with two 32-bit accesses? I
still don't think we've had any answer to that question. If so, pick the
one that makes the most sense and move on.

In Linux, we try to write portable drivers. This way anyone can compile
the driver on any host architecture with whatever compiler they're
using. Otherwise, they have to download a cross compiler for the target
architecture to simply build test the code. Also, the Linux kernel is
fairly portable. We try to limit architecture specific code to arch/ and
so anything in drivers/ is ideally portable code.