From: Dmitry Osipenko <digetx@gmail.com>
To: Sowjanya Komatineni <skomatineni@nvidia.com>,
Peter De Schrijver <pdeschrijver@nvidia.com>,
Joseph Lo <josephl@nvidia.com>
Cc: thierry.reding@gmail.com, jonathanh@nvidia.com,
tglx@linutronix.de, jason@lakedaemon.net, marc.zyngier@arm.com,
linus.walleij@linaro.org, stefan@agner.ch, mark.rutland@arm.com,
pgaikwad@nvidia.com, sboyd@kernel.org, linux-clk@vger.kernel.org,
linux-gpio@vger.kernel.org, jckuo@nvidia.com, talho@nvidia.com,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
mperttunen@nvidia.com, spatra@nvidia.com, robh+dt@kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
Date: Wed, 17 Jul 2019 01:00:38 +0300 [thread overview]
Message-ID: <cedfafd0-4114-0821-0c4b-efc17c213449@gmail.com> (raw)
In-Reply-To: <0ee06d1a-310d-59f7-0aa6-b688b33447f5@nvidia.com>
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.
next prev parent reply other threads:[~2019-07-16 22:00 UTC|newest]
Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 2:12 [PATCH V5 00/18] SC7 entry and exit support for Tegra210 Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 01/18] irqchip: tegra: Do not disable COP IRQ during suspend Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support Sowjanya Komatineni
2019-06-28 11:56 ` Dmitry Osipenko
2019-06-28 12:05 ` Dmitry Osipenko
2019-06-28 23:00 ` Sowjanya Komatineni
2019-06-29 12:38 ` Dmitry Osipenko
2019-06-29 15:40 ` Dmitry Osipenko
2019-06-29 15:46 ` Dmitry Osipenko
2019-06-29 15:58 ` Dmitry Osipenko
2019-06-29 16:28 ` Dmitry Osipenko
2019-07-04 7:31 ` Linus Walleij
2019-07-04 10:40 ` Dmitry Osipenko
2019-07-13 5:31 ` Sowjanya Komatineni
2019-07-14 21:41 ` Dmitry Osipenko
2019-07-13 5:48 ` Sowjanya Komatineni
2019-07-04 7:26 ` Linus Walleij
2019-06-28 2:12 ` [PATCH V5 03/18] clk: tegra: Save and restore divider rate Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 04/18] clk: tegra: pllout: Save and restore pllout context Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 05/18] clk: tegra: pll: Save and restore pll context Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 06/18] clk: tegra: Save and restore CPU and System clocks context Sowjanya Komatineni
2019-06-29 13:33 ` Dmitry Osipenko
2019-06-29 15:26 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 07/18] clk: tegra: Support for saving and restoring OSC context Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 08/18] clk: tegra: Add suspend resume support for DFLL Sowjanya Komatineni
2019-06-29 13:28 ` Dmitry Osipenko
2019-06-29 21:45 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 09/18] clk: tegra: Add save and restore context support for peripheral clocks Sowjanya Komatineni
2019-06-29 13:17 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 10/18] clk: tegra210: Use fence_udelay during PLLU init Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks Sowjanya Komatineni
2019-06-29 13:14 ` Dmitry Osipenko
2019-06-29 15:10 ` Dmitry Osipenko
2019-07-13 5:54 ` Sowjanya Komatineni
2019-07-14 21:41 ` Dmitry Osipenko
2019-07-16 0:35 ` Sowjanya Komatineni
2019-07-16 3:00 ` Sowjanya Komatineni
2019-07-16 3:41 ` Sowjanya Komatineni
2019-07-16 3:50 ` Dmitry Osipenko
2019-07-16 4:37 ` Sowjanya Komatineni
2019-07-16 5:37 ` Dmitry Osipenko
2019-07-16 6:20 ` Dmitry Osipenko
2019-07-16 6:35 ` Sowjanya Komatineni
2019-07-16 7:24 ` Joseph Lo
2019-07-16 8:06 ` Peter De Schrijver
2019-07-16 15:00 ` Dmitry Osipenko
2019-07-16 16:50 ` Sowjanya Komatineni
2019-07-16 18:19 ` Sowjanya Komatineni
2019-07-16 18:25 ` Dmitry Osipenko
2019-07-16 18:30 ` Sowjanya Komatineni
2019-07-16 18:43 ` Dmitry Osipenko
2019-07-16 19:26 ` Sowjanya Komatineni
2019-07-16 20:47 ` Dmitry Osipenko
2019-07-16 21:12 ` Sowjanya Komatineni
2019-07-16 21:21 ` Dmitry Osipenko
2019-07-16 21:35 ` Sowjanya Komatineni
2019-07-16 22:00 ` Dmitry Osipenko [this message]
2019-07-16 22:06 ` Sowjanya Komatineni
2019-07-17 2:18 ` Sowjanya Komatineni
2019-07-17 2:35 ` Sowjanya Komatineni
2019-07-17 4:11 ` Dmitry Osipenko
[not found] ` <77df234f-aa40-0319-a593-f1f19f0f1c2a@nvidia.com>
2019-07-17 5:42 ` Dmitry Osipenko
2019-07-17 5:55 ` Sowjanya Komatineni
2019-07-17 6:33 ` Dmitry Osipenko
2019-07-17 6:36 ` Sowjanya Komatineni
2019-07-17 15:17 ` Dmitry Osipenko
2019-07-17 17:29 ` Sowjanya Komatineni
2019-07-17 18:32 ` Dmitry Osipenko
2019-07-17 18:51 ` Sowjanya Komatineni
2019-07-17 18:54 ` Sowjanya Komatineni
2019-07-17 19:43 ` Dmitry Osipenko
2019-07-17 20:01 ` Sowjanya Komatineni
2019-07-17 20:11 ` Sowjanya Komatineni
2019-07-17 21:29 ` Sowjanya Komatineni
2019-07-17 21:30 ` Dmitry Osipenko
2019-07-17 21:51 ` Sowjanya Komatineni
2019-07-17 21:57 ` Sowjanya Komatineni
2019-07-17 22:48 ` Dmitry Osipenko
2019-07-17 23:36 ` Sowjanya Komatineni
2019-07-17 23:44 ` Dmitry Osipenko
2019-07-18 0:25 ` Sowjanya Komatineni
2019-07-18 1:15 ` Sowjanya Komatineni
2019-07-18 16:34 ` Dmitry Osipenko
2019-07-18 17:22 ` Sowjanya Komatineni
2019-07-18 17:41 ` Sowjanya Komatineni
2019-07-18 18:29 ` Sowjanya Komatineni
2019-07-18 19:11 ` Sowjanya Komatineni
2019-07-18 19:42 ` Peter De Schrijver
2019-07-18 20:26 ` Dmitry Osipenko
2019-07-18 20:36 ` Sowjanya Komatineni
2019-07-18 22:52 ` Dmitry Osipenko
2019-07-18 23:08 ` Sowjanya Komatineni
2019-07-18 23:52 ` Dmitry Osipenko
2019-07-17 3:54 ` Dmitry Osipenko
2019-07-17 4:01 ` Sowjanya Komatineni
2019-07-18 19:18 ` Peter De Schrijver
2019-07-18 19:24 ` Sowjanya Komatineni
2019-07-18 20:11 ` Dmitry Osipenko
2019-07-18 20:32 ` Dmitry Osipenko
2019-07-18 19:15 ` Peter De Schrijver
2019-06-29 15:13 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 12/18] soc/tegra: pmc: Allow support for more tegra wake Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 13/18] soc/tegra: pmc: Add pmc wake support for tegra210 Sowjanya Komatineni
2019-06-29 13:11 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 14/18] arm64: tegra: Enable wake from deep sleep on RTC alarm Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 15/18] soc/tegra: pmc: Configure core power request polarity Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 16/18] soc/tegra: pmc: Configure deep sleep control settings Sowjanya Komatineni
2019-06-29 13:00 ` Dmitry Osipenko
2019-06-29 13:02 ` Dmitry Osipenko
2019-06-28 2:12 ` [PATCH V5 17/18] arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings Sowjanya Komatineni
2019-06-28 2:12 ` [PATCH V5 18/18] arm64: dts: tegra210-p3450: Jetson nano " Sowjanya Komatineni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cedfafd0-4114-0821-0c4b-efc17c213449@gmail.com \
--to=digetx@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=jckuo@nvidia.com \
--cc=jonathanh@nvidia.com \
--cc=josephl@nvidia.com \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=mperttunen@nvidia.com \
--cc=pdeschrijver@nvidia.com \
--cc=pgaikwad@nvidia.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=skomatineni@nvidia.com \
--cc=spatra@nvidia.com \
--cc=stefan@agner.ch \
--cc=talho@nvidia.com \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).