[PATCH 0/2] defer clk_gets on orphan clocks

[PATCH 0/2] defer clk_gets on orphan clocks

[Emilio López]

Hi everyone,

This is mostly a resend of Heiko's patch from [0] together with a fix for
the breakage seen on sunxi when it was applied.

Ideally we would move off our own protected clocks thing to the CCF
implementation when it is merged and solve this nicely. Meanwhile, we
can solve this non-intrusively by delaying the clock protection a bit.
This way the rk people can enjoy the happiness brought by working sensors
and WiFi[1] while sunxi boards still boot and operate just fine.

I have tested this on a Cubieboard 1 (A10) and A10S OLinuXino Micro (A10S)
as well as remotely boot-tested a Radxa Rock2 Square (rk3288)

As always, all comments welcome :)

Thanks!
Emilio

[0] http://www.spinics.net/lists/arm-kernel/msg462821.html
[1] http://www.spinics.net/lists/arm-kernel/msg473236.html

Emilio López (1):
  clk: sunxi: delay protected clocks until arch initcall

Heiko Stuebner (1):
  clk: defer clk_gets on orphan clocks

 drivers/clk/clk-conf.c        |  5 +++--
 drivers/clk/clk.c             | 48 ++++++++++++++++++++++++++++++++++---------
 drivers/clk/clk.h             | 10 ++++++---
 drivers/clk/clkdev.c          |  4 ++--
 drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++----
 5 files changed, 68 insertions(+), 21 deletions(-)

-- 
2.7.0

[PATCH 0/2] defer clk_gets on orphan clocks

[Emilio López]

Hi everyone,

This is mostly a resend of Heiko's patch from [0] together with a fix for
the breakage seen on sunxi when it was applied.

Ideally we would move off our own protected clocks thing to the CCF
implementation when it is merged and solve this nicely. Meanwhile, we
can solve this non-intrusively by delaying the clock protection a bit.
This way the rk people can enjoy the happiness brought by working sensors
and WiFi[1] while sunxi boards still boot and operate just fine.

I have tested this on a Cubieboard 1 (A10) and A10S OLinuXino Micro (A10S)
as well as remotely boot-tested a Radxa Rock2 Square (rk3288)

As always, all comments welcome :)

Thanks!
Emilio

[0] http://www.spinics.net/lists/arm-kernel/msg462821.html
[1] http://www.spinics.net/lists/arm-kernel/msg473236.html

Emilio L?pez (1):
  clk: sunxi: delay protected clocks until arch initcall

Heiko Stuebner (1):
  clk: defer clk_gets on orphan clocks

 drivers/clk/clk-conf.c        |  5 +++--
 drivers/clk/clk.c             | 48 ++++++++++++++++++++++++++++++++++---------
 drivers/clk/clk.h             | 10 ++++++---
 drivers/clk/clkdev.c          |  4 ++--
 drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++----
 5 files changed, 68 insertions(+), 21 deletions(-)

-- 
2.7.0

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Emilio López]

Clocks are registered early on, and unused clocks get disabled on
late initcall, so we can delay protecting important clocks a bit.
If we do this too early, it may happen that some clocks are orphans
and therefore enabling them may not work as intended. If we do this
too late, a driver may reparent some clock and cause another important
clock to be disabled as a byproduct.

arch_initcall should be a good spot to do this, as clock drivers using
the OF mechanisms will be all registered by then, and drivers won't
have started probing yet.

Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
 drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 5ba2188..285e8ee 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
 	}
 }
 
+/* By default, don't protect any clocks */
+static const char **protected_clocks __initdata;
+static int protected_clocks_nr __initdata;
+
 static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
 {
-	unsigned int i;
-
 	/* Register divided output clocks */
 	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
 
@@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
 	/* Register mux clocks */
 	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
 
+	/* We shall protect these clocks when everything is ready */
+	protected_clocks = clocks;
+	protected_clocks_nr = nclocks;
+}
+
+static int __init sunxi_init_clock_protection(void)
+{
+	unsigned int i;
+
 	/* Protect the clocks that needs to stay on */
-	for (i = 0; i < nclocks; i++) {
-		struct clk *clk = clk_get(NULL, clocks[i]);
+	for (i = 0; i < protected_clocks_nr; i++) {
+		struct clk *clk = clk_get(NULL, protected_clocks[i]);
 
 		if (!IS_ERR(clk))
 			clk_prepare_enable(clk);
 	}
+
+	return 0;
 }
+arch_initcall(sunxi_init_clock_protection);
 
 static const char *sun4i_a10_critical_clocks[] __initdata = {
 	"pll5_ddr",
-- 
2.7.0

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Emilio López]

Clocks are registered early on, and unused clocks get disabled on
late initcall, so we can delay protecting important clocks a bit.
If we do this too early, it may happen that some clocks are orphans
and therefore enabling them may not work as intended. If we do this
too late, a driver may reparent some clock and cause another important
clock to be disabled as a byproduct.

arch_initcall should be a good spot to do this, as clock drivers using
the OF mechanisms will be all registered by then, and drivers won't
have started probing yet.

Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
---
 drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 5ba2188..285e8ee 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
 	}
 }
 
+/* By default, don't protect any clocks */
+static const char **protected_clocks __initdata;
+static int protected_clocks_nr __initdata;
+
 static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
 {
-	unsigned int i;
-
 	/* Register divided output clocks */
 	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
 
@@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
 	/* Register mux clocks */
 	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
 
+	/* We shall protect these clocks when everything is ready */
+	protected_clocks = clocks;
+	protected_clocks_nr = nclocks;
+}
+
+static int __init sunxi_init_clock_protection(void)
+{
+	unsigned int i;
+
 	/* Protect the clocks that needs to stay on */
-	for (i = 0; i < nclocks; i++) {
-		struct clk *clk = clk_get(NULL, clocks[i]);
+	for (i = 0; i < protected_clocks_nr; i++) {
+		struct clk *clk = clk_get(NULL, protected_clocks[i]);
 
 		if (!IS_ERR(clk))
 			clk_prepare_enable(clk);
 	}
+
+	return 0;
 }
+arch_initcall(sunxi_init_clock_protection);
 
 static const char *sun4i_a10_critical_clocks[] __initdata = {
 	"pll5_ddr",
-- 
2.7.0

[PATCH 2/2] clk: defer clk_gets on orphan clocks

[Emilio López]

From: Heiko Stuebner <heiko.stuebner@collabora.com>

Orphan clocks or children of orphan clocks don't have rate information at
all and can produce strange results if they're allowed to be used and the
parent becomes available later on.

This change, based on one from Stephen Boyd defers __clk_create_clk()
calls on orphan clocks in all regular cases.

One special case that gets handled, is accessing such orphan clocks when
handling assigned-clocks configurations. In the boot-defaults it may be
the case that a clock is connected to an orphan parent which then might
be needed to get reparented to an actually usable clock using
assigned-clock-parents. In this case even orphaned clocks should be
usable, but only for the set-parent case.

The added of_clk_get_from_provider_with_orphans() is only available
to ccf internal parts to prevent abuse.

Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com>
---
 drivers/clk/clk-conf.c |  5 +++--
 drivers/clk/clk.c      | 48 ++++++++++++++++++++++++++++++++++++++----------
 drivers/clk/clk.h      | 10 +++++++---
 drivers/clk/clkdev.c   |  4 ++--
 4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 43a218f..60ebfd9 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/printk.h>
+#include "clk.h"
 
 static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 {
@@ -38,7 +39,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 		}
 		if (clkspec.np == node && !clk_supplier)
 			return 0;
-		pclk = of_clk_get_from_provider(&clkspec);
+		pclk = of_clk_get_from_provider_with_orphans(&clkspec);
 		if (IS_ERR(pclk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
@@ -53,7 +54,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			rc = 0;
 			goto err;
 		}
-		clk = of_clk_get_from_provider(&clkspec);
+		clk = of_clk_get_from_provider_with_orphans(&clkspec);
 		if (IS_ERR(clk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b4db67a..36fe1a0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2494,15 +2494,11 @@ out:
 	return ret;
 }
 
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id)
+struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
+			      const char *con_id)
 {
 	struct clk *clk;
 
-	/* This is to allow this function to be chained to others */
-	if (IS_ERR_OR_NULL(hw))
-		return (struct clk *) hw;
-
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return ERR_PTR(-ENOMEM);
@@ -2519,6 +2515,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	return clk;
 }
 
+struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+			     const char *con_id, bool with_orphans)
+{
+	/* This is to allow this function to be chained to others */
+	if (IS_ERR_OR_NULL(hw))
+		return (struct clk *) hw;
+
+	if (hw->core->orphan && !with_orphans)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return clk_hw_create_clk(hw, dev_id, con_id);
+}
+
 void __clk_free_clk(struct clk *clk)
 {
 	clk_prepare_lock();
@@ -2587,7 +2596,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 	INIT_HLIST_HEAD(&core->clks);
 
-	hw->clk = __clk_create_clk(hw, NULL, NULL);
+	hw->clk = clk_hw_create_clk(hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
 		goto fail_parent_names_copy;
@@ -3019,7 +3028,8 @@ void of_clk_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_clk_del_provider);
 
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
-				       const char *dev_id, const char *con_id)
+				       const char *dev_id, const char *con_id,
+				       bool with_orphans)
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
@@ -3034,7 +3044,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 			clk = provider->get(clkspec, provider->data);
 		if (!IS_ERR(clk)) {
 			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
-					       con_id);
+					       con_id, with_orphans);
 
 			if (!IS_ERR(clk) && !__clk_get(clk)) {
 				__clk_free_clk(clk);
@@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
  */
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
-	return __of_clk_get_from_provider(clkspec, NULL, __func__);
+	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
+}
+
+/**
+ * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ *
+ * The difference to of_clk_get_from_provider() is that this function will
+ * also successfully lookup orphan-clocks, as it in some cases may be
+ * necessary to access such orphan-clocks as well.
+ */
+struct clk *
+of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)
+{
+	return __of_clk_get_from_provider(clkspec, NULL, __func__, true);
 }
 
 int of_clk_get_parent_count(struct device_node *np)
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 00b35a1..5a70f09 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -13,17 +13,21 @@ struct clk_hw;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
-				       const char *dev_id, const char *con_id);
+				       const char *dev_id, const char *con_id,
+				       bool with_orphans);
+struct clk *
+of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec);
 #endif
 
 #ifdef CONFIG_COMMON_CLK
 struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id);
+			     const char *con_id, bool with_orphans);
 void __clk_free_clk(struct clk *clk);
 #else
 /* All these casts to avoid ifdefs in clkdev... */
 static inline struct clk *
-__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id,
+		 bool with_orphans)
 {
 	return (struct clk *)hw;
 }
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 779b6ff..eda20c2 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -43,7 +43,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index,
 	if (rc)
 		return ERR_PTR(rc);
 
-	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
+	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id, false);
 	of_node_put(clkspec.np);
 
 	return clk;
@@ -177,7 +177,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 	if (!cl)
 		goto out;
 
-	clk = __clk_create_clk(cl->clk_hw, dev_id, con_id);
+	clk = __clk_create_clk(cl->clk_hw, dev_id, con_id, false);
 	if (IS_ERR(clk))
 		goto out;
 
-- 
2.7.0

[PATCH 2/2] clk: defer clk_gets on orphan clocks

[Emilio López]

From: Heiko Stuebner <heiko.stuebner@collabora.com>

Orphan clocks or children of orphan clocks don't have rate information at
all and can produce strange results if they're allowed to be used and the
parent becomes available later on.

This change, based on one from Stephen Boyd defers __clk_create_clk()
calls on orphan clocks in all regular cases.

One special case that gets handled, is accessing such orphan clocks when
handling assigned-clocks configurations. In the boot-defaults it may be
the case that a clock is connected to an orphan parent which then might
be needed to get reparented to an actually usable clock using
assigned-clock-parents. In this case even orphaned clocks should be
usable, but only for the set-parent case.

The added of_clk_get_from_provider_with_orphans() is only available
to ccf internal parts to prevent abuse.

Signed-off-by: Heiko Stuebner <heiko.stuebner@collabora.com>
---
 drivers/clk/clk-conf.c |  5 +++--
 drivers/clk/clk.c      | 48 ++++++++++++++++++++++++++++++++++++++----------
 drivers/clk/clk.h      | 10 +++++++---
 drivers/clk/clkdev.c   |  4 ++--
 4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 43a218f..60ebfd9 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/printk.h>
+#include "clk.h"
 
 static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 {
@@ -38,7 +39,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 		}
 		if (clkspec.np == node && !clk_supplier)
 			return 0;
-		pclk = of_clk_get_from_provider(&clkspec);
+		pclk = of_clk_get_from_provider_with_orphans(&clkspec);
 		if (IS_ERR(pclk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
@@ -53,7 +54,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			rc = 0;
 			goto err;
 		}
-		clk = of_clk_get_from_provider(&clkspec);
+		clk = of_clk_get_from_provider_with_orphans(&clkspec);
 		if (IS_ERR(clk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b4db67a..36fe1a0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2494,15 +2494,11 @@ out:
 	return ret;
 }
 
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id)
+struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
+			      const char *con_id)
 {
 	struct clk *clk;
 
-	/* This is to allow this function to be chained to others */
-	if (IS_ERR_OR_NULL(hw))
-		return (struct clk *) hw;
-
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return ERR_PTR(-ENOMEM);
@@ -2519,6 +2515,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	return clk;
 }
 
+struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+			     const char *con_id, bool with_orphans)
+{
+	/* This is to allow this function to be chained to others */
+	if (IS_ERR_OR_NULL(hw))
+		return (struct clk *) hw;
+
+	if (hw->core->orphan && !with_orphans)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return clk_hw_create_clk(hw, dev_id, con_id);
+}
+
 void __clk_free_clk(struct clk *clk)
 {
 	clk_prepare_lock();
@@ -2587,7 +2596,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 	INIT_HLIST_HEAD(&core->clks);
 
-	hw->clk = __clk_create_clk(hw, NULL, NULL);
+	hw->clk = clk_hw_create_clk(hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
 		goto fail_parent_names_copy;
@@ -3019,7 +3028,8 @@ void of_clk_del_provider(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_clk_del_provider);
 
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
-				       const char *dev_id, const char *con_id)
+				       const char *dev_id, const char *con_id,
+				       bool with_orphans)
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
@@ -3034,7 +3044,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 			clk = provider->get(clkspec, provider->data);
 		if (!IS_ERR(clk)) {
 			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
-					       con_id);
+					       con_id, with_orphans);
 
 			if (!IS_ERR(clk) && !__clk_get(clk)) {
 				__clk_free_clk(clk);
@@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
  */
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
-	return __of_clk_get_from_provider(clkspec, NULL, __func__);
+	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
+}
+
+/**
+ * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ *
+ * The difference to of_clk_get_from_provider() is that this function will
+ * also successfully lookup orphan-clocks, as it in some cases may be
+ * necessary to access such orphan-clocks as well.
+ */
+struct clk *
+of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)
+{
+	return __of_clk_get_from_provider(clkspec, NULL, __func__, true);
 }
 
 int of_clk_get_parent_count(struct device_node *np)
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 00b35a1..5a70f09 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -13,17 +13,21 @@ struct clk_hw;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
-				       const char *dev_id, const char *con_id);
+				       const char *dev_id, const char *con_id,
+				       bool with_orphans);
+struct clk *
+of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec);
 #endif
 
 #ifdef CONFIG_COMMON_CLK
 struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id);
+			     const char *con_id, bool with_orphans);
 void __clk_free_clk(struct clk *clk);
 #else
 /* All these casts to avoid ifdefs in clkdev... */
 static inline struct clk *
-__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id,
+		 bool with_orphans)
 {
 	return (struct clk *)hw;
 }
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 779b6ff..eda20c2 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -43,7 +43,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index,
 	if (rc)
 		return ERR_PTR(rc);
 
-	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
+	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id, false);
 	of_node_put(clkspec.np);
 
 	return clk;
@@ -177,7 +177,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 	if (!cl)
 		goto out;
 
-	clk = __clk_create_clk(cl->clk_hw, dev_id, con_id);
+	clk = __clk_create_clk(cl->clk_hw, dev_id, con_id, false);
 	if (IS_ERR(clk))
 		goto out;
 
-- 
2.7.0

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

Hi Emilio,

On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio López wrote:
> Clocks are registered early on, and unused clocks get disabled on
> late initcall, so we can delay protecting important clocks a bit.
> If we do this too early, it may happen that some clocks are orphans
> and therefore enabling them may not work as intended. If we do this
> too late, a driver may reparent some clock and cause another important
> clock to be disabled as a byproduct.
> 
> arch_initcall should be a good spot to do this, as clock drivers using
> the OF mechanisms will be all registered by then, and drivers won't
> have started probing yet.
> 
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> ---
>  drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 5ba2188..285e8ee 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
>  	}
>  }
>  
> +/* By default, don't protect any clocks */
> +static const char **protected_clocks __initdata;
> +static int protected_clocks_nr __initdata;
> +
>  static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>  {
> -	unsigned int i;
> -
>  	/* Register divided output clocks */
>  	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
>  
> @@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>  	/* Register mux clocks */
>  	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
>  
> +	/* We shall protect these clocks when everything is ready */
> +	protected_clocks = clocks;
> +	protected_clocks_nr = nclocks;
> +}
> +
> +static int __init sunxi_init_clock_protection(void)
> +{
> +	unsigned int i;
> +
>  	/* Protect the clocks that needs to stay on */
> -	for (i = 0; i < nclocks; i++) {
> -		struct clk *clk = clk_get(NULL, clocks[i]);
> +	for (i = 0; i < protected_clocks_nr; i++) {
> +		struct clk *clk = clk_get(NULL, protected_clocks[i]);
>  
>  		if (!IS_ERR(clk))
>  			clk_prepare_enable(clk);
>  	}
> +
> +	return 0;
>  }
> +arch_initcall(sunxi_init_clock_protection);

You also need to filter that by the machine compatible in case you're
running it on a !sunxi SoC.

Overall, I'm a bit skeptical about the approach. It doesn't really fix
everything, just hides it behind a curtain, and I'm pretty sure the
clocks not registered by this code would still be broken (the mod0
clocks for example).

The real fix would be to make sure we don't have any orphan clock in
the first place, by using CLK_OF_DECLARE everywhere. I did submit a
patch doing just that when the clocks broke, but I never got any
answer to it. I thought the patches were simply dropped and the
rockchip people just took another approach.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

Hi Emilio,

On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio L?pez wrote:
> Clocks are registered early on, and unused clocks get disabled on
> late initcall, so we can delay protecting important clocks a bit.
> If we do this too early, it may happen that some clocks are orphans
> and therefore enabling them may not work as intended. If we do this
> too late, a driver may reparent some clock and cause another important
> clock to be disabled as a byproduct.
> 
> arch_initcall should be a good spot to do this, as clock drivers using
> the OF mechanisms will be all registered by then, and drivers won't
> have started probing yet.
> 
> Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
> ---
>  drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 5ba2188..285e8ee 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
>  	}
>  }
>  
> +/* By default, don't protect any clocks */
> +static const char **protected_clocks __initdata;
> +static int protected_clocks_nr __initdata;
> +
>  static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>  {
> -	unsigned int i;
> -
>  	/* Register divided output clocks */
>  	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
>  
> @@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>  	/* Register mux clocks */
>  	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
>  
> +	/* We shall protect these clocks when everything is ready */
> +	protected_clocks = clocks;
> +	protected_clocks_nr = nclocks;
> +}
> +
> +static int __init sunxi_init_clock_protection(void)
> +{
> +	unsigned int i;
> +
>  	/* Protect the clocks that needs to stay on */
> -	for (i = 0; i < nclocks; i++) {
> -		struct clk *clk = clk_get(NULL, clocks[i]);
> +	for (i = 0; i < protected_clocks_nr; i++) {
> +		struct clk *clk = clk_get(NULL, protected_clocks[i]);
>  
>  		if (!IS_ERR(clk))
>  			clk_prepare_enable(clk);
>  	}
> +
> +	return 0;
>  }
> +arch_initcall(sunxi_init_clock_protection);

You also need to filter that by the machine compatible in case you're
running it on a !sunxi SoC.

Overall, I'm a bit skeptical about the approach. It doesn't really fix
everything, just hides it behind a curtain, and I'm pretty sure the
clocks not registered by this code would still be broken (the mod0
clocks for example).

The real fix would be to make sure we don't have any orphan clock in
the first place, by using CLK_OF_DECLARE everywhere. I did submit a
patch doing just that when the clocks broke, but I never got any
answer to it. I thought the patches were simply dropped and the
rockchip people just took another approach.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160127/0c2c62cc/attachment-0001.sig>

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Heiko Stübner]

Hi,

Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> I thought the patches were simply dropped and the
> rockchip people just took another approach.

nope still on track ... especially as it was Stephen's believe that orphans 
shouldn't even be usable to general clock users :-).

I just remember that the proposed general solution was based on Mike's 
upcoming generic critical clock handling (the handoff thingy), which would 
move critical clock handling out of architecture-specific code, so I've been 
prodding Mike mainly.

Another option might be to allow clock-controllers to handle orphans and only 
deny orphan usage to outside clock users, maybe expanding on what I did with 
the clock-conf part in patch2.


Heiko

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Heiko Stübner]

Hi,

Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> I thought the patches were simply dropped and the
> rockchip people just took another approach.

nope still on track ... especially as it was Stephen's believe that orphans 
shouldn't even be usable to general clock users :-).

I just remember that the proposed general solution was based on Mike's 
upcoming generic critical clock handling (the handoff thingy), which would 
move critical clock handling out of architecture-specific code, so I've been 
prodding Mike mainly.

Another option might be to allow clock-controllers to handle orphans and only 
deny orphan usage to outside clock users, maybe expanding on what I did with 
the clock-conf part in patch2.


Heiko

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Emilio López]

Hi Maxime,

El 27/01/16 a las 12:37, Maxime Ripard escribió:
> Hi Emilio,
>
> On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio López wrote:
>> Clocks are registered early on, and unused clocks get disabled on
>> late initcall, so we can delay protecting important clocks a bit.
>> If we do this too early, it may happen that some clocks are orphans
>> and therefore enabling them may not work as intended. If we do this
>> too late, a driver may reparent some clock and cause another important
>> clock to be disabled as a byproduct.
>>
>> arch_initcall should be a good spot to do this, as clock drivers using
>> the OF mechanisms will be all registered by then, and drivers won't
>> have started probing yet.
>>
>> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>> ---
>>   drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 5ba2188..285e8ee 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
>>   	}
>>   }
>>
>> +/* By default, don't protect any clocks */
>> +static const char **protected_clocks __initdata;
>> +static int protected_clocks_nr __initdata;
>> +
>>   static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>>   {
>> -	unsigned int i;
>> -
>>   	/* Register divided output clocks */
>>   	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
>>
>> @@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>>   	/* Register mux clocks */
>>   	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
>>
>> +	/* We shall protect these clocks when everything is ready */
>> +	protected_clocks = clocks;
>> +	protected_clocks_nr = nclocks;
>> +}
>> +
>> +static int __init sunxi_init_clock_protection(void)
>> +{
>> +	unsigned int i;
>> +
>>   	/* Protect the clocks that needs to stay on */
>> -	for (i = 0; i < nclocks; i++) {
>> -		struct clk *clk = clk_get(NULL, clocks[i]);
>> +	for (i = 0; i < protected_clocks_nr; i++) {
>> +		struct clk *clk = clk_get(NULL, protected_clocks[i]);
>>
>>   		if (!IS_ERR(clk))
>>   			clk_prepare_enable(clk);
>>   	}
>> +
>> +	return 0;
>>   }
>> +arch_initcall(sunxi_init_clock_protection);
>
> You also need to filter that by the machine compatible in case you're
> running it on a !sunxi SoC.

protected_clocks_nr will be 0 on a !sunxi machine, so this is 
effectively a noop there.

> Overall, I'm a bit skeptical about the approach. It doesn't really fix
> everything, just hides it behind a curtain, and I'm pretty sure the
> clocks not registered by this code would still be broken (the mod0
> clocks for example).

This is only meant to solve the problems observed when trying to grab 
critical clocks before letting all the basic/OF clock types register. 
The actual clock trees are complete once all the built-in clock 
compatibles are probed, so this just pushes the protection after that 
point in time. The plan on the long term should be to use the 
CCF-built-in clock protection, once it's finished and merged, but it's 
not here yet.

Regarding your example, I'm not aware of any critical mod0 clocks (not 
that it should matter, as they won't be orphans either).

> The real fix would be to make sure we don't have any orphan clock in
> the first place, by using CLK_OF_DECLARE everywhere. I did submit a
> patch doing just that when the clocks broke, but I never got any
> answer to it.

As I said, there shouldn't be any after all the built in clocks are probed.

That patch also moved the clock protection to arch_initcall btw :) You 
could say this is a subset of your patch. Moving everything to 
OF_CLK_DECLARE is unnecessary in my opinion, and it will also probably 
be slower (I see a bunch of extra of_match_nodes being run for every 
compatible)

> I thought the patches were simply dropped and the
> rockchip people just took another approach.

As far as I know, rockchip SoCs are still suffering the breakage this 
set aims to fix :)

Thanks for the review!
Emilio

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Emilio López]

Hi Maxime,

El 27/01/16 a las 12:37, Maxime Ripard escribi?:
> Hi Emilio,
>
> On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio L?pez wrote:
>> Clocks are registered early on, and unused clocks get disabled on
>> late initcall, so we can delay protecting important clocks a bit.
>> If we do this too early, it may happen that some clocks are orphans
>> and therefore enabling them may not work as intended. If we do this
>> too late, a driver may reparent some clock and cause another important
>> clock to be disabled as a byproduct.
>>
>> arch_initcall should be a good spot to do this, as clock drivers using
>> the OF mechanisms will be all registered by then, and drivers won't
>> have started probing yet.
>>
>> Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
>> ---
>>   drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 5ba2188..285e8ee 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
>>   	}
>>   }
>>
>> +/* By default, don't protect any clocks */
>> +static const char **protected_clocks __initdata;
>> +static int protected_clocks_nr __initdata;
>> +
>>   static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>>   {
>> -	unsigned int i;
>> -
>>   	/* Register divided output clocks */
>>   	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
>>
>> @@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>>   	/* Register mux clocks */
>>   	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
>>
>> +	/* We shall protect these clocks when everything is ready */
>> +	protected_clocks = clocks;
>> +	protected_clocks_nr = nclocks;
>> +}
>> +
>> +static int __init sunxi_init_clock_protection(void)
>> +{
>> +	unsigned int i;
>> +
>>   	/* Protect the clocks that needs to stay on */
>> -	for (i = 0; i < nclocks; i++) {
>> -		struct clk *clk = clk_get(NULL, clocks[i]);
>> +	for (i = 0; i < protected_clocks_nr; i++) {
>> +		struct clk *clk = clk_get(NULL, protected_clocks[i]);
>>
>>   		if (!IS_ERR(clk))
>>   			clk_prepare_enable(clk);
>>   	}
>> +
>> +	return 0;
>>   }
>> +arch_initcall(sunxi_init_clock_protection);
>
> You also need to filter that by the machine compatible in case you're
> running it on a !sunxi SoC.

protected_clocks_nr will be 0 on a !sunxi machine, so this is 
effectively a noop there.

> Overall, I'm a bit skeptical about the approach. It doesn't really fix
> everything, just hides it behind a curtain, and I'm pretty sure the
> clocks not registered by this code would still be broken (the mod0
> clocks for example).

This is only meant to solve the problems observed when trying to grab 
critical clocks before letting all the basic/OF clock types register. 
The actual clock trees are complete once all the built-in clock 
compatibles are probed, so this just pushes the protection after that 
point in time. The plan on the long term should be to use the 
CCF-built-in clock protection, once it's finished and merged, but it's 
not here yet.

Regarding your example, I'm not aware of any critical mod0 clocks (not 
that it should matter, as they won't be orphans either).

> The real fix would be to make sure we don't have any orphan clock in
> the first place, by using CLK_OF_DECLARE everywhere. I did submit a
> patch doing just that when the clocks broke, but I never got any
> answer to it.

As I said, there shouldn't be any after all the built in clocks are probed.

That patch also moved the clock protection to arch_initcall btw :) You 
could say this is a subset of your patch. Moving everything to 
OF_CLK_DECLARE is unnecessary in my opinion, and it will also probably 
be slower (I see a bunch of extra of_match_nodes being run for every 
compatible)

> I thought the patches were simply dropped and the
> rockchip people just took another approach.

As far as I know, rockchip SoCs are still suffering the breakage this 
set aims to fix :)

Thanks for the review!
Emilio

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hi,

On Wed, Jan 27, 2016 at 05:14:17PM +0100, Heiko Stübner wrote:
> Hi,
> 
> Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> > I thought the patches were simply dropped and the
> > rockchip people just took another approach.
> 
> nope still on track ... especially as it was Stephen's believe that orphans 
> shouldn't even be usable to general clock users :-).
> 
> I just remember that the proposed general solution was based on Mike's 
> upcoming generic critical clock handling (the handoff thingy), which would 
> move critical clock handling out of architecture-specific code, so I've been 
> prodding Mike mainly.
> 
> Another option might be to allow clock-controllers to handle orphans and only 
> deny orphan usage to outside clock users, maybe expanding on what I did with 
> the clock-conf part in patch2.

I'm not sure that would solve anything in our case. All our clocks
drivers are different ones, so I'm not sure how we could handle that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

Hi,

On Wed, Jan 27, 2016 at 05:14:17PM +0100, Heiko St?bner wrote:
> Hi,
> 
> Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> > I thought the patches were simply dropped and the
> > rockchip people just took another approach.
> 
> nope still on track ... especially as it was Stephen's believe that orphans 
> shouldn't even be usable to general clock users :-).
> 
> I just remember that the proposed general solution was based on Mike's 
> upcoming generic critical clock handling (the handoff thingy), which would 
> move critical clock handling out of architecture-specific code, so I've been 
> prodding Mike mainly.
> 
> Another option might be to allow clock-controllers to handle orphans and only 
> deny orphan usage to outside clock users, maybe expanding on what I did with 
> the clock-conf part in patch2.

I'm not sure that would solve anything in our case. All our clocks
drivers are different ones, so I'm not sure how we could handle that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160127/01f08228/attachment.sig>

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Heiko Stübner]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

Am Mittwoch, 27. Januar 2016, 21:38:16 schrieb Maxime Ripard:
> Hi,
> 
> On Wed, Jan 27, 2016 at 05:14:17PM +0100, Heiko Stübner wrote:
> > Hi,
> > 
> > Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> > > I thought the patches were simply dropped and the
> > > rockchip people just took another approach.
> > 
> > nope still on track ... especially as it was Stephen's believe that
> > orphans
> > shouldn't even be usable to general clock users :-).
> > 
> > I just remember that the proposed general solution was based on Mike's
> > upcoming generic critical clock handling (the handoff thingy), which would
> > move critical clock handling out of architecture-specific code, so I've
> > been prodding Mike mainly.
> > 
> > Another option might be to allow clock-controllers to handle orphans and
> > only deny orphan usage to outside clock users, maybe expanding on what I
> > did with the clock-conf part in patch2.
> 
> I'm not sure that would solve anything in our case. All our clocks
> drivers are different ones, so I'm not sure how we could handle that.

the core issue is, that a clk_get on an orphan is going to return EPROBE_DEFER 
after the second patch, which is also true for sunxi critical clocks.

The clock-conf has the same issue in the case where you know on the board-
level that a clock will stay orphaned indefinitly and want to reparent it away 
to some sane parent.

That's why I added of_clk_get_from_provider_with_orphans() (limited to use in 
the ccf) in the second patch to allow orphans to be reparented via assigned-
clocks foo. In theory one could argue that clock controller generally know 
what they're doing and add something like clk_get_with_orphans() or whatever 
that might be called then.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Heiko Stübner]

Am Mittwoch, 27. Januar 2016, 21:38:16 schrieb Maxime Ripard:
> Hi,
> 
> On Wed, Jan 27, 2016 at 05:14:17PM +0100, Heiko St?bner wrote:
> > Hi,
> > 
> > Am Mittwoch, 27. Januar 2016, 16:37:22 schrieb Maxime Ripard:
> > > I thought the patches were simply dropped and the
> > > rockchip people just took another approach.
> > 
> > nope still on track ... especially as it was Stephen's believe that
> > orphans
> > shouldn't even be usable to general clock users :-).
> > 
> > I just remember that the proposed general solution was based on Mike's
> > upcoming generic critical clock handling (the handoff thingy), which would
> > move critical clock handling out of architecture-specific code, so I've
> > been prodding Mike mainly.
> > 
> > Another option might be to allow clock-controllers to handle orphans and
> > only deny orphan usage to outside clock users, maybe expanding on what I
> > did with the clock-conf part in patch2.
> 
> I'm not sure that would solve anything in our case. All our clocks
> drivers are different ones, so I'm not sure how we could handle that.

the core issue is, that a clk_get on an orphan is going to return EPROBE_DEFER 
after the second patch, which is also true for sunxi critical clocks.

The clock-conf has the same issue in the case where you know on the board-
level that a clock will stay orphaned indefinitly and want to reparent it away 
to some sane parent.

That's why I added of_clk_get_from_provider_with_orphans() (limited to use in 
the ccf) in the second patch to allow orphans to be reparented via assigned-
clocks foo. In theory one could argue that clock controller generally know 
what they're doing and add something like clk_get_with_orphans() or whatever 
that might be called then.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160127/e73aa75f/attachment.sig>

Re: [PATCH 2/2] clk: defer clk_gets on orphan clocks

[Stephen Boyd]

On 01/21, Emilio López wrote:
> @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
>   */
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
> -	return __of_clk_get_from_provider(clkspec, NULL, __func__);
> +	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
> +}
> +
> +/**
> + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider
> + * @clkspec: pointer to a clock specifier data structure
> + *
> + * This function looks up a struct clk from the registered list of clock
> + * providers, an input is a clock specifier data structure as returned
> + * from the of_parse_phandle_with_args() function call.
> + *
> + * The difference to of_clk_get_from_provider() is that this function will
> + * also successfully lookup orphan-clocks, as it in some cases may be
> + * necessary to access such orphan-clocks as well.
> + */
> +struct clk *
> +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)

Dislike. In fact, the whole clk conf approach is odd here. When
we're doing of_clk_init() we do a best effort loop around
parent_ready(), waiting for clk providers to register as long as
we have a clocks property in our provider node. We should do
something similar in the non of_clk_init() case too, because
of_clk_init() isn't special.

Furthermore, the assigned parents and rates feature doesn't need
the clocks that we're assigning parents and rates to to even be
provided or consumed by the provider that's probing, so I'm lost
why we're checking the provider's node for a clocks property. It
would be better to check the assigned-clocks and assigned-parents
properties and make sure that those are all non-orphans. If
they're orphaned, we should delay until another clk provider is
registered. Eventually we'll unstick the orphans and then the
tree can be configured. Running the configuration at the end of
of_clk_init() even if we still can't get the clocks doesn't make
any sense to me.

To be really nice, we could build up a set of configuration
actions (set this parent, set this rate), and run those actions
when we drop the orphan flag. If some clock is orphaned that
we're trying to configure, we can attach the action to a list in
the clk_core structure. Otherwise we'll run the action
immediately. This way, we do a best effort to run as much of the
configuration as possible when the provider is registered the
first time and skip the overhead of cycling through a potentially
long list of provider actions to see if we can run them now. This
last part may be over-engineered though. I'm not sure if we
really have any such scenario today.

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

[PATCH 2/2] clk: defer clk_gets on orphan clocks

[Stephen Boyd]

On 01/21, Emilio L?pez wrote:
> @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
>   */
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
> -	return __of_clk_get_from_provider(clkspec, NULL, __func__);
> +	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
> +}
> +
> +/**
> + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider
> + * @clkspec: pointer to a clock specifier data structure
> + *
> + * This function looks up a struct clk from the registered list of clock
> + * providers, an input is a clock specifier data structure as returned
> + * from the of_parse_phandle_with_args() function call.
> + *
> + * The difference to of_clk_get_from_provider() is that this function will
> + * also successfully lookup orphan-clocks, as it in some cases may be
> + * necessary to access such orphan-clocks as well.
> + */
> +struct clk *
> +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)

Dislike. In fact, the whole clk conf approach is odd here. When
we're doing of_clk_init() we do a best effort loop around
parent_ready(), waiting for clk providers to register as long as
we have a clocks property in our provider node. We should do
something similar in the non of_clk_init() case too, because
of_clk_init() isn't special.

Furthermore, the assigned parents and rates feature doesn't need
the clocks that we're assigning parents and rates to to even be
provided or consumed by the provider that's probing, so I'm lost
why we're checking the provider's node for a clocks property. It
would be better to check the assigned-clocks and assigned-parents
properties and make sure that those are all non-orphans. If
they're orphaned, we should delay until another clk provider is
registered. Eventually we'll unstick the orphans and then the
tree can be configured. Running the configuration at the end of
of_clk_init() even if we still can't get the clocks doesn't make
any sense to me.

To be really nice, we could build up a set of configuration
actions (set this parent, set this rate), and run those actions
when we drop the orphan flag. If some clock is orphaned that
we're trying to configure, we can attach the action to a list in
the clk_core structure. Otherwise we'll run the action
immediately. This way, we do a best effort to run as much of the
configuration as possible when the provider is registered the
first time and skip the overhead of cycling through a potentially
long list of provider actions to see if we can run them now. This
last part may be over-engineered though. I'm not sure if we
really have any such scenario today.

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

Re: [PATCH 2/2] clk: defer clk_gets on orphan clocks

[Heiko Stübner]

Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd:
> On 01/21, Emilio L=F3pez wrote:
> > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struc=
t
> > of_phandle_args *clkspec,>=20
> >   */
> > =20
> >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clksp=
ec)
> >  {
> >=20
> > -=09return __of_clk_get_from_provider(clkspec, NULL, __func__);
> > +=09return __of_clk_get_from_provider(clkspec, NULL, __func__, fals=
e);
> > +}
> > +
> > +/**
> > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a c=
lock
> > provider + * @clkspec: pointer to a clock specifier data structure
> > + *
> > + * This function looks up a struct clk from the registered list of=
 clock
> > + * providers, an input is a clock specifier data structure as retu=
rned
> > + * from the of_parse_phandle_with_args() function call.
> > + *
> > + * The difference to of_clk_get_from_provider() is that this funct=
ion
> > will
> > + * also successfully lookup orphan-clocks, as it in some cases may=
 be
> > + * necessary to access such orphan-clocks as well.
> > + */
> > +struct clk *
> > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clks=
pec)
>=20
> Dislike. In fact, the whole clk conf approach is odd here. When
> we're doing of_clk_init() we do a best effort loop around
> parent_ready(), waiting for clk providers to register as long as
> we have a clocks property in our provider node. We should do
> something similar in the non of_clk_init() case too, because
> of_clk_init() isn't special.

At least to me being able to reparent orphan clocks when knowing that t=
hey=20
won't ever get supplied is special.

The Rockchip clock controller has quite a number of clocks that can eit=
her be=20
supplied by some external source. xin32k supplied by some i2c chip bein=
g the=20
most prominent. But while this one will get supplied eventually, there =
are=20
others who will never get a supply and stay orphans forever.

Example:
rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscill=
ator or=20
some separate clock input connected some chip pin. Reset-default seems =
to be=20
the external supply, but on all boards I've seen so far doesn't get con=
nected.

So we know on a per-board level if this is connected and want to move a=
way=20
from the non-existent source. Which is why the limit of this new functi=
on is=20
limited to be ccf internal and the assigned-clock-parents path.


I guess if you really dislike that approach the other option would be=20=

reparenting all the time in the clock-controller driver and then let th=
e=20
board-dts reparent back if needed. Which would also work for that clock=
, but=20
may cause other glitches down the road when it affects some pre-setup t=
hings.


> Furthermore, the assigned parents and rates feature doesn't need
> the clocks that we're assigning parents and rates to to even be
> provided or consumed by the provider that's probing, so I'm lost
> why we're checking the provider's node for a clocks property. It
> would be better to check the assigned-clocks and assigned-parents
> properties and make sure that those are all non-orphans. If
> they're orphaned, we should delay until another clk provider is
> registered. Eventually we'll unstick the orphans and then the
> tree can be configured. Running the configuration at the end of
> of_clk_init() even if we still can't get the clocks doesn't make
> any sense to me.
>=20
> To be really nice, we could build up a set of configuration
> actions (set this parent, set this rate), and run those actions
> when we drop the orphan flag. If some clock is orphaned that
> we're trying to configure, we can attach the action to a list in
> the clk_core structure. Otherwise we'll run the action
> immediately. This way, we do a best effort to run as much of the
> configuration as possible when the provider is registered the
> first time and skip the overhead of cycling through a potentially
> long list of provider actions to see if we can run them now. This
> last part may be over-engineered though. I'm not sure if we
> really have any such scenario today.

[PATCH 2/2] clk: defer clk_gets on orphan clocks

[Heiko Stübner]

Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd:
> On 01/21, Emilio L?pez wrote:
> > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct
> > of_phandle_args *clkspec,> 
> >   */
> >  
> >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> >  {
> > 
> > -	return __of_clk_get_from_provider(clkspec, NULL, __func__);
> > +	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
> > +}
> > +
> > +/**
> > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock
> > provider + * @clkspec: pointer to a clock specifier data structure
> > + *
> > + * This function looks up a struct clk from the registered list of clock
> > + * providers, an input is a clock specifier data structure as returned
> > + * from the of_parse_phandle_with_args() function call.
> > + *
> > + * The difference to of_clk_get_from_provider() is that this function
> > will
> > + * also successfully lookup orphan-clocks, as it in some cases may be
> > + * necessary to access such orphan-clocks as well.
> > + */
> > +struct clk *
> > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)
> 
> Dislike. In fact, the whole clk conf approach is odd here. When
> we're doing of_clk_init() we do a best effort loop around
> parent_ready(), waiting for clk providers to register as long as
> we have a clocks property in our provider node. We should do
> something similar in the non of_clk_init() case too, because
> of_clk_init() isn't special.

At least to me being able to reparent orphan clocks when knowing that they 
won't ever get supplied is special.

The Rockchip clock controller has quite a number of clocks that can either be 
supplied by some external source. xin32k supplied by some i2c chip being the 
most prominent. But while this one will get supplied eventually, there are 
others who will never get a supply and stay orphans forever.

Example:
rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscillator or 
some separate clock input connected some chip pin. Reset-default seems to be 
the external supply, but on all boards I've seen so far doesn't get connected.

So we know on a per-board level if this is connected and want to move away 
from the non-existent source. Which is why the limit of this new function is 
limited to be ccf internal and the assigned-clock-parents path.


I guess if you really dislike that approach the other option would be 
reparenting all the time in the clock-controller driver and then let the 
board-dts reparent back if needed. Which would also work for that clock, but 
may cause other glitches down the road when it affects some pre-setup things.


> Furthermore, the assigned parents and rates feature doesn't need
> the clocks that we're assigning parents and rates to to even be
> provided or consumed by the provider that's probing, so I'm lost
> why we're checking the provider's node for a clocks property. It
> would be better to check the assigned-clocks and assigned-parents
> properties and make sure that those are all non-orphans. If
> they're orphaned, we should delay until another clk provider is
> registered. Eventually we'll unstick the orphans and then the
> tree can be configured. Running the configuration at the end of
> of_clk_init() even if we still can't get the clocks doesn't make
> any sense to me.
> 
> To be really nice, we could build up a set of configuration
> actions (set this parent, set this rate), and run those actions
> when we drop the orphan flag. If some clock is orphaned that
> we're trying to configure, we can attach the action to a list in
> the clk_core structure. Otherwise we'll run the action
> immediately. This way, we do a best effort to run as much of the
> configuration as possible when the provider is registered the
> first time and skip the overhead of cycling through a potentially
> long list of provider actions to see if we can run them now. This
> last part may be over-engineered though. I'm not sure if we
> really have any such scenario today.

Re: [PATCH 2/2] clk: defer clk_gets on orphan clocks

[Stephen Boyd]

On 01/28, Heiko Stübner wrote:
> Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd:
> > On 01/21, Emilio López wrote:
> > > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct
> > > of_phandle_args *clkspec,> 
> > >   */
> > >  
> > >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> > >  {
> > > 
> > > -	return __of_clk_get_from_provider(clkspec, NULL, __func__);
> > > +	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
> > > +}
> > > +
> > > +/**
> > > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock
> > > provider + * @clkspec: pointer to a clock specifier data structure
> > > + *
> > > + * This function looks up a struct clk from the registered list of clock
> > > + * providers, an input is a clock specifier data structure as returned
> > > + * from the of_parse_phandle_with_args() function call.
> > > + *
> > > + * The difference to of_clk_get_from_provider() is that this function
> > > will
> > > + * also successfully lookup orphan-clocks, as it in some cases may be
> > > + * necessary to access such orphan-clocks as well.
> > > + */
> > > +struct clk *
> > > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)
> > 
> > Dislike. In fact, the whole clk conf approach is odd here. When
> > we're doing of_clk_init() we do a best effort loop around
> > parent_ready(), waiting for clk providers to register as long as
> > we have a clocks property in our provider node. We should do
> > something similar in the non of_clk_init() case too, because
> > of_clk_init() isn't special.
> 
> At least to me being able to reparent orphan clocks when knowing that they 
> won't ever get supplied is special.
> 
> The Rockchip clock controller has quite a number of clocks that can either be 
> supplied by some external source. xin32k supplied by some i2c chip being the 
> most prominent. But while this one will get supplied eventually, there are 
> others who will never get a supply and stay orphans forever.
> 
> Example:
> rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscillator or 
> some separate clock input connected some chip pin. Reset-default seems to be 
> the external supply, but on all boards I've seen so far doesn't get connected.
> 
> So we know on a per-board level if this is connected and want to move away 
> from the non-existent source. Which is why the limit of this new function is 
> limited to be ccf internal and the assigned-clock-parents path.

Right, this problem isn't limited to clk-conf though. If some
clock is orphaned by its reset default, then consumers that want
to reparent them to something else will never be able to get the
clocks without some sort of clk_get_ignore_orphan() API. We can't
rely on DTS files telling us to do the reparent. This is a whole
other problem to be dealt with before we can defer clk_get()
orphans.

In this case, I wonder why we haven't specified some sort of
"ground" clock that has a rate of 0? In the example above, I
would imagine that we have our dts file specify a fixed rate
clock with frequency of 0 that's named whatever the external
supply clock is named when that pin isn't connected.

> 
> 
> I guess if you really dislike that approach the other option would be 
> reparenting all the time in the clock-controller driver and then let the 
> board-dts reparent back if needed. Which would also work for that clock, but 
> may cause other glitches down the road when it affects some pre-setup things.
> 
> 

For all we know that could cause some audio pop on a speaker 
something. Let's avoid this if we can.

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

[PATCH 2/2] clk: defer clk_gets on orphan clocks

[Stephen Boyd]

On 01/28, Heiko St?bner wrote:
> Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd:
> > On 01/21, Emilio L?pez wrote:
> > > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct
> > > of_phandle_args *clkspec,> 
> > >   */
> > >  
> > >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> > >  {
> > > 
> > > -	return __of_clk_get_from_provider(clkspec, NULL, __func__);
> > > +	return __of_clk_get_from_provider(clkspec, NULL, __func__, false);
> > > +}
> > > +
> > > +/**
> > > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock
> > > provider + * @clkspec: pointer to a clock specifier data structure
> > > + *
> > > + * This function looks up a struct clk from the registered list of clock
> > > + * providers, an input is a clock specifier data structure as returned
> > > + * from the of_parse_phandle_with_args() function call.
> > > + *
> > > + * The difference to of_clk_get_from_provider() is that this function
> > > will
> > > + * also successfully lookup orphan-clocks, as it in some cases may be
> > > + * necessary to access such orphan-clocks as well.
> > > + */
> > > +struct clk *
> > > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec)
> > 
> > Dislike. In fact, the whole clk conf approach is odd here. When
> > we're doing of_clk_init() we do a best effort loop around
> > parent_ready(), waiting for clk providers to register as long as
> > we have a clocks property in our provider node. We should do
> > something similar in the non of_clk_init() case too, because
> > of_clk_init() isn't special.
> 
> At least to me being able to reparent orphan clocks when knowing that they 
> won't ever get supplied is special.
> 
> The Rockchip clock controller has quite a number of clocks that can either be 
> supplied by some external source. xin32k supplied by some i2c chip being the 
> most prominent. But while this one will get supplied eventually, there are 
> others who will never get a supply and stay orphans forever.
> 
> Example:
> rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscillator or 
> some separate clock input connected some chip pin. Reset-default seems to be 
> the external supply, but on all boards I've seen so far doesn't get connected.
> 
> So we know on a per-board level if this is connected and want to move away 
> from the non-existent source. Which is why the limit of this new function is 
> limited to be ccf internal and the assigned-clock-parents path.

Right, this problem isn't limited to clk-conf though. If some
clock is orphaned by its reset default, then consumers that want
to reparent them to something else will never be able to get the
clocks without some sort of clk_get_ignore_orphan() API. We can't
rely on DTS files telling us to do the reparent. This is a whole
other problem to be dealt with before we can defer clk_get()
orphans.

In this case, I wonder why we haven't specified some sort of
"ground" clock that has a rate of 0? In the example above, I
would imagine that we have our dts file specify a fixed rate
clock with frequency of 0 that's named whatever the external
supply clock is named when that pin isn't connected.

> 
> 
> I guess if you really dislike that approach the other option would be 
> reparenting all the time in the clock-controller driver and then let the 
> board-dts reparent back if needed. Which would also work for that clock, but 
> may cause other glitches down the road when it affects some pre-setup things.
> 
> 

For all we know that could cause some audio pop on a speaker 
something. Let's avoid this if we can.

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

Re: [PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4045 bytes --]

Hi,

On Wed, Jan 27, 2016 at 03:53:57PM -0300, Emilio López wrote:
> Hi Maxime,
> 
> El 27/01/16 a las 12:37, Maxime Ripard escribió:
> >Hi Emilio,
> >
> >On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio López wrote:
> >>Clocks are registered early on, and unused clocks get disabled on
> >>late initcall, so we can delay protecting important clocks a bit.
> >>If we do this too early, it may happen that some clocks are orphans
> >>and therefore enabling them may not work as intended. If we do this
> >>too late, a driver may reparent some clock and cause another important
> >>clock to be disabled as a byproduct.
> >>
> >>arch_initcall should be a good spot to do this, as clock drivers using
> >>the OF mechanisms will be all registered by then, and drivers won't
> >>have started probing yet.
> >>
> >>Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> >>---
> >>  drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
> >>  1 file changed, 18 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> >>index 5ba2188..285e8ee 100644
> >>--- a/drivers/clk/sunxi/clk-sunxi.c
> >>+++ b/drivers/clk/sunxi/clk-sunxi.c
> >>@@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
> >>  	}
> >>  }
> >>
> >>+/* By default, don't protect any clocks */
> >>+static const char **protected_clocks __initdata;
> >>+static int protected_clocks_nr __initdata;
> >>+
> >>  static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
> >>  {
> >>-	unsigned int i;
> >>-
> >>  	/* Register divided output clocks */
> >>  	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
> >>
> >>@@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
> >>  	/* Register mux clocks */
> >>  	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
> >>
> >>+	/* We shall protect these clocks when everything is ready */
> >>+	protected_clocks = clocks;
> >>+	protected_clocks_nr = nclocks;
> >>+}
> >>+
> >>+static int __init sunxi_init_clock_protection(void)
> >>+{
> >>+	unsigned int i;
> >>+
> >>  	/* Protect the clocks that needs to stay on */
> >>-	for (i = 0; i < nclocks; i++) {
> >>-		struct clk *clk = clk_get(NULL, clocks[i]);
> >>+	for (i = 0; i < protected_clocks_nr; i++) {
> >>+		struct clk *clk = clk_get(NULL, protected_clocks[i]);
> >>
> >>  		if (!IS_ERR(clk))
> >>  			clk_prepare_enable(clk);
> >>  	}
> >>+
> >>+	return 0;
> >>  }
> >>+arch_initcall(sunxi_init_clock_protection);
> >
> >You also need to filter that by the machine compatible in case you're
> >running it on a !sunxi SoC.
> 
> protected_clocks_nr will be 0 on a !sunxi machine, so this is effectively a
> noop there.

Ah, yes, good point.

> >Overall, I'm a bit skeptical about the approach. It doesn't really fix
> >everything, just hides it behind a curtain, and I'm pretty sure the
> >clocks not registered by this code would still be broken (the mod0
> >clocks for example).
> 
> This is only meant to solve the problems observed when trying to grab
> critical clocks before letting all the basic/OF clock types register. The
> actual clock trees are complete once all the built-in clock compatibles are
> probed, so this just pushes the protection after that point in time. The
> plan on the long term should be to use the CCF-built-in clock protection,
> once it's finished and merged, but it's not here yet.
> 
> Regarding your example, I'm not aware of any critical mod0 clocks (not that
> it should matter, as they won't be orphans either).

My bad, the A13 mbus clock is one. The A23 is one too.

Both of these are probed through CLK_OF_DECLARE, and use directly
clk_prepare_enable on the clock given back by clk_register, which
won't work in your case.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/2] clk: sunxi: delay protected clocks until arch initcall

[Maxime Ripard]

Hi,

On Wed, Jan 27, 2016 at 03:53:57PM -0300, Emilio L?pez wrote:
> Hi Maxime,
> 
> El 27/01/16 a las 12:37, Maxime Ripard escribi?:
> >Hi Emilio,
> >
> >On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio L?pez wrote:
> >>Clocks are registered early on, and unused clocks get disabled on
> >>late initcall, so we can delay protecting important clocks a bit.
> >>If we do this too early, it may happen that some clocks are orphans
> >>and therefore enabling them may not work as intended. If we do this
> >>too late, a driver may reparent some clock and cause another important
> >>clock to be disabled as a byproduct.
> >>
> >>arch_initcall should be a good spot to do this, as clock drivers using
> >>the OF mechanisms will be all registered by then, and drivers won't
> >>have started probing yet.
> >>
> >>Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
> >>---
> >>  drivers/clk/sunxi/clk-sunxi.c | 22 ++++++++++++++++++----
> >>  1 file changed, 18 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> >>index 5ba2188..285e8ee 100644
> >>--- a/drivers/clk/sunxi/clk-sunxi.c
> >>+++ b/drivers/clk/sunxi/clk-sunxi.c
> >>@@ -1153,10 +1153,12 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
> >>  	}
> >>  }
> >>
> >>+/* By default, don't protect any clocks */
> >>+static const char **protected_clocks __initdata;
> >>+static int protected_clocks_nr __initdata;
> >>+
> >>  static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
> >>  {
> >>-	unsigned int i;
> >>-
> >>  	/* Register divided output clocks */
> >>  	of_sunxi_table_clock_setup(clk_divs_match, sunxi_divs_clk_setup);
> >>
> >>@@ -1169,14 +1171,26 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
> >>  	/* Register mux clocks */
> >>  	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
> >>
> >>+	/* We shall protect these clocks when everything is ready */
> >>+	protected_clocks = clocks;
> >>+	protected_clocks_nr = nclocks;
> >>+}
> >>+
> >>+static int __init sunxi_init_clock_protection(void)
> >>+{
> >>+	unsigned int i;
> >>+
> >>  	/* Protect the clocks that needs to stay on */
> >>-	for (i = 0; i < nclocks; i++) {
> >>-		struct clk *clk = clk_get(NULL, clocks[i]);
> >>+	for (i = 0; i < protected_clocks_nr; i++) {
> >>+		struct clk *clk = clk_get(NULL, protected_clocks[i]);
> >>
> >>  		if (!IS_ERR(clk))
> >>  			clk_prepare_enable(clk);
> >>  	}
> >>+
> >>+	return 0;
> >>  }
> >>+arch_initcall(sunxi_init_clock_protection);
> >
> >You also need to filter that by the machine compatible in case you're
> >running it on a !sunxi SoC.
> 
> protected_clocks_nr will be 0 on a !sunxi machine, so this is effectively a
> noop there.

Ah, yes, good point.

> >Overall, I'm a bit skeptical about the approach. It doesn't really fix
> >everything, just hides it behind a curtain, and I'm pretty sure the
> >clocks not registered by this code would still be broken (the mod0
> >clocks for example).
> 
> This is only meant to solve the problems observed when trying to grab
> critical clocks before letting all the basic/OF clock types register. The
> actual clock trees are complete once all the built-in clock compatibles are
> probed, so this just pushes the protection after that point in time. The
> plan on the long term should be to use the CCF-built-in clock protection,
> once it's finished and merged, but it's not here yet.
> 
> Regarding your example, I'm not aware of any critical mod0 clocks (not that
> it should matter, as they won't be orphans either).

My bad, the A13 mbus clock is one. The A23 is one too.

Both of these are probed through CLK_OF_DECLARE, and use directly
clk_prepare_enable on the clock given back by clk_register, which
won't work in your case.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160201/f9b58d28/attachment.sig>