On Mon, Sep 21, 2015 at 03:06:18PM +0200, Thomas Gleixner wrote: > On Tue, 4 Aug 2015, Russell King - ARM Linux wrote: > > On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote: > > > Consider clk_enable/disable/set_parent/setfreq operations. none of these > > > operations are "atomic" from hardware point of view. instead, they are a > > > set of steps which culminates to moving from state A to state B of the > > > clock tree configuration. > > > > There's a world of difference between clk_enable()/clk_disable() and > > the rest of the clk API. > > > > clk_enable()/clk_disable() _should_ be callable from any context, since > > you may need to enable or disable a clock from any context. The remainder > > of the clk API is callable only from contexts where sleeping is permissible. > > > > The reason we have this split is because clk_enable()/clk_disable() have > > historically been used in interrupt handlers, and they're specifically > > not supposed to impose big delays. > > > > Things like waiting for a PLL to re-lock is time-consuming, so it's not > > something I'd expect to see behind a clk_enable() implementation (the > > fact you can't sleep in there is a big hint.) Such waits should be in > > the clk_prepare() stage instead. > > You wish. Drivers with loop/udelays in the enable/disable callbacks: Most of what I've said above is entirely factual. What idiotic games people play inside clk_enable() is not my problem (I'm not even the CCF maintainer - something which Linaro "took over".) > Of course we could solve that by making enable_lock a raw_spinlock, > but looking at the various implementations of clk_ops.enable tells me > that this is not a brilliant idea. See the PLL loops/delays crap > above. There is another issue: > > Some callbacks have their own spinlocks which then need to be > converted to raw_spinlocks as well. Not a big deal, but some of the > clk drivers use that very same spinlock, which is supposed to protect > register access, for all kind of other crap, which is going to > introduce latencies. And that's a ratsnest of locks down to > regmap->lock .... > > So for RT the only sensible choice at the moment is to leave > enable_lock as non raw spinlock and deal with the very few places > where clk_enable/disable() is really called from atomic context. Right. Let's put it in the most direct and blunt way possible. Those responsible for this mess won't like it. They're not _meant_ to like this statement, because they're supposed to feel bad about the situation they've created. Maybe, in the face of this leve of humiliation, they'll assess whether they _should_ be doing better, and try to step up their game. We have: clk_enable() & clk_disable() - which are _supposed_ to be the atomic level operators, which take a spinlock and are supposed to be _fast_. clk_prepare() & clk_unprepare() - which are _supposed_ to be the time consuming bits of clk_enable()/clk_disable() However, due to the generally idiotic nature of Linux programmers, they've totally destroyed the reason for clk_enable() and clk_disable() existing. So, we now need a _third_ level of API to work around their idiotic nature, maybe clk_atomic_enable() and clk_atomic_disable(), which do what the original clk_enable() and clk_disable() do. So, we end up needing three levels because Linux programmers are basically idiots. And yes, if what you've found is correct, I think those who have created the crap have very much earned the prestigious title of being an "idiotic programmer". I'm pissed at these idiotic programmers. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
