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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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@nvidia.com> пишет:
>>>>>>
>>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>>> 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


>>>>> 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).
>
> So i was trying to say dfll/cclk_g restore can't be done in normal way 
> thru clk_ops save/restore context
>