| Message ID | 1453385439-10154-2-git-send-email-emilio.lopez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
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",
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(-)