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

[Dmitry Osipenko <digetx@gmail.com>]

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.

>>>> For OVR regulator:
>>>> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
>>>>     to hardwired boot voltage.
>>>> 3) Disable DFLL PWM output
>>>>
>>>> For I2C regulator:
>>>> 2) Program vdd_cpu regulator voltage to the boot voltage
>>>> 3) Disable DFLL I2C output
>>>>
>>>> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
>>>>     frequency
>>>> 5) Change cclk_g parent to saved parent
>>>> 6) Disable DFLL
>>
>> This is the same sequence currently implemented. But dfll
>> suspend/resume calls are thru Tegra210 clock driver.
>>
>> Dmitry wants to have dfll suspend/resume along with CCLK_G restore to
>> happen from CPUFreq driver pm_ops rather than tegra210 clock driver or
>> tegra dfll driver.
>>
>> Will move it to CPUFreq driver...
>>
> Thanks!