[GIT PULL 4/8] clk: tegra: Changes for v4.2-rc1

[mturquette@linaro.org (Michael Turquette)]

Quoting Mikko Perttunen (2015-05-28 00:03:01)
> On 05/27/2015 10:50 PM, Stephen Boyd wrote:
> > On 05/21, Mikko Perttunen wrote:
> >>
> >> Hi Stephen,
> >>
> >> the way the EMC clock rate is set in hardware is similar to other
> >> clocks, i.e. there's a register and you write the new divider and parent
> >> id to it. However, with EMC, you cannot just do this any time you want;
> >> writing to the register initiates a state machine in the clock hardware
> >> that changes a large number of other parameters regarding DRAM timings
> >> as well. These parameters need to be programmed into shadow registers
> >> before the rate change write is done. This means that both the new
> >> divisor and the parent must be written in the same register write.
> >>
> >> The CCF has a callback, set_rate_and_parent, which allows for both to be
> >> passed to the driver at the same time. However, it also requires
> >> set_rate and set_parent to be implemented, which the EMC driver cannot do.
> > 
> > Ok so we should change the framework to allow drivers to only
> > implement set_rate_and_parent and use that in set_rate and
> > set_parent implementations if it's the only option available. Or
> > is there a problem if CCF allows clk_set_parent() on the EMC
> > clock by calling set_rate_and_parent() with the new parent and
> > whatever recalc_rate returns for the new parent's rate going into
> > the clock?
> 
> There isn't really a problem, but the EMC driver cannot implement this
> operation sensibly so it would just always return an error if the (rate,
> parent) pair given to set_rate_and_parent() doesn't exactly match one of
> the entries specified in device tree.
> 
> > 
> >>
> >> Furthermore, the CCF cannot help with parent selection for the EMC clock
> >> at all. The parent for each rate is selected by the board designer.
> > 
> > I'm not following this part. The CCF mostly asks clock providers
> > what parent should be used when clk_set_rate() is called.
> 
> Yep, that much is fine; what I was alluding was the above (set_parent
> and set_rate_and_parent with an unlisted (rate, parent) pair are invalid)
> 
> > 
> >>
> >> There is also the issue that sometimes, the EMC driver cannot directly
> >> switch to the target (rate, parent) pair; instead it is necessary to
> >> switch first to another pair we call a backup timing. In this situation,
> >> we temporarily have a parent that is neither the one we had before the
> >> set_rate call or the one it will be after. Especially, if the switch to
> >> the backup timing succeeds but the following switch to the target timing
> >> fails, then without the reparent call the parent in hardware would be
> >> left inconsistent with the software state.
> > 
> > Yes, I've sent a patch a while ago to support an idea of a "safe
> > parent" or a backup timing that can be used while a clock is
> > reprogrammed. Mike has also proposed the concept of "coordinated
> > clock rates" which would do something similar and allow clock
> > providers to control complicated rate transitions themselves. It
> > seems that we may be able to use the "safe parent" concept here,
> > where first we switch to some other parent, call the set_rate*
> > op, and then switch the parent back if we're not already using
> > the parent that we should be using.
> 
> While I'm not sure how much sophistication is warranted in the CCF, for
> our use case this backup timing has to be able to be a function of the
> timing we are leaving and preferably also the target timing. Also, as
> usual, the backup timings are (rate, parent) pairs, and just changing
> rate or just changing parent are both impossible.
> 
> > 
> > This sort of thing becomes important for things like per-clock
> > locking where we need to know how the clock tree is going to
> > change *before* we modify the clock tree. If we go back and forth
> > between the clock providers and the clock tree while we're in the
> > middle of making irreversible changes to the hardware, we may get
> > stuck in a situation where we need to lock more clocks and then
> > we may need to undo hardware changes.
> > 
> 
> Yeah, makes sense.
> 
> Do you think you can still pull this patchset? There's other code in
> linux-next that depends on it and I'd prefer to have a working driver in
> the kernel; if/when the improvements to CCF materialize we could update
> the driver to use them.

I'm not really sure what to do with this PR. This seems to fall into the
same category as the Exynos "cpu clocks" series: you have a complex
sequence that requires multiple clock nodes to be changes in a
coordinated fashion.

I'm working on some core infrastructure to fix this. I'd like to get
that on the list asap and possibly merged for 4.3. I think it can
benefit your case and remove the need to export clk_hw_reparent, which
is pretty nasty.

What exactly will break if this is not pulled? I appreciate your offer
to update this driver when the core changes are merged, but I would
prefer to do it the right way first, instead of fixing up something that
is already merged after the fact.

Regards,
Mike

> 
> Thanks,
> Mikko.