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
