From: mikko.perttunen@kapsi•fi (Mikko Perttunen)
To: linux-arm-kernel@lists•infradead.org
Subject: [GIT PULL 4/8] clk: tegra: Changes for v4.2-rc1
Date: Mon, 22 Jun 2015 09:59:15 +0300 [thread overview]
Message-ID: <5587B243.2070300@kapsi.fi> (raw)
In-Reply-To: <20150620204104.9112.83328@quantum>
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
>
next prev parent reply other threads:[~2015-06-22 6:59 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-13 13:49 [GIT PULL 0/8] ARM: tegra: Changes for v4.2-rc1 Thierry Reding
2015-05-13 13:49 ` [GIT PULL 1/8] ARM: tegra: Cleanup patches " Thierry Reding
2015-05-13 15:54 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 2/8] ARM: tegra: Memory controller updates " Thierry Reding
2015-05-13 15:55 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 3/8] ARM: tegra: RAM code access " Thierry Reding
2015-05-13 15:58 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 4/8] clk: tegra: Changes " Thierry Reding
2015-05-20 19:54 ` Stephen Boyd
2015-05-21 6:25 ` Mikko Perttunen
2015-05-27 14:59 ` Mikko Perttunen
2015-05-27 19:50 ` Stephen Boyd
2015-05-28 7:03 ` Mikko Perttunen
2015-06-18 23:41 ` Michael Turquette
2015-06-20 20:41 ` Michael Turquette
2015-06-22 6:59 ` Mikko Perttunen [this message]
2015-06-22 7:03 ` Mikko Perttunen
2015-06-29 8:54 ` Thierry Reding
2015-05-28 10:13 ` Peter De Schrijver
2015-05-13 13:49 ` [GIT PULL 5/8] ARM: tegra: Add EMC driver " Thierry Reding
2015-05-13 16:00 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 6/8] ARM: tegra: Core SoC changes " Thierry Reding
2015-05-13 16:02 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 7/8] ARM: tegra: Devicetree " Thierry Reding
2015-05-13 16:09 ` Arnd Bergmann
2015-05-15 10:43 ` Thierry Reding
2015-05-15 11:06 ` Arnd Bergmann
2015-05-13 13:49 ` [GIT PULL 8/8] ARM: tegra: Default configuration updates " Thierry Reding
2015-05-13 16:12 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5587B243.2070300@kapsi.fi \
--to=mikko.perttunen@kapsi$(echo .)fi \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox