* Applied "spi: gpio: Add SPI_MASTER_GPIO_SS flag" to the spi tree
From: Mark Brown @ 2019-07-17 17:58 UTC (permalink / raw)
To: Linus Walleij; +Cc: Andrey Smirnov, linux-gpio, linux-spi, Mark Brown
In-Reply-To: <20190716204651.7743-1-linus.walleij@linaro.org>
The patch
spi: gpio: Add SPI_MASTER_GPIO_SS flag
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.3
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 2922d1cc1696200a3e1fd6d82a7798fcd2cadf12 Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Tue, 16 Jul 2019 22:46:51 +0200
Subject: [PATCH] spi: gpio: Add SPI_MASTER_GPIO_SS flag
The GPIO SPI master has some code in its local CS
callback to set the initial sck GPIO value. This was
lost in the commit converting it to use SPI core
GPIO handling as this callback isn't called if the
internal GPIO handling is active.
Add the special SPI_MASTER_GPIO_SS to ascertain it
gets called anyway so we get the initial SCK setting
right. There is some platform provided GPIO handling
there as well but this will be skipped as the cs_gpios
will be NULL.
My test targets seem not to care about the initial
SCK value so I am uncertain if this is a regression,
but to preserve the previous semantic we better do
this.
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Fixes: 249e2632dcd0 ("spi: gpio: Don't request CS GPIO in DT use-case")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/r/20190716204651.7743-1-linus.walleij@linaro.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
drivers/spi/spi-gpio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index eca9d52ecf65..9eb82150666e 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -410,6 +410,12 @@ static int spi_gpio_probe(struct platform_device *pdev)
bb = &spi_gpio->bitbang;
bb->master = master;
+ /*
+ * There is some additional business, apart from driving the CS GPIO
+ * line, that we need to do on selection. This makes the local
+ * callback for chipselect always get called.
+ */
+ master->flags |= SPI_MASTER_GPIO_SS;
bb->chipselect = spi_gpio_chipselect;
bb->set_line_direction = spi_gpio_set_direction;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 17:29 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <16f8b146-2581-a842-4997-53ab05b62c70@gmail.com>
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
>> 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.
^ permalink raw reply
* linusw/devel boot: 49 boots: 0 failed, 49 passed (v5.2-10808-g9637d517347e)
From: kernelci.org bot @ 2019-07-17 17:08 UTC (permalink / raw)
To: linux-gpio, fellows
linusw/devel boot: 49 boots: 0 failed, 49 passed (v5.2-10808-g9637d517347e)
Full Boot Summary: https://kernelci.org/boot/all/job/linusw/branch/devel/kernel/v5.2-10808-g9637d517347e/
Full Build Summary: https://kernelci.org/build/linusw/branch/devel/kernel/v5.2-10808-g9637d517347e/
Tree: linusw
Branch: devel
Git Describe: v5.2-10808-g9637d517347e
Git Commit: 9637d517347e80ee2fe1c5d8ce45ba1b88d8b5cd
Git URL: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/
Tested: 33 unique boards, 15 SoC families, 3 builds out of 6
---
For more info write to <info@kernelci.org>
^ permalink raw reply
* linusw/devel build: 6 builds: 0 failed, 6 passed, 3 warnings (v5.2-10808-g9637d517347e)
From: kernelci.org bot @ 2019-07-17 16:23 UTC (permalink / raw)
To: linux-gpio, fellows
linusw/devel build: 6 builds: 0 failed, 6 passed, 3 warnings (v5.2-10808-g9637d517347e)
Full Build Summary: https://kernelci.org/build/linusw/branch/devel/kernel/v5.2-10808-g9637d517347e/
Tree: linusw
Branch: devel
Git Describe: v5.2-10808-g9637d517347e
Git Commit: 9637d517347e80ee2fe1c5d8ce45ba1b88d8b5cd
Git URL: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/
Built: 6 unique architectures
Warnings Detected:
arc:
arm64:
arm:
mips:
32r2el_defconfig (gcc-8): 3 warnings
riscv:
x86_64:
Warnings summary:
3 <stdin>:1511:2: warning: #warning syscall clone3 not implemented [-Wcpp]
================================================================================
Detailed per-defconfig build reports:
--------------------------------------------------------------------------------
32r2el_defconfig (mips, gcc-8) — PASS, 0 errors, 3 warnings, 0 section mismatches
Warnings:
<stdin>:1511:2: warning: #warning syscall clone3 not implemented [-Wcpp]
<stdin>:1511:2: warning: #warning syscall clone3 not implemented [-Wcpp]
<stdin>:1511:2: warning: #warning syscall clone3 not implemented [-Wcpp]
--------------------------------------------------------------------------------
defconfig (riscv, gcc-8) — PASS, 0 errors, 0 warnings, 0 section mismatches
--------------------------------------------------------------------------------
defconfig (arm64, gcc-8) — PASS, 0 errors, 0 warnings, 0 section mismatches
--------------------------------------------------------------------------------
multi_v7_defconfig (arm, gcc-8) — PASS, 0 errors, 0 warnings, 0 section mismatches
--------------------------------------------------------------------------------
nsim_hs_defconfig (arc, gcc-8) — PASS, 0 errors, 0 warnings, 0 section mismatches
--------------------------------------------------------------------------------
x86_64_defconfig (x86_64, gcc-8) — PASS, 0 errors, 0 warnings, 0 section mismatches
---
For more info write to <info@kernelci.org>
^ permalink raw reply
* [GIT PULL] GPIO fixes for v5.3 take one
From: Linus Walleij @ 2019-07-17 15:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: open list:GPIO SUBSYSTEM, linux-kernel, Bartosz Golaszewski
Hi Linus,
some fixes arrived early so let's merge them early so we
have as little problems as possible in -rc1.
This is based on the most recent HEAD commit that day.
Please pull it in, details in the signed tag.
Yours,
Linus Walleij
The following changes since commit 9637d517347e80ee2fe1c5d8ce45ba1b88d8b5cd:
Merge tag 'for-linus-20190715' of git://git.kernel.dk/linux-block
(2019-07-15 21:20:52 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
tags/gpio-v5.3-2
for you to fetch changes up to 88785b7fa74ae2dc52f879140b976984b5374c79:
Merge tag 'gpio-v5.3-rc1-fixes-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux into fixes
(2019-07-16 11:12:14 +0200)
----------------------------------------------------------------
GPIO fixes for the v5.3 merge window:
- Revert a SPIO GPIO fix that didn't fix anything instead created new
problems.
- Remove the EM GPIO irqdomain in a safe manner.
- Fix a memory leak in the gpio quirks.
- Make the DaVinci error path silent on probe deferral.
----------------------------------------------------------------
Bartosz Golaszewski (1):
gpio: em: remove the gpiochip before removing the irq domain
Keerthy (1):
gpio: davinci: silence error prints in case of EPROBE_DEFER
Linus Walleij (2):
Revert "gpio/spi: Fix spi-gpio regression on active high CS"
Merge tag 'gpio-v5.3-rc1-fixes-for-linus' of
git://git.kernel.org/.../brgl/linux into fixes
Nishka Dasgupta (1):
gpiolib: of: fix a memory leak in of_gpio_flags_quirks()
drivers/gpio/gpio-davinci.c | 5 +++--
drivers/gpio/gpio-em.c | 33 +++++++++++++++------------------
drivers/gpio/gpiolib-of.c | 10 ++--------
3 files changed, 20 insertions(+), 28 deletions(-)
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 15:17 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <6e73dcee-6e24-b646-97a4-4b34aedd231d@nvidia.com>
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.
> 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.
^ permalink raw reply
* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Harish Jenny K N @ 2019-07-17 13:51 UTC (permalink / raw)
To: Rob Herring, Linus Walleij
Cc: Bartosz Golaszewski, Mark Rutland, devicetree,
open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan
In-Reply-To: <f1616784-4dbf-d0fa-b33e-c85fd569383a@mentor.com>
Hi Linus,
On 10/07/19 1:58 PM, Harish Jenny K N wrote:
> Hi,
>
> On 09/07/19 9:38 PM, Rob Herring wrote:
>> On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>> Hi Rob,
>>>
>>>
>>> On 09/07/19 4:06 AM, Rob Herring wrote:
>>>> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
>>>> <harish_kandiga@mentor.com> wrote:
>>>>> Document the device tree binding for the inverter gpio
>>>>> controller to configure the polarity of the gpio pins
>>>>> used by the consumers.
>>>>>
>>>>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>>>>> ---
>>>>> .../devicetree/bindings/gpio/gpio-inverter.txt | 29 ++++++++++++++++++++++
>>>>> 1 file changed, 29 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> new file mode 100644
>>>>> index 0000000..8bb6b2e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> @@ -0,0 +1,29 @@
>>>>> +GPIO-INVERTER
>>>>> +======
>>>>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>>>>> +allows to properly describe the gpio polarities on the hardware.
>>>> I don't understand. Please explain this in terms of the hardware, not a driver.
>>> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
>>> The polarity of pins can change from hardware to hardware with the use of inverters.
>> Yes, I know what an inverter is.
>>
>>> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
>> We already define the active state of GPIOs in the consumers. If
>> there's an inverter in the middle, the consumer active state is simply
>> inverted. I don't agree that that is a hack as Linus said without some
>> reasoning why an inverter needs to be modeled in DT. Anything about
>> what 'userspace' needs is not a reason. That's a Linux thing that has
>> little to do with hardware description.
>
> Yes we are talking about the hardware level inversions here. The usecase is for those without the gpio consumer driver. The usecase started with the concept of allowing an abstraction of the underlying hardware for the userland controlling program such that this program does not care whether the GPIO lines are inverted or not physically. In other words, a single userland controlling program can work unmodified across a variety of hardware platforms with the device tree mapping the logical to physical relationship of the GPIO hardware.
> I totally understand anything about what 'userspace' needs is not a reason, but this is not restricted to userspace alone as kernel drivers may need this just as much. Also we are just modelling/describing the hardware state in the device tree.
>
> Just to mention that Linus Walleij had proposed this inverter model to describe the hardware and the gpio inverter driver is developed based on comments/review from him.
>
> Also my sincere request to Linus Walleij to please let his opinion know on this.
>
> Thanks,
>
> Best Regards,
> Harish Jenny K N
Can you please give your opinion on this.
Thanks.
Best Regards,
Harish Jenny K N
>
^ permalink raw reply
* [PATCH v4 14/15] docs: ABI: obsolete, removed: don't escape ReST-incompatible chars
From: Mauro Carvalho Chehab @ 2019-07-17 12:28 UTC (permalink / raw)
To: gregkh
Cc: Mauro Carvalho Chehab, Linus Walleij, Bartosz Golaszewski,
Jonathan Corbet, linux-gpio, linux-doc
In-Reply-To: <cover.1563365880.git.mchehab+samsung@kernel.org>
With just a single fix, the contents there can be parsed properly
without the need to escape any ReST incompatible stuff.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/ABI/obsolete/sysfs-gpio | 2 ++
Documentation/admin-guide/abi-obsolete.rst | 1 +
Documentation/admin-guide/abi-removed.rst | 1 +
3 files changed, 4 insertions(+)
diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
index e0d4e5e2dd90..b8b0fd341c17 100644
--- a/Documentation/ABI/obsolete/sysfs-gpio
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -13,6 +13,8 @@ Description:
GPIOs are identified as they are inside the kernel, using integers in
the range 0..INT_MAX. See Documentation/admin-guide/gpio for more information.
+ ::
+
/sys/class/gpio
/export ... asks the kernel to export a GPIO to userspace
/unexport ... to return a GPIO to the kernel
diff --git a/Documentation/admin-guide/abi-obsolete.rst b/Documentation/admin-guide/abi-obsolete.rst
index cda9168445a5..d095867899c5 100644
--- a/Documentation/admin-guide/abi-obsolete.rst
+++ b/Documentation/admin-guide/abi-obsolete.rst
@@ -8,3 +8,4 @@ The description of the interface will document the reason why it is
obsolete and when it can be expected to be removed.
.. kernel-abi:: $srctree/Documentation/ABI/obsolete
+ :rst:
diff --git a/Documentation/admin-guide/abi-removed.rst b/Documentation/admin-guide/abi-removed.rst
index 497978fc9632..f7e9e43023c1 100644
--- a/Documentation/admin-guide/abi-removed.rst
+++ b/Documentation/admin-guide/abi-removed.rst
@@ -2,3 +2,4 @@ ABI removed symbols
===================
.. kernel-abi:: $srctree/Documentation/ABI/removed
+ :rst:
--
2.21.0
^ permalink raw reply related
* Re: WARNING in gpio_to_desc
From: syzbot @ 2019-07-17 12:07 UTC (permalink / raw)
To: andreyknvl, bgolaszewski, cuissard, johan, linus.walleij,
linux-gpio, linux-kernel, linux-usb, sameo, syzkaller-bugs
In-Reply-To: <CAAeHK+x4KAy2koonjij26iowtPuj67F=Mx+g0kYF968Zr-h8xg@mail.gmail.com>
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com
Tested on:
commit: 6a3599ce usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
kernel config: https://syzkaller.appspot.com/x/.config?x=d90745bdf884fc0a
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
patch: https://syzkaller.appspot.com/x/patch.diff?x=126cd1a4600000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply
* Re: WARNING in gpio_to_desc
From: Andrey Konovalov @ 2019-07-17 11:48 UTC (permalink / raw)
To: Johan Hovold
Cc: Linus Walleij, syzbot, Vincent Cuissard, Bartosz Golaszewski,
open list:GPIO SUBSYSTEM, linux-kernel@vger.kernel.org, linux-usb,
syzkaller-bugs, Samuel Ortiz
In-Reply-To: <20190717091621.GC16694@localhost>
[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]
On Wed, Jul 17, 2019 at 11:16 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Jul 16, 2019 at 11:52:19PM +0200, Linus Walleij wrote:
> > On Wed, Jul 10, 2019 at 1:07 PM syzbot
> > <syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com> wrote:
> >
> > > HEAD commit: 7829a896 usb-fuzzer: main usb gadget fuzzer driver
> > (...)
> > > __gpio_set_value include/asm-generic/gpio.h:104 [inline]
> > > gpio_set_value include/linux/gpio.h:71 [inline]
> > > nfcmrvl_chip_halt+0x4e/0x70 drivers/nfc/nfcmrvl/main.c:259
> > > nfcmrvl_nci_register_dev+0x2d4/0x378 drivers/nfc/nfcmrvl/main.c:176
> > > nfcmrvl_probe+0x4e9/0x5e0 drivers/nfc/nfcmrvl/usb.c:344
> >
> > This bug is somewhere in the drivers/nfc/nfcmrvl* code handling
> > GPIOs.
>
> Right, and it's my bug.
>
> > It should be converted to GPIO descriptors and fixed up, see
> > drivers/gpio/TODO for details on how to do this.
>
> Conversion will have to wait, let's fix the regression first. :)
>
> > Johan/Vincent, tell me if you want me to forward the full fuzzing
> > robot crash dump.
>
> No need, thanks. I got it the report.
>
> Something like the below compiles and should fix it. Vacation starts
> today so I'll revisit and send a proper patch in a couple of weeks.
>
> Perhaps someone can feed it to the bot meanwhile (no time to play with
> it right now).
#syz test: https://github.com/google/kasan.git usb-fuzzer
>
> Note that this issue has been there since 4.12, so guess no one uses
> these devices...
>
> Johan
>
>
> From e9d9d0ef5ffd6b306cffb2f4e2514f503aa626a5 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Wed, 17 Jul 2019 11:07:13 +0200
> Subject: [PATCH] NFC: nfcmrvl: fix gpio-handling regression
>
> FIXME
>
> Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
> Not-Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> drivers/nfc/nfcmrvl/main.c | 4 ++--
> drivers/nfc/nfcmrvl/usb.c | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index e65d027b91fa..529be35ac178 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
> /* Reset possible fault of previous session */
> clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
>
> - if (priv->config.reset_n_io) {
> + if (gpio_is_valid(priv->config.reset_n_io)) {
> nfc_info(priv->dev, "reset the chip\n");
> gpio_set_value(priv->config.reset_n_io, 0);
> usleep_range(5000, 10000);
> @@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
>
> void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
> {
> - if (priv->config.reset_n_io)
> + if (gpio_is_valid(priv->config.reset_n_io))
> gpio_set_value(priv->config.reset_n_io, 0);
> }
>
> diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
> index 945cc903d8f1..888e298f610b 100644
> --- a/drivers/nfc/nfcmrvl/usb.c
> +++ b/drivers/nfc/nfcmrvl/usb.c
> @@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
>
> /* No configuration for USB */
> memset(&config, 0, sizeof(config));
> + config.reset_n_io = -EINVAL;
>
> nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
>
> --
> 2.22.0
>
[-- Attachment #2: usb-gpio.patch --]
[-- Type: text/x-patch, Size: 1690 bytes --]
From e9d9d0ef5ffd6b306cffb2f4e2514f503aa626a5 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Wed, 17 Jul 2019 11:07:13 +0200
Subject: [PATCH] NFC: nfcmrvl: fix gpio-handling regression
FIXME
Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
Not-Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/nfc/nfcmrvl/main.c | 4 ++--
drivers/nfc/nfcmrvl/usb.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index e65d027b91fa..529be35ac178 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
/* Reset possible fault of previous session */
clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
- if (priv->config.reset_n_io) {
+ if (gpio_is_valid(priv->config.reset_n_io)) {
nfc_info(priv->dev, "reset the chip\n");
gpio_set_value(priv->config.reset_n_io, 0);
usleep_range(5000, 10000);
@@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
{
- if (priv->config.reset_n_io)
+ if (gpio_is_valid(priv->config.reset_n_io))
gpio_set_value(priv->config.reset_n_io, 0);
}
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 945cc903d8f1..888e298f610b 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
/* No configuration for USB */
memset(&config, 0, sizeof(config));
+ config.reset_n_io = -EINVAL;
nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
--
2.22.0
^ permalink raw reply related
* [PATCH v3 18/20] docs: ABI: don't escape ReST-incompatible chars from obsolete and removed
From: Mauro Carvalho Chehab @ 2019-07-17 11:05 UTC (permalink / raw)
To: gregkh
Cc: Mauro Carvalho Chehab, Linus Walleij, Bartosz Golaszewski,
Jonathan Corbet, linux-gpio, linux-doc
In-Reply-To: <cover.1563360659.git.mchehab+samsung@kernel.org>
With just a single fix, the contents there can be parsed properly
without the need to escape any ReST incompatible stuff.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/ABI/obsolete/sysfs-gpio | 2 ++
Documentation/admin-guide/abi-obsolete.rst | 1 +
Documentation/admin-guide/abi-removed.rst | 1 +
3 files changed, 4 insertions(+)
diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
index e0d4e5e2dd90..b8b0fd341c17 100644
--- a/Documentation/ABI/obsolete/sysfs-gpio
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -13,6 +13,8 @@ Description:
GPIOs are identified as they are inside the kernel, using integers in
the range 0..INT_MAX. See Documentation/admin-guide/gpio for more information.
+ ::
+
/sys/class/gpio
/export ... asks the kernel to export a GPIO to userspace
/unexport ... to return a GPIO to the kernel
diff --git a/Documentation/admin-guide/abi-obsolete.rst b/Documentation/admin-guide/abi-obsolete.rst
index cda9168445a5..d095867899c5 100644
--- a/Documentation/admin-guide/abi-obsolete.rst
+++ b/Documentation/admin-guide/abi-obsolete.rst
@@ -8,3 +8,4 @@ The description of the interface will document the reason why it is
obsolete and when it can be expected to be removed.
.. kernel-abi:: $srctree/Documentation/ABI/obsolete
+ :rst:
diff --git a/Documentation/admin-guide/abi-removed.rst b/Documentation/admin-guide/abi-removed.rst
index 497978fc9632..f7e9e43023c1 100644
--- a/Documentation/admin-guide/abi-removed.rst
+++ b/Documentation/admin-guide/abi-removed.rst
@@ -2,3 +2,4 @@ ABI removed symbols
===================
.. kernel-abi:: $srctree/Documentation/ABI/removed
+ :rst:
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2] gpio: of: Break out OF-only code
From: Andy Shevchenko @ 2019-07-17 9:28 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-gpio, Bartosz Golaszewski
In-Reply-To: <20190717071001.3858-1-linus.walleij@linaro.org>
On Wed, Jul 17, 2019 at 09:10:01AM +0200, Linus Walleij wrote:
> The core gpiolib should not contain any OF/device tree-only
> code. Try to break out the main part of it and push it down
> into the optional gpiolib-of.c part of the library.
>
> Create a local gpiolib-of.h header and move stuff around a
> bit to get a clean cut.
Are you going to split ACPI parts as well (at least to a header)?
One nit to address below (sorry, didn't notice before), and take mine
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> -static int gpiochip_alloc_valid_mask(struct gpio_chip *gpiochip)
> +static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
> {
> -#ifdef CONFIG_OF_GPIO
> - int size;
> - struct device_node *np = gpiochip->of_node;
> -
> - size = of_property_count_u32_elems(np, "gpio-reserved-ranges");
> - if (size > 0 && size % 2 == 0)
> - gpiochip->need_valid_mask = true;
> -#endif
> -
> - if (!gpiochip->need_valid_mask)
> + if (IS_ENABLED(CONFIG_OF_GPIO))
> + gc->need_valid_mask = of_gpio_need_valid_mask(gc);
gpiochip->need_valid_mask = of_gpio_need_valid_mask(gpiochip);
Even with full name it fits 80.
Can you drop renaming here?
> + if (!gc->need_valid_mask)
> return 0;
>
> - gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);
> - if (!gpiochip->valid_mask)
> + gc->valid_mask = gpiochip_allocate_mask(gc);
> + if (!gc->valid_mask)
> return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: WARNING in gpio_to_desc
From: Dmitry Vyukov @ 2019-07-17 9:23 UTC (permalink / raw)
To: Johan Hovold
Cc: Linus Walleij, syzbot, Vincent Cuissard, Andrey Konovalov,
Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
linux-kernel@vger.kernel.org, linux-usb, syzkaller-bugs,
Samuel Ortiz
In-Reply-To: <20190717091621.GC16694@localhost>
On Wed, Jul 17, 2019 at 11:16 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Jul 16, 2019 at 11:52:19PM +0200, Linus Walleij wrote:
> > On Wed, Jul 10, 2019 at 1:07 PM syzbot
> > <syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com> wrote:
> >
> > > HEAD commit: 7829a896 usb-fuzzer: main usb gadget fuzzer driver
> > (...)
> > > __gpio_set_value include/asm-generic/gpio.h:104 [inline]
> > > gpio_set_value include/linux/gpio.h:71 [inline]
> > > nfcmrvl_chip_halt+0x4e/0x70 drivers/nfc/nfcmrvl/main.c:259
> > > nfcmrvl_nci_register_dev+0x2d4/0x378 drivers/nfc/nfcmrvl/main.c:176
> > > nfcmrvl_probe+0x4e9/0x5e0 drivers/nfc/nfcmrvl/usb.c:344
> >
> > This bug is somewhere in the drivers/nfc/nfcmrvl* code handling
> > GPIOs.
>
> Right, and it's my bug.
>
> > It should be converted to GPIO descriptors and fixed up, see
> > drivers/gpio/TODO for details on how to do this.
>
> Conversion will have to wait, let's fix the regression first. :)
>
> > Johan/Vincent, tell me if you want me to forward the full fuzzing
> > robot crash dump.
>
> No need, thanks. I got it the report.
>
> Something like the below compiles and should fix it. Vacation starts
> today so I'll revisit and send a proper patch in a couple of weeks.
>
> Perhaps someone can feed it to the bot meanwhile (no time to play with
> it right now).
>
> Note that this issue has been there since 4.12, so guess no one uses
> these devices...
>
> Johan
>
>
> From e9d9d0ef5ffd6b306cffb2f4e2514f503aa626a5 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Wed, 17 Jul 2019 11:07:13 +0200
> Subject: [PATCH] NFC: nfcmrvl: fix gpio-handling regression
>
> FIXME
>
> Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
> Not-Signed-off-by: Johan Hovold <johan@kernel.org>
Please don't forget:
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com
> ---
> drivers/nfc/nfcmrvl/main.c | 4 ++--
> drivers/nfc/nfcmrvl/usb.c | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index e65d027b91fa..529be35ac178 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
> /* Reset possible fault of previous session */
> clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
>
> - if (priv->config.reset_n_io) {
> + if (gpio_is_valid(priv->config.reset_n_io)) {
> nfc_info(priv->dev, "reset the chip\n");
> gpio_set_value(priv->config.reset_n_io, 0);
> usleep_range(5000, 10000);
> @@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
>
> void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
> {
> - if (priv->config.reset_n_io)
> + if (gpio_is_valid(priv->config.reset_n_io))
> gpio_set_value(priv->config.reset_n_io, 0);
> }
>
> diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
> index 945cc903d8f1..888e298f610b 100644
> --- a/drivers/nfc/nfcmrvl/usb.c
> +++ b/drivers/nfc/nfcmrvl/usb.c
> @@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
>
> /* No configuration for USB */
> memset(&config, 0, sizeof(config));
> + config.reset_n_io = -EINVAL;
>
> nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
>
> --
> 2.22.0
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20190717091621.GC16694%40localhost.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: WARNING in gpio_to_desc
From: Johan Hovold @ 2019-07-17 9:16 UTC (permalink / raw)
To: Linus Walleij
Cc: syzbot, Johan Hovold, Vincent Cuissard, Andrey Konovalov,
Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
linux-kernel@vger.kernel.org, linux-usb, syzkaller-bugs,
Samuel Ortiz
In-Reply-To: <CACRpkdYbuJ_yJ+UKtvWrkmBCbQzfk-1mn1-A836dkNMrmTtZow@mail.gmail.com>
On Tue, Jul 16, 2019 at 11:52:19PM +0200, Linus Walleij wrote:
> On Wed, Jul 10, 2019 at 1:07 PM syzbot
> <syzbot+cf35b76f35e068a1107f@syzkaller.appspotmail.com> wrote:
>
> > HEAD commit: 7829a896 usb-fuzzer: main usb gadget fuzzer driver
> (...)
> > __gpio_set_value include/asm-generic/gpio.h:104 [inline]
> > gpio_set_value include/linux/gpio.h:71 [inline]
> > nfcmrvl_chip_halt+0x4e/0x70 drivers/nfc/nfcmrvl/main.c:259
> > nfcmrvl_nci_register_dev+0x2d4/0x378 drivers/nfc/nfcmrvl/main.c:176
> > nfcmrvl_probe+0x4e9/0x5e0 drivers/nfc/nfcmrvl/usb.c:344
>
> This bug is somewhere in the drivers/nfc/nfcmrvl* code handling
> GPIOs.
Right, and it's my bug.
> It should be converted to GPIO descriptors and fixed up, see
> drivers/gpio/TODO for details on how to do this.
Conversion will have to wait, let's fix the regression first. :)
> Johan/Vincent, tell me if you want me to forward the full fuzzing
> robot crash dump.
No need, thanks. I got it the report.
Something like the below compiles and should fix it. Vacation starts
today so I'll revisit and send a proper patch in a couple of weeks.
Perhaps someone can feed it to the bot meanwhile (no time to play with
it right now).
Note that this issue has been there since 4.12, so guess no one uses
these devices...
Johan
From e9d9d0ef5ffd6b306cffb2f4e2514f503aa626a5 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Wed, 17 Jul 2019 11:07:13 +0200
Subject: [PATCH] NFC: nfcmrvl: fix gpio-handling regression
FIXME
Fixes: e33a3f84f88f ("NFC: nfcmrvl: allow gpio 0 for reset signalling")
Not-Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/nfc/nfcmrvl/main.c | 4 ++--
drivers/nfc/nfcmrvl/usb.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index e65d027b91fa..529be35ac178 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -244,7 +244,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
/* Reset possible fault of previous session */
clear_bit(NFCMRVL_PHY_ERROR, &priv->flags);
- if (priv->config.reset_n_io) {
+ if (gpio_is_valid(priv->config.reset_n_io)) {
nfc_info(priv->dev, "reset the chip\n");
gpio_set_value(priv->config.reset_n_io, 0);
usleep_range(5000, 10000);
@@ -255,7 +255,7 @@ void nfcmrvl_chip_reset(struct nfcmrvl_private *priv)
void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
{
- if (priv->config.reset_n_io)
+ if (gpio_is_valid(priv->config.reset_n_io))
gpio_set_value(priv->config.reset_n_io, 0);
}
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 945cc903d8f1..888e298f610b 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -305,6 +305,7 @@ static int nfcmrvl_probe(struct usb_interface *intf,
/* No configuration for USB */
memset(&config, 0, sizeof(config));
+ config.reset_n_io = -EINVAL;
nfc_info(&udev->dev, "intf %p id %p\n", intf, id);
--
2.22.0
^ permalink raw reply related
* [PATCH v2] gpio: of: Break out OF-only code
From: Linus Walleij @ 2019-07-17 7:10 UTC (permalink / raw)
To: linux-gpio; +Cc: Bartosz Golaszewski, Linus Walleij, Andy Shevchenko
The core gpiolib should not contain any OF/device tree-only
code. Try to break out the main part of it and push it down
into the optional gpiolib-of.c part of the library.
Create a local gpiolib-of.h header and move stuff around a
bit to get a clean cut.
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Add missing header file.
---
drivers/gpio/gpiolib-of.c | 114 ++++++++++++++++++++++++++++++++++++++
drivers/gpio/gpiolib-of.h | 45 +++++++++++++++
drivers/gpio/gpiolib.c | 113 +++----------------------------------
drivers/gpio/gpiolib.h | 27 ---------
4 files changed, 167 insertions(+), 132 deletions(-)
create mode 100644 drivers/gpio/gpiolib-of.h
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2bc0bcd7a410..00ed60304cd4 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -21,6 +21,34 @@
#include <linux/gpio/machine.h>
#include "gpiolib.h"
+#include "gpiolib-of.h"
+
+/*
+ * This is used by external users of of_gpio_count() from <linux/of_gpio.h>
+ *
+ * FIXME: get rid of those external users by converting them to GPIO
+ * descriptors and let them all use gpiod_get_count()
+ */
+int of_gpio_get_count(struct device *dev, const char *con_id)
+{
+ int ret;
+ char propname[32];
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
+ if (con_id)
+ snprintf(propname, sizeof(propname), "%s-%s",
+ con_id, gpio_suffixes[i]);
+ else
+ snprintf(propname, sizeof(propname), "%s",
+ gpio_suffixes[i]);
+
+ ret = of_gpio_named_count(dev->of_node, propname);
+ if (ret > 0)
+ break;
+ }
+ return ret ? ret : -ENOENT;
+}
static int of_gpiochip_match_node_and_xlate(struct gpio_chip *chip, void *data)
{
@@ -53,6 +81,23 @@ static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
return gpiochip_get_desc(chip, ret);
}
+/**
+ * of_gpio_need_valid_mask() - figure out if the OF GPIO driver needs
+ * to set the .valid_mask
+ * @dev: the device for the GPIO provider
+ * @return: true if the valid mask needs to be set
+ */
+bool of_gpio_need_valid_mask(struct gpio_chip *gc)
+{
+ int size;
+ struct device_node *np = gc->of_node;
+
+ size = of_property_count_u32_elems(np, "gpio-reserved-ranges");
+ if (size > 0 && size % 2 == 0)
+ return true;
+ return false;
+}
+
static void of_gpio_flags_quirks(struct device_node *np,
const char *propname,
enum of_gpio_flags *flags,
@@ -237,6 +282,75 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
}
EXPORT_SYMBOL(of_get_named_gpio_flags);
+/**
+ * gpiod_get_from_of_node() - obtain a GPIO from an OF node
+ * @node: handle of the OF node
+ * @propname: name of the DT property representing the GPIO
+ * @index: index of the GPIO to obtain for the consumer
+ * @dflags: GPIO initialization flags
+ * @label: label to attach to the requested GPIO
+ *
+ * Returns:
+ * On successful request the GPIO pin is configured in accordance with
+ * provided @dflags.
+ *
+ * In case of error an ERR_PTR() is returned.
+ */
+struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
+ const char *propname, int index,
+ enum gpiod_flags dflags,
+ const char *label)
+{
+ unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
+ struct gpio_desc *desc;
+ enum of_gpio_flags flags;
+ bool active_low = false;
+ bool single_ended = false;
+ bool open_drain = false;
+ bool transitory = false;
+ int ret;
+
+ desc = of_get_named_gpiod_flags(node, propname,
+ index, &flags);
+
+ if (!desc || IS_ERR(desc)) {
+ return desc;
+ }
+
+ active_low = flags & OF_GPIO_ACTIVE_LOW;
+ single_ended = flags & OF_GPIO_SINGLE_ENDED;
+ open_drain = flags & OF_GPIO_OPEN_DRAIN;
+ transitory = flags & OF_GPIO_TRANSITORY;
+
+ ret = gpiod_request(desc, label);
+ if (ret == -EBUSY && (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
+ return desc;
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (active_low)
+ lflags |= GPIO_ACTIVE_LOW;
+
+ if (single_ended) {
+ if (open_drain)
+ lflags |= GPIO_OPEN_DRAIN;
+ else
+ lflags |= GPIO_OPEN_SOURCE;
+ }
+
+ if (transitory)
+ lflags |= GPIO_TRANSITORY;
+
+ ret = gpiod_configure_flags(desc, propname, lflags, dflags);
+ if (ret < 0) {
+ gpiod_put(desc);
+ return ERR_PTR(ret);
+ }
+
+ return desc;
+}
+EXPORT_SYMBOL(gpiod_get_from_of_node);
+
/*
* The SPI GPIO bindings happened before we managed to establish that GPIO
* properties should be named "foo-gpios" so we have this special kludge for
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
new file mode 100644
index 000000000000..34954921d96e
--- /dev/null
+++ b/drivers/gpio/gpiolib-of.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef GPIOLIB_OF_H
+#define GPIOLIB_OF_H
+
+struct gpio_chip;
+enum of_gpio_flags;
+
+#ifdef CONFIG_OF_GPIO
+struct gpio_desc *of_find_gpio(struct device *dev,
+ const char *con_id,
+ unsigned int idx,
+ unsigned long *lookupflags);
+struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
+ const char *list_name, int index, enum of_gpio_flags *flags);
+int of_gpiochip_add(struct gpio_chip *gc);
+void of_gpiochip_remove(struct gpio_chip *gc);
+int of_gpio_get_count(struct device *dev, const char *con_id);
+bool of_gpio_need_valid_mask(struct gpio_chip *gc);
+#else
+static inline struct gpio_desc *of_find_gpio(struct device *dev,
+ const char *con_id,
+ unsigned int idx,
+ unsigned long *lookupflags)
+{
+ return ERR_PTR(-ENOENT);
+}
+static inline struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
+ const char *list_name, int index, enum of_gpio_flags *flags)
+{
+ return ERR_PTR(-ENOENT);
+}
+static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; }
+static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
+static inline int of_gpio_get_count(struct device *dev, const char *con_id)
+{
+ return 0;
+}
+static inline bool of_gpio_need_valid_mask(struct gpio_chip *gc)
+{
+ return false;
+}
+#endif /* CONFIG_OF_GPIO */
+
+#endif /* GPIOLIB_OF_H */
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 24300f401fce..3e262a280f95 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -11,7 +11,6 @@
#include <linux/debugfs.h>
#include <linux/seq_file.h>
#include <linux/gpio.h>
-#include <linux/of_gpio.h>
#include <linux/idr.h>
#include <linux/slab.h>
#include <linux/acpi.h>
@@ -30,6 +29,7 @@
#include <uapi/linux/gpio.h>
#include "gpiolib.h"
+#include "gpiolib-of.h"
#define CREATE_TRACE_POINTS
#include <trace/events/gpio.h>
@@ -360,22 +360,15 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *chip)
return p;
}
-static int gpiochip_alloc_valid_mask(struct gpio_chip *gpiochip)
+static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
{
-#ifdef CONFIG_OF_GPIO
- int size;
- struct device_node *np = gpiochip->of_node;
-
- size = of_property_count_u32_elems(np, "gpio-reserved-ranges");
- if (size > 0 && size % 2 == 0)
- gpiochip->need_valid_mask = true;
-#endif
-
- if (!gpiochip->need_valid_mask)
+ if (IS_ENABLED(CONFIG_OF_GPIO))
+ gc->need_valid_mask = of_gpio_need_valid_mask(gc);
+ if (!gc->need_valid_mask)
return 0;
- gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);
- if (!gpiochip->valid_mask)
+ gc->valid_mask = gpiochip_allocate_mask(gc);
+ if (!gc->valid_mask)
return -ENOMEM;
return 0;
@@ -3993,27 +3986,6 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
return desc;
}
-static int dt_gpio_count(struct device *dev, const char *con_id)
-{
- int ret;
- char propname[32];
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
- if (con_id)
- snprintf(propname, sizeof(propname), "%s-%s",
- con_id, gpio_suffixes[i]);
- else
- snprintf(propname, sizeof(propname), "%s",
- gpio_suffixes[i]);
-
- ret = of_gpio_named_count(dev->of_node, propname);
- if (ret > 0)
- break;
- }
- return ret ? ret : -ENOENT;
-}
-
static int platform_gpio_count(struct device *dev, const char *con_id)
{
struct gpiod_lookup_table *table;
@@ -4046,7 +4018,7 @@ int gpiod_count(struct device *dev, const char *con_id)
int count = -ENOENT;
if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
- count = dt_gpio_count(dev, con_id);
+ count = of_gpio_get_count(dev, con_id);
else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
count = acpi_gpio_count(dev, con_id);
@@ -4247,75 +4219,6 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
}
EXPORT_SYMBOL_GPL(gpiod_get_index);
-/**
- * gpiod_get_from_of_node() - obtain a GPIO from an OF node
- * @node: handle of the OF node
- * @propname: name of the DT property representing the GPIO
- * @index: index of the GPIO to obtain for the consumer
- * @dflags: GPIO initialization flags
- * @label: label to attach to the requested GPIO
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
- const char *propname, int index,
- enum gpiod_flags dflags,
- const char *label)
-{
- unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
- struct gpio_desc *desc;
- enum of_gpio_flags flags;
- bool active_low = false;
- bool single_ended = false;
- bool open_drain = false;
- bool transitory = false;
- int ret;
-
- desc = of_get_named_gpiod_flags(node, propname,
- index, &flags);
-
- if (!desc || IS_ERR(desc)) {
- return desc;
- }
-
- active_low = flags & OF_GPIO_ACTIVE_LOW;
- single_ended = flags & OF_GPIO_SINGLE_ENDED;
- open_drain = flags & OF_GPIO_OPEN_DRAIN;
- transitory = flags & OF_GPIO_TRANSITORY;
-
- ret = gpiod_request(desc, label);
- if (ret == -EBUSY && (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
- return desc;
- if (ret)
- return ERR_PTR(ret);
-
- if (active_low)
- lflags |= GPIO_ACTIVE_LOW;
-
- if (single_ended) {
- if (open_drain)
- lflags |= GPIO_OPEN_DRAIN;
- else
- lflags |= GPIO_OPEN_SOURCE;
- }
-
- if (transitory)
- lflags |= GPIO_TRANSITORY;
-
- ret = gpiod_configure_flags(desc, propname, lflags, dflags);
- if (ret < 0) {
- gpiod_put(desc);
- return ERR_PTR(ret);
- }
-
- return desc;
-}
-EXPORT_SYMBOL(gpiod_get_from_of_node);
-
/**
* fwnode_get_named_gpiod - obtain a GPIO from firmware node
* @fwnode: handle of the firmware node
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 7c52c2442173..75d2e909d8c6 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -16,7 +16,6 @@
#include <linux/module.h>
#include <linux/cdev.h>
-enum of_gpio_flags;
struct acpi_device;
/**
@@ -92,32 +91,6 @@ struct acpi_gpio_info {
/* gpio suffixes used for ACPI and device tree lookup */
static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
-#ifdef CONFIG_OF_GPIO
-struct gpio_desc *of_find_gpio(struct device *dev,
- const char *con_id,
- unsigned int idx,
- unsigned long *lookupflags);
-struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
- const char *list_name, int index, enum of_gpio_flags *flags);
-int of_gpiochip_add(struct gpio_chip *gc);
-void of_gpiochip_remove(struct gpio_chip *gc);
-#else
-static inline struct gpio_desc *of_find_gpio(struct device *dev,
- const char *con_id,
- unsigned int idx,
- unsigned long *lookupflags)
-{
- return ERR_PTR(-ENOENT);
-}
-static inline struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
- const char *list_name, int index, enum of_gpio_flags *flags)
-{
- return ERR_PTR(-ENOENT);
-}
-static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; }
-static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
-#endif /* CONFIG_OF_GPIO */
-
#ifdef CONFIG_ACPI
void acpi_gpiochip_add(struct gpio_chip *chip);
void acpi_gpiochip_remove(struct gpio_chip *chip);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH V2 3/3] arm64: defconfig: Select CONFIG_PINCTRL_IMX8MN by default
From: Shawn Guo @ 2019-07-17 6:54 UTC (permalink / raw)
To: Anson.Huang
Cc: aisheng.dong, festevam, stefan, kernel, linus.walleij, robh+dt,
mark.rutland, s.hauer, catalin.marinas, will.deacon,
maxime.ripard, olof, horms+renesas, jagan, bjorn.andersson,
leonard.crestez, dinguyen, enric.balletbo, linux-gpio, devicetree,
linux-arm-kernel, linux-kernel, Linux-imx
In-Reply-To: <20190611122535.23583-3-Anson.Huang@nxp.com>
On Tue, Jun 11, 2019 at 08:25:35PM +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Enable CONFIG_PINCTRL_IMX8MN by default to support i.MX8MN
> pinctrl driver.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 6:36 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190717093317.70fefb27@dimatab>
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.
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
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 6:33 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <093462f3-8c6d-d084-9822-ae4eff041c64@nvidia.com>
В 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).
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 5:55 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190717084221.2e9af56c@dimatab>
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.
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.
>> Also I don't see Tegra124 CPU Freq driver using flag
>> CPUFREQ_NEED_INITIAL_FREQ_CHECK.
>>
>> Tegra124 CPUFreq driver is not using cpufreq_driver
>>
>>
>> <https://elixir.bootlin.com/linux/v5.2.1/ident/cpufreq_driver>
>>
> T124 driver is a wrapper around cpufreq-dt driver.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 5:42 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <77df234f-aa40-0319-a593-f1f19f0f1c2a@nvidia.com>
В 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?
> Also I don't see Tegra124 CPU Freq driver using flag
> CPUFREQ_NEED_INITIAL_FREQ_CHECK.
>
> Tegra124 CPUFreq driver is not using cpufreq_driver
>
>
> <https://elixir.bootlin.com/linux/v5.2.1/ident/cpufreq_driver>
>
T124 driver is a wrapper around cpufreq-dt driver.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 4:11 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <055457fd-621b-6c93-b671-d5e5380698c6@nvidia.com>
В 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).
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 4:01 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190717065431.27f27a07@dimatab>
On 7/16/19 8:54 PM, Dmitry Osipenko wrote:
> В Tue, 16 Jul 2019 19:18:19 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>
>> 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
>
> It doesn't matter much when CPUFreq driver suspends, it's only
> important that it suspends before CaR.
>
> I'm not sure why do you need anything else from DFLL driver other than
> what is already exposed via generic CCF API. It looks to me
> that switching CPU's parent clock away from DFLL and then disabling
> DFLL's clock is enough for suspend, accordingly to what Peter wrote. And
> resuming is the same as what's done on CPUFreq's driver probe. The CCLK
> policy should be saved and restored by the CaR driver, you don't need to
> care about it. The cpufreq-dt driver sets
> CPUFREQ_NEED_INITIAL_FREQ_CHECK, hence you don't need to care about
> restoring the original CPU freq on resume, IIUC.
>
>
CPU policy save/restore are part of this patch and it wasn't there
earlier and when I moved suspend/resume to clock-dfll I moved cpu
restore also to clock-dfll driver.
Thought to restore cpu source to dfll in CPUFreq driver which requires
exporting API from clock driver. But leaving CPU restore in Tegra210
clock driver, we can do dfll suspend/resume thru CPUFreq driver pm_ops
without exporting clock APIs
Will update ...
> With
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-17 3:51 UTC (permalink / raw)
To: Rob Herring, Joel Stanley
Cc: devicetree, Linus Walleij, linux-aspeed, open list:GPIO SUBSYSTEM,
Linux ARM
In-Reply-To: <CAL_Jsq+AJDNZ-676iP=vv6G-pjWqBJyZ3bJ26o7i=c=KWbozSw@mail.gmail.com>
On Wed, 17 Jul 2019, at 00:35, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> > >
> > > The Aspeed pinctl schema have errors in the 'compatible' schema:
> > >
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> > >
> > > Flow style sequences have to be quoted if the vales contain ','. Fix
> > > this by using the more common one line per entry formatting.
> >
> > >
> > > properties:
> > > compatible:
> > > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > > + enum:
> > > + - aspeed,ast2400-pinctrl
> > > + - aspeed,g4-pinctrl
> >
> > Thanks for the fix. However, we've standardised on the first form for
> > all of our device trees, so we can drop the second compatible string
> > from the bindings.
>
> Doing that would introduce validation warnings until the dts file is
> updated. So we still need this change until that happens.
My series takes care of that.
Andrew
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 3:54 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <c6c0a205-c083-fd46-361c-175bd8840c6e@nvidia.com>
В Tue, 16 Jul 2019 19:18:19 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> 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
It doesn't matter much when CPUFreq driver suspends, it's only
important that it suspends before CaR.
I'm not sure why do you need anything else from DFLL driver other than
what is already exposed via generic CCF API. It looks to me
that switching CPU's parent clock away from DFLL and then disabling
DFLL's clock is enough for suspend, accordingly to what Peter wrote. And
resuming is the same as what's done on CPUFreq's driver probe. The CCLK
policy should be saved and restored by the CaR driver, you don't need to
care about it. The cpufreq-dt driver sets
CPUFREQ_NEED_INITIAL_FREQ_CHECK, hence you don't need to care about
restoring the original CPU freq on resume, IIUC.
^ permalink raw reply
* Re: [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-17 3:49 UTC (permalink / raw)
To: Hongwei Zhang, Joel Stanley, Linus Walleij, devicetree
Cc: Rob Herring, Mark Rutland, Bartosz Golaszewski, linux-aspeed,
linux-kernel, linux-arm-kernel, linux-gpio
In-Reply-To: <1563313711-17961-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
On Wed, 17 Jul 2019, at 07:18, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..8c3a747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
Nice description.
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins; 0 if not used)
It's unclear to me what you mean by "0 if not used" here. The property is
required, so its description in a devicetree should always have a non-zero
value of `status = "okay";`, as 0 is an invalid value according to the
datasheet (sensibly so). If `status = "disabled";` then it doesn't really
matter, which makes the comment not terribly useful.
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a
> programmable devisor
I'd leave off the parent clock information. Practically speaking it's probably
always going to be the APB clock, but who knows. From a devicetree writer's
perspective they just want to say "make it 7MHz" or whatever speed they,
and it shouldn't matter too much how we get there.
Finally, as mentioned on the driver patch, please send v4 without the history
at the bottom.
Cheers,
Andrew
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
> Thanks Andrew, please see above v3 and inline comments at below.
> --Hongwei
>
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Sunday, July 14, 2019 10:25 PM
> > To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree@vger.kernel.org
> > Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-gpio@vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
> >
> > Hello Hongwei,
> >
> > On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > > Add bindings to support SGPIO on AST2400 or AST2500.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > > create mode 100755
> > > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > new file mode 100755
> > > index 0000000..3ae2b79
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > @@ -0,0 +1,43 @@
> > > +Aspeed SGPIO controller Device Tree Bindings
> > > +-------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > > +
> > > +- #gpio-cells : Should be two
> > > + - First cell is the GPIO line number
> > > + - Second cell is used to specify optional
> > > + parameters (unused)
> > > +
> > > +- reg : Address and length of the register set for the device
> > > +- gpio-controller : Marks the device node as a GPIO controller.
> > > +- interrupts : Interrupt specifier (see interrupt bindings for
> > > + details)
> > > +
> > > +- interrupt-controller : Mark the GPIO controller as an
> > > interrupt-controller
> > > +
> > > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > > 8, up to 80 pins)
> > > + if not specified, defaults to 80.
> >
> > This appears to be a statement about the driver implementation, but bindings documents are about
> > describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> > in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> > disabled at this point, so it's probably moot. The point is the true default value from a hardware
> > perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> > However:
> >
> > You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> > optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> > *not* optional (i.e. make it
> > required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> > coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> > default value.
> >
>
> Added some descriptions and updated nr-gpios, please see v3.
>
> > > +
> > > +- clocks : A phandle to the APB clock for SGPM clock
> > > division
> > > +
> > > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > > MHz
> >
> > Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> > PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> > gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
> >
> > Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> > thus don't need words about defaults that turn out to be about the driver, not about the hardware.
> >
>
> updated, please see v3.
>
> > Finally, when updating patches in response to feedback, please send the full series again, and bump the
> > series version number. That way people can review a coherent set of patches and not have to hunt
> > around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> > patches :)
> >
> > Cheers,
> >
> > Andrew
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox