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

[Stephen Boyd]

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.