[PATCH 0/5] clk: sunxi: Rework the clocks code to deal with orphans

Hi everyone,

Here is an attempt at making the whole clk-sunxi code register the
clocks without creating any orphans by relying on CLK_OF_DECLARE.

This also has the net benefit of not having to add the SoC compatible
to the list of compatibles that trigger the clock registration, which
has started to be a pain.

I tested this on an A13 (R8 actually), everything seems to be working
just fine. I also tested it both with and without Heiko's patch to
defer clk_get if the clock is orphan, and previously broke everything.

Let me know what you think,
Maxime

Maxime Ripard (5):
  clk: sunxi: Make clocks setup functions return their clock
  clk: sunxi: Make clocks setup functions take const pointer
  clk: sunxi: convert current clocks registration to CLK_OF_DECLARE
  clk: sunxi: Remove old probe and protection code
  clk: sunxi: Remove clk_register_clkdev calls

 drivers/clk/sunxi/clk-a10-hosc.c         |   3 +-
 drivers/clk/sunxi/clk-a20-gmac.c         |   2 -
 drivers/clk/sunxi/clk-factors.c          |   7 -
 drivers/clk/sunxi/clk-factors.h          |   1 -
 drivers/clk/sunxi/clk-mod0.c             |   1 +
 drivers/clk/sunxi/clk-sun6i-apb0-gates.c |   2 -
 drivers/clk/sunxi/clk-sun9i-core.c       |   1 +
 drivers/clk/sunxi/clk-sunxi.c            | 263 ++++++++++++++++---------------
 8 files changed, 135 insertions(+), 145 deletions(-)

-- 
2.6.4

The clocks registration code in clk-sunxi was most of the time not
returning the struct clk (or struct clk array) that was registered,
preventing the users of such functions to manipulate it, for example to
protect it.

Make them return it so that we can start using it.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-sunxi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index e460a6b0068c..67ef94948544 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -615,8 +615,8 @@ static const struct mux_data sun8i_h3_ahb2_mux_data __initconst = {
 	.shift = 0,
 };
 
-static void __init sunxi_mux_clk_setup(struct device_node *node,
-				       struct mux_data *data)
+static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
+					       struct mux_data *data)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;

@@ -638,6 +638,8 @@ static void __init sunxi_mux_clk_setup(struct device_node *node,
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
 		clk_register_clkdev(clk, clk_name, NULL);
 	}
+
+	return clk;
 }
 
 

@@ -722,7 +724,6 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 }
 
 
-
 /**
  * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks
  */

@@ -808,8 +809,8 @@ static const struct divs_data sun6i_a31_pll6_divs_data __initconst = {
  *           |________________________|
  */
 
-static void __init sunxi_divs_clk_setup(struct device_node *node,
-					struct divs_data *data)
+static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
+						 struct divs_data *data)
 {
 	struct clk_onecell_data *clk_data;
 	const char *parent;

@@ -836,7 +837,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,
 
 	clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL);
 	if (!clk_data)
-		return;
+		return NULL;
 
 	clks = kcalloc(ndivs, sizeof(*clks), GFP_KERNEL);
 	if (!clks)

@@ -922,7 +923,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,
 
 	of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 
-	return;
+	return clks;
 
 free_gate:
 	kfree(gate);

@@ -930,6 +931,7 @@ free_clks:
 	kfree(clks);
 free_clkdata:
 	kfree(clk_data);
+	return NULL;
 }
 
 

-- 
2.6.4

On Tue, Feb 2, 2016 at 4:47 PM, Maxime Ripard
[off-list ref] wrote:

The clocks registration code in clk-sunxi was most of the time not
returning the struct clk (or struct clk array) that was registered,
preventing the users of such functions to manipulate it, for example to
protect it.

Make them return it so that we can start using it.

Should they return error codes as well? So the CLK_OF_DECLARE setup functions
can report errors. Or make these helper functions report errors directly?

ChenYu

Hi Maxime,

On 02/02/16 10:35, Chen-Yu Tsai wrote:

On Tue, Feb 2, 2016 at 4:47 PM, Maxime Ripard
[off-list ref] wrote:

quoted

The clocks registration code in clk-sunxi was most of the time not
returning the struct clk (or struct clk array) that was registered,
preventing the users of such functions to manipulate it, for example to
protect it.

Make them return it so that we can start using it.

Should they return error codes as well? So the CLK_OF_DECLARE setup functions
can report errors. Or make these helper functions report errors directly?

Yeah, I was about the reply the same.
It looks like we can use the ERR_PTR facility to achieve this, as it's
already used in other parts of the clk framework.

For instance ...

ChenYu

quoted

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-sunxi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index e460a6b0068c..67ef94948544 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -615,8 +615,8 @@ static const struct mux_data sun8i_h3_ahb2_mux_data __initconst = {
        .shift = 0,
 };

-static void __init sunxi_mux_clk_setup(struct device_node *node,
-                                      struct mux_data *data)
+static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
+                                              struct mux_data *data)
 {
        struct clk *clk;
        const char *clk_name = node->name;

@@ -638,6 +638,8 @@ static void __init sunxi_mux_clk_setup(struct device_node *node,
                of_clk_add_provider(node, of_clk_src_simple_get, clk);
                clk_register_clkdev(clk, clk_name, NULL);

... here we have two functions that could fail and actually return an
error code.
This whole clock tree is quite complex already, so having clock
initialisations fail without any error message is not really helping.
Doing: "if (ret) {pr_warn(...); return ERR_PTR(ret);}" should be doable,
I think. Or we do it in the upper layers, when we have the error reason.

Otherwise I like the approach!

Cheers,
Andre.

quoted

        }
+
+       return clk;
 }

@@ -722,7 +724,6 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 }


-
 /**
  * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks
  */

@@ -808,8 +809,8 @@ static const struct divs_data sun6i_a31_pll6_divs_data __initconst = {
  *           |________________________|
  */

-static void __init sunxi_divs_clk_setup(struct device_node *node,
-                                       struct divs_data *data)
+static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
+                                                struct divs_data *data)
 {
        struct clk_onecell_data *clk_data;
        const char *parent;

@@ -836,7 +837,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,

        clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL);
        if (!clk_data)
-               return;
+               return NULL;

        clks = kcalloc(ndivs, sizeof(*clks), GFP_KERNEL);
        if (!clks)

@@ -922,7 +923,7 @@ static void __init sunxi_divs_clk_setup(struct device_node *node,

        of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);

-       return;
+       return clks;

 free_gate:
        kfree(gate);

@@ -930,6 +931,7 @@ free_clks:
        kfree(clks);
 free_clkdata:
        kfree(clk_data);
+       return NULL;
 }


--

2.6.4

Hi,

On Tue, Feb 02, 2016 at 06:35:29PM +0800, Chen-Yu Tsai wrote:

On Tue, Feb 2, 2016 at 4:47 PM, Maxime Ripard
[off-list ref] wrote:

quoted

The clocks registration code in clk-sunxi was most of the time not
returning the struct clk (or struct clk array) that was registered,
preventing the users of such functions to manipulate it, for example to
protect it.

Make them return it so that we can start using it.

Should they return error codes as well? So the CLK_OF_DECLARE setup functions
can report errors. Or make these helper functions report errors directly?

CLK_OF_DECLARE doesn't handle any kind of error, as the setup function
must return void, and the error printing should rather be in the
common functions in order to avoid duplicating it everywhere, so I'm
not sure. Or did you had something else in mind?

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/20160202/9af172d6/attachment.sig>

On Tue, Feb 02, 2016 at 09:47:10AM +0100, Maxime Ripard wrote:

The clocks registration code in clk-sunxi was most of the time not
returning the struct clk (or struct clk array) that was registered,
preventing the users of such functions to manipulate it, for example to
protect it.

Make them return it so that we can start using it.

Signed-off-by: Maxime Ripard <redacted>

After discussing it with Chen-Yu on IRC, the error logging will be
done as a subsequent patch.

Applied.

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/20160204/72a9f1ac/attachment.sig>

All the data structure that we pass to the clocks setup functions are
declared const, while our setup functions expects a regular pointer. This
was hidden by the fact that we cast a void * pointer back to these
structures, which made it go unnoticed.

Fix the functions prototype.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-sunxi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 67ef94948544..a3d706f5b21c 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -616,7 +616,7 @@ static const struct mux_data sun8i_h3_ahb2_mux_data __initconst = {
 };
 
 static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
-					       struct mux_data *data)
+					       const struct mux_data *data)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;

@@ -700,7 +700,7 @@ static const struct div_data sun4i_apb0_data __initconst = {
 };
 
 static void __init sunxi_divider_clk_setup(struct device_node *node,
-					   struct div_data *data)
+					   const struct div_data *data)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;

@@ -810,7 +810,7 @@ static const struct divs_data sun6i_a31_pll6_divs_data __initconst = {
  */
 
 static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
-						 struct divs_data *data)
+						 const struct divs_data *data)
 {
 	struct clk_onecell_data *clk_data;
 	const char *parent;

-- 
2.6.4

On Tue, Feb 2, 2016 at 4:47 PM, Maxime Ripard
[off-list ref] wrote:

All the data structure that we pass to the clocks setup functions are
declared const, while our setup functions expects a regular pointer. This
was hidden by the fact that we cast a void * pointer back to these
structures, which made it go unnoticed.

Fix the functions prototype.

Signed-off-by: Maxime Ripard <redacted>

Acked-by: Chen-Yu Tsai <redacted>

On 02/02/16 08:47, Maxime Ripard wrote:

All the data structure that we pass to the clocks setup functions are
declared const, while our setup functions expects a regular pointer. This
was hidden by the fact that we cast a void * pointer back to these
structures, which made it go unnoticed.

Fix the functions prototype.

Signed-off-by: Maxime Ripard <redacted>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

On Tue, Feb 02, 2016 at 09:47:11AM +0100, Maxime Ripard wrote:

All the data structure that we pass to the clocks setup functions are
declared const, while our setup functions expects a regular pointer. This
was hidden by the fact that we cast a void * pointer back to these
structures, which made it go unnoticed.

Fix the functions prototype.

Signed-off-by: Maxime Ripard <redacted>

Applied with Andre and Chen-Yu Acked-by's

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/20160204/b27f5495/attachment-0001.sig>

The current clock registration and protection code has a few drawbacks, the
two main ones being that we create a lot of orphans clock in the
registration phase, which will be troublesome when we will start being less
relaxed about them.

The protection code also relies on clkdev, which we don't really use but
for this particular case.

Fix both at the same time by moving everyone to the CLK_OF_DECLARE that
will probe our clock tree in the right and thus avoid orphans, and by
protecting directly the clock returned by our registration function.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-sunxi.c | 150 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 133 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index a3d706f5b21c..da6c1bc763d1 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -585,6 +585,41 @@ static struct clk * __init sunxi_factors_clk_setup(struct device_node *node,
 	return sunxi_factors_register(node, data, &clk_lock, reg);
 }
 
+static void __init sun4i_pll1_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun4i_pll1_data);
+}
+CLK_OF_DECLARE(sun4i_pll1, "allwinner,sun4i-a10-pll1-clk",
+	       sun4i_pll1_clk_setup);
+
+static void __init sun6i_pll1_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun6i_a31_pll1_data);
+}
+CLK_OF_DECLARE(sun6i_pll1, "allwinner,sun6i-a31-pll1-clk",
+	       sun6i_pll1_clk_setup);
+
+static void __init sun8i_pll1_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun8i_a23_pll1_data);
+}
+CLK_OF_DECLARE(sun8i_pll1, "allwinner,sun8i-a23-pll1-clk",
+	       sun8i_pll1_clk_setup);
+
+static void __init sun7i_pll4_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun7i_a20_pll4_data);
+}
+CLK_OF_DECLARE(sun7i_pll4, "allwinner,sun7i-a20-pll4-clk",
+	       sun7i_pll4_clk_setup);
+
+static void __init sun5i_ahb_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun5i_a13_ahb_data);
+}
+CLK_OF_DECLARE(sun5i_ahb, "allwinner,sun5i-a13-ahb-clk",
+	       sun5i_ahb_clk_setup);
+
 static void __init sun6i_ahb1_clk_setup(struct device_node *node)
 {
 	sunxi_factors_clk_setup(node, &sun6i_ahb1_data);

@@ -592,6 +627,20 @@ static void __init sun6i_ahb1_clk_setup(struct device_node *node)
 CLK_OF_DECLARE(sun6i_a31_ahb1, "allwinner,sun6i-a31-ahb1-clk",
 	       sun6i_ahb1_clk_setup);
 
+static void __init sun4i_apb1_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun4i_apb1_data);
+}
+CLK_OF_DECLARE(sun4i_apb1, "allwinner,sun4i-a10-apb1-clk",
+	       sun4i_apb1_clk_setup);
+
+static void __init sun7i_out_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun7i_a20_out_data);
+}
+CLK_OF_DECLARE(sun7i_out, "allwinner,sun7i-a20-out-clk",
+	       sun7i_out_clk_setup);
+
 
 /**
  * sunxi_mux_clk_setup() - Setup function for muxes

@@ -642,6 +691,34 @@ static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
 	return clk;
 }
 
+static void __init sun4i_cpu_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+
+	clk = sunxi_mux_clk_setup(node, &sun4i_cpu_mux_data);
+	if (!clk)
+		return;
+
+	/* Protect CPU clock */
+	__clk_get(clk);
+	clk_prepare_enable(clk);
+}
+CLK_OF_DECLARE(sun4i_cpu, "allwinner,sun4i-a10-cpu-clk",
+	       sun4i_cpu_clk_setup);
+
+static void __init sun6i_ahb1_mux_clk_setup(struct device_node *node)
+{
+	sunxi_mux_clk_setup(node, &sun6i_a31_ahb1_mux_data);
+}
+CLK_OF_DECLARE(sun6i_ahb1_mux, "allwinner,sun6i-a31-ahb1-mux-clk",
+	       sun6i_ahb1_mux_clk_setup);
+
+static void __init sun8i_ahb2_clk_setup(struct device_node *node)
+{
+	sunxi_mux_clk_setup(node, &sun8i_h3_ahb2_mux_data);
+}
+CLK_OF_DECLARE(sun8i_ahb2, "allwinner,sun8i-a31-ahb2-clk",
+	       sun8i_ahb2_clk_setup);
 
 
 /**

@@ -723,6 +800,34 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 	}
 }
 
+static void __init sun4i_ahb_clk_setup(struct device_node *node)
+{
+	sunxi_divider_clk_setup(node, &sun4i_ahb_data);
+}
+CLK_OF_DECLARE(sun4i_ahb, "allwinner,sun4i-a10-ahb-clk",
+	       sun4i_ahb_clk_setup);
+
+static void __init sun4i_apb0_clk_setup(struct device_node *node)
+{
+	sunxi_divider_clk_setup(node, &sun4i_apb0_data);
+}
+CLK_OF_DECLARE(sun4i_apb0, "allwinner,sun4i-a10-apb0-clk",
+	       sun4i_apb0_clk_setup);
+
+static void __init sun4i_axi_clk_setup(struct device_node *node)
+{
+	sunxi_divider_clk_setup(node, &sun4i_axi_data);
+}
+CLK_OF_DECLARE(sun4i_axi, "allwinner,sun4i-a10-axi-clk",
+	       sun4i_axi_clk_setup);
+
+static void __init sun8i_axi_clk_setup(struct device_node *node)
+{
+	sunxi_divider_clk_setup(node, &sun8i_a23_axi_data);
+}
+CLK_OF_DECLARE(sun8i_axi, "allwinner,sun8i-a23-axi-clk",
+	       sun8i_axi_clk_setup);
+
 
 /**
  * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks

@@ -934,42 +1039,53 @@ free_clkdata:
 	return NULL;
 }
 
+static void __init sun4i_pll5_clk_setup(struct device_node *node)
+{
+	struct clk **clks;
+
+	clks = sunxi_divs_clk_setup(node, &pll5_divs_data);
+	if (!clks)
+		return;
+
+	/* Protect PLL5_DDR */
+	__clk_get(clks[0]);
+	clk_prepare_enable(clks[0]);
+}
+CLK_OF_DECLARE(sun4i_pll5, "allwinner,sun4i-a10-pll5-clk",
+	       sun4i_pll5_clk_setup);
+
+static void __init sun4i_pll6_clk_setup(struct device_node *node)
+{
+	sunxi_divs_clk_setup(node, &pll6_divs_data);
+}
+CLK_OF_DECLARE(sun4i_pll6, "allwinner,sun4i-a10-pll6-clk",
+	       sun4i_pll6_clk_setup);
+
+static void __init sun6i_pll6_clk_setup(struct device_node *node)
+{
+	sunxi_divs_clk_setup(node, &sun6i_a31_pll6_divs_data);
+}
+CLK_OF_DECLARE(sun6i_pll6, "allwinner,sun6i-a31-pll6-clk",
+	       sun6i_pll6_clk_setup);
 
 
 /* Matches for factors clocks */
 static const struct of_device_id clk_factors_match[] __initconst = {
-	{.compatible = "allwinner,sun4i-a10-pll1-clk", .data = &sun4i_pll1_data,},
-	{.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,},
-	{.compatible = "allwinner,sun8i-a23-pll1-clk", .data = &sun8i_a23_pll1_data,},
-	{.compatible = "allwinner,sun7i-a20-pll4-clk", .data = &sun7i_a20_pll4_data,},
-	{.compatible = "allwinner,sun5i-a13-ahb-clk", .data = &sun5i_a13_ahb_data,},
-	{.compatible = "allwinner,sun4i-a10-apb1-clk", .data = &sun4i_apb1_data,},
-	{.compatible = "allwinner,sun7i-a20-out-clk", .data = &sun7i_a20_out_data,},
 	{}
 };
 
 /* Matches for divider clocks */
 static const struct of_device_id clk_div_match[] __initconst = {
-	{.compatible = "allwinner,sun4i-a10-axi-clk", .data = &sun4i_axi_data,},
-	{.compatible = "allwinner,sun8i-a23-axi-clk", .data = &sun8i_a23_axi_data,},
-	{.compatible = "allwinner,sun4i-a10-ahb-clk", .data = &sun4i_ahb_data,},
-	{.compatible = "allwinner,sun4i-a10-apb0-clk", .data = &sun4i_apb0_data,},
 	{}
 };
 
 /* Matches for divided outputs */
 static const struct of_device_id clk_divs_match[] __initconst = {
-	{.compatible = "allwinner,sun4i-a10-pll5-clk", .data = &pll5_divs_data,},
-	{.compatible = "allwinner,sun4i-a10-pll6-clk", .data = &pll6_divs_data,},
-	{.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_divs_data,},
 	{}
 };
 
 /* Matches for mux clocks */
 static const struct of_device_id clk_mux_match[] __initconst = {
-	{.compatible = "allwinner,sun4i-a10-cpu-clk", .data = &sun4i_cpu_mux_data,},
-	{.compatible = "allwinner,sun6i-a31-ahb1-mux-clk", .data = &sun6i_a31_ahb1_mux_data,},
-	{.compatible = "allwinner,sun8i-h3-ahb2-clk", .data = &sun8i_h3_ahb2_mux_data,},
 	{}
 };
 

-- 
2.6.4

Hi Maxime,

On 02/02/16 08:47, Maxime Ripard wrote:

The current clock registration and protection code has a few drawbacks, the
two main ones being that we create a lot of orphans clock in the
registration phase, which will be troublesome when we will start being less
relaxed about them.

The protection code also relies on clkdev, which we don't really use but
for this particular case.

Fix both at the same time by moving everyone to the CLK_OF_DECLARE that
will probe our clock tree in the right and thus avoid orphans, and by
protecting directly the clock returned by our registration function.

I very much appreciate this cleanup and like the idea. Any chance we can
have this rather quickly, so that I can rebase the A64 support series on it?

One issue below, though:

...

I don't find this clock in my tree (which is mripard/sunxi/for-next).
Instead I only have "allwinner,sun8i-h3-ahb2-clk", as mentioned below.
But as you remove this clock below from the old code and instead
instantiate this new clock here, this looks somehow wrong to me. Can you
confirm this or am I utterly confused?

Apart from that I checked each and every clock mentioned in this patch
and can confirm that the transformation is correct. So if you fix this,
I can send a Reviewed-by.

Cheers,
Andre.

Hi,

On Tue, Feb 02, 2016 at 02:16:54PM +0000, Andre Przywara wrote:

Hi Maxime,

On 02/02/16 08:47, Maxime Ripard wrote:

quoted

The current clock registration and protection code has a few drawbacks, the
two main ones being that we create a lot of orphans clock in the
registration phase, which will be troublesome when we will start being less
relaxed about them.

The protection code also relies on clkdev, which we don't really use but
for this particular case.

Fix both at the same time by moving everyone to the CLK_OF_DECLARE that
will probe our clock tree in the right and thus avoid orphans, and by
protecting directly the clock returned by our registration function.

I very much appreciate this cleanup and like the idea. Any chance we can
have this rather quickly, so that I can rebase the A64 support series on it?

I actually count on that :)

I wasn't really happy about your allwinner,sunxi compatible, so I just
gave you an easier way out ;)

quoted

+static void __init sun8i_ahb2_clk_setup(struct device_node *node)
+{
+	sunxi_mux_clk_setup(node, &sun8i_h3_ahb2_mux_data);
+}
+CLK_OF_DECLARE(sun8i_ahb2, "allwinner,sun8i-a31-ahb2-clk",
+	       sun8i_ahb2_clk_setup);

I don't find this clock in my tree (which is mripard/sunxi/for-next).
Instead I only have "allwinner,sun8i-h3-ahb2-clk", as mentioned below.
But as you remove this clock below from the old code and instead
instantiate this new clock here, this looks somehow wrong to me. Can you
confirm this or am I utterly confused?

Damn, you're right, it's just a silly copy-paste issue, I'll fix it.

Apart from that I checked each and every clock mentioned in this patch
and can confirm that the transformation is correct. So if you fix this,
I can send a Reviewed-by.

Thanks,
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/20160202/b628781e/attachment.sig>

Hi,

On 02/02/16 17:00, Maxime Ripard wrote:

Hi,

On Tue, Feb 02, 2016 at 02:16:54PM +0000, Andre Przywara wrote:

quoted

Hi Maxime,

On 02/02/16 08:47, Maxime Ripard wrote:

quoted

The current clock registration and protection code has a few drawbacks, the
two main ones being that we create a lot of orphans clock in the
registration phase, which will be troublesome when we will start being less
relaxed about them.

The protection code also relies on clkdev, which we don't really use but
for this particular case.

Fix both at the same time by moving everyone to the CLK_OF_DECLARE that
will probe our clock tree in the right and thus avoid orphans, and by
protecting directly the clock returned by our registration function.

I very much appreciate this cleanup and like the idea. Any chance we can
have this rather quickly, so that I can rebase the A64 support series on it?

I actually count on that :)

Great!

I wasn't really happy about your allwinner,sunxi compatible,

Me neither, honestly, but I didn't dare to touch clk-sunxi.c even more ;-)

so I just
gave you an easier way out ;)

Appreciated!

quoted

quoted

+static void __init sun8i_ahb2_clk_setup(struct device_node *node)
+{
+	sunxi_mux_clk_setup(node, &sun8i_h3_ahb2_mux_data);
+}
+CLK_OF_DECLARE(sun8i_ahb2, "allwinner,sun8i-a31-ahb2-clk",
+	       sun8i_ahb2_clk_setup);

I don't find this clock in my tree (which is mripard/sunxi/for-next).
Instead I only have "allwinner,sun8i-h3-ahb2-clk", as mentioned below.
But as you remove this clock below from the old code and instead
instantiate this new clock here, this looks somehow wrong to me. Can you
confirm this or am I utterly confused?

Damn, you're right, it's just a silly copy-paste issue, I'll fix it.

I am sure you left it in to see if somebody actually checks it ;-)

Merci!
Andre

quoted

Apart from that I checked each and every clock mentioned in this patch
and can confirm that the transformation is correct. So if you fix this,
I can send a Reviewed-by.

Thanks,
Maxime

On Tue, Feb 02, 2016 at 09:47:12AM +0100, Maxime Ripard wrote:

The current clock registration and protection code has a few drawbacks, the
two main ones being that we create a lot of orphans clock in the
registration phase, which will be troublesome when we will start being less
relaxed about them.

The protection code also relies on clkdev, which we don't really use but
for this particular case.

Fix both at the same time by moving everyone to the CLK_OF_DECLARE that
will probe our clock tree in the right and thus avoid orphans, and by
protecting directly the clock returned by our registration function.

Signed-off-by: Maxime Ripard <redacted>

Fixed the issue reported by Andre, and applied.

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/20160204/d458471c/attachment.sig>

Now that we don't have any user left for the old registration code, we can
remove it.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-sunxi.c | 114 ------------------------------------------
 1 file changed, 114 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index da6c1bc763d1..5f2f3e0408bd 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -1067,117 +1067,3 @@ static void __init sun6i_pll6_clk_setup(struct device_node *node)
 }
 CLK_OF_DECLARE(sun6i_pll6, "allwinner,sun6i-a31-pll6-clk",
 	       sun6i_pll6_clk_setup);
-
-
-/* Matches for factors clocks */
-static const struct of_device_id clk_factors_match[] __initconst = {
-	{}
-};
-
-/* Matches for divider clocks */
-static const struct of_device_id clk_div_match[] __initconst = {
-	{}
-};
-
-/* Matches for divided outputs */
-static const struct of_device_id clk_divs_match[] __initconst = {
-	{}
-};
-
-/* Matches for mux clocks */
-static const struct of_device_id clk_mux_match[] __initconst = {
-	{}
-};
-
-
-static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match,
-					      void *function)
-{
-	struct device_node *np;
-	const struct div_data *data;
-	const struct of_device_id *match;
-	void (*setup_function)(struct device_node *, const void *) = function;
-
-	for_each_matching_node_and_match(np, clk_match, &match) {
-		data = match->data;
-		setup_function(np, data);
-	}
-}
-
-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);
-
-	/* Register factor clocks */
-	of_sunxi_table_clock_setup(clk_factors_match, sunxi_factors_clk_setup);
-
-	/* Register divider clocks */
-	of_sunxi_table_clock_setup(clk_div_match, sunxi_divider_clk_setup);
-
-	/* Register mux clocks */
-	of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup);
-
-	/* Protect the clocks that needs to stay on */
-	for (i = 0; i < nclocks; i++) {
-		struct clk *clk = clk_get(NULL, clocks[i]);
-
-		if (!IS_ERR(clk))
-			clk_prepare_enable(clk);
-	}
-}
-
-static const char *sun4i_a10_critical_clocks[] __initdata = {
-	"pll5_ddr",
-};
-
-static void __init sun4i_a10_init_clocks(struct device_node *node)
-{
-	sunxi_init_clocks(sun4i_a10_critical_clocks,
-			  ARRAY_SIZE(sun4i_a10_critical_clocks));
-}
-CLK_OF_DECLARE(sun4i_a10_clk_init, "allwinner,sun4i-a10", sun4i_a10_init_clocks);
-
-static const char *sun5i_critical_clocks[] __initdata = {
-	"cpu",
-	"pll5_ddr",
-};
-
-static void __init sun5i_init_clocks(struct device_node *node)
-{
-	sunxi_init_clocks(sun5i_critical_clocks,
-			  ARRAY_SIZE(sun5i_critical_clocks));
-}
-CLK_OF_DECLARE(sun5i_a10s_clk_init, "allwinner,sun5i-a10s", sun5i_init_clocks);
-CLK_OF_DECLARE(sun5i_a13_clk_init, "allwinner,sun5i-a13", sun5i_init_clocks);
-CLK_OF_DECLARE(sun5i_r8_clk_init, "allwinner,sun5i-r8", sun5i_init_clocks);
-CLK_OF_DECLARE(sun7i_a20_clk_init, "allwinner,sun7i-a20", sun5i_init_clocks);
-
-static const char *sun6i_critical_clocks[] __initdata = {
-	"cpu",
-};
-
-static void __init sun6i_init_clocks(struct device_node *node)
-{
-	sunxi_init_clocks(sun6i_critical_clocks,
-			  ARRAY_SIZE(sun6i_critical_clocks));
-}
-CLK_OF_DECLARE(sun6i_a31_clk_init, "allwinner,sun6i-a31", sun6i_init_clocks);
-CLK_OF_DECLARE(sun6i_a31s_clk_init, "allwinner,sun6i-a31s", sun6i_init_clocks);
-CLK_OF_DECLARE(sun8i_a23_clk_init, "allwinner,sun8i-a23", sun6i_init_clocks);
-CLK_OF_DECLARE(sun8i_a33_clk_init, "allwinner,sun8i-a33", sun6i_init_clocks);
-CLK_OF_DECLARE(sun8i_h3_clk_init, "allwinner,sun8i-h3", sun6i_init_clocks);
-
-static void __init sun8i_a83t_init_clocks(struct device_node *node)
-{
-	sunxi_init_clocks(NULL, 0);
-}
-CLK_OF_DECLARE(sun8i_a83t_clk_init, "allwinner,sun8i-a83t", sun8i_a83t_init_clocks);
-
-static void __init sun9i_init_clocks(struct device_node *node)
-{
-	sunxi_init_clocks(NULL, 0);
-}
-CLK_OF_DECLARE(sun9i_a80_clk_init, "allwinner,sun9i-a80", sun9i_init_clocks);

-- 
2.6.4

On Tue, Feb 02, 2016 at 09:47:13AM +0100, Maxime Ripard wrote:

Now that we don't have any user left for the old registration code, we can
remove it.

Signed-off-by: Maxime Ripard <redacted>

Applied.

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/20160204/dbb69dad/attachment.sig>

Now that our protection code doesn't use the global name lookup anymore, we
can remove the clkdev registrations.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi/clk-a10-hosc.c         | 3 +--
 drivers/clk/sunxi/clk-a20-gmac.c         | 2 --
 drivers/clk/sunxi/clk-factors.c          | 7 -------
 drivers/clk/sunxi/clk-factors.h          | 1 -
 drivers/clk/sunxi/clk-mod0.c             | 1 +
 drivers/clk/sunxi/clk-sun6i-apb0-gates.c | 2 --
 drivers/clk/sunxi/clk-sun9i-core.c       | 1 +
 drivers/clk/sunxi/clk-sunxi.c            | 3 ---
 8 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/sunxi/clk-a10-hosc.c b/drivers/clk/sunxi/clk-a10-hosc.c
index 0481d5d673d6..6b598c6a0213 100644
--- a/drivers/clk/sunxi/clk-a10-hosc.c
+++ b/drivers/clk/sunxi/clk-a10-hosc.c

@@ -15,9 +15,9 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/clkdev.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/slab.h>
 
 #define SUNXI_OSC24M_GATE	0
 

@@ -61,7 +61,6 @@ static void __init sun4i_osc_clk_setup(struct device_node *node)
 		goto err_free_gate;
 
 	of_clk_add_provider(node, of_clk_src_simple_get, clk);
-	clk_register_clkdev(clk, clk_name, NULL);
 
 	return;
 

diff --git a/drivers/clk/sunxi/clk-a20-gmac.c b/drivers/clk/sunxi/clk-a20-gmac.c
index 1611b036421c..3437f734c9bf 100644
--- a/drivers/clk/sunxi/clk-a20-gmac.c
+++ b/drivers/clk/sunxi/clk-a20-gmac.c

@@ -17,7 +17,6 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/clkdev.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/slab.h>

@@ -107,7 +106,6 @@ static void __init sun7i_a20_gmac_clk_setup(struct device_node *node)
 		goto iounmap_reg;
 
 	of_clk_add_provider(node, of_clk_src_simple_get, clk);
-	clk_register_clkdev(clk, clk_name, NULL);
 
 	return;
 

diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index 6e97f46c0c37..ddefe9668863 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c

@@ -258,14 +258,8 @@ struct clk *sunxi_factors_register(struct device_node *node,
 	if (ret)
 		goto err_provider;
 
-	ret = clk_register_clkdev(clk, clk_name, NULL);
-	if (ret)
-		goto err_clkdev;
-
 	return clk;
 
-err_clkdev:
-	of_clk_del_provider(node);
 err_provider:
 	/* TODO: The composite clock stuff will leak a bit here. */
 	clk_unregister(clk);

@@ -291,7 +285,6 @@ void sunxi_factors_unregister(struct device_node *node, struct clk *clk)
 	factors = to_clk_factors(hw);
 	name = clk_hw_get_name(hw);
 
-	/* No unregister call for clkdev_* */
 	of_clk_del_provider(node);
 	/* TODO: The composite clock stuff will leak a bit here. */
 	clk_unregister(clk);

diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h
index a44a865a6b9e..1e63c5b2d5f4 100644
--- a/drivers/clk/sunxi/clk-factors.h
+++ b/drivers/clk/sunxi/clk-factors.h

@@ -2,7 +2,6 @@
 #define __MACH_SUNXI_CLK_FACTORS_H
 
 #include <linux/clk-provider.h>
-#include <linux/clkdev.h>
 #include <linux/spinlock.h>
 
 #define SUNXI_FACTORS_NOT_APPLICABLE	(0)

diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c
index 578bff0dd251..b38d71cec74c 100644
--- a/drivers/clk/sunxi/clk-mod0.c
+++ b/drivers/clk/sunxi/clk-mod0.c

@@ -15,6 +15,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>

diff --git a/drivers/clk/sunxi/clk-sun6i-apb0-gates.c b/drivers/clk/sunxi/clk-sun6i-apb0-gates.c
index 23d042aabb4f..68021fa5ecd9 100644
--- a/drivers/clk/sunxi/clk-sun6i-apb0-gates.c
+++ b/drivers/clk/sunxi/clk-sun6i-apb0-gates.c

@@ -9,7 +9,6 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/clkdev.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>

@@ -87,7 +86,6 @@ static int sun6i_a31_apb0_gates_clk_probe(struct platform_device *pdev)
 						      clk_parent, 0, reg, i,
 						      0, NULL);
 		WARN_ON(IS_ERR(clk_data->clks[i]));
-		clk_register_clkdev(clk_data->clks[i], clk_name, NULL);
 
 		j++;
 	}

diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
index a81211062ecb..43f014f85803 100644
--- a/drivers/clk/sunxi/clk-sun9i-core.c
+++ b/drivers/clk/sunxi/clk-sun9i-core.c

@@ -15,6 +15,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 5f2f3e0408bd..91f6d8ef81df 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c

@@ -685,7 +685,6 @@ static struct clk * __init sunxi_mux_clk_setup(struct device_node *node,
 
 	if (clk) {
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
-		clk_register_clkdev(clk, clk_name, NULL);
 	}
 
 	return clk;

@@ -796,7 +795,6 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
 					 data->table, &clk_lock);
 	if (clk) {
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
-		clk_register_clkdev(clk, clk_name, NULL);
 	}
 }
 

@@ -1020,7 +1018,6 @@ static struct clk ** __init sunxi_divs_clk_setup(struct device_node *node,
 						 clkflags);
 
 		WARN_ON(IS_ERR(clk_data->clks[i]));
-		clk_register_clkdev(clks[i], clk_name, NULL);
 	}
 
 	/* Adjust to the real max */

-- 
2.6.4

On Tue,  2 Feb 2016 09:47:14 +0100
Maxime Ripard [off-list ref] wrote:

Why?

 #include <linux/clk-provider.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

On Tue, Feb 02, 2016 at 11:26:04AM +0100, Jean-Francois Moine wrote:

On Tue,  2 Feb 2016 09:47:14 +0100
Maxime Ripard [off-list ref] wrote:

quoted

diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c
index 578bff0dd251..b38d71cec74c 100644
--- a/drivers/clk/sunxi/clk-mod0.c
+++ b/drivers/clk/sunxi/clk-mod0.c

@@ -15,6 +15,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>

Why?

Because this driver uses __clk_get, that it was previously included in
clk-factors.h and is not anymore, and that otherwise it won't compile,
obviously.

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/20160203/bfdb411e/attachment.sig>

On Tue, Feb 02, 2016 at 09:47:14AM +0100, Maxime Ripard wrote:

Now that our protection code doesn't use the global name lookup anymore, we
can remove the clkdev registrations.

Signed-off-by: Maxime Ripard <redacted>

Applied.

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/20160204/0ddf838d/attachment.sig>