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

From: Sowjanya Komatineni
Date: Wed Jul 17 2019 - 17:57:17 EST

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@xxxxxxxxxx> ÐÐÑÐÑ:

On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
Ð Tue, 16 Jul 2019 22:25:25 -0700
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:
On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
Ð Tue, 16 Jul 2019 19:35:49 -0700
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:
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,
Lo wrote:
OK, Will add to CPUFreq driver...
The other thing that also need
attention is
that T124 CPUFreq
implicitly relies on DFLL driver to be
first, which is
Should I add check for successful dfll clk
register explicitly in
CPUFreq driver probe and defer till dfll
Probably you should use the "device links".
[1][2] for the



Return EPROBE_DEFER instead of EINVAL if
device_link_add() fails.
use of_find_device_by_node() to get the DFLL's
device, see [3].


Will go thru and add...
Looks like I initially confused this case with
orphaned clock.
I'm now seeing that the DFLL driver registers the
clock and then
clk_get(dfll) should be returning EPROBE_DEFER
DFLL driver is
probed, hence everything should be fine as-is and
there is no real
for the 'device link'. Sorry for the confusion!
Sorry, I didn't follow the mail thread. Just
regarding the DFLL

As you know it, the DFLL clock is one of the
clock sources and
integrated with DVFS control logic with the
regulator. We will not
CPU to other clock sources once we
switched to
DFLL. Because the
CPU has
been regulated by the DFLL HW with the DVFS
you see
in the driver.). We shouldn't reparent it to
other sources with
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
DFLL freq to
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
to PLLP during
Sorry, please ignore my above comment. During
suspend, need to change
CCLK_G source to PLLP when dfll is in closed
mode first and
dfll need to be set to open loop.
And I don't exactly understand why we
need to
switch to PLLP in
driver. Just keep it on CL-DVFS mode all the

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
sequence to
turn on
regulator and power up the CPU cluster. And
it on PLL_P.
resuming to the kernel, we re-init DFLL,
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
parent during of
early clocks-state restoring by CaR driver.
instead of having
hacks in the CaR driver, it is much nicer to
have a
proper suspend-resume sequencing of the device
drivers. In this case
driver is the driver that enables DFLL and
CPU to that
source, which means that this driver is also
be responsible for
management of the DFLL's state during of
suspend/resume process. If
CPUFreq driver disables DFLL during suspend
re-enables it
resume, then looks like the CaR driver hacks
DFLL are not
The DFLL part looks good to me. BTW,
change the
patch subject to
suspend-resume support" seems more
appropriate to
To clarify this, the sequences for DFLL use
are as
follows (assuming
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
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
vdd_cpu voltage
1) Program DFLL to open loop mode
I see during switch away from DFLL (suspend),
parent is not
changed to PLLP before changing dfll to open

Will add this ...
The CPUFreq driver switches parent to PLLP during
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
other more
appropriate intermediate parent should be
CPU parents are PLL_X, PLL_P, and dfll. PLL_X
runs at higher
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
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

During bootup CPUG sources from PLL_X. By PLL_P
above I meant

As per clock policies, PLL_X is always used for high
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
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
Yes at Vmin CPU Fmax is ~800Mhz. So anything below that
work at Vmin voltage and PLLP max is 408Mhz.
Thank you for the clarification. It would be good to have
in the code as well.
OK, Will add...
Regarding, adding suspend/resume to CPUFreq, CPUFreq
happens very early even before disabling non-boot CPUs and
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
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,
Clock policy registers which actually sets source to DFLL
with other CPU Policy register restore.

CPU Freq driver Resume:

ÂÂÂÂÂÂ ÂÂÂ ÂÂÂ - do clk_prepare_enable which acutally sets
Closed loop mode

Adding one more note: Switching CPU Clock to PLLP is not
as CPU CLock can be from dfll in open-loop mode as DFLL
is not
disabled anywhere throught the suspend/resume path and SC7
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
clock will disable DFLL completely and then you'd want to
the DFLL on resume any ways. So better to just disable DFLL
completely on suspend, which should happen on
Will switch to PLLP during CPUFreq suspend. With decision of
clk_disable during suspend, its mandatory to switch to PLLP as
is completely disabled.

My earlier concern was on restoring CPU policy as we can't do
from CPUFreq driver and need export from clock driver.

Clear now and will do CPU clock policy restore in after dfll
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

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

Also, during clock context we cant restore cclk_g as cclk_g
will be dfll and dfll will not be resumed/re-initialized by the
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
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
controller for re-initialization.

So dfll resume/re-init is done in clk-tegra210 at end of all
restore in V5 series but instead of in clk-tegra210 driver I
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
(hence it has nothing to do in regards to CCLK) and given that
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
If CCLK_G is restored before the PLLs, then just change the clocks
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
from parent to children, hence I don't understand how it can ever
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

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
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
happens. So during clk_restore_context, dfll re-init doesnt happen
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
the clocks restore.
My above comment is in reference to your request of doing
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

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
on suspend. Hence CCLK can't be from DFLL if CPU is reparented to
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

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

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.

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.

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.

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.

the way of all clocks order is good except cclk_g which has dependency on multiple clocks.

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

ÂÂÂÂÂ14. CCLK context restored
CCLK cant be restored here as context could be either dfll or pllx
which is the source orginally it was actually using before we force
switch to safe PLLP for suspend entry. So we can't use clk_ops restore
for DFLL
See my comment to step 10. CCLK should be restored to the *CaR's saved*
context, which is either PLLX or PLLP policy.

15. PLLP/PLLX context restored
ÂÂÂÂÂ16. Peripheral context restored

16. CPUFreq driver resume happens
ÂÂÂÂÂ17. DFLL re-inited (Invoking DFLL re-init in CPUFreq resume need
exporting DFLL reinit from Clock driver to CPUFreq driver)
Will be nice if clk_enable(dfll) could be enough to re-init DFLL. That
should achievable with my next comment to step 18.

ÂÂÂÂÂ18. CPU is reparented to DFLL or PLLX based on saved context from
step 9.

Note: instead of exporting, we can do DFLL re-init from clock-dfll
driver itself thru dfll-fcpu pm_ops resume. So dfll will be
re-initialized by the time CPUFreq driver resumes and switches to use
DFLL source.

Yes, I guess DFLL hardware should be fully reset on DFLL's driver resume
to be on a safe side any ways.

But(!) we could probably just fix clk_core_restore_context(), like I
suggested in step 13. Then DFLL clock could use generic save / restore
context and CPUFreq driver won't have to do anything at all because DFLL
clock will be saved first and resumed *after* all of the peripherals by
the CCF. In the end CCLK will be switched to DFLL by the CCF restore as