linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Convert clk-fixed into module platform driver
@ 2016-06-08 10:19 Ricardo Ribalda Delgado
  2016-06-08 10:19 ` [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-08 10:19 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, s.hauer, jeremy.kerr, linux-clk,
	linux-kernel
  Cc: Ricardo Ribalda Delgado

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.

We first introduce a new function clk.c that will check if a device_node is
already a clock provider or not.

Later we add two patches, one for clk-fixed-factor and another to
clk-fixed-rate that make us of that function and also convert both drivers
to module platform driver, but keeping all the previous functionality intact.

Ricardo Ribalda Delgado (3):
  clk: Add new function of_clk_is_provider()
  clk: fixed-factor: Convert into a module platform driver
  clk: fixed-rate: Convert into a module platform driver

 drivers/clk/clk-fixed-factor.c | 66 ++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/clk-fixed-rate.c   | 62 +++++++++++++++++++++++++++++++++++++--
 drivers/clk/clk.c              | 20 +++++++++++++
 include/linux/clk-provider.h   |  5 ++++
 4 files changed, 148 insertions(+), 5 deletions(-)

-- 
2.8.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] clk: Add new function of_clk_is_provider()
  2016-06-08 10:19 [PATCH 0/3] Convert clk-fixed into module platform driver Ricardo Ribalda Delgado
@ 2016-06-08 10:19 ` Ricardo Ribalda Delgado
  2016-06-16  0:44   ` Stephen Boyd
  2016-06-08 10:20 ` [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver Ricardo Ribalda Delgado
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-08 10:19 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, s.hauer, jeremy.kerr, linux-clk,
	linux-kernel
  Cc: Ricardo Ribalda Delgado

of_clk_is_provider() checks if a device_node has already been added to
the clk provider list. This can be used to avoid adding the same clock
provider twice.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk.c            | 20 ++++++++++++++++++++
 include/linux/clk-provider.h |  5 +++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d584004f7af7..2423c6373906 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3120,6 +3120,26 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
 	return hw;
 }
 
+/**
+ * of_clk_is_provider() - Reports if a device node is already a clk provider
+ * @np: Device node pointer under test
+ */
+bool of_clk_is_provider(struct device_node *np)
+{
+	struct of_clk_provider *cp;
+
+	mutex_lock(&of_clk_mutex);
+	list_for_each_entry(cp, &of_clk_providers, link) {
+		if (cp->node == np) {
+			mutex_unlock(&of_clk_mutex);
+			return true;
+		}
+	}
+	mutex_unlock(&of_clk_mutex);
+	return false;
+}
+EXPORT_SYMBOL_GPL(of_clk_is_provider);
+
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 				       const char *dev_id, const char *con_id)
 {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index fb39d5add173..a01b18797418 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -787,6 +787,7 @@ int of_clk_add_hw_provider(struct device_node *np,
 						 void *data),
 			   void *data);
 void of_clk_del_provider(struct device_node *np);
+bool of_clk_is_provider(struct device_node *np);
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				  void *data);
 struct clk_hw *of_clk_hw_simple_get(struct of_phandle_args *clkspec,
@@ -819,6 +820,10 @@ static inline int of_clk_add_hw_provider(struct device_node *np,
 	return 0;
 }
 static inline void of_clk_del_provider(struct device_node *np) {}
+static inline bool of_clk_is_provider(struct device_node *np)
+{
+	return false;
+}
 static inline struct clk *of_clk_src_simple_get(
 	struct of_phandle_args *clkspec, void *data)
 {
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver
  2016-06-08 10:19 [PATCH 0/3] Convert clk-fixed into module platform driver Ricardo Ribalda Delgado
  2016-06-08 10:19 ` [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado
@ 2016-06-08 10:20 ` Ricardo Ribalda Delgado
  2016-06-16  0:47   ` Stephen Boyd
  2016-06-08 10:20 ` [PATCH 3/3] clk: fixed-rate: " Ricardo Ribalda Delgado
  2016-06-14 17:39 ` [PATCH 0/3] Convert clk-fixed into " Stephen Boyd
  3 siblings, 1 reply; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-08 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, s.hauer, jeremy.kerr, linux-clk,
	linux-kernel
  Cc: 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 | 66 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 75cd6c792cb8..6dfce84ef10c 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
@@ -145,7 +146,7 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_factor);
 /**
  * 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;
@@ -155,13 +156,13 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 	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);
@@ -171,8 +172,67 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 					mult, div);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+	return clk;
+}
+
+void __init of_fixed_factor_clk_setup(struct device_node *node)
+{
+	_of_fixed_factor_clk_setup(node);
+	return;
 }
 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;
+
+	/*
+	 * Don't do anything if of_clk_init() has already
+	 * added this clock to the provider list
+	 */
+	if (of_clk_is_provider(pdev->dev.of_node))
+		return 0;
+
+	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,
+};
+
+module_platform_driver(of_fixed_factor_clk_driver);
+
 #endif
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] clk: fixed-rate: Convert into a module platform driver
  2016-06-08 10:19 [PATCH 0/3] Convert clk-fixed into module platform driver Ricardo Ribalda Delgado
  2016-06-08 10:19 ` [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado
  2016-06-08 10:20 ` [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver Ricardo Ribalda Delgado
@ 2016-06-08 10:20 ` Ricardo Ribalda Delgado
  2016-06-14 17:39 ` [PATCH 0/3] Convert clk-fixed into " Stephen Boyd
  3 siblings, 0 replies; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-08 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, s.hauer, jeremy.kerr, linux-clk,
	linux-kernel
  Cc: 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 | 62 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 8e4453eb54e8..190b1e8ea9ac 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
@@ -149,7 +150,7 @@ EXPORT_SYMBOL_GPL(clk_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;
@@ -157,7 +158,7 @@ void of_fixed_clk_setup(struct device_node *node)
 	u32 accuracy = 0;
 
 	if (of_property_read_u32(node, "clock-frequency", &rate))
-		return;
+		return ERR_PTR(-EIO);
 
 	of_property_read_u32(node, "clock-accuracy", &accuracy);
 
@@ -167,7 +168,64 @@ void of_fixed_clk_setup(struct device_node *node)
 						    0, rate, accuracy);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+	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;
+
+	/*
+	 * Don't do anything if of_clk_init() has already
+	 * added this clock to the provider list
+	 */
+	if (of_clk_is_provider(pdev->dev.of_node))
+		return 0;
+
+	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,
+};
+
+module_platform_driver(of_fixed_clk_driver);
 #endif
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] Convert clk-fixed into module platform driver
  2016-06-08 10:19 [PATCH 0/3] Convert clk-fixed into module platform driver Ricardo Ribalda Delgado
                   ` (2 preceding siblings ...)
  2016-06-08 10:20 ` [PATCH 3/3] clk: fixed-rate: " Ricardo Ribalda Delgado
@ 2016-06-14 17:39 ` Stephen Boyd
  2016-06-14 22:59   ` Ricardo Ribalda Delgado
  3 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2016-06-14 17:39 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Michael Turquette, s.hauer, jeremy.kerr, linux-clk, linux-kernel

On 06/08, Ricardo Ribalda Delgado wrote:
> 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.

Who's the provider here? It isn't clear to me why we're
populating fixed factor and fixed rate clks from DT for i2c
devices? Presumably there's an i2c device driver that should be
populating clks from C code instead?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] Convert clk-fixed into module platform driver
  2016-06-14 17:39 ` [PATCH 0/3] Convert clk-fixed into " Stephen Boyd
@ 2016-06-14 22:59   ` Ricardo Ribalda Delgado
  2016-06-16  0:27     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-14 22:59 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, Sascha Hauer, jeremy.kerr, linux-clk, LKML

Hi Stephen

Assume this device tree overlay:


&bus0{

axi_clk: axi_clk {
compatible = "fixed-clock";
#clock-cells = <0x0>;
clock-frequency = <125000000>;
};

iic_0: iic {
#address-cells = <1>;
#size-cells = <1>;
compatible = "xlnx,xps-iic-2.00.a";
reg = < 0x00030000 0x10000 >;
interrupt-parent = <&xps_intc_0>;
interrupts = < 2 2 >;
clocks = <&axi_clk>;
 } ;

}

Which is basically a new i2c master and a fixed clock definition.

The fixed clock driver will only be probed to the driver at arch
initialization, when of_clk_init is called.  The device overlay can be
added at any point, usually after arch init. Which will result in ii_0
failing to probe,  because it is missing its clock.

This changeset allows fixed clocks to be added after of_clk_init is
called, by converting the driver into a platform driver.


Thanks!b

On Tue, Jun 14, 2016 at 7:39 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/08, Ricardo Ribalda Delgado wrote:
>> 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.
>
> Who's the provider here? It isn't clear to me why we're
> populating fixed factor and fixed rate clks from DT for i2c
> devices? Presumably there's an i2c device driver that should be
> populating clks from C code instead?
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project



-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] Convert clk-fixed into module platform driver
  2016-06-14 22:59   ` Ricardo Ribalda Delgado
@ 2016-06-16  0:27     ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2016-06-16  0:27 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Michael Turquette, Sascha Hauer, jeremy.kerr, linux-clk, LKML

On 06/15, Ricardo Ribalda Delgado wrote:
> Hi Stephen
> 
> Assume this device tree overlay:
> 
> 
> &bus0{
> 
> axi_clk: axi_clk {
> compatible = "fixed-clock";
> #clock-cells = <0x0>;
> clock-frequency = <125000000>;
> };
> 
> iic_0: iic {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "xlnx,xps-iic-2.00.a";
> reg = < 0x00030000 0x10000 >;
> interrupt-parent = <&xps_intc_0>;
> interrupts = < 2 2 >;
> clocks = <&axi_clk>;
>  } ;
> 
> }
> 
> Which is basically a new i2c master and a fixed clock definition.
> 
> The fixed clock driver will only be probed to the driver at arch
> initialization, when of_clk_init is called.  The device overlay can be
> added at any point, usually after arch init. Which will result in ii_0
> failing to probe,  because it is missing its clock.
> 
> This changeset allows fixed clocks to be added after of_clk_init is
> called, by converting the driver into a platform driver.
> 

Ok, so this is about supporting clks in overlays. I2C only comes
into the picture here because it's a consumer and happens to also
be on the same overlay.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()
  2016-06-08 10:19 ` [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado
@ 2016-06-16  0:44   ` Stephen Boyd
  2016-06-20 13:31     ` Ricardo Ribalda Delgado
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2016-06-16  0:44 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Michael Turquette, s.hauer, linux-clk, linux-kernel

On 06/08, Ricardo Ribalda Delgado wrote:
> of_clk_is_provider() checks if a device_node has already been added to
> the clk provider list. This can be used to avoid adding the same clock
> provider twice.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

While I don't disagree with the concept, I'd like to do this
outside of the clk framework checking for nodes, because the
problem doesn't seem clk specific. From digging in the OF
platform layer I see of_node_test_and_set_flag(OF_POPULATED) may
be what we should be using. It looks like this can be used to
make sure that any clk provider nodes aren't populated as
platform devices when we've initialized them early.

The only problem now is that we have drivers using a hybrid
approach with of_clk_init(). Sometimes drivers need to get clks
up early for timers, so they have CLK_OF_DECLARE() in their
driver, but then they also use a platform driver to handle the
non-timer related clks. If we mark all nodes as populated in
of_clk_init() we'll preclude these drivers from working. The
solution there is to make those drivers specifically clear the
populated flag in the clk init callback. Or we can automatically
do that with some new CLK_OF_DECLARE_EARLY() macro that hides
this clearing from them. Either way, the drivers will need to
indicate they're using this hybrid style so that we still
populate platform devices.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver
  2016-06-08 10:20 ` [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver Ricardo Ribalda Delgado
@ 2016-06-16  0:47   ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2016-06-16  0:47 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Michael Turquette, s.hauer, jeremy.kerr, linux-clk, linux-kernel

On 06/08, Ricardo Ribalda Delgado wrote:
>  	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);
> @@ -171,8 +172,67 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
>  					mult, div);
>  	if (!IS_ERR(clk))
>  		of_clk_add_provider(node, of_clk_src_simple_get, clk);

What if this fails now?

> +
> +	return clk;
> +}
> +
[..]
> +
> +static const struct of_device_id of_fixed_factor_clk_ids[] = {
> +	{
> +		.compatible = "fixed-factor-clock",
> +	},
> +	{ },
> +};

Nitpick: Do it this way:

  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,
> +};
> +
> +module_platform_driver(of_fixed_factor_clk_driver);
> +

This should be builtin_platform_driver() because these are obj-y
right now.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()
  2016-06-16  0:44   ` Stephen Boyd
@ 2016-06-20 13:31     ` Ricardo Ribalda Delgado
  2016-06-21  1:30       ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-20 13:31 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Michael Turquette, Sascha Hauer, linux-clk, LKML

Hi Stephen

When the device tree is populated or when an overlay is added, all its
nodes have the flag OF_POPULATED set. The flag is enabled recursively
in
of_platform_bus_create->of_platform_device_create_pdata()
So we cannot use that flag to mark what is enabled and what is not.

The other issue that I see is of_clk_mutex. Whatever final
implementation that we decide to do, it should take into consideration
that mutex, otherwise it will not be thread-safe.
of_clk_is_provider() is already taking care of it.

Another advantage of of_clk_is_provider() is that it opens the door to
implement something like:  CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove)
 That allows a driver to implement early clk and platform clk at the
same time and automatically, following a logic similar to what I have
done in fixed-clk.

I will send a v2 with the changes you proposed to fixed-clk

Thanks and best regards!

On Thu, Jun 16, 2016 at 2:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/08, Ricardo Ribalda Delgado wrote:
>> of_clk_is_provider() checks if a device_node has already been added to
>> the clk provider list. This can be used to avoid adding the same clock
>> provider twice.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>
> While I don't disagree with the concept, I'd like to do this
> outside of the clk framework checking for nodes, because the
> problem doesn't seem clk specific. From digging in the OF
> platform layer I see of_node_test_and_set_flag(OF_POPULATED) may
> be what we should be using. It looks like this can be used to
> make sure that any clk provider nodes aren't populated as
> platform devices when we've initialized them early.
>
> The only problem now is that we have drivers using a hybrid
> approach with of_clk_init(). Sometimes drivers need to get clks
> up early for timers, so they have CLK_OF_DECLARE() in their
> driver, but then they also use a platform driver to handle the
> non-timer related clks. If we mark all nodes as populated in
> of_clk_init() we'll preclude these drivers from working. The
> solution there is to make those drivers specifically clear the
> populated flag in the clk init callback. Or we can automatically
> do that with some new CLK_OF_DECLARE_EARLY() macro that hides
> this clearing from them. Either way, the drivers will need to
> indicate they're using this hybrid style so that we still
> populate platform devices.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project



-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()
  2016-06-20 13:31     ` Ricardo Ribalda Delgado
@ 2016-06-21  1:30       ` Stephen Boyd
  2016-06-21  8:38         ` Ricardo Ribalda Delgado
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2016-06-21  1:30 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado; +Cc: Michael Turquette, Sascha Hauer, linux-clk, LKML

(Please don't top post)

On 06/20, Ricardo Ribalda Delgado wrote:
> Hi Stephen
> 
> When the device tree is populated or when an overlay is added, all its
> nodes have the flag OF_POPULATED set. The flag is enabled recursively
> in
> of_platform_bus_create->of_platform_device_create_pdata()
> So we cannot use that flag to mark what is enabled and what is not.

Sorry I don't follow the reasoning here. I'm not asking to test
that flag in an of_clk_is_provider() API. The goal is to not have
an of_clk_is_provider() API.

I was thinking that of_clk_init() would mark any nodes that
matched and provided clk providers as OF_POPULATED. That way,
when of_platform_populate() ran, it would *not* add platform
devices for clk providers that we registered during the
of_clk_init() phase. Then we could have platform drivers and
CLK_OF_DECLARE drivers for the same compatible strings, but we
wouldn't probe random platform drivers for the nodes that we
handled early on and we wouldn't need to litter
of_clk_is_provider() in driver probe routines.

> 
> The other issue that I see is of_clk_mutex. Whatever final
> implementation that we decide to do, it should take into consideration
> that mutex, otherwise it will not be thread-safe.
> of_clk_is_provider() is already taking care of it.
> 
> Another advantage of of_clk_is_provider() is that it opens the door to
> implement something like:  CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove)
>  That allows a driver to implement early clk and platform clk at the
> same time and automatically, following a logic similar to what I have
> done in fixed-clk.

Something like CLK_OF_DECLARE_EARLY_PLATFORM() sounds like it may
be good (I haven't looked at the patch). We'll need something to
say that there's CLK_OF_DECLARE and a platform driver for the
same node.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()
  2016-06-21  1:30       ` Stephen Boyd
@ 2016-06-21  8:38         ` Ricardo Ribalda Delgado
  0 siblings, 0 replies; 12+ messages in thread
From: Ricardo Ribalda Delgado @ 2016-06-21  8:38 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Michael Turquette, Sascha Hauer, linux-clk, LKML

Hi Stephen

On Tue, Jun 21, 2016 at 3:30 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> (Please don't top post)

Sorry about that

>
> I was thinking that of_clk_init() would mark any nodes that
> matched and provided clk providers as OF_POPULATED. That way,
> when of_platform_populate() ran, it would *not* add platform
> devices for clk providers that we registered during the
> of_clk_init() phase. Then we could have platform drivers and
> CLK_OF_DECLARE drivers for the same compatible strings, but we
> wouldn't probe random platform drivers for the nodes that we
> handled early on and we wouldn't need to litter
> of_clk_is_provider() in driver probe routines.
>

Ok, now I get it. I didn't realised that you wanted to set the flag.

I have prepared two new patches.  now setup does something like:

void __init of_fixed_factor_clk_setup(struct device_node *node)
{
        if (!_of_fixed_factor_clk_setup(node))
                  of_node_set_flag(node, OF_POPULATED);
}

If we probe this method to be valid, on a future stage I can make a MACRO like:

CLK_OF_DECLARE_PLATFORM(fixed_factor_clk, "fixed-factor-clock",
_of_fixed_factor_clk_setup, clk_unregister_fixed_factor);

That sets the flag, instantiate MODULE_DEVICE_TABLE,
builtin_platform_driver,  and does the platform_set_drvdata.....

Driver developers can choose to use CLK_OF_DECLARE_PLATFORM or CLK_OF_DECLARE


Thanks!

-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-06-21  8:39 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-08 10:19 [PATCH 0/3] Convert clk-fixed into module platform driver Ricardo Ribalda Delgado
2016-06-08 10:19 ` [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado
2016-06-16  0:44   ` Stephen Boyd
2016-06-20 13:31     ` Ricardo Ribalda Delgado
2016-06-21  1:30       ` Stephen Boyd
2016-06-21  8:38         ` Ricardo Ribalda Delgado
2016-06-08 10:20 ` [PATCH 2/3] clk: fixed-factor: Convert into a module platform driver Ricardo Ribalda Delgado
2016-06-16  0:47   ` Stephen Boyd
2016-06-08 10:20 ` [PATCH 3/3] clk: fixed-rate: " Ricardo Ribalda Delgado
2016-06-14 17:39 ` [PATCH 0/3] Convert clk-fixed into " Stephen Boyd
2016-06-14 22:59   ` Ricardo Ribalda Delgado
2016-06-16  0:27     ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).