Messages in this thread | | | Subject | Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks | From | Sowjanya Komatineni <> | Date | Wed, 17 Jul 2019 17:25:39 -0700 |
| |
On 7/17/19 4:44 PM, Dmitry Osipenko wrote: > 18.07.2019 2:36, Sowjanya Komatineni пишет: >> On 7/17/19 3:48 PM, Dmitry Osipenko wrote: >>> 18.07.2019 0:57, Sowjanya Komatineni пишет: >>>> On 7/17/19 2:51 PM, Sowjanya Komatineni wrote: >>>>> On 7/17/19 2:30 PM, Dmitry Osipenko wrote: >>>>>> 17.07.2019 23:11, Sowjanya Komatineni пишет: >>>>>>> On 7/17/19 1:01 PM, Sowjanya Komatineni wrote: >>>>>>>> On 7/17/19 12:43 PM, Dmitry Osipenko wrote: >>>>>>>>> 17.07.2019 21:54, Sowjanya Komatineni пишет: >>>>>>>>>> On 7/17/19 11:51 AM, Sowjanya Komatineni wrote: >>>>>>>>>>> On 7/17/19 11:32 AM, Dmitry Osipenko wrote: >>>>>>>>>>>> 17.07.2019 20:29, Sowjanya Komatineni пишет: >>>>>>>>>>>>> On 7/17/19 8:17 AM, Dmitry Osipenko wrote: >>>>>>>>>>>>>> 17.07.2019 9:36, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>> В Tue, 16 Jul 2019 22:55:52 -0700 >>>>>>>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700 >>>>>>>>>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет: >>>>>>>>>>>>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700 >>>>>>>>>>>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет: >>>>>>>>>>>>>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote: >>>>>>>>>>>>>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote: >>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> пишет: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> +0800, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Joseph >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver... >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> attention is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to be >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> probed >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> icky. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> dfll clk >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> dfll >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clk >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> registers? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> links". >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> See >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> example. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [1] >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [2] >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> And >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL's >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> device, see [3]. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [3] >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Will go thru and add... >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> with >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> getting >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> orphaned clock. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> registers the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock and then >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> EPROBE_DEFER >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> until >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL driver is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> as-is and >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> there is no real >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> confusion! >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> thread. Just >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> part. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> with the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switched to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU has >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DVFS >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you see >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other sources with >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> unknew >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> guaranteed to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> work. We >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> enforce >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP's >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> voltage. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So no >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> need to enforce >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CCLK_G >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> source >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to PLLP during >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> During >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend, need to change >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> closed >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> loop >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode first and >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> then >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Okay. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> need to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> idle >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> time. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> moves it >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the rest >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn off >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> handle >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sequence to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn on >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cluster. And >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> leave >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> After >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> restore >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> moving to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CCLK to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> parent during of >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hence >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instead of having >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odd >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> nicer to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> device >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switches >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to that >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> also >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be responsible for >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> re-enables it >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> during >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> hacks >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> around >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL are not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> needed. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> change the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> "Add >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> appropriate to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> me. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> use >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> are as >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> done) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> tristate to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> output >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> any >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (suspend), >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cclk_g >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> parent is not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> open >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> loop >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Will add this ... >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> during >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> probe, similar >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should be done on suspend. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP in the probe. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP, then >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> some >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other more >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> selected. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> always >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs at higher >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> rate >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> prior to >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> dfll clock enable >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should be safe. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> also a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> divided output of >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> off a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> fast PLLX during >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> KEXEC. I >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock-related problems as >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> very >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> important at the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> moment. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [snip] >>>>>>>>>>>>>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P >>>>>>>>>>>>>>>>>>>>>>>>>>>>> source >>>>>>>>>>>>>>>>>>>>>>>>>>>>> above I meant >>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLL_P_OUT4. >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used >>>>>>>>>>>>>>>>>>>>>>>>>>>>> for high >>>>>>>>>>>>>>>>>>>>>>>>>>>>> freq >>>>>>>>>>>>>>>>>>>>>>>>>>>>> like >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 800Mhz >>>>>>>>>>>>>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from >>>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP. >>>>>>>>>>>>>>>>>>>>>>>>>>>> Alright, then please don't forget to >>>>>>>>>>>>>>>>>>>>>>>>>>>> pre-initialize >>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a >>>>>>>>>>>>>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or >>>>>>>>>>>>>>>>>>>>>>>>>>>> assigned-clocks. >>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is >>>>>>>>>>>>>>>>>>>>>>>>>>> safe to >>>>>>>>>>>>>>>>>>>>>>>>>>> run at >>>>>>>>>>>>>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin >>>>>>>>>>>>>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same >>>>>>>>>>>>>>>>>>>>>>>>>> voltage as >>>>>>>>>>>>>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me >>>>>>>>>>>>>>>>>>>>>>>>>> from the >>>>>>>>>>>>>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is >>>>>>>>>>>>>>>>>>>>>>>>>> defined, >>>>>>>>>>>>>>>>>>>>>>>>>> I see >>>>>>>>>>>>>>>>>>>>>>>>>> that there is the min_millivolts >>>>>>>>>>>>>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined >>>>>>>>>>>>>>>>>>>>>>>>>> per-table. >>>>>>>>>>>>>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below >>>>>>>>>>>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>>>>>>>> will >>>>>>>>>>>>>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz. >>>>>>>>>>>>>>>>>>>>>>>> Thank you for the clarification. It would be good >>>>>>>>>>>>>>>>>>>>>>>> to have >>>>>>>>>>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>>>>>>> commented >>>>>>>>>>>>>>>>>>>>>>>> in the code as well. >>>>>>>>>>>>>>>>>>>>>>> OK, Will add... >>>>>>>>>>>>>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq >>>>>>>>>>>>>>>>>>>>>> suspend >>>>>>>>>>>>>>>>>>>>>> happens very early even before disabling non-boot >>>>>>>>>>>>>>>>>>>>>> CPUs and >>>>>>>>>>>>>>>>>>>>>> also >>>>>>>>>>>>>>>>>>>>>> need to export clock driver APIs to CPUFreq. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Was thinking of below way of implementing this... >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Clock DFLL driver Suspend: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> - Save CPU clock policy registers, and >>>>>>>>>>>>>>>>>>>>>> Perform >>>>>>>>>>>>>>>>>>>>>> dfll >>>>>>>>>>>>>>>>>>>>>> suspend which sets in open loop mode >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> CPU Freq driver Suspend: does nothing >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Clock DFLL driver Resume: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, >>>>>>>>>>>>>>>>>>>>>> restore >>>>>>>>>>>>>>>>>>>>>> CPU >>>>>>>>>>>>>>>>>>>>>> Clock policy registers which actually sets source to >>>>>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>>>>> along >>>>>>>>>>>>>>>>>>>>>> with other CPU Policy register restore. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> CPU Freq driver Resume: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> - do clk_prepare_enable which acutally >>>>>>>>>>>>>>>>>>>>>> sets >>>>>>>>>>>>>>>>>>>>>> DFLL in >>>>>>>>>>>>>>>>>>>>>> Closed loop mode >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP >>>>>>>>>>>>>>>>>>>>>> is not >>>>>>>>>>>>>>>>>>>>>> needed >>>>>>>>>>>>>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as >>>>>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>>>>> is not >>>>>>>>>>>>>>>>>>>>>> disabled anywhere throught the suspend/resume path >>>>>>>>>>>>>>>>>>>>>> and SC7 >>>>>>>>>>>>>>>>>>>>>> entry >>>>>>>>>>>>>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP. >>>>>>>>>>>>>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend >>>>>>>>>>>>>>>>>>>> it on >>>>>>>>>>>>>>>>>>>> PLLP as well. And besides, seems that currently >>>>>>>>>>>>>>>>>>>> disabling >>>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>>> clock will disable DFLL completely and then you'd >>>>>>>>>>>>>>>>>>>> want to >>>>>>>>>>>>>>>>>>>> re-init >>>>>>>>>>>>>>>>>>>> the DFLL on resume any ways. So better to just disable >>>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>>> completely on suspend, which should happen on >>>>>>>>>>>>>>>>>>>> clk_disable(dfll). >>>>>>>>>>>>>>>>>>> Will switch to PLLP during CPUFreq suspend. With >>>>>>>>>>>>>>>>>>> decision of >>>>>>>>>>>>>>>>>>> using >>>>>>>>>>>>>>>>>>> clk_disable during suspend, its mandatory to switch to >>>>>>>>>>>>>>>>>>> PLLP as >>>>>>>>>>>>>>>>>>> DFLL >>>>>>>>>>>>>>>>>>> is completely disabled. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> My earlier concern was on restoring CPU policy as we >>>>>>>>>>>>>>>>>>> can't do >>>>>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>> from CPUFreq driver and need export from clock driver. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Clear now and will do CPU clock policy restore in after >>>>>>>>>>>>>>>>>>> dfll >>>>>>>>>>>>>>>>>>> re-init. >>>>>>>>>>>>>>>>>> Why the policy can't be saved/restored by the CaR driver >>>>>>>>>>>>>>>>>> as a >>>>>>>>>>>>>>>>>> context of any other clock? >>>>>>>>>>>>>>>>> restoring cpu clock policy involves programming source and >>>>>>>>>>>>>>>>> super_cclkg_divider. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> cclk_g is registered as clk_super_mux and it doesn't use >>>>>>>>>>>>>>>>> frac_div ops >>>>>>>>>>>>>>>>> to do save/restore its divider. >>>>>>>>>>>>>>>> That can be changed of course and I guess it also could >>>>>>>>>>>>>>>> be as >>>>>>>>>>>>>>>> simple as >>>>>>>>>>>>>>>> saving and restoring of two raw u32 values of the >>>>>>>>>>>>>>>> policy/divider >>>>>>>>>>>>>>>> registers. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Also, during clock context we cant restore cclk_g as cclk_g >>>>>>>>>>>>>>>>> source >>>>>>>>>>>>>>>>> will be dfll and dfll will not be resumed/re-initialized >>>>>>>>>>>>>>>>> by the >>>>>>>>>>>>>>>>> time >>>>>>>>>>>>>>>>> clk_super_mux save/restore happens. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> we can't use save/restore context for dfll clk_ops because >>>>>>>>>>>>>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and >>>>>>>>>>>>>>>>> dfll_ref >>>>>>>>>>>>>>>>> and dfll_soc peripheral clocks are not restored by the >>>>>>>>>>>>>>>>> time dfll >>>>>>>>>>>>>>>>> restore happens. Also dfll peripheral clock enables need >>>>>>>>>>>>>>>>> to be >>>>>>>>>>>>>>>>> restored before dfll restore happens which involves >>>>>>>>>>>>>>>>> programming >>>>>>>>>>>>>>>>> dfll >>>>>>>>>>>>>>>>> controller for re-initialization. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> So dfll resume/re-init is done in clk-tegra210 at end of >>>>>>>>>>>>>>>>> all >>>>>>>>>>>>>>>>> clocks >>>>>>>>>>>>>>>>> restore in V5 series but instead of in clk-tegra210 >>>>>>>>>>>>>>>>> driver I >>>>>>>>>>>>>>>>> moved >>>>>>>>>>>>>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies >>>>>>>>>>>>>>>>> will be >>>>>>>>>>>>>>>>> restored thru clk_restore_context by then. This will be in >>>>>>>>>>>>>>>>> V6. >>>>>>>>>>>>>>>> Since DFLL is now guaranteed to be disabled across CaR >>>>>>>>>>>>>>>> suspend/resume >>>>>>>>>>>>>>>> (hence it has nothing to do in regards to CCLK) and given >>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>> PLLs >>>>>>>>>>>>>>>> state is restored before the rest of the clocks, I don't >>>>>>>>>>>>>>>> see why >>>>>>>>>>>>>>>> not to >>>>>>>>>>>>>>>> implement CCLK save/restore in a generic fasion. CPU policy >>>>>>>>>>>>>>>> wull be >>>>>>>>>>>>>>>> restored to either PLLP or PLLX (if CPUFreq driver is >>>>>>>>>>>>>>>> disabled). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> CCLK_G save/restore should happen in clk_super_mux ops >>>>>>>>>>>>>>> save/context and >>>>>>>>>>>>>>> clk_super_mux save/restore happens very early as cclk_g is >>>>>>>>>>>>>>> first >>>>>>>>>>>>>>> in the >>>>>>>>>>>>>>> clock tree and save/restore traverses through the tree >>>>>>>>>>>>>>> top-bottom >>>>>>>>>>>>>>> order. >>>>>>>>>>>>>> If CCLK_G is restored before the PLLs, then just change the >>>>>>>>>>>>>> clocks >>>>>>>>>>>>>> order >>>>>>>>>>>>>> such that it won't happen. >>>>>>>>>>>>>> >>>>>>>>>>>>> I dont think we can change clocks order for CCLK_G. >>>>>>>>>>>>> >>>>>>>>>>>>> During bootup, cclk_g is registered after all pll's and >>>>>>>>>>>>> peripheral >>>>>>>>>>>>> clocks which is the way we wanted, So cclk_g will be the first >>>>>>>>>>>>> one in >>>>>>>>>>>>> the clk list as clk_register adds new clock first in the list. >>>>>>>>>>>>> >>>>>>>>>>>>> When clk_save_context and clk_restore_context APIs iterates >>>>>>>>>>>>> over the >>>>>>>>>>>>> list, cclk_g is the first >>>>>>>>>>>> Looking at clk_core_restore_context(), I see that it walks up >>>>>>>>>>>> CLKs >>>>>>>>>>>> list >>>>>>>>>>>> from parent to children, hence I don't understand how it can >>>>>>>>>>>> ever >>>>>>>>>>>> happen >>>>>>>>>>>> that CCLK will be restored before the parent. The clocks >>>>>>>>>>>> registration >>>>>>>>>>>> order doesn't matter at all in that case. >>>>>>>>>>> yes from parent to children and dfllCPU_out is the top in the >>>>>>>>>>> list and >>>>>>>>>>> its child is cclk_g. >>>>>>>>>>> >>>>>>>>>>> the way clocks are registered is the order I see in the clock >>>>>>>>>>> list and >>>>>>>>>>> looking into clk_register API it adds new node first in the list. >>>>>>>>>>> >>>>>>>>>> cclkg_g & dfll register happens after all plls and peripheral >>>>>>>>>> clocks as >>>>>>>>>> it need ref, soc and peripheral clocks to be enabled. >>>>>>>>>>> So they are the last to get registered and so becomes first in >>>>>>>>>>> the >>>>>>>>>>> list. >>>>>>>>>>> >>>>>>>>>>> During save/restore context, it traverses thru this list and >>>>>>>>>>> first in >>>>>>>>>>> the list is dfllcpu_OUT (parent) and its child (cclk_g) >>>>>>>>>>> >>>>>>>>>>> saving should not be an issue at all but we cant restore >>>>>>>>>>> cclk_g/dfll >>>>>>>>>>> in normal way thru clk_ops restore as plls and peripherals >>>>>>>>>>> restore >>>>>>>>>>> doesn't happen by that time. >>>>>>>>>>> >>>>>>>>>> I was referring to clk_restore_context where it iterates thru >>>>>>>>>> root list >>>>>>>>>> and for each core from the root list clk_core_restore does >>>>>>>>>> restore of >>>>>>>>>> parent and children. >>>>>>>>>> >>>>>>>>>> dfllCPU_Out gets first in the list and its child is cclk_g >>>>>>>>>> >>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/clk/clk.c#L1105 >>>>>>>>>> >>>>>>>>>> >>>>>>>>> What list you're talking about? clk_summary? It shows current >>>>>>>>> *active* >>>>>>>>> clocks configuration, if you'll try to disable CPUFreq driver then >>>>>>>>> the >>>>>>>>> parent of CCLK_G should be PLLX. Similarly when CPU is >>>>>>>>> reparented to >>>>>>>>> PLLP on driver's suspend, then PLLP is the parent. >>>>>>>>> >>>>>>>>>>>>>>> DFLL enable thru CPUFreq resume happens after all >>>>>>>>>>>>>>> clk_restore_context >>>>>>>>>>>>>>> happens. So during clk_restore_context, dfll re-init doesnt >>>>>>>>>>>>>>> happen >>>>>>>>>>>>>>> and >>>>>>>>>>>>>>> doing cpu clock policy restore during super_mux clk_ops will >>>>>>>>>>>>>>> crash as >>>>>>>>>>>>>>> DFLL is not initialized and its clock is not enabled but CPU >>>>>>>>>>>>>>> clock >>>>>>>>>>>>>>> restore sets source to DFLL if we restore during >>>>>>>>>>>>>>> super_clk_mux >>>>>>>>>>>>>> If CPU was suspended on PLLP, then it will be restored on >>>>>>>>>>>>>> PLLP by >>>>>>>>>>>>>> CaR. I >>>>>>>>>>>>>> don't understand what DFLL has to do with the CCLK in that >>>>>>>>>>>>>> case >>>>>>>>>>>>>> during >>>>>>>>>>>>>> the clocks restore. >>>>>>>>>>>>> My above comment is in reference to your request of doing >>>>>>>>>>>>> save/restore >>>>>>>>>>>>> for cclk_g in normal fashion thru save/restore context. Because >>>>>>>>>>>>> of the >>>>>>>>>>>>> clk order I mentioned above, we cclk_g will be the first one to >>>>>>>>>>>>> go thru >>>>>>>>>>>>> save/context. >>>>>>>>>>>>> >>>>>>>>>>>>> During save_context of cclk_g, source can be from PLLX, dfll. >>>>>>>>>>>>> >>>>>>>>>>>>> Issue will be when we do restore during clk_restore_context of >>>>>>>>>>>>> cclk_g as >>>>>>>>>>>>> by that time PLLX/dfll will not be restored. >>>>>>>>>>>>> >>>>>>>>>>>> Seems we already agreed that DFLL will be disabled by the >>>>>>>>>>>> CPUFreq >>>>>>>>>>>> driver >>>>>>>>>>>> on suspend. Hence CCLK can't be from DFLL if CPU is >>>>>>>>>>>> reparented to >>>>>>>>>>>> PLLP >>>>>>>>>>>> on CPUFreq driver's suspend, otherwise CPU keeps running from a >>>>>>>>>>>> boot-state PLLX if CPUFreq driver is disabled. >>>>>>>>>>> Yes suspend should not be an issue but issue will be during >>>>>>>>>>> resume >>>>>>>>>>> where if we do cclk_g restore in normal way thru >>>>>>>>>>> clk_restore_context, >>>>>>>>>>> cclk_g restore happens very early as dfllCPU out is the first >>>>>>>>>>> one that >>>>>>>>>>> goes thru restore context and plls/peripherals are not resumed by >>>>>>>>>>> then. >>>>>>>>>>> >>>>>>>>>>> CPU runs from PLLX if dfll clock enable fails during boot. So >>>>>>>>>>> when it >>>>>>>>>>> gets to suspend, we save CPU running clock source as either >>>>>>>>>>> PLLX or >>>>>>>>>>> DFLL and then we switch to PLLP. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On resume, CPU runs from PLLP by warm boot code and we need to >>>>>>>>>>> restore >>>>>>>>>>> back its source to the one it was using from saved source context >>>>>>>>>>> (which can be either PLLX or DFLL) >>>>>>>>>>> >>>>>>>>>>> So PLLs & DFLL resume need to happen before CCLKG restore/resume. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> With all above discussions, we do DFLL disable in CPUFreq >>>>>>>>>>> driver on >>>>>>>>>>> suspend and on CPUFreq resume we enable DFLL back and restore CPU >>>>>>>>>>> clock source it was using during suspend (which will be either >>>>>>>>>>> PLLX if >>>>>>>>>>> dfll enable fails during probe or it will be using DFLL). >>>>>>>>> During suspend CPU's parent shall be PLLP and not DFLL (note that >>>>>>>>> it is >>>>>>>>> disabled) after reparenting to PLLP by the CPUFreq driver. >>>>>>>>> >>>>>>>> CPU source context should be saved before switching to safe >>>>>>>> source of >>>>>>>> PLLP as on resume we need to restore back to source it was using >>>>>>>> before we switch to safe source during suspend entry. >>>>>>>> >>>>>>>> So saved context for CPU Source will be either dfll or PLLX >>>>>>>> >>>>>>> PLLP reparenting is only during suspend/entry to have it as safe >>>>>>> source >>>>>>> but actual CPU source it was running from before suspending is either >>>>>>> dfll/pllx which should be the one to be restored on CPUFreq resume. >>>>>>> Resume happens with CPU running from PLLP till it gets to the >>>>>>> point of >>>>>>> restoring its original source (dfll or pllx) >>>>>> CaR should restore CPU to PLLP or PLLX, while CPUFreq driver restores >>>>>> CPU to DFLL. Please see more comments below. >>>>>> >>>>>>>>>>> So i was trying to say dfll/cclk_g restore can't be done in >>>>>>>>>>> normal way >>>>>>>>>>> thru clk_ops save/restore context >>>>>>>>> Let's see what happens if CPUFreq is active: >>>>>>>>> >>>>>>>>> 1. CPUFreq driver probe happens >>>>>>>>> 2. CPU is reparented to PLLP >>>>>>>>> 3. DFLL inited >>>>>>>>> 4. CPU is reparented to DFLL >>>>>>>>> >>>>>>>>> 5. CPUFreq driver suspend happens >>>>>>>>> 6. CPU is reparented to PLLP >>>>>>>>> 7. DFLL is disabled >>>>>>>>> >>>>>>>>> 8. Car suspend happens >>>>>>>>> 9. DFLL context saved >>>>>>>>> 10. PLLP/PLLX context saved >>>>>>>>> 11. CCLK context saved >>>>>>>>> >>>>>>>>> 12. Car resume happens >>>>>>>>> 13. DFLL context restored >>>>>>>>> 14. PLLP/PLLX context restored >>>>>>>>> 15. CCLK context restored >>>>>>>>> >>>>>>>>> 16. CPUFreq driver resume happens >>>>>>>>> 17. DFLL re-inited >>>>>>>>> 18. CPU is reparented to DFLL >>>>>>>> Below is the order of sequence it should be based on the order of >>>>>>>> clk >>>>>>>> register. >>>>>>>> >>>>>>>> My comments inline in this sequence. >>>>>>>> >>>>>>>> 1. CPUFreq driver probe happens >>>>>>>> 2. CPU is reparented to PLLP >>>>>>>> 3. DFLL inited >>>>>>>> 4. CPU is reparented to DFLL >>>>>>>> >>>>>>>> >>>>>>>> 5. CPUFreq driver suspend happens >>>>>>>> 6. Save CPU source which could be either dfll or pllx >>>>>> Please see my next comment. >>>>>> >>>>>>>> 7. CPU is reparented to safe known source PLLP >>>>>>>> 8. DFLL is disabled >>>>>>>> >>>>>>>> 8. Car suspend happens >>>>>>>> 9. DFLL context saved (With DFLL disabled in CPUFreq suspend, >>>>>>>> nothing to be saved here as last freq req will always be saved). >>>>>>>> 10. CCLK context saved (CPU clock source will be saved in >>>>>>>> CPUFreq >>>>>>>> driver suspend which could be either dfll or pllx) >>>>>> That I don't understand. The CPU's clock source state should be >>>>>> saved at >>>>>> the moment of the CaR's suspending (i.e. CCLK policy will be set to >>>>>> PLLP >>>>>> or PLLX) and then CCLK state should be also restored by the CaR in >>>>>> step 14. >>>>> CPU clock to be saved and restored should be the source used before we >>>>> switch it to safe PLLP for suspend/resume operation. >>>>> >>>>> This original source could be either PLLX or DFLL which it was using >>>>> before we disable DFLL during CPU Freq suspend. >>>>> >>>>> If we save CPU clock source at moment of CAR suspending, it will be >>>>> PLLP as we switch to safe PLLP in CPUFreq driver suspend. >>>>> >>>>> Infact, we dont need to restore CPU clock source to PLLP anywhere in >>>>> resume as it comes up with PLLP source from warm boot code itself. >>> You must always maintain proper suspend/resume encapsulation, otherwise >>> it's a total mess. It doesn't matter that CCLK is restored to PLLP even >>> that CPU is already running off PLLP after warmboot. >>> >>>>> But we need to restore CPU source to original source it was using >>>>> before we switch to safe PLLP source for suspend operation. This >>>>> original source could be PLLX/DFLL and this should be re-stored in >>>>> CPUFreq resume as by this time PLLs and peripherals are restored and >>>>> dfll is re-initialized. >>>>> >>>>> So saving actual CPU source before switching to intermediate safe PLLP >>>>> in CPUFreq driver and then restoring back during CPUFreq resume should >>>>> be good as CPUFreq resume happens right after all clocks (plls >>>>> restore, peripherals restore, dfll resume)>> >>>>>> CPUFreq driver should only switch CPU to PLLP and disable DFLL on >>>>>> suspend in step 5, that's it. On resume CPUFreq driver will restore >>>>>> CPU >>>>>> to DFLL in step 18. >>>>> Also I don't think we should hard-code to force CPU source to DFLL on >>>>> CPUFreq resume. >>>>> >>>>> Reason is during CPU Probe when it tries to switch to dfll source, for >>>>> some reason if dfll enable fails it sets CPU to its original source >>>>> which will be PLLX. >>> No! >>> >>> 1. CPU voltage could be too low for PLLX >>> 2. PLLX rate can't be changed without manual reparenting CPU to >>> intermediate clock >>> 3. CPUFreq can't manually manage CPU voltage >>> >>> DFLL-restoring failure is an extreme case. CPU must be kept on a safe >>> PLLP in that case and disable_cpufreq() must be invoked as well. >> OK, PLLX option was also in my mind. So If we just consider sources as >> DFLL or PLLP, then we can save source in CCLK save context and restore >> in CCLK restore basically it will be PLLP. >> >> Later during CPUFreq resume we can just switch to DFLL and if DFLL >> enable fails we will keep source as PLLP. Yes will invoke >> disable_cpufreq as well in case of dfll enable failure for some reason. > Sounds good! > >>>>> So CPU source could be either DFLL or PLLX in CPUFreq >>>>> tegra124_cpu_switch_to_dfll >>>>> >>>>>>>> 11. PLLP/PLLX context saved >>>>>>>> 12. Peripheral Clock saved >>>>>>>> >>>>>>>> 12. Car resume happens >>>>>>>> 13. DFLL context restored : No DFLL context to be restored >>>>>>>> and we >>>>>>>> only need to reinitialize DFLL and re-initialize can't be done >>>>>>>> here as >>>>>>>> this is the 1st to get restored and PLL/Peripheral clocks are not >>>>>>>> restored by this time. So we can't use clk_ops restore for DFLL >>>>>> It looks to me that clk_core_restore_context() should just do >>>>>> hlist_for_each_entry *_reverse*. Don't you think so? >>>>> Thought of that but this is in core driver and is used by other >>>>> non-tegra clock driver and not sure if that impacts for those. >>> The reverse ordering should be correct, I think it's just a shortcoming >>> of the CCF that need to be fixed. But will better to make a more >>> thorough research, asking Stephen and Michael for the clarification. >>> >>>>> But with decision of switching CPUFreq with dfll clock enable/disable >>>>> during CPUFreq suspend/resume, we can re-init dfll during dfll-fcpu >>>>> driver resume and we don't need CCLK save/restore. >>>>> > Actually CPUFreq driver should implement suspend/resume regardless of > CaR ability to restore DFLL or whatever, simply to properly handle > possible clock restoring failure on resume as we just found out. > >>>> the way of all clocks order is good except cclk_g which has dependency >>>> on multiple clocks. >>> CCLK_G has a single parent at a time. What "multiple clocks" you're >>> talking about? Please explain. >> 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. > Please try to fix all missing dependencies and orderings.
Peter,
dfllCPU_OUT is the first one to go thru restore when clk_restore_context traverses thru the list.
dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this dependency is unknown to clock-tree.
We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during register so dfllCPU_OUT save/restore happens after their parents are restored.
But DFLL needs both of these to be restored before DFLLCPU_Out and as DFLL_SOC restore always happens after the REF, thinking to add DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after their dependencies.
Please comment.
| |