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?:

quoted

Hi Emilio,

On Thu, Jan 21, 2016 at 11:10:38AM -0300, Emilio L?pez wrote:

quoted

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 <redacted>
---
 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.

quoted

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>