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

[Dmitry Osipenko <digetx@xxxxxxxxx>]

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