[PATCH 0/6] clk: clocking-wizard: modernize probe

[PATCH 0/6] clk: clocking-wizard: modernize probe

[Harry Austen]

Improve utilised clk/notifier APIs, making use of device managed versions
of functions and make dynamic reconfiguration support optional (because it is
in hardware).

This is currently untested on hardware, so any help testing this would be
much appreciated!

This patchset is based on a previous one [1] ([PATCH v3 0/9] clk:
clocking-wizard: add user clock monitor support), whereby I was attempting to
add support for the user clock monitor functionality. Those three patches (DT
binding, clk driver and UIO driver) have now been removed, with the intention of
getting these simpler tidyup changes merged first, while reworking the design of
the user clock monitor support. These remaining six patches are unchanged from
the aforementioned patchset.

[1] https://lore.kernel.org/20240826123602.1872-1-hpausten@protonmail.com

Harry Austen (6):
  clk: clocking-wizard: simplify probe/remove with devres helpers
  clk: clocking-wizard: use newer clk_hw API
  clk: clocking-wizard: use devres versions of clk_hw API
  clk: clocking-wizard: move clock registration to separate function
  dt-bindings: clock: xilinx: describe whether dynamic reconfig is
    enabled
  clk: clocking-wizard: move dynamic reconfig setup behind flag

 .../bindings/clock/xlnx,clocking-wizard.yaml  |   7 +
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c    | 281 ++++++++----------
 2 files changed, 128 insertions(+), 160 deletions(-)

-- 
2.46.0

[PATCH 1/6] clk: clocking-wizard: simplify probe/remove with devres helpers

[Harry Austen]

Remove need to do various operations in remove callback and error paths
by utilising device managed versions of clock and notifier APIs.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 48 ++++++----------------
 1 file changed, 13 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 19eb3fb7ae319..0ca045849ea3e 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1001,21 +1001,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
 				     "clk_in1 not found\n");
 
-	clk_wzrd->axi_clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
+	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
 	if (IS_ERR(clk_wzrd->axi_clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
 				     "s_axi_aclk not found\n");
-	ret = clk_prepare_enable(clk_wzrd->axi_clk);
-	if (ret) {
-		dev_err(&pdev->dev, "enabling s_axi_aclk failed\n");
-		return ret;
-	}
 	rate = clk_get_rate(clk_wzrd->axi_clk);
 	if (rate > WZRD_ACLK_MAX_FREQ) {
 		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
 			rate);
-		ret = -EINVAL;
-		goto err_disable_clk;
+		return -EINVAL;
 	}
 
 	data = device_get_match_data(&pdev->dev);
@@ -1023,16 +1017,12 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		is_versal = data->is_versal;
 
 	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
-	if (ret || nr_outputs > WZRD_NUM_OUTPUTS) {
-		ret = -EINVAL;
-		goto err_disable_clk;
-	}
+	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
+		return -EINVAL;
 
 	clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_out0", dev_name(&pdev->dev));
-	if (!clkout_name) {
-		ret = -ENOMEM;
-		goto err_disable_clk;
-	}
+	if (!clkout_name)
+		return -ENOMEM;
 
 	if (is_versal) {
 		if (nr_outputs == 1) {
@@ -1090,18 +1080,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		div = 1000;
 	}
 	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
-	if (!clk_name) {
-		ret = -ENOMEM;
-		goto err_disable_clk;
-	}
+	if (!clk_name)
+		return -ENOMEM;
 	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
 			(&pdev->dev, clk_name,
 			 __clk_get_name(clk_wzrd->clk_in1),
 			0, mult, div);
 	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
 		dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
-		goto err_disable_clk;
+		return PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
 	}
 
 	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
@@ -1197,13 +1184,14 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	if (clk_wzrd->speed_grade) {
 		clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
 
-		ret = clk_notifier_register(clk_wzrd->clk_in1,
-					    &clk_wzrd->nb);
+		ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
+						 &clk_wzrd->nb);
 		if (ret)
 			dev_warn(&pdev->dev,
 				 "unable to register clock notifier\n");
 
-		ret = clk_notifier_register(clk_wzrd->axi_clk, &clk_wzrd->nb);
+		ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
+						 &clk_wzrd->nb);
 		if (ret)
 			dev_warn(&pdev->dev,
 				 "unable to register clock notifier\n");
@@ -1215,9 +1203,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	clk_unregister(clk_wzrd->clks_internal[1]);
 err_rm_int_clk:
 	clk_unregister(clk_wzrd->clks_internal[0]);
-err_disable_clk:
-	clk_disable_unprepare(clk_wzrd->axi_clk);
-
 	return ret;
 }
 
@@ -1232,13 +1217,6 @@ static void clk_wzrd_remove(struct platform_device *pdev)
 		clk_unregister(clk_wzrd->clkout[i]);
 	for (i = 0; i < wzrd_clk_int_max; i++)
 		clk_unregister(clk_wzrd->clks_internal[i]);
-
-	if (clk_wzrd->speed_grade) {
-		clk_notifier_unregister(clk_wzrd->axi_clk, &clk_wzrd->nb);
-		clk_notifier_unregister(clk_wzrd->clk_in1, &clk_wzrd->nb);
-	}
-
-	clk_disable_unprepare(clk_wzrd->axi_clk);
 }
 
 static const struct of_device_id clk_wzrd_ids[] = {
-- 
2.46.0

[PATCH 2/6] clk: clocking-wizard: use newer clk_hw API

[Harry Austen]

Utilise clock provider API with struct clk_hw instances instead of the
consumer-side struct clk.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 77 +++++++++++-----------
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 0ca045849ea3e..cd795a4952099 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/math64.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/err.h>
 #include <linux/iopoll.h>
 
@@ -121,26 +122,24 @@ enum clk_wzrd_int_clks {
 /**
  * struct clk_wzrd - Clock wizard private data structure
  *
- * @clk_data:		Clock data
  * @nb:			Notifier block
  * @base:		Memory base
  * @clk_in1:		Handle to input clock 'clk_in1'
  * @axi_clk:		Handle to input clock 's_axi_aclk'
  * @clks_internal:	Internal clocks
- * @clkout:		Output clocks
  * @speed_grade:	Speed grade of the device
  * @suspended:		Flag indicating power state of the device
+ * @clk_data:		Output clock data
  */
 struct clk_wzrd {
-	struct clk_onecell_data clk_data;
 	struct notifier_block nb;
 	void __iomem *base;
 	struct clk *clk_in1;
 	struct clk *axi_clk;
-	struct clk *clks_internal[wzrd_clk_int_max];
-	struct clk *clkout[WZRD_NUM_OUTPUTS];
+	struct clk_hw *clks_internal[wzrd_clk_int_max];
 	unsigned int speed_grade;
 	bool suspended;
+	struct clk_hw_onecell_data clk_data;
 };
 
 /**
@@ -765,7 +764,7 @@ static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
 	.recalc_rate = clk_wzrd_recalc_ratef,
 };
 
-static struct clk *clk_wzrd_register_divf(struct device *dev,
+static struct clk_hw *clk_wzrd_register_divf(struct device *dev,
 					  const char *name,
 					  const char *parent_name,
 					  unsigned long flags,
@@ -805,10 +804,10 @@ static struct clk *clk_wzrd_register_divf(struct device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return hw->clk;
+	return hw;
 }
 
-static struct clk *clk_wzrd_ver_register_divider(struct device *dev,
+static struct clk_hw *clk_wzrd_ver_register_divider(struct device *dev,
 						 const char *name,
 						 const char *parent_name,
 						 unsigned long flags,
@@ -852,10 +851,10 @@ static struct clk *clk_wzrd_ver_register_divider(struct device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return hw->clk;
+	return hw;
 }
 
-static struct clk *clk_wzrd_register_divider(struct device *dev,
+static struct clk_hw *clk_wzrd_register_divider(struct device *dev,
 					     const char *name,
 					     const char *parent_name,
 					     unsigned long flags,
@@ -898,7 +897,7 @@ static struct clk *clk_wzrd_register_divider(struct device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return hw->clk;
+	return hw;
 }
 
 static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
@@ -978,7 +977,12 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	int nr_outputs;
 	int i, ret;
 
-	clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
+	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
+	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
+		return -EINVAL;
+
+	clk_wzrd = devm_kzalloc(&pdev->dev, struct_size(clk_wzrd, clk_data.hws, nr_outputs),
+				GFP_KERNEL);
 	if (!clk_wzrd)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, clk_wzrd);
@@ -1016,17 +1020,13 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	if (data)
 		is_versal = data->is_versal;
 
-	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
-	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
-		return -EINVAL;
-
 	clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_out0", dev_name(&pdev->dev));
 	if (!clkout_name)
 		return -ENOMEM;
 
 	if (is_versal) {
 		if (nr_outputs == 1) {
-			clk_wzrd->clkout[0] = clk_wzrd_ver_register_divider
+			clk_wzrd->clk_data.hws[0] = clk_wzrd_ver_register_divider
 				(&pdev->dev, clkout_name,
 				__clk_get_name(clk_wzrd->clk_in1), 0,
 				clk_wzrd->base, WZRD_CLK_CFG_REG(is_versal, 3),
@@ -1059,7 +1059,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		div = 64;
 	} else {
 		if (nr_outputs == 1) {
-			clk_wzrd->clkout[0] = clk_wzrd_register_divider
+			clk_wzrd->clk_data.hws[0] = clk_wzrd_register_divider
 				(&pdev->dev, clkout_name,
 				__clk_get_name(clk_wzrd->clk_in1), 0,
 				clk_wzrd->base, WZRD_CLK_CFG_REG(is_versal, 3),
@@ -1082,7 +1082,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
 	if (!clk_name)
 		return -ENOMEM;
-	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
+	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_hw_register_fixed_factor
 			(&pdev->dev, clk_name,
 			 __clk_get_name(clk_wzrd->clk_in1),
 			0, mult, div);
@@ -1108,15 +1108,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		if (!div)
 			div = 1;
 
-		clk_mul_name = __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
+		clk_mul_name = clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
 		clk_wzrd->clks_internal[wzrd_clk_mul_div] =
-			clk_register_fixed_factor(&pdev->dev, clk_name,
-						  clk_mul_name, 0, 1, div);
+			clk_hw_register_fixed_factor(&pdev->dev, clk_name,
+						     clk_mul_name, 0, 1, div);
 	} else {
 		ctrl_reg = clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0);
-		clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_divider
+		clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_hw_register_divider
 			(&pdev->dev, clk_name,
-			 __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
+			 clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
 			flags, ctrl_reg, 0, 8, CLK_DIVIDER_ONE_BASED |
 			CLK_DIVIDER_ALLOW_ZERO, &clkwzrd_lock);
 	}
@@ -1136,7 +1136,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		}
 
 		if (is_versal) {
-			clk_wzrd->clkout[i] = clk_wzrd_ver_register_divider
+			clk_wzrd->clk_data.hws[i] = clk_wzrd_ver_register_divider
 						(&pdev->dev,
 						 clkout_name, clk_name, 0,
 						 clk_wzrd->base,
@@ -1148,7 +1148,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 						 DIV_O, &clkwzrd_lock);
 		} else {
 			if (!i)
-				clk_wzrd->clkout[i] = clk_wzrd_register_divf
+				clk_wzrd->clk_data.hws[i] = clk_wzrd_register_divf
 					(&pdev->dev, clkout_name, clk_name, flags, clk_wzrd->base,
 					(WZRD_CLK_CFG_REG(is_versal, 2) + i * 12),
 					WZRD_CLKOUT_DIVIDE_SHIFT,
@@ -1156,7 +1156,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 					CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
 					DIV_O, &clkwzrd_lock);
 			else
-				clk_wzrd->clkout[i] = clk_wzrd_register_divider
+				clk_wzrd->clk_data.hws[i] = clk_wzrd_register_divider
 					(&pdev->dev, clkout_name, clk_name, 0, clk_wzrd->base,
 					(WZRD_CLK_CFG_REG(is_versal, 2) + i * 12),
 					WZRD_CLKOUT_DIVIDE_SHIFT,
@@ -1164,22 +1164,25 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 					CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
 					DIV_O, &clkwzrd_lock);
 		}
-		if (IS_ERR(clk_wzrd->clkout[i])) {
+		if (IS_ERR(clk_wzrd->clk_data.hws[i])) {
 			int j;
 
 			for (j = i + 1; j < nr_outputs; j++)
-				clk_unregister(clk_wzrd->clkout[j]);
+				clk_hw_unregister(clk_wzrd->clk_data.hws[j]);
 			dev_err(&pdev->dev,
 				"unable to register divider clock\n");
-			ret = PTR_ERR(clk_wzrd->clkout[i]);
+			ret = PTR_ERR(clk_wzrd->clk_data.hws[i]);
 			goto err_rm_int_clks;
 		}
 	}
 
 out:
-	clk_wzrd->clk_data.clks = clk_wzrd->clkout;
-	clk_wzrd->clk_data.clk_num = ARRAY_SIZE(clk_wzrd->clkout);
-	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_wzrd->clk_data);
+	clk_wzrd->clk_data.num = nr_outputs;
+	ret = of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register clock provider\n");
+		return ret;
+	}
 
 	if (clk_wzrd->speed_grade) {
 		clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
@@ -1200,9 +1203,9 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	return 0;
 
 err_rm_int_clks:
-	clk_unregister(clk_wzrd->clks_internal[1]);
+	clk_hw_unregister(clk_wzrd->clks_internal[1]);
 err_rm_int_clk:
-	clk_unregister(clk_wzrd->clks_internal[0]);
+	clk_hw_unregister(clk_wzrd->clks_internal[0]);
 	return ret;
 }
 
@@ -1214,9 +1217,9 @@ static void clk_wzrd_remove(struct platform_device *pdev)
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
-		clk_unregister(clk_wzrd->clkout[i]);
+		clk_hw_unregister(clk_wzrd->clk_data.hws[i]);
 	for (i = 0; i < wzrd_clk_int_max; i++)
-		clk_unregister(clk_wzrd->clks_internal[i]);
+		clk_hw_unregister(clk_wzrd->clks_internal[i]);
 }
 
 static const struct of_device_id clk_wzrd_ids[] = {
-- 
2.46.0

[PATCH 3/6] clk: clocking-wizard: use devres versions of clk_hw API

[Harry Austen]

Use device managed versions of the clk_hw API, entirely removing the
need for the driver's remove() callback and greatly simplifying the
probe() function's error paths.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 52 +++++-----------------
 1 file changed, 11 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index cd795a4952099..f332e0eee56c8 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1082,7 +1082,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
 	if (!clk_name)
 		return -ENOMEM;
-	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_hw_register_fixed_factor
+	clk_wzrd->clks_internal[wzrd_clk_mul] = devm_clk_hw_register_fixed_factor
 			(&pdev->dev, clk_name,
 			 __clk_get_name(clk_wzrd->clk_in1),
 			0, mult, div);
@@ -1092,10 +1092,8 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	}
 
 	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
-	if (!clk_name) {
-		ret = -ENOMEM;
-		goto err_rm_int_clk;
-	}
+	if (!clk_name)
+		return -ENOMEM;
 
 	if (is_versal) {
 		edged = !!(readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 20)) &
@@ -1110,11 +1108,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 
 		clk_mul_name = clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
 		clk_wzrd->clks_internal[wzrd_clk_mul_div] =
-			clk_hw_register_fixed_factor(&pdev->dev, clk_name,
-						     clk_mul_name, 0, 1, div);
+			devm_clk_hw_register_fixed_factor(&pdev->dev, clk_name,
+							  clk_mul_name, 0, 1, div);
 	} else {
 		ctrl_reg = clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0);
-		clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_hw_register_divider
+		clk_wzrd->clks_internal[wzrd_clk_mul_div] = devm_clk_hw_register_divider
 			(&pdev->dev, clk_name,
 			 clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
 			flags, ctrl_reg, 0, 8, CLK_DIVIDER_ONE_BASED |
@@ -1122,18 +1120,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	}
 	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
 		dev_err(&pdev->dev, "unable to register divider clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
-		goto err_rm_int_clk;
+		return PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
 	}
 
 	/* register div per output */
 	for (i = nr_outputs - 1; i >= 0 ; i--) {
 		clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 					     "%s_out%d", dev_name(&pdev->dev), i);
-		if (!clkout_name) {
-			ret = -ENOMEM;
-			goto err_rm_int_clk;
-		}
+		if (!clkout_name)
+			return -ENOMEM;
 
 		if (is_versal) {
 			clk_wzrd->clk_data.hws[i] = clk_wzrd_ver_register_divider
@@ -1165,20 +1160,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 					DIV_O, &clkwzrd_lock);
 		}
 		if (IS_ERR(clk_wzrd->clk_data.hws[i])) {
-			int j;
-
-			for (j = i + 1; j < nr_outputs; j++)
-				clk_hw_unregister(clk_wzrd->clk_data.hws[j]);
 			dev_err(&pdev->dev,
 				"unable to register divider clock\n");
-			ret = PTR_ERR(clk_wzrd->clk_data.hws[i]);
-			goto err_rm_int_clks;
+			return PTR_ERR(clk_wzrd->clk_data.hws[i]);
 		}
 	}
 
 out:
 	clk_wzrd->clk_data.num = nr_outputs;
-	ret = of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
+	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register clock provider\n");
 		return ret;
@@ -1201,25 +1191,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_rm_int_clks:
-	clk_hw_unregister(clk_wzrd->clks_internal[1]);
-err_rm_int_clk:
-	clk_hw_unregister(clk_wzrd->clks_internal[0]);
-	return ret;
-}
-
-static void clk_wzrd_remove(struct platform_device *pdev)
-{
-	int i;
-	struct clk_wzrd *clk_wzrd = platform_get_drvdata(pdev);
-
-	of_clk_del_provider(pdev->dev.of_node);
-
-	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
-		clk_hw_unregister(clk_wzrd->clk_data.hws[i]);
-	for (i = 0; i < wzrd_clk_int_max; i++)
-		clk_hw_unregister(clk_wzrd->clks_internal[i]);
 }
 
 static const struct of_device_id clk_wzrd_ids[] = {
@@ -1238,7 +1209,6 @@ static struct platform_driver clk_wzrd_driver = {
 		.pm = &clk_wzrd_dev_pm_ops,
 	},
 	.probe = clk_wzrd_probe,
-	.remove_new = clk_wzrd_remove,
 };
 module_platform_driver(clk_wzrd_driver);
 
-- 
2.46.0

[PATCH 4/6] clk: clocking-wizard: move clock registration to separate function

[Harry Austen]

Provide clear separation of dynamic reconfiguration logic, by moving its
setup procedure to its own dedicated function.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 143 +++++++++++----------
 1 file changed, 75 insertions(+), 68 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index f332e0eee56c8..1a65a7d153c35 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -962,72 +962,30 @@ static const struct versal_clk_data versal_data = {
 	.is_versal	= true,
 };
 
-static int clk_wzrd_probe(struct platform_device *pdev)
+static int clk_wzrd_register_output_clocks(struct device *dev, int nr_outputs)
 {
 	const char *clkout_name, *clk_name, *clk_mul_name;
+	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
 	u32 regl, regh, edge, regld, reghd, edged, div;
-	struct device_node *np = pdev->dev.of_node;
 	const struct versal_clk_data *data;
-	struct clk_wzrd *clk_wzrd;
 	unsigned long flags = 0;
+	bool is_versal = false;
 	void __iomem *ctrl_reg;
 	u32 reg, reg_f, mult;
-	bool is_versal = false;
-	unsigned long rate;
-	int nr_outputs;
-	int i, ret;
-
-	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
-	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
-		return -EINVAL;
-
-	clk_wzrd = devm_kzalloc(&pdev->dev, struct_size(clk_wzrd, clk_data.hws, nr_outputs),
-				GFP_KERNEL);
-	if (!clk_wzrd)
-		return -ENOMEM;
-	platform_set_drvdata(pdev, clk_wzrd);
-
-	clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(clk_wzrd->base))
-		return PTR_ERR(clk_wzrd->base);
-
-	ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
-	if (!ret) {
-		if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
-			dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
-				 clk_wzrd->speed_grade);
-			clk_wzrd->speed_grade = 0;
-		}
-	}
-
-	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
-	if (IS_ERR(clk_wzrd->clk_in1))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
-				     "clk_in1 not found\n");
+	int i;
 
-	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
-	if (IS_ERR(clk_wzrd->axi_clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
-				     "s_axi_aclk not found\n");
-	rate = clk_get_rate(clk_wzrd->axi_clk);
-	if (rate > WZRD_ACLK_MAX_FREQ) {
-		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
-			rate);
-		return -EINVAL;
-	}
-
-	data = device_get_match_data(&pdev->dev);
+	data = device_get_match_data(dev);
 	if (data)
 		is_versal = data->is_versal;
 
-	clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_out0", dev_name(&pdev->dev));
+	clkout_name = devm_kasprintf(dev, GFP_KERNEL, "%s_out0", dev_name(dev));
 	if (!clkout_name)
 		return -ENOMEM;
 
 	if (is_versal) {
 		if (nr_outputs == 1) {
 			clk_wzrd->clk_data.hws[0] = clk_wzrd_ver_register_divider
-				(&pdev->dev, clkout_name,
+				(dev, clkout_name,
 				__clk_get_name(clk_wzrd->clk_in1), 0,
 				clk_wzrd->base, WZRD_CLK_CFG_REG(is_versal, 3),
 				WZRD_CLKOUT_DIVIDE_SHIFT,
@@ -1035,7 +993,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 				CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
 				DIV_ALL, &clkwzrd_lock);
 
-			goto out;
+			return 0;
 		}
 		/* register multiplier */
 		edge = !!(readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0)) &
@@ -1060,7 +1018,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	} else {
 		if (nr_outputs == 1) {
 			clk_wzrd->clk_data.hws[0] = clk_wzrd_register_divider
-				(&pdev->dev, clkout_name,
+				(dev, clkout_name,
 				__clk_get_name(clk_wzrd->clk_in1), 0,
 				clk_wzrd->base, WZRD_CLK_CFG_REG(is_versal, 3),
 				WZRD_CLKOUT_DIVIDE_SHIFT,
@@ -1068,7 +1026,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 				CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
 				DIV_ALL, &clkwzrd_lock);
 
-			goto out;
+			return 0;
 		}
 		reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0));
 		reg_f = reg & WZRD_CLKFBOUT_FRAC_MASK;
@@ -1079,19 +1037,19 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		mult = (reg * 1000) + reg_f;
 		div = 1000;
 	}
-	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
+	clk_name = devm_kasprintf(dev, GFP_KERNEL, "%s_mul", dev_name(dev));
 	if (!clk_name)
 		return -ENOMEM;
 	clk_wzrd->clks_internal[wzrd_clk_mul] = devm_clk_hw_register_fixed_factor
-			(&pdev->dev, clk_name,
+			(dev, clk_name,
 			 __clk_get_name(clk_wzrd->clk_in1),
 			0, mult, div);
 	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
-		dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
+		dev_err(dev, "unable to register fixed-factor clock\n");
 		return PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
 	}
 
-	clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
+	clk_name = devm_kasprintf(dev, GFP_KERNEL, "%s_mul_div", dev_name(dev));
 	if (!clk_name)
 		return -ENOMEM;
 
@@ -1108,31 +1066,29 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 
 		clk_mul_name = clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
 		clk_wzrd->clks_internal[wzrd_clk_mul_div] =
-			devm_clk_hw_register_fixed_factor(&pdev->dev, clk_name,
-							  clk_mul_name, 0, 1, div);
+			devm_clk_hw_register_fixed_factor(dev, clk_name, clk_mul_name, 0, 1, div);
 	} else {
 		ctrl_reg = clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0);
 		clk_wzrd->clks_internal[wzrd_clk_mul_div] = devm_clk_hw_register_divider
-			(&pdev->dev, clk_name,
+			(dev, clk_name,
 			 clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
 			flags, ctrl_reg, 0, 8, CLK_DIVIDER_ONE_BASED |
 			CLK_DIVIDER_ALLOW_ZERO, &clkwzrd_lock);
 	}
 	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
-		dev_err(&pdev->dev, "unable to register divider clock\n");
+		dev_err(dev, "unable to register divider clock\n");
 		return PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
 	}
 
 	/* register div per output */
 	for (i = nr_outputs - 1; i >= 0 ; i--) {
-		clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-					     "%s_out%d", dev_name(&pdev->dev), i);
+		clkout_name = devm_kasprintf(dev, GFP_KERNEL, "%s_out%d", dev_name(dev), i);
 		if (!clkout_name)
 			return -ENOMEM;
 
 		if (is_versal) {
 			clk_wzrd->clk_data.hws[i] = clk_wzrd_ver_register_divider
-						(&pdev->dev,
+						(dev,
 						 clkout_name, clk_name, 0,
 						 clk_wzrd->base,
 						 (WZRD_CLK_CFG_REG(is_versal, 3) + i * 8),
@@ -1144,7 +1100,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		} else {
 			if (!i)
 				clk_wzrd->clk_data.hws[i] = clk_wzrd_register_divf
-					(&pdev->dev, clkout_name, clk_name, flags, clk_wzrd->base,
+					(dev, clkout_name, clk_name, flags, clk_wzrd->base,
 					(WZRD_CLK_CFG_REG(is_versal, 2) + i * 12),
 					WZRD_CLKOUT_DIVIDE_SHIFT,
 					WZRD_CLKOUT_DIVIDE_WIDTH,
@@ -1152,7 +1108,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 					DIV_O, &clkwzrd_lock);
 			else
 				clk_wzrd->clk_data.hws[i] = clk_wzrd_register_divider
-					(&pdev->dev, clkout_name, clk_name, 0, clk_wzrd->base,
+					(dev, clkout_name, clk_name, 0, clk_wzrd->base,
 					(WZRD_CLK_CFG_REG(is_versal, 2) + i * 12),
 					WZRD_CLKOUT_DIVIDE_SHIFT,
 					WZRD_CLKOUT_DIVIDE_WIDTH,
@@ -1160,13 +1116,64 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 					DIV_O, &clkwzrd_lock);
 		}
 		if (IS_ERR(clk_wzrd->clk_data.hws[i])) {
-			dev_err(&pdev->dev,
-				"unable to register divider clock\n");
+			dev_err(dev, "unable to register divider clock\n");
 			return PTR_ERR(clk_wzrd->clk_data.hws[i]);
 		}
 	}
 
-out:
+	return 0;
+}
+
+static int clk_wzrd_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk_wzrd *clk_wzrd;
+	unsigned long rate;
+	int nr_outputs;
+	int ret;
+
+	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
+	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
+		return -EINVAL;
+
+	clk_wzrd = devm_kzalloc(&pdev->dev, struct_size(clk_wzrd, clk_data.hws, nr_outputs),
+				GFP_KERNEL);
+	if (!clk_wzrd)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, clk_wzrd);
+
+	clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(clk_wzrd->base))
+		return PTR_ERR(clk_wzrd->base);
+
+	ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
+	if (!ret) {
+		if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
+			dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
+				 clk_wzrd->speed_grade);
+			clk_wzrd->speed_grade = 0;
+		}
+	}
+
+	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
+	if (IS_ERR(clk_wzrd->clk_in1))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
+				     "clk_in1 not found\n");
+
+	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
+	if (IS_ERR(clk_wzrd->axi_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
+				     "s_axi_aclk not found\n");
+	rate = clk_get_rate(clk_wzrd->axi_clk);
+	if (rate > WZRD_ACLK_MAX_FREQ) {
+		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
+		return -EINVAL;
+	}
+
+	ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
+	if (ret)
+		return ret;
+
 	clk_wzrd->clk_data.num = nr_outputs;
 	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
 	if (ret) {
-- 
2.46.0

[PATCH 5/6] dt-bindings: clock: xilinx: describe whether dynamic reconfig is enabled

[Harry Austen]

Xilinx clocking wizard IP core's dynamic reconfiguration support is
optionally enabled at build time. Add a devicetree boolean property to
describe whether the hardware supports this feature or not.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/clock/xlnx,clocking-wizard.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
index 9d5324dc1027a..46547ccdc9005 100644
--- a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
+++ b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
@@ -40,6 +40,12 @@ properties:
       - const: s_axi_aclk
 
 
+  xlnx,dynamic-reconfig:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicate whether the core has been configured with support for dynamic
+      runtime reconfguration of the clocking primitive MMCM/PLL.
+
   xlnx,speed-grade:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 3]
@@ -70,6 +76,7 @@ examples:
         compatible = "xlnx,clocking-wizard";
         reg = <0xb0000000 0x10000>;
         #clock-cells = <1>;
+        xlnx,dynamic-reconfig;
         xlnx,speed-grade = <1>;
         xlnx,nr-outputs = <6>;
         clock-names = "clk_in1", "s_axi_aclk";
-- 
2.46.0

[PATCH 6/6] clk: clocking-wizard: move dynamic reconfig setup behind flag

[Harry Austen]

Xilinx clocking wizard IP core's dynamic reconfiguration support is
optionally enabled at build time. Use the new boolean devicetree
property to indicate whether the hardware supports this feature or not.

Signed-off-by: Harry Austen <hpausten@protonmail.com>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 73 +++++++++++-----------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 1a65a7d153c35..967eacc28050d 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	if (IS_ERR(clk_wzrd->base))
 		return PTR_ERR(clk_wzrd->base);
 
-	ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
-	if (!ret) {
-		if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
-			dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
-				 clk_wzrd->speed_grade);
-			clk_wzrd->speed_grade = 0;
-		}
-	}
-
-	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
-	if (IS_ERR(clk_wzrd->clk_in1))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
-				     "clk_in1 not found\n");
-
 	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
 	if (IS_ERR(clk_wzrd->axi_clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
@@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
-	if (ret)
-		return ret;
-
-	clk_wzrd->clk_data.num = nr_outputs;
-	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to register clock provider\n");
-		return ret;
-	}
+	if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) {
+		ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
+		if (!ret) {
+			if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
+				dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
+					 clk_wzrd->speed_grade);
+				clk_wzrd->speed_grade = 0;
+			}
+		}
 
-	if (clk_wzrd->speed_grade) {
-		clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
+		clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
+		if (IS_ERR(clk_wzrd->clk_in1))
+			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
+					     "clk_in1 not found\n");
 
-		ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
-						 &clk_wzrd->nb);
+		ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
 		if (ret)
-			dev_warn(&pdev->dev,
-				 "unable to register clock notifier\n");
+			return ret;
+
+		clk_wzrd->clk_data.num = nr_outputs;
+		ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+						  &clk_wzrd->clk_data);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to register clock provider\n");
+			return ret;
+		}
 
-		ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
-						 &clk_wzrd->nb);
-		if (ret)
-			dev_warn(&pdev->dev,
-				 "unable to register clock notifier\n");
+		if (clk_wzrd->speed_grade) {
+			clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
+
+			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
+							 &clk_wzrd->nb);
+			if (ret)
+				dev_warn(&pdev->dev,
+					 "unable to register clock notifier\n");
+
+			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
+							 &clk_wzrd->nb);
+			if (ret)
+				dev_warn(&pdev->dev,
+					 "unable to register clock notifier\n");
+		}
 	}
 
 	return 0;
-- 
2.46.0

Re: [PATCH 6/6] clk: clocking-wizard: move dynamic reconfig setup behind flag

[Stephen Boyd]

Quoting Harry Austen (2024-08-31 04:13:26)
> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 1a65a7d153c35..967eacc28050d 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>         if (IS_ERR(clk_wzrd->base))
>                 return PTR_ERR(clk_wzrd->base);
>  
> -       ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
> -       if (!ret) {
> -               if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
> -                       dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
> -                                clk_wzrd->speed_grade);
> -                       clk_wzrd->speed_grade = 0;
> -               }
> -       }
> -
> -       clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
> -       if (IS_ERR(clk_wzrd->clk_in1))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
> -                                    "clk_in1 not found\n");
> -
>         clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
>         if (IS_ERR(clk_wzrd->axi_clk))
>                 return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> @@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>  
> -       ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
> -       if (ret)
> -               return ret;
> -
> -       clk_wzrd->clk_data.num = nr_outputs;
> -       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
> -       if (ret) {
> -               dev_err(&pdev->dev, "unable to register clock provider\n");
> -               return ret;
> -       }
> +       if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) {

Is this going to break the existing DTBs? Before the property existed,
the driver seems to have always tried to read xlnx,speed-grade and then
setup a notifier, etc. Why would xlnx,speed-grade be set if
xlnx,dynamic-reconfig wasn't set?

The binding has implicitly had xlnx,dynamic-reconfig set before this
patch, and we should strive to maintain that. Perhaps the property
should be inverted, i.e. xlnx,static-config, and the absence of that
property can imply the reconfig property.

Re: [PATCH 6/6] clk: clocking-wizard: move dynamic reconfig setup behind flag

[Harry Austen]

On Thu Sep 5, 2024 at 8:06 PM BST, Stephen Boyd wrote:
> Quoting Harry Austen (2024-08-31 04:13:26)
> > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > index 1a65a7d153c35..967eacc28050d 100644
> > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > @@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> >         if (IS_ERR(clk_wzrd->base))
> >                 return PTR_ERR(clk_wzrd->base);
> >
> > -       ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
> > -       if (!ret) {
> > -               if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
> > -                       dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
> > -                                clk_wzrd->speed_grade);
> > -                       clk_wzrd->speed_grade = 0;
> > -               }
> > -       }
> > -
> > -       clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
> > -       if (IS_ERR(clk_wzrd->clk_in1))
> > -               return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
> > -                                    "clk_in1 not found\n");
> > -
> >         clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> >         if (IS_ERR(clk_wzrd->axi_clk))
> >                 return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> > @@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > -       ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
> > -       if (ret)
> > -               return ret;
> > -
> > -       clk_wzrd->clk_data.num = nr_outputs;
> > -       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data);
> > -       if (ret) {
> > -               dev_err(&pdev->dev, "unable to register clock provider\n");
> > -               return ret;
> > -       }
> > +       if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) {
>
> Is this going to break the existing DTBs? Before the property existed,
> the driver seems to have always tried to read xlnx,speed-grade and then
> setup a notifier, etc. Why would xlnx,speed-grade be set if
> xlnx,dynamic-reconfig wasn't set?

I did wonder about this. What is the kernel's policy on breaking DT ABI?
Especially in this case where there are no such DTs in the kernel source
tree (AMD/Xilinx have their own tools for devicetree generation, e.g. see
the clocking wizard DT node generation TCL script on GitHub [1]). Agree it
would be better to maintain compatibility with existing DTs if it makes
sense to do so though.

In terms of speed grade, as you say it currently only affects how you
would want to interact with the dynamic reconfiguration registers. But
that's not to say that it might be relevant to some other register added in
future (since it is just a generic property of the chip). So using presence
of the xlnx,speed-grade property to indicate dynamic reconfiguration
enablement feels like potentially a bad idea as well.

>
> The binding has implicitly had xlnx,dynamic-reconfig set before this
> patch, and we should strive to maintain that. Perhaps the property
> should be inverted, i.e. xlnx,static-config, and the absence of that
> property can imply the reconfig property.

I don't mind this. A bit of a shame that it's then inverted to how the IP
core is configured through Vivado, but that's not the end of the world.
Unless anyone can think of a better idea, will probably go with this in v2
of this patchset.

Thanks for the review!
Harry

[1]: https://github.com/Xilinx/device-tree-xlnx/blob/master/axi_clk_wiz/data/axi_clk_wiz.tcl