Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni <skomatineni@nvidia.com>
Date: 2019-07-18 01:15:35
Also in:
linux-clk, linux-gpio, linux-tegra, lkml
On 7/17/19 5:25 PM, Sowjanya Komatineni wrote:
On 7/17/19 4:44 PM, Dmitry Osipenko wrote:quoted
18.07.2019 2:36, Sowjanya Komatineni пишет:quoted
On 7/17/19 3:48 PM, Dmitry Osipenko wrote:quoted
18.07.2019 0:57, Sowjanya Komatineni пишет:quoted
On 7/17/19 2:51 PM, Sowjanya Komatineni wrote:quoted
On 7/17/19 2:30 PM, Dmitry Osipenko wrote:quoted
17.07.2019 23:11, Sowjanya Komatineni пишет:quoted
On 7/17/19 1:01 PM, Sowjanya Komatineni wrote:quoted
On 7/17/19 12:43 PM, Dmitry Osipenko wrote:quoted
17.07.2019 21:54, Sowjanya Komatineni пишет:quoted
On 7/17/19 11:51 AM, Sowjanya Komatineni wrote:quoted
On 7/17/19 11:32 AM, Dmitry Osipenko wrote:quoted
17.07.2019 20:29, Sowjanya Komatineni пишет:quoted
On 7/17/19 8:17 AM, Dmitry Osipenko wrote:quoted
17.07.2019 9:36, Sowjanya Komatineni пишет:quoted
On 7/16/19 11:33 PM, Dmitry Osipenko wrote:quoted
В Tue, 16 Jul 2019 22:55:52 -0700 Sowjanya Komatineni [off-list ref] пишет:quoted
On 7/16/19 10:42 PM, Dmitry Osipenko wrote:quoted
В Tue, 16 Jul 2019 22:25:25 -0700 Sowjanya Komatineni [off-list ref] пишет:quoted
On 7/16/19 9:11 PM, Dmitry Osipenko wrote:quoted
В Tue, 16 Jul 2019 19:35:49 -0700 Sowjanya Komatineni [off-list ref] пишет:quoted
On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:quoted
On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:quoted
On 7/16/19 3:00 PM, Dmitry Osipenko wrote:quoted
17.07.2019 0:35, Sowjanya Komatineni пишет:quoted
On 7/16/19 2:21 PM, Dmitry Osipenko wrote:quoted
17.07.2019 0:12, Sowjanya Komatineni пишет:quoted
On 7/16/19 1:47 PM, Dmitry Osipenko wrote:quoted
16.07.2019 22:26, Sowjanya Komatineni пишет:quoted
On 7/16/19 11:43 AM, Dmitry Osipenko wrote:quoted
16.07.2019 21:30, Sowjanya Komatineni пишет:quoted
On 7/16/19 11:25 AM, Dmitry Osipenko wrote:quoted
16.07.2019 21:19, Sowjanya Komatineni пишет:quoted
On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:quoted
On 7/16/19 8:00 AM, Dmitry Osipenko wrote:quoted
16.07.2019 11:06, Peter De Schrijver пишет:quoted
On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:quoted
quoted
OK, Will add to CPUFreq driver...quoted
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#n100Will 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!quoted
quoted
quoted
quoted
quoted
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 suspendSorry, 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.quoted
quoted
quoted
quoted
quoted
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.quoted
quoted
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 modeI 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 likequoted
800Mhzand 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 @ VminSo 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.quoted
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 firstLooking 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.quoted
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#L1105What 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.quoted
quoted
quoted
quoted
quoted
quoted
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_muxIf 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 PLLXPLLP 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.quoted
quoted
quoted
quoted
quoted
So i was trying to say dfll/cclk_g restore can't be done in normal way thru clk_ops save/restore contextLet'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 DFLLBelow 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 pllxPlease see my next comment.quoted
quoted
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.quoted
quoted
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)>>quoted
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!quoted
quoted
quoted
quoted
So CPU source could be either DFLL or PLLX in CPUFreq tegra124_cpu_switch_to_dfllquoted
quoted
quoted
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 DFLLIt 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.quoted
quoted
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.quoted
quoted
quoted
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.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 clientsWhy? 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.
Did quick try and I see by adding dfll-soc as parent to dfllCPU_OUT, its in proper order after all its dependencies. Can now add dfll save/restore to do dfll reinit during restore..