[PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Doug Anderson]

Hi,

On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin at rock-chips.com> wrote:
> [Correct Doug's email address]
>
> On 2018/3/15 17:12, hl wrote:
>>
>> Hi Shawn,
>>
>>
>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>>>
>>> Hi Huang,
>>>
>>> On 2018/3/15 16:54, Lin Huang wrote:
>>>>
>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>>> assign frequency for them in dts.
>>>
>>>
>>> I'm curious under which condition that we need assign frequency for
>>> hclk_sd?
>>
>> We may set CPLL frequency higher than 800MHz, with that we need assign
>> clock
>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru
>> register value to get hclk_sd for now.
>> you can check
>>
>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5
>
>
> Okay, thanks for pointing me to the actual user case.
> I'm fine with that, however, what I am thinking now is:
>
> (1) It's worth elaborating more in the commit msg.

Seems like a nice idea to me.


> (2) The reason you need set hclk_sd is that it depends on the
> defualt settings but lack real owner to explicitly set it to <=200MHz.

Actually, in the case of hclk_sd there _is_ an owner to set it to <=
200 MHz.  I'll comment on the gerrit review, but the assigned clock
for hclk_sd probably belongs in the node "sdmmc: dwmmc at fe320000".

...but in any case, you still need the clock ID to do that.


> So my another question is if that is more about aspect of power
> consumption, then it looks okay to me. But if that is a SoC limitation,
> then we are under risk for that. Either we should never rely on the
> default settings, or we should set its rate range after registering this
> clock provider.

I have always wondered about perhaps encoding the min/max clock rate
for each clock somewhere in the source code.  Then whenever we change
clock rates we could enforce that we don't ever go past this min/max.
In think, in theory, this is possible by registering for all the right
callbacks / notifications.  ...but I suspect that doing in a generic /
performant way might be very difficult.  Specifically, whenever a
clock changes you may need to make all sorts of decisions about
re-parenting and also checking whether all your children are out fo
spec.

So without encoding the min/max and having it magically auto-resolve,
the best we can do is just to assign a sane clock rate.  If there's no
subsystem owning this clock then doing so at clock init time is sane
(and that's what we do with many clocks).  If a subsystem owns it,
doing it when the subsystem is probed is sane.


So, to summarize, I'm happy with this change.  I wouldn't mind a
little more justification in the CL description but personally I won't
make a big deal about it.

Reviewed-by: Douglas Anderson <dianders at chromium.org>


-Doug