В Thu, 18 Jul 2019 13:36:35 -0700
Sowjanya Komatineni [off-list ref] пишет:

On 7/18/19 1:26 PM, Dmitry Osipenko wrote:

quoted

18.07.2019 22:42, Peter De Schrijver пишет:  

quoted

On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:  

quoted

quoted

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.
 

quoted

quoted

quoted

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.

Okay, my point is that the parents should be properly specified any
ways.

quoted

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...

Sounds good.