On Mon, 12 Jan 2015 15:44:24 -0800 Mike Turquette <mturquette@xxxxxxxxxx> wrote: > Quoting Boris Brezillon (2015-01-12 07:12:50) > > All slow clk users are not properly requesting it (get + prepare + enable) > > before using it. > > If all users properly requesting this clock decide that they don't need it > > anymore (or are removed), this lead to this clock being disabled while > > faulty users are still requiring it, which in turn hangs the system. > > The correct fix is for all users to claim the clock and enable it. Is > there a plan to implement that any time soon? Yes, hopefully for the next release, but this requires identifying all the offending drivers, fixing them and testing all the changes. > > > > > Prevent slow oscillator clock from being disabled until all users are > > properly requesting it. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> > > Reported-by: Bo Shen <voice.shen@xxxxxxxxx> > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > Hi Mike, > > > > Sorry for the noise, but I forgot to add the LKML and LAKML in Cc. > > > > Can you have a look at this fix and let me know if this is how you want this > > problem addressed ? > > I can also request (get + prepare + enable) the clk in the pmc probe function, > > so that it can never be disabled. > > > > If you're fine with the approach, can you queue it for the next -rc ? > > I can queue something for the next -rc, but maybe not this fix. Great. > > > > > Best Regards, > > > > Boris > > > > drivers/clk/at91/clk-slow.c | 28 ++++++++++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/at91/clk-slow.c b/drivers/clk/at91/clk-slow.c > > index 32f7c1b..effe3b0 100644 > > --- a/drivers/clk/at91/clk-slow.c > > +++ b/drivers/clk/at91/clk-slow.c > > @@ -96,7 +96,19 @@ static void clk_slow_osc_unprepare(struct clk_hw *hw) > > if (tmp & AT91_SCKC_OSC32BYP) > > return; > > > > - writel(tmp & ~AT91_SCKC_OSC32EN, sckcr); > > + /* > > + * FIXME: All slow clk users are not properly requesting it (get + > > + * prepare + enable) before using it. > > + * If all users properly requesting this clock decide that they don't > > + * need it anymore (or are removed), this lead to this clock being > > + * disabled while faulty users are still requiring it, which in turn > > + * hangs the system. > > + * Prevent this clock from being disabled until all users are properly > > + * requesting it. > > + * Once this is done we should re-introduce this line: > > + * > > + * writel(tmp & ~AT91_SCKC_OSC32EN, sckcr); > > + */ > > The main problem here is that the clk prepare and enable usecounts are a > lie. The use counts may be zero but the clock signal is still active. I > think a better fix is for the clock driver to get, prepare & enable this > clock in its probe function as you described above. If someone stumbles > across this clock signal and wonders why it won't quiesce, at least the > debug values will be accurate. Yep, that makes sense. I'll rework my patch. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html