Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 7/18/19 1:36 PM, Sowjanya Komatineni wrote:
>
> On 7/18/19 1:26 PM, Dmitry Osipenko wrote:
>> 18.07.2019 22:42, Peter De Schrijver пишет:
>>> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS 
>>>>> peripheral
>>>>> clocks which need to be restored prior to DFLL reinit.
>>>> Okay, but that shouldn't be a problem if clock dependencies are set up
>>>> properly.
>>>>
>>>>>>> reverse list order during restore might not work as all other 
>>>>>>> clocks are
>>>>>>> in proper order no with any ref clocks for plls getting restored 
>>>>>>> prior
>>>>>>> to their clients
>>>>>> Why? The ref clocks should be registered first and be the roots 
>>>>>> for PLLs
>>>>>> and the rest. If it's not currently the case, then this need to be
>>>>>> fixed. You need to ensure that each clock is modeled properly. If 
>>>>>> some
>>>>>> child clock really depends on multiple parents, then the parents 
>>>>>> need to
>>>>>> in the correct order or CCF need to be taught about such
>>>>>> multi-dependencies.
>>>>>>
>>>>>> If some required feature is missed, then you have to implement it
>>>>>> properly and for all, that's how things are done in upstream. 
>>>>>> Sometimes
>>>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>>>> the end.
>>>>>>
>>>>>> [snip]
>>>>> Yes, we should register ref/parents before their clients.
>>>>>
>>>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>>>> registers during clock init.
>>>>>
>>>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>>>> gets added to the clock list.
>>>>>
>>>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>>>> dependencies for dfllCPU_out thru clock list.
>>>>>
>>>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk 
>>>>> reference
>>>>> thru DT.
>>> The dfll does not have any parents. It has some clocks which are needed
>>> for the logic part of the dfll to function, but there's no parent clock
>>> as such unlike for peripheral clocks or PLLs where the parent is at
>>> least used as a reference. The I2C controller of the DFLL shares the
>>> lines with a normal I2C controller using some arbitration logic. That
>>> logic only works if the clock for the normal I2C controller is enabled.
>>> So you need probably 3 clocks enabled to initialize the dfll in that
>>> case. I don't think it makes sense to add complicated logic to the 
>>> clock
>>> core to deal with this rather strange case. To me it makes more 
>>> sense to
>>> use pmops and open code the sequence there.
>> It looks to me that dfllCPU is a PLL and dfll_ref is its reference
>> parent, while dfll_soc clocks the logic that dynamically reconfigures
>> dfllCPU in background. I see that PLLP is defined as a parent for
>> dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
>> a parent for dfllCPU, no?
>
> dfll_soc will not be restored by the time dfllCPU resume happens after 
> dfll_ref.
>
> without dfll_soc, dfllCPU cannot be resumed either. So if we decide to 
> use parent we should use dfll_soc.
>
>> Either way is good to me, given that DFLL will be disabled during
>> suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
>> good. And then it also will be better to error out if DFLL is active
>> during suspend on the DFLL's driver suspend.
>
> Doing in dfll-fcpu pm_ops is much better as it happens right after all 
> clocks are restored and unlike other clock enables, dfll need dfll 
> controller programming as well and is actually registered in dfll-fcpu 
> driver.
>
> With this, below is the sequence:
>
> CPUFreq suspend switches CPU to PLLP and disables dfll
>
> Will add dfll_suspend/resume in dfll-fcpu driver and in dfll suspend 
> will check for dfll active and will error out suspend.
>
> dfll resume does dfll reinit.
>
> CPUFreq resume enables dfll and switches CPU to dfll.
>
>
> Will go with doing in dfll-fcpu pm_ops rather than parenting 
> dfllCPU_OUT...
>
Does is make sense to return error EBUSY if dfll is not disabled by the 
time dfll-fcpu suspend happens?

Or should I use ETIMEOUT?