Hi Jerome, On Mon, Apr 8, 2019 at 9:38 AM Jerome Brunet <jbru...@baylibre.com> wrote: > > On Fri, 2019-04-05 at 13:43 -0700, Stephen Boyd wrote: > > Quoting Michael Turquette (2019-04-05 08:43:40) > > > Hi Jerome, > > > > > > On Fri, Mar 29, 2019 at 3:58 PM Jerome Brunet <jbru...@baylibre.com> > > > wrote: > > > > On Fri, 2019-03-29 at 15:14 -0700, Stephen Boyd wrote: > > > > > > > We actively discourage using init callbacks. Can you do this some > > > > > > > other > > > > > > > way? > > > > > > > > > > > > Yes I'm aware of that but init it the right place to do this. > > > > > > To be clear, this is not initializing the clock to some particular > > > > > > rate, the > > > > > > rate is preserved. > > > > > > > > > > > > It just applies the necessary settings that needs to be done only > > > > > > once to make > > > > > > sure the clock is in working order and that the rate calculated is > > > > > > actually > > > > > > accurate. > > > > > > > > > > Ok, but can you do that in your driver's probe routine instead of > > > > > attaching to the init callback? We want to get rid of "init" at some > > > > > point so throwing the init sequence stuff into the driver probe around > > > > > registration is a solution. Or we should think about not discouraging > > > > > the init callback > > > > > > > > Is is callback really a problem after all ? > > > > > > It is a problem, insofar as Stephen and I want to remove .init some day. > > > > > > > I think we should actively prevent using it to set a particular rate. > > > > > > > > Here, the goal is put the clock in working order. The bootloader does > > > > not > > > > always do that for us. > > > > > > The above two sentences make it sound like this sequence belongs in > > > .prepare(). > > > > > > I know that you're concerned with setting rate, but I guess it is safe > > > to assume that you'll always complete .prepare() before you begin to > > > execute .set_rate(), no? This also avoids the ugliness of putting it > > > in the .probe(), which I agree doesn't feel like an accurate thing to > > > do for a clock's own programming sequence. > > > > > > .probe() is an OK place to put policy (turn these clocks on or set > > > them to this rate, for a known-good configuration). > > > > > > > Following along with this reasoning, maybe we need a 'prepare_once' > > callback that does this the first time the clk is prepared or set_rate > > is called on it. The problem I have with the init callback is that the > > semantics of when it's called and what has happened before it's called > > isn't clearly defined. I'm afraid to remove it and also afraid to move > > around where it's called from. I've been itching to get it out of under > > all the locks we're holding at registration time, but I don't know if > > that's feasible, for example. > > > > If removing .init() is important for you, I would prefer to help guys. That > being said, we need a decent solution to some use case if this is going to > work. > > I'll illustrate with examples from the meson code but I think they represent > fairly common cases: > > 1) clk-pll: Without the work done init(), the pll just won't lock ... > > I agree that we would see the problem when the clock is finally enabled, so we > could do "init" sequence in .prepare() each time it is called. The sequence > might be "long" (with a couple of delays in it) so doing it on each call to > .prepare() is wasting time but it could work. > > Something like .prepare_once would help for this. > > 2) clk-mpll: Without the work done in .init(), the fractional part of the > divider will not work, only the integer part works => the rate calculated is > off by a significant margin. IOW, until the initial setup is done, the result > of .recalc_rate() is off and cannot be trusted. > > .prepare() (and .prepare_once() if called a the same stage) is too late. We > would need something that runs before any call to .recalc_rate() is made. > > We could also think about clocks with the ability to observe and feedback > (read-only) on the actual output signal. Such thing might need an initial() > setup as well. > > 3) sclk: This is a kind of divider which gates when the value is zero. We need > to save the divider value on .disable() to restore it on .enable(). In > .init(), if divider value is 0 (gated) we init cached value to the maximum > divider value. This done so a call to .enable() works, even without prior > calls to .set_rate(). > > Here, again, I think .prepare() is a too late. > > Maybe it is a bit extreme but we could also think about weird muxes not > reporting the parent accurately until some prior setting is done. For those > you need something that runs before all the orphan mechanism in the clock > register. > > Whatever the name we give it, It think it would help if we had a (not > discouraged) callback that is guaranteed to run before anything else is called > on the clock. This is what I tried to do with 541debae0adf ("clk: call the > clock init() callback before any other ops callback"). > > Having the counterpart callback, guaranteed to run last, would allow a clock > to allocate (and de-allocate) stuff. It would be an interesting addition IMO. > For clocks which needs to store things (such as sclk), it would allow to take > the runtime data out of the init data. > > What about .register() and .deregister() ? It would map nicely to the CCF API > ?
I like .register & .deregister. I propose that we merge your .init to keep things moving BUT ONLY if you pinky swear to follow up with .register & .deregister conversion and convert all existing .init users over to .register. Deal? Stephen, thoughts? Thanks, Mike > > > > -- Michael Turquette CEO BayLibre - At the Heart of Embedded Linux http://baylibre.com/ Schedule a meeting: https://calendly.com/mturquette