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

[Dmitry Osipenko]

Ð Thu, 18 Jul 2019 16:08:48 -0700
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:

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

Yes

> Or should I use ETIMEOUT?

No