Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu, 2018-03-08 at 19:46 +0800, Shawn Lin wrote:
> > > > Good point!
> > > > 
> > > > (1)So either we should update phase once its rate is updated, for
> > > > instance, doing it in clk_change_rate, or
> > > 
> > > I don't think this would be a good option. The phase depending on the rate is
> > > just one example. I'm pretty sure one could come up with another example
> > > 
> > > > 
> > > > (2)we should always try to get the actual phase here.
> > > 
> > > If the callback is available, lets use it. I prefer this second option
> > > 
> 
> okay.
> 
> > > > 
> > > > If we do anyway call ->get_phase, we should never need core->phase since
> > > > it doesn't reflect the actual phase maybe.
> > > 
> > > The cached value is used by debugfs so it is still a valuable information. I
> > > would keep it around for now.
> > 
> > Oh ! and let's not forget that a clock driver is allowed to implement
> > set_phase() w/o implementing get_phase() ... so you definitively want to keep
> > the cached value around
> > 
> 
> Yes, it's a little complicated. I just thought a case that, if the
> driver need a fixed phase i.e. by call clk_set_phase(180), but the clk's
> phase depends on the clk rate. So if the clk rate changed, the phase is 
> changed as well. This isn't good to the IP should sample the data based
> on the phase shift but never get notified that the phase is changed.

I suppose this use case comes from the mmc (most phase users are mmc drivers) ?

The fact that you would want ,for your use case, to lock the phase whatever the
rate changes seems like a choice. It makes sense for sure, but maybe not all
driver would want to enforce that. I'm not sure we should bake that in the
framework itself.

For such case, a solution could be to use clock notifiers to call
clk_set_phase() on rate change,

OR

in the clock driver set_rate() callback, cache the phase at the beginning and
call set_phase() again before returning.

Anyway, let's keep it simple for now and report the phase correctly. We can
still deal these particular situations with some other patches.