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

Sowjanya Komatineni <skomatineni@xxxxxxxxxx> · Tue, 16 Jul 2019 15:06:16 -0700

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