On Wed, Apr 29, 2015 at 04:05:49PM -0700, Michael Turquette wrote: > > > Could you elaborate why you still want to have the clk-always-on in the > > > device tree instead of in the kernel where it can be removed when > > > necessary? What's your problem with enabling the critical clocks using > > > clk_prepare_enable like other SoCs already do? > > > > I've explained what my issues are already. I'm not a fan of > > hand-rolling and duplicating code which can be consolidated and make > > generic. Call me old fashioned. :) > > I agree with Lee that the open code stuff isn't great, but I'm less and > less convinced that the DT method is the way to go. Maxime has done a > good job of reminding me why I always pushed back on this type of > approach in the past. > > Having a clock provider driver call clk_get, clk_prepare_enable on a clk > is just fine. I think what needs to be done is to look at the platforms > that open code this and find out how to replace this with some common > code that any clock provider driver can call. Perhaps we can: > > 1) use the struct clk_ops.init callback (which is used very little) and > pass in a generic function to handle this case, or I'm not sure that would work with clocks that provide several outputs, and need only one (or at least not all of them) to be enabled. Or we would need to pass some arguments to this function when we register our clock. > 2) we can create a new per-clk flag which is used by __clk_init to call > clk_prepare_enable, or That would work. It's what people usually expect from CLK_IGNORE_UNUSED, and it's the first thing they try, so it would be the easiest I guess. > 3) we can add a new generic function like clk_register which sets any > specified defaults (clk_set_defaults), but using C code and not DT What kind of defaults are we talking about here? Just the clock state or do you include boundaries, rate, etc. in it? > I would need to look at the drivers that open code their > clk_prepare_enable calls for non-Linux devices and see what similarities > exist. What really worked for us is to call clk_prepare_enable straight after the call to clk_register. Having a single place where we enable all the clocks causes a few issues, mostly because we're not even sure the clock has been registered at this time, and we do have to consider the clock name as an ABI, which caused some issues for us in the past. > But clearly the DT element of Lee's approach is causing some push > back, so we should consider if there is a less controversial way to do > this (and a way that benefits non-DT platforms as well). > > I do think Lee's idea of consolidating around a single solution to a > common problem is a great idea, but maybe not by using Devicetree. Agreed. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
