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

From: Sowjanya Komatineni
Date: Thu Jul 18 2019 - 19:08:58 EST


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?