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

[Dmitry Osipenko]

Ð Thu, 18 Jul 2019 13:36:35 -0700
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:

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

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

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