18.07.2019 2:36, Sowjanya Komatineni ÐÐÑÐÑ:
On 7/17/19 3:48 PM, Dmitry Osipenko wrote:Sounds good!
18.07.2019 0:57, Sowjanya Komatineni ÐÐÑÐÑ:OK, PLLX option was also in my mind. So If we just consider sources as
On 7/17/19 2:51 PM, Sowjanya Komatineni wrote:You must always maintain proper suspend/resume encapsulation, otherwise
On 7/17/19 2:30 PM, Dmitry Osipenko wrote:
17.07.2019 23:11, Sowjanya Komatineni ÐÐÑÐÑ:CPU clock to be saved and restored should be the source used before we
On 7/17/19 1:01 PM, Sowjanya Komatineni wrote:CaR should restore CPU to PLLP or PLLX, while CPUFreq driver restores
On 7/17/19 12:43 PM, Dmitry Osipenko wrote:PLLP reparenting is only during suspend/entry to have it as safe
17.07.2019 21:54, Sowjanya Komatineni ÐÐÑÐÑ:CPU source context should be saved before switching to safe
On 7/17/19 11:51 AM, Sowjanya Komatineni wrote:What list you're talking about? clk_summary? It shows current
On 7/17/19 11:32 AM, Dmitry Osipenko wrote:cclkg_g & dfll register happens after all plls and peripheral
17.07.2019 20:29, Sowjanya Komatineni ÐÐÑÐÑ:yes from parent to children and dfllCPU_out is the top in the
On 7/17/19 8:17 AM, Dmitry Osipenko wrote:Looking at clk_core_restore_context(), I see that it walks up
17.07.2019 9:36, Sowjanya Komatineni ÐÐÑÐÑ:I dont think we can change clocks order for CCLK_G.
On 7/16/19 11:33 PM, Dmitry Osipenko wrote:If CCLK_G is restored before the PLLs, then just change the
Ð Tue, 16 Jul 2019 22:55:52 -0700CCLK_G save/restore should happen in clk_super_mux ops
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:
On 7/16/19 10:42 PM, Dmitry Osipenko wrote:That can be changed of course and I guess it also could
Ð Tue, 16 Jul 2019 22:25:25 -0700restoring cpu clock policy involves programming source and
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:
On 7/16/19 9:11 PM, Dmitry Osipenko wrote:Why the policy can't be saved/restored by the CaR driver
Ð Tue, 16 Jul 2019 19:35:49 -0700Will switch to PLLP during CPUFreq suspend. With
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> ÐÐÑÐÑ:
On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:Since CPU resumes on PLLP, it will be cleaner to suspend
On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
On 7/16/19 3:00 PM, Dmitry Osipenko wrote:Regarding, adding suspend/resume to CPUFreq, CPUFreq
17.07.2019 0:35, Sowjanya Komatineni ÐÐÑÐÑ:OK, Will add...
On 7/16/19 2:21 PM, Dmitry Osipenko wrote:Thank you for the clarification. It would be good
17.07.2019 0:12, Sowjanya Komatineni ÐÐÑÐÑ:Yes at Vmin CPU Fmax is ~800Mhz. So anything below
On 7/16/19 1:47 PM, Dmitry Osipenko wrote:So even 204MHz CVB entries are having the same
16.07.2019 22:26, Sowjanya Komatineni ÐÐÑÐÑ:PLLP_OUT4 rate update is not needed as it is
On 7/16/19 11:43 AM, Dmitry Osipenko wrote:Alright, then please don't forget to
16.07.2019 21:30, Sowjanya Komatineni ÐÐÑÐÑ:During bootup CPUG sources from PLL_X. By PLL_P
On 7/16/19 11:25 AM, Dmitry Osipenko wrote:AFAIK, PLLX could run at ~200MHz. There is
16.07.2019 21:19, Sowjanya Komatineni ÐÐÑÐÑ:CPU parents are PLL_X, PLL_P, and dfll. PLL_X
On 7/16/19 9:50 AM, Sowjanya KomatineniLooks like I initially confused this case
wrote:
On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
16.07.2019 11:06, Peter De SchrijverWill go thru and add...
ÐÐÑÐÑ:
On Tue, Jul 16, 2019 at 03:24:26PMProbably you should use the "device
+0800,
Joseph
Lo wrote:
OK, Will add to CPUFreq driver...
The other thing that also needShould I add check for successful
attention is
that T124 CPUFreq
driver
implicitly relies on DFLL driver
to be
probed
first, which is
icky.
dfll clk
register explicitly in
CPUFreq driver probe and defer till
dfll
clk
registers?
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
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!
Okay.Sorry, please ignore my above comment.PLLP freq is safe to work for any CPUOkay, then the CPUFreq driver willSorry, 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.
have to
enforce
DFLL freq to
PLLP's
rate before switching to PLLP in order to
have a
proper CPU voltage.
voltage.
So no
need to enforce
DFLL freq to PLLP rate before changing
CCLK_G
source
to PLLP during
suspend
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.
The CPUFreq driver switches parent to PLLPI see during switch away from DFLLThe DFLL is re-inited after switchingAnd 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.
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,To clarify this, the sequences for DFLL
change the
patch subject to
"Add
suspend-resume support" seems more
appropriate to
me.
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
(suspend),
cclk_g
parent is not
changed to PLLP before changing dfll to
open
loop
mode.
Will add this ...
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.
always
runs at higher
rate
so switching to PLL_P during CPUFreq probe
prior to
dfll clock enable
should be safe.
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]
source
above I meant
PLL_P_OUT4.
As per clock policies, PLL_X is always used
for high
freq
like
800Mhzand for low frequency it will be sourced from
PLLP.
pre-initialize
PLLP_OUT4 rate to a
reasonable value using tegra_clk_init_table or
assigned-clocks.
safe to
run at
408Mhz because it is below fmax @ Vmin
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.
that
will
work at Vmin voltage and PLLP max is 408Mhz.
to have
that
commented
in the code as well.
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.
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).
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.
as a
context of any other clock?
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.
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_gSince DFLL is now guaranteed to be disabled across CaR
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.
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).
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.
clocks
order
such that it won't happen.
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
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.
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.
clocks as
it need ref, soc and peripheral clocks to be enabled.
So they are the last to get registered and so becomes first inI was referring to clk_restore_context where it iterates thru
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.
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
*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.
During suspend CPU's parent shall be PLLP and not DFLL (note thatYes suspend should not be an issue but issue will be duringSeems we already agreed that DFLL will be disabled by theMy above comment is in reference to your request of doingDFLL enable thru CPUFreq resume happens after allIf CPU was suspended on PLLP, then it will be restored on
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
PLLP by
CaR. I
don't understand what DFLL has to do with the CCLK in that
case
during
the clocks restore.
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.
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.
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).
it is
disabled) after reparenting to PLLP by the CPUFreq driver.
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
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)
CPU to DFLL. Please see more comments below.
Please see my next comment.Below is the order of sequence it should be based on the order ofLet's see what happens if CPUFreq is active:So i was trying to say dfll/cclk_g restore can't be done in
normal way
thru clk_ops save/restore context
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
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
That I don't understand. The CPU's clock source state should beÂÂÂÂÂÂ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)
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.
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.
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.
No!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 onAlso I don't think we should hard-code to force CPU source to DFLL on
suspend in step 5, that's it. On resume CPUFreq driver will restore
CPU
to DFLL in step 18.
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.
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.
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.
Actually CPUFreq driver should implement suspend/resume regardless ofThe reverse ordering should be correct, I think it's just a shortcomingSo CPU source could be either DFLL or PLLX in CPUFreq
tegra124_cpu_switch_to_dfll
Thought of that but this is in core driver and is used by otherIt looks to me that clk_core_restore_context() should just doÂÂÂÂÂÂ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
hlist_for_each_entry *_reverse*. Don't you think so?
non-tegra clock driver and not sure if that impacts for those.
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.
CaR ability to restore DFLL or whatever, simply to properly handle
possible clock restoring failure on resume as we just found out.
Okay, but that shouldn't be a problem if clock dependencies are set updependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheralthe way of all clocks order is good except cclk_g which has dependencyCCLK_G has a single parent at a time. What "multiple clocks" you're
on multiple clocks.
talking about? Please explain.
clocks which need to be restored prior to DFLL reinit.
properly.
Please try to fix all missing dependencies and orderings.Yes, we should register ref/parents before their clients.reverse list order during restore might not work as all other clocks areWhy? The ref clocks should be registered first and be the roots for PLLs
in proper order no with any ref clocks for plls getting restored prior
to their clients
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]
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.