[PATCH v5 0/8] clk: Don't duplicate initialization on platform_dev

[PATCH v5 0/8] clk: Don't duplicate initialization on platform_dev

[Ricardo Ribalda Delgado]

Clock providers can be probed as a normal platform device, or via
of_clk_init() before the rest of the platform devices are initialized
using the CLK_OF_DECLARE() macro.

If a driver required both initialization methodologies, the core would call
both probe/init functions.

This changeset prevent the dual initialization using the OF_POPULATE flag.

It also creates a new macro, CLK_OF_DECLARE_DRIVER, used by drivers that
require double initialization.

Finally, it adds module platform driver initialization to fixed-factor
and fixed-rate, enabling its use in dt overlays.

The order of the patches allows future bisects of the change. This explains
why  Avoid double initialization of clocks() is done almost at the end.

v5: Changes proposed by: Stephen Boyd <sboyd@codeaurora.org>
-Create CLK_OF_DECLARE_DRIVER() macro
-use it in clk-artpec6, sunxi,nxp

v4: Huge MACRO(), not posted to the list

v3: Use OF_POPULATE flag inside fixed-rate and fixed-factor

v2: Changes proposed by: Stephen Boyd <sboyd@codeaurora.org>
-Add error check
-CodeStyle on of_device_ide
-Use builtin_platform_driver()

When clock providers are added to the device tree after of_clk_init is called
they are not added to the clock provider list. This makes that drivers such
as i2c-xiic.c fail to init, as they may depend on the unadded clock provider.

Ricardo Ribalda Delgado (8):
  clk: core: New macro CLK_OF_DECLARE_DRIVER
  clk: axis: Use new macro CLK_OF_DECLARE_DRIVER
  clk: npx: Use new macro CLK_OF_DECLARE_DRIVER
  clk: sunxi: mod0: Use new macro CLK_OF_DECLARE_DRIVER
  clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER
  clk: core: Avoid double initialization of clocks
  clk: fixed-factor: Convert into a module platform driver
  clk: fixed-rate: Convert into a module platform driver

 drivers/clk/axis/clk-artpec6.c     |  4 +--
 drivers/clk/clk-fixed-factor.c     | 72 +++++++++++++++++++++++++++++++++++---
 drivers/clk/clk-fixed-rate.c       | 69 +++++++++++++++++++++++++++++++++---
 drivers/clk/clk.c                  |  4 +++
 drivers/clk/nxp/clk-lpc18xx-creg.c |  3 +-
 drivers/clk/sunxi/clk-mod0.c       |  3 +-
 drivers/clk/sunxi/clk-sun8i-apb0.c |  4 +--
 include/linux/clk-provider.h       | 12 +++++++
 8 files changed, 156 insertions(+), 15 deletions(-)

-- 
2.8.1

[PATCH v5 1/8] clk: core: New macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This will be used by drivers that requires initialization at
of_clk_init() time and also during platform device probing.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 include/linux/clk-provider.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a39c0c530778..f403b8a5f8ca 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -780,6 +780,18 @@ extern struct of_device_id __clk_of_table;
 
 #define CLK_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
 
+/*
+ * Use this macro when you have a driver that requires two initialization
+ * routines, one at of_clk_init(), and one at platform device probe
+ */
+#define CLK_OF_DECLARE_DRIVER(name, compat, fn) \
+	static void name##_of_clk_init_driver(struct device_node *np)	\
+	{								\
+		of_node_clear_flag(np, OF_POPULATED);			\
+		fn(np);							\
+	}								\
+	OF_DECLARE_1(clk, name, compat, name##_of_clk_init_driver)
+
 #ifdef CONFIG_OF
 int of_clk_add_provider(struct device_node *np,
 			struct clk *(*clk_src_get)(struct of_phandle_args *args,
-- 
2.8.1

[PATCH v5 2/8] clk: axis: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via of_artpec6_clkctrl_setup
and then continues the initialization on artpec6_clkctrl_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/axis/clk-artpec6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/axis/clk-artpec6.c b/drivers/clk/axis/clk-artpec6.c
index ffc988b098e4..da1a073c2236 100644
--- a/drivers/clk/axis/clk-artpec6.c
+++ b/drivers/clk/axis/clk-artpec6.c
@@ -113,8 +113,8 @@ static void of_artpec6_clkctrl_setup(struct device_node *np)
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clkdata->clk_data);
 }
 
-CLK_OF_DECLARE(artpec6_clkctrl, "axis,artpec6-clkctrl",
-	       of_artpec6_clkctrl_setup);
+CLK_OF_DECLARE_DRIVER(artpec6_clkctrl, "axis,artpec6-clkctrl",
+		      of_artpec6_clkctrl_setup);
 
 static int artpec6_clkctrl_probe(struct platform_device *pdev)
 {
-- 
2.8.1

[PATCH v5 3/8] clk: npx: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via lpc18xx_creg_clk_init
and then continues the initialization on lpc18xx_creg_clk_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/nxp/clk-lpc18xx-creg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/nxp/clk-lpc18xx-creg.c b/drivers/clk/nxp/clk-lpc18xx-creg.c
index 9e35749dafdf..c6e802e7e6ec 100644
--- a/drivers/clk/nxp/clk-lpc18xx-creg.c
+++ b/drivers/clk/nxp/clk-lpc18xx-creg.c
@@ -184,7 +184,8 @@ static void __init lpc18xx_creg_clk_init(struct device_node *np)
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_creg_early_data);
 }
-CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
+CLK_OF_DECLARE_DRIVER(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk",
+		      lpc18xx_creg_clk_init);
 
 static struct clk *clk_creg[CREG_CLK_MAX];
 static struct clk_onecell_data clk_creg_data = {
-- 
2.8.1

[PATCH v5 4/8] clk: sunxi: mod0: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via sun4i_a10_mod0_setup
and then continues the initialization on sun4i_a10_mod0_clk_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/sunxi/clk-mod0.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c
index b38d71cec74c..e54266cc1c51 100644
--- a/drivers/clk/sunxi/clk-mod0.c
+++ b/drivers/clk/sunxi/clk-mod0.c
@@ -91,7 +91,8 @@ static void __init sun4i_a10_mod0_setup(struct device_node *node)
 	sunxi_factors_register(node, &sun4i_a10_mod0_data,
 			       &sun4i_a10_mod0_lock, reg);
 }
-CLK_OF_DECLARE(sun4i_a10_mod0, "allwinner,sun4i-a10-mod0-clk", sun4i_a10_mod0_setup);
+CLK_OF_DECLARE_DRIVER(sun4i_a10_mod0, "allwinner,sun4i-a10-mod0-clk",
+		      sun4i_a10_mod0_setup);
 
 static int sun4i_a10_mod0_clk_probe(struct platform_device *pdev)
 {
-- 
2.8.1

[PATCH v5 4/8] clk: sunxi: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via sun4i_a10_mod0_setup
and then continues the initialization on sun4i_a10_mod0_clk_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/sunxi/clk-mod0.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c
index b38d71cec74c..e54266cc1c51 100644
--- a/drivers/clk/sunxi/clk-mod0.c
+++ b/drivers/clk/sunxi/clk-mod0.c
@@ -91,7 +91,8 @@ static void __init sun4i_a10_mod0_setup(struct device_node *node)
 	sunxi_factors_register(node, &sun4i_a10_mod0_data,
 			       &sun4i_a10_mod0_lock, reg);
 }
-CLK_OF_DECLARE(sun4i_a10_mod0, "allwinner,sun4i-a10-mod0-clk", sun4i_a10_mod0_setup);
+CLK_OF_DECLARE_DRIVER(sun4i_a10_mod0, "allwinner,sun4i-a10-mod0-clk",
+		      sun4i_a10_mod0_setup);
 
 static int sun4i_a10_mod0_clk_probe(struct platform_device *pdev)
 {
-- 
2.8.1

[PATCH v5 5/8] clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via sun8i_a23_apb0_setup
and then continues the initialization on sun8i_a23_apb0_clk_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/sunxi/clk-sun8i-apb0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sun8i-apb0.c b/drivers/clk/sunxi/clk-sun8i-apb0.c
index 2ea61debffc1..b6be5d2d9440 100644
--- a/drivers/clk/sunxi/clk-sun8i-apb0.c
+++ b/drivers/clk/sunxi/clk-sun8i-apb0.c
@@ -82,8 +82,8 @@ err_unmap:
 	of_address_to_resource(node, 0, &res);
 	release_mem_region(res.start, resource_size(&res));
 }
-CLK_OF_DECLARE(sun8i_a23_apb0, "allwinner,sun8i-a23-apb0-clk",
-	       sun8i_a23_apb0_setup);
+CLK_OF_DECLARE_DRIVER(sun8i_a23_apb0, "allwinner,sun8i-a23-apb0-clk",
+		      sun8i_a23_apb0_setup);
 
 static int sun8i_a23_apb0_clk_probe(struct platform_device *pdev)
 {
-- 
2.8.1

[PATCH v5 5/8] clk: sunxi: Use new macro CLK_OF_DECLARE_DRIVER

[Ricardo Ribalda Delgado]

This driver initializes a clock provider via sun8i_a23_apb0_setup
and then continues the initialization on sun8i_a23_apb0_clk_probe.

Use the new macro to notify the clk subsystem about this behaviour.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/sunxi/clk-sun8i-apb0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sun8i-apb0.c b/drivers/clk/sunxi/clk-sun8i-apb0.c
index 2ea61debffc1..b6be5d2d9440 100644
--- a/drivers/clk/sunxi/clk-sun8i-apb0.c
+++ b/drivers/clk/sunxi/clk-sun8i-apb0.c
@@ -82,8 +82,8 @@ err_unmap:
 	of_address_to_resource(node, 0, &res);
 	release_mem_region(res.start, resource_size(&res));
 }
-CLK_OF_DECLARE(sun8i_a23_apb0, "allwinner,sun8i-a23-apb0-clk",
-	       sun8i_a23_apb0_setup);
+CLK_OF_DECLARE_DRIVER(sun8i_a23_apb0, "allwinner,sun8i-a23-apb0-clk",
+		      sun8i_a23_apb0_setup);
 
 static int sun8i_a23_apb0_clk_probe(struct platform_device *pdev)
 {
-- 
2.8.1

[PATCH v5 6/8] clk: core: Avoid double initialization of clocks

[Ricardo Ribalda Delgado]

Some clock providers can be initialized via of_clk_init() and also via
platform device probe.

Avoid double initialization of them by setting the OF_POPULATED flag.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 820a939fb6bb..f0403b87f468 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3451,6 +3451,10 @@ void __init of_clk_init(const struct of_device_id *matches)
 					&clk_provider_list, node) {
 			if (force || parent_ready(clk_provider->np)) {
 
+				/* Don't populate platform devices */
+				of_node_set_flag(clk_provider->np,
+						 OF_POPULATED);
+
 				clk_provider->clk_init_cb(clk_provider->np);
 				of_clk_set_defaults(clk_provider->np, true);
 
-- 
2.8.1

[PATCH v5 7/8] clk: fixed-factor: Convert into a module platform driver

[Ricardo Ribalda Delgado]

Adds support for fixed-factor clock providers which have not been
enabled via of_clk_init().

This is required by Device trees overlays that introduce clocks
providers.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk-fixed-factor.c | 72 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 4db3be214077..3fc14d425fba 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
 
 /*
  * DOC: basic fixed multiplier and divider clock that cannot gate
@@ -150,24 +151,25 @@ static const struct of_device_id set_rate_parent_matches[] = {
 /**
  * of_fixed_factor_clk_setup() - Setup function for simple fixed factor clock
  */
-void __init of_fixed_factor_clk_setup(struct device_node *node)
+struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;
 	const char *parent_name;
 	unsigned long flags = 0;
 	u32 div, mult;
+	int ret;
 
 	if (of_property_read_u32(node, "clock-div", &div)) {
 		pr_err("%s Fixed factor clock <%s> must have a clock-div property\n",
 			__func__, node->name);
-		return;
+		return ERR_PTR(-EIO);
 	}
 
 	if (of_property_read_u32(node, "clock-mult", &mult)) {
 		pr_err("%s Fixed factor clock <%s> must have a clock-mult property\n",
 			__func__, node->name);
-		return;
+		return ERR_PTR(-EIO);
 	}
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
@@ -178,10 +180,70 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 
 	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
 					mult, div);
-	if (!IS_ERR(clk))
-		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (IS_ERR(clk))
+		return clk;
+
+	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (ret) {
+		clk_unregister(clk);
+		return ERR_PTR(ret);
+	}
+
+	return clk;
+}
+
+void __init of_fixed_factor_clk_setup(struct device_node *node)
+{
+	_of_fixed_factor_clk_setup(node);
 }
 EXPORT_SYMBOL_GPL(of_fixed_factor_clk_setup);
 CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
 		of_fixed_factor_clk_setup);
+
+static int of_fixed_factor_clk_remove(struct platform_device *pdev)
+{
+	struct clk *clk = platform_get_drvdata(pdev);
+
+	if (clk)
+		clk_unregister_fixed_factor(clk);
+
+	return 0;
+}
+
+static int of_fixed_factor_clk_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+
+	/*
+	 * This function is not executed when of_fixed_factor_clk_setup
+	 * succeeded.
+	 */
+
+	clk = _of_fixed_factor_clk_setup(pdev->dev.of_node);
+
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	platform_set_drvdata(pdev, clk);
+
+	return 0;
+}
+
+static const struct of_device_id of_fixed_factor_clk_ids[] = {
+	{ .compatible = "fixed-factor-clock" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_fixed_factor_clk_ids);
+
+static struct platform_driver of_fixed_factor_clk_driver = {
+	.driver = {
+		.name = "of_fixed_factor_clk",
+		.of_match_table = of_fixed_factor_clk_ids,
+	},
+	.probe = of_fixed_factor_clk_probe,
+	.remove = of_fixed_factor_clk_remove,
+};
+
+builtin_platform_driver(of_fixed_factor_clk_driver);
+
 #endif
-- 
2.8.1

[PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Ricardo Ribalda Delgado]

Adds support for fixed-rate clock providers which have not been
enabled via of_clk_init().

This is required by Device trees overlays that introduce clocks
providers.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk-fixed-rate.c | 69 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 2edb39342a02..e64ba2315880 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -15,6 +15,7 @@
 #include <linux/io.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
 
 /*
  * DOC: basic fixed-rate clock that cannot gate
@@ -160,15 +161,16 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
 /**
  * of_fixed_clk_setup() - Setup function for simple fixed rate clock
  */
-void of_fixed_clk_setup(struct device_node *node)
+struct clk *_of_fixed_clk_setup(struct device_node *node)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;
 	u32 rate;
 	u32 accuracy = 0;
+	int ret;
 
 	if (of_property_read_u32(node, "clock-frequency", &rate))
-		return;
+		return ERR_PTR(-EIO);
 
 	of_property_read_u32(node, "clock-accuracy", &accuracy);
 
@@ -176,9 +178,68 @@ void of_fixed_clk_setup(struct device_node *node)
 
 	clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
 						    0, rate, accuracy);
-	if (!IS_ERR(clk))
-		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (IS_ERR(clk))
+		return clk;
+
+	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (ret) {
+		clk_unregister(clk);
+		return ERR_PTR(ret);
+	}
+
+	return clk;
+}
+
+void of_fixed_clk_setup(struct device_node *node)
+{
+	_of_fixed_clk_setup(node);
 }
 EXPORT_SYMBOL_GPL(of_fixed_clk_setup);
 CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
+
+static int of_fixed_clk_remove(struct platform_device *pdev)
+{
+	struct clk *clk = platform_get_drvdata(pdev);
+
+	if (clk)
+		clk_unregister_fixed_rate(clk);
+
+	return 0;
+}
+
+static int of_fixed_clk_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+
+	/*
+	 * This function is not executed when of_fixed_clk_setup
+	 * succeeded.
+	 */
+
+	clk = _of_fixed_clk_setup(pdev->dev.of_node);
+
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	platform_set_drvdata(pdev, clk);
+
+	return 0;
+}
+
+static const struct of_device_id of_fixed_clk_ids[] = {
+	{ .compatible = "fixed-clock" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_fixed_clk_ids);
+
+static struct platform_driver of_fixed_clk_driver = {
+	.driver = {
+		.name = "of_fixed_clk",
+		.of_match_table = of_fixed_clk_ids,
+	},
+	.probe = of_fixed_clk_probe,
+	.remove = of_fixed_clk_remove,
+};
+
+builtin_platform_driver(of_fixed_clk_driver);
 #endif
-- 
2.8.1

Re: [PATCH v5 0/8] clk: Don't duplicate initialization on platform_dev

[Ricardo Ribalda Delgado]

Hi Stephen


It is anything that am I missing? Can I help you somehow?


Regards!

On Tue, Jul 5, 2016 at 6:23 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Clock providers can be probed as a normal platform device, or via
> of_clk_init() before the rest of the platform devices are initialized
> using the CLK_OF_DECLARE() macro.
>
> If a driver required both initialization methodologies, the core would call
> both probe/init functions.
>
> This changeset prevent the dual initialization using the OF_POPULATE flag.
>
> It also creates a new macro, CLK_OF_DECLARE_DRIVER, used by drivers that
> require double initialization.
>
> Finally, it adds module platform driver initialization to fixed-factor
> and fixed-rate, enabling its use in dt overlays.
>
> The order of the patches allows future bisects of the change. This explains
> why  Avoid double initialization of clocks() is done almost at the end.
>
> v5: Changes proposed by: Stephen Boyd <sboyd@codeaurora.org>
> -Create CLK_OF_DECLARE_DRIVER() macro
> -use it in clk-artpec6, sunxi,nxp
>
> v4: Huge MACRO(), not posted to the list
>
> v3: Use OF_POPULATE flag inside fixed-rate and fixed-factor
>
> v2: Changes proposed by: Stephen Boyd <sboyd@codeaurora.org>
> -Add error check
> -CodeStyle on of_device_ide
> -Use builtin_platform_driver()
>
> When clock providers are added to the device tree after of_clk_init is called
> they are not added to the clock provider list. This makes that drivers such
> as i2c-xiic.c fail to init, as they may depend on the unadded clock provider.
>
> Ricardo Ribalda Delgado (8):
>   clk: core: New macro CLK_OF_DECLARE_DRIVER
>   clk: axis: Use new macro CLK_OF_DECLARE_DRIVER
>   clk: npx: Use new macro CLK_OF_DECLARE_DRIVER
>   clk: sunxi: mod0: Use new macro CLK_OF_DECLARE_DRIVER
>   clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER
>   clk: core: Avoid double initialization of clocks
>   clk: fixed-factor: Convert into a module platform driver
>   clk: fixed-rate: Convert into a module platform driver
>
>  drivers/clk/axis/clk-artpec6.c     |  4 +--
>  drivers/clk/clk-fixed-factor.c     | 72 +++++++++++++++++++++++++++++++++++---
>  drivers/clk/clk-fixed-rate.c       | 69 +++++++++++++++++++++++++++++++++---
>  drivers/clk/clk.c                  |  4 +++
>  drivers/clk/nxp/clk-lpc18xx-creg.c |  3 +-
>  drivers/clk/sunxi/clk-mod0.c       |  3 +-
>  drivers/clk/sunxi/clk-sun8i-apb0.c |  4 +--
>  include/linux/clk-provider.h       | 12 +++++++
>  8 files changed, 156 insertions(+), 15 deletions(-)
>
> --
> 2.8.1
>



-- 
Ricardo Ribalda

Re: [PATCH v5 1/8] clk: core: New macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This will be used by drivers that requires initialization at
> of_clk_init() time and also during platform device probing.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 2/8] clk: axis: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via of_artpec6_clkctrl_setup
> and then continues the initialization on artpec6_clkctrl_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 3/8] clk: npx: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via lpc18xx_creg_clk_init
> and then continues the initialization on lpc18xx_creg_clk_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 4/8] clk: sunxi: mod0: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via sun4i_a10_mod0_setup
> and then continues the initialization on sun4i_a10_mod0_clk_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 4/8] clk: sunxi: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via sun4i_a10_mod0_setup
> and then continues the initialization on sun4i_a10_mod0_clk_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 5/8] clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via sun8i_a23_apb0_setup
> and then continues the initialization on sun8i_a23_apb0_clk_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 5/8] clk: sunxi: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> This driver initializes a clock provider via sun8i_a23_apb0_setup
> and then continues the initialization on sun8i_a23_apb0_clk_probe.
> 
> Use the new macro to notify the clk subsystem about this behaviour.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 6/8] clk: core: Avoid double initialization of clocks

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> Some clock providers can be initialized via of_clk_init() and also via
> platform device probe.
> 
> Avoid double initialization of them by setting the OF_POPULATED flag.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 7/8] clk: fixed-factor: Convert into a module platform driver

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> Adds support for fixed-factor clock providers which have not been
> enabled via of_clk_init().
> 
> This is required by Device trees overlays that introduce clocks
> providers.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Stephen Boyd]

On 07/05, Ricardo Ribalda Delgado wrote:
> Adds support for fixed-rate clock providers which have not been
> enabled via of_clk_init().
> 
> This is required by Device trees overlays that introduce clocks
> providers.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Alan Tull]

On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:

I've stumbled across a of_node_get/put imbalance that happens when the
fixed rate clock is added and deleted using device tree.  The cause is
that this driver calls of_clk_add_provider() when probed, but doesn't
call of_clk_del_provider() when removed.

It looks like a lot of clock drivers share that issue:

$ cd drivers/clk/
$ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
131

It should be a one line fix, but for many files.

I'm not a clock subsystem expert, so please let me know whether I'm
missing something here.

Thanks,
Alan

>
> Adds support for fixed-rate clock providers which have not been
> enabled via of_clk_init().
>
> This is required by Device trees overlays that introduce clocks
> providers.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/clk/clk-fixed-rate.c | 69 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index 2edb39342a02..e64ba2315880 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -15,6 +15,7 @@
>  #include <linux/io.h>
>  #include <linux/err.h>
>  #include <linux/of.h>
> +#include <linux/platform_device.h>
>
>  /*
>   * DOC: basic fixed-rate clock that cannot gate
> @@ -160,15 +161,16 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
>  /**
>   * of_fixed_clk_setup() - Setup function for simple fixed rate clock
>   */
> -void of_fixed_clk_setup(struct device_node *node)
> +struct clk *_of_fixed_clk_setup(struct device_node *node)
>  {
>         struct clk *clk;
>         const char *clk_name = node->name;
>         u32 rate;
>         u32 accuracy = 0;
> +       int ret;
>
>         if (of_property_read_u32(node, "clock-frequency", &rate))
> -               return;
> +               return ERR_PTR(-EIO);
>
>         of_property_read_u32(node, "clock-accuracy", &accuracy);
>
> @@ -176,9 +178,68 @@ void of_fixed_clk_setup(struct device_node *node)
>
>         clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
>                                                     0, rate, accuracy);
> -       if (!IS_ERR(clk))
> -               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +       if (ret) {
> +               clk_unregister(clk);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return clk;
> +}
> +
> +void of_fixed_clk_setup(struct device_node *node)
> +{
> +       _of_fixed_clk_setup(node);
>  }
>  EXPORT_SYMBOL_GPL(of_fixed_clk_setup);
>  CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
> +
> +static int of_fixed_clk_remove(struct platform_device *pdev)
> +{
> +       struct clk *clk = platform_get_drvdata(pdev);
> +
> +       if (clk)
> +               clk_unregister_fixed_rate(clk);
> +
> +       return 0;
> +}
> +
> +static int of_fixed_clk_probe(struct platform_device *pdev)
> +{
> +       struct clk *clk;
> +
> +       /*
> +        * This function is not executed when of_fixed_clk_setup
> +        * succeeded.
> +        */
> +
> +       clk = _of_fixed_clk_setup(pdev->dev.of_node);
> +
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       platform_set_drvdata(pdev, clk);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id of_fixed_clk_ids[] = {
> +       { .compatible = "fixed-clock" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, of_fixed_clk_ids);
> +
> +static struct platform_driver of_fixed_clk_driver = {
> +       .driver = {
> +               .name = "of_fixed_clk",
> +               .of_match_table = of_fixed_clk_ids,
> +       },
> +       .probe = of_fixed_clk_probe,
> +       .remove = of_fixed_clk_remove,
> +};
> +
> +builtin_platform_driver(of_fixed_clk_driver);
>  #endif
> --
> 2.8.1
>

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Ricardo Ribalda Delgado]

Hi Alan


On Thu, Oct 18, 2018 at 9:21 PM Alan Tull <atull@kernel.org> wrote:
>
> On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>
> I've stumbled across a of_node_get/put imbalance that happens when the
> fixed rate clock is added and deleted using device tree.  The cause is
> that this driver calls of_clk_add_provider() when probed, but doesn't
> call of_clk_del_provider() when removed.
>
> It looks like a lot of clock drivers share that issue:
>
> $ cd drivers/clk/
> $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> 131
>
> It should be a one line fix, but for many files.
>
> I'm not a clock subsystem expert, so please let me know whether I'm
> missing something here.

Not an expert either, but I believe that the affected drivers are much
less. We have to consider only the drivers that can be removed

git grep -l  of_clk_add_provider  | xargs grep -l platform_driver |
xargs grep -l  remove | xargs grep -L of_clk_del_provider
drivers/clk/clk-fixed-factor.c
drivers/clk/clk-fixed-rate.c
drivers/clk/davinci/psc.c
drivers/clk/ti/adpll.c
drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c


Also there is something else that I do not undersand:
of_clk_add_hw_provider() is the same as of_clk_add_provider() ?

Maybe is a good idea to remove one of them and use the devm_of_add_hw_provider.

I can try to do that with coccinelle if you want.

Cheers




>
> Thanks,
> Alan
>
> >
> > Adds support for fixed-rate clock providers which have not been
> > enabled via of_clk_init().
> >
> > This is required by Device trees overlays that introduce clocks
> > providers.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  drivers/clk/clk-fixed-rate.c | 69 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 65 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> > index 2edb39342a02..e64ba2315880 100644
> > --- a/drivers/clk/clk-fixed-rate.c
> > +++ b/drivers/clk/clk-fixed-rate.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/io.h>
> >  #include <linux/err.h>
> >  #include <linux/of.h>
> > +#include <linux/platform_device.h>
> >
> >  /*
> >   * DOC: basic fixed-rate clock that cannot gate
> > @@ -160,15 +161,16 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
> >  /**
> >   * of_fixed_clk_setup() - Setup function for simple fixed rate clock
> >   */
> > -void of_fixed_clk_setup(struct device_node *node)
> > +struct clk *_of_fixed_clk_setup(struct device_node *node)
> >  {
> >         struct clk *clk;
> >         const char *clk_name = node->name;
> >         u32 rate;
> >         u32 accuracy = 0;
> > +       int ret;
> >
> >         if (of_property_read_u32(node, "clock-frequency", &rate))
> > -               return;
> > +               return ERR_PTR(-EIO);
> >
> >         of_property_read_u32(node, "clock-accuracy", &accuracy);
> >
> > @@ -176,9 +178,68 @@ void of_fixed_clk_setup(struct device_node *node)
> >
> >         clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
> >                                                     0, rate, accuracy);
> > -       if (!IS_ERR(clk))
> > -               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > +       if (IS_ERR(clk))
> > +               return clk;
> > +
> > +       ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > +       if (ret) {
> > +               clk_unregister(clk);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return clk;
> > +}
> > +
> > +void of_fixed_clk_setup(struct device_node *node)
> > +{
> > +       _of_fixed_clk_setup(node);
> >  }
> >  EXPORT_SYMBOL_GPL(of_fixed_clk_setup);
> >  CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
> > +
> > +static int of_fixed_clk_remove(struct platform_device *pdev)
> > +{
> > +       struct clk *clk = platform_get_drvdata(pdev);
> > +
> > +       if (clk)
> > +               clk_unregister_fixed_rate(clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static int of_fixed_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct clk *clk;
> > +
> > +       /*
> > +        * This function is not executed when of_fixed_clk_setup
> > +        * succeeded.
> > +        */
> > +
> > +       clk = _of_fixed_clk_setup(pdev->dev.of_node);
> > +
> > +       if (IS_ERR(clk))
> > +               return PTR_ERR(clk);
> > +
> > +       platform_set_drvdata(pdev, clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id of_fixed_clk_ids[] = {
> > +       { .compatible = "fixed-clock" },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_fixed_clk_ids);
> > +
> > +static struct platform_driver of_fixed_clk_driver = {
> > +       .driver = {
> > +               .name = "of_fixed_clk",
> > +               .of_match_table = of_fixed_clk_ids,
> > +       },
> > +       .probe = of_fixed_clk_probe,
> > +       .remove = of_fixed_clk_remove,
> > +};
> > +
> > +builtin_platform_driver(of_fixed_clk_driver);
> >  #endif
> > --
> > 2.8.1
> >



-- 
Ricardo Ribalda

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Stephen Boyd]

Quoting Alan Tull (2018-10-18 12:20:58)
> On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> 
> I've stumbled across a of_node_get/put imbalance that happens when the
> fixed rate clock is added and deleted using device tree.  The cause is
> that this driver calls of_clk_add_provider() when probed, but doesn't
> call of_clk_del_provider() when removed.
> 
> It looks like a lot of clock drivers share that issue:
> 
> $ cd drivers/clk/
> $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> 131
> 
> It should be a one line fix, but for many files.
> 
> I'm not a clock subsystem expert, so please let me know whether I'm
> missing something here.
> 

Patches welcome. Please include Fixes: tags for backports. Probably
drivers don't care because clk devices are almost never removed. That
isn't to say it shouldn't be fixed, but just giving some background on
why nobody has fixed it.

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Stephen Boyd]

Quoting Ricardo Ribalda Delgado (2018-10-18 13:02:01)
> Hi Alan
> 
> 
> On Thu, Oct 18, 2018 at 9:21 PM Alan Tull <atull@kernel.org> wrote:
> >
> > On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> > <ricardo.ribalda@gmail.com> wrote:
> >
> > I've stumbled across a of_node_get/put imbalance that happens when the
> > fixed rate clock is added and deleted using device tree.  The cause is
> > that this driver calls of_clk_add_provider() when probed, but doesn't
> > call of_clk_del_provider() when removed.
> >
> > It looks like a lot of clock drivers share that issue:
> >
> > $ cd drivers/clk/
> > $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> > 131
> >
> > It should be a one line fix, but for many files.
> >
> > I'm not a clock subsystem expert, so please let me know whether I'm
> > missing something here.
> 
> Not an expert either, but I believe that the affected drivers are much
> less. We have to consider only the drivers that can be removed
> 
> git grep -l  of_clk_add_provider  | xargs grep -l platform_driver |
> xargs grep -l  remove | xargs grep -L of_clk_del_provider
> drivers/clk/clk-fixed-factor.c
> drivers/clk/clk-fixed-rate.c
> drivers/clk/davinci/psc.c
> drivers/clk/ti/adpll.c
> drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c
> 
> 
> Also there is something else that I do not undersand:
> of_clk_add_hw_provider() is the same as of_clk_add_provider() ?

The difference is the hw provider API deals with struct clk_hw and the
other API deals with struct clk pointers. The preference is to use the
clk_hw based APIs.

Re: [PATCH v5 8/8] clk: fixed-rate: Convert into a module platform driver

[Alan Tull]

On Thu, Oct 18, 2018 at 3:24 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Alan Tull (2018-10-18 12:20:58)
> > On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> > <ricardo.ribalda@gmail.com> wrote:
> >
> > I've stumbled across a of_node_get/put imbalance that happens when the
> > fixed rate clock is added and deleted using device tree.  The cause is
> > that this driver calls of_clk_add_provider() when probed, but doesn't
> > call of_clk_del_provider() when removed.
> >
> > It looks like a lot of clock drivers share that issue:
> >
> > $ cd drivers/clk/
> > $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> > 131
> >
> > It should be a one line fix, but for many files.
> >
> > I'm not a clock subsystem expert, so please let me know whether I'm
> > missing something here.
> >
>
> Patches welcome. Please include Fixes: tags for backports. Probably
> drivers don't care because clk devices are almost never removed. That
> isn't to say it shouldn't be fixed, but just giving some background on
> why nobody has fixed it.

Thanks for the context.  I ran into this while testing some devicetree
overlay code changes.  My use is FPGAs where a clock may be added or
removed if the FPGA is reprogrammed.

Alan