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

[Mikko Perttunen]

On 06/20/15 23:41, Michael Turquette wrote:
> Quoting Michael Turquette (2015-06-18 16:41:26)
>> 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've decided to pull this in the interest of fairness. I'm taking the
> Exynos cpu clock patches, which will need to be updated in the future to
> use some new infrastructure for coordinating rate changes across
> multiple clock nodes. I'm merging the EMC stuff with the same
> expectation that it will need to migrate to the new infrastructure when
> it becomes available and we'll get rid of clk_hw_reparent.
>
> Sound good?

Sounds good! Please keep us informed and once the framework changes land 
we'll fix up this one.

>
> Regards,
> Mike
>

Thanks,
Mikko.

>>
>> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>