Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Michael Turquette]

Hi Jerome,

On Mon, Apr 8, 2019 at 9:38 AM Jerome Brunet <jbrunet@xxxxxxxxxxxx> 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 <jbrunet@xxxxxxxxxxxx> 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