Message-ID: <c5853e1a-d812-2dbd-3bec-0a9b0b0f6f3e@nvidia.com>
Date:   Tue, 16 Jul 2019 15:24:26 +0800
From:   Joseph Lo <josephl@...dia.com>
To:     Sowjanya Komatineni <skomatineni@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>
CC:     <thierry.reding@...il.com>, <jonathanh@...dia.com>,
        <tglx@...utronix.de>, <jason@...edaemon.net>,
        <marc.zyngier@....com>, <linus.walleij@...aro.org>,
        <stefan@...er.ch>, <mark.rutland@....com>,
        <pdeschrijver@...dia.com>, <pgaikwad@...dia.com>,
        <sboyd@...nel.org>, <linux-clk@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <jckuo@...dia.com>,
        <talho@...dia.com>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <mperttunen@...dia.com>,
        <spatra@...dia.com>, <robh+dt@...nel.org>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks



On 7/16/19 2:35 PM, Sowjanya Komatineni wrote:
> 
> On 7/15/19 10:37 PM, Dmitry Osipenko wrote:
>> В Mon, 15 Jul 2019 21:37:09 -0700
>> Sowjanya Komatineni <skomatineni@...dia.com> пишет:
>>
>>> On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
>>>> 16.07.2019 6:00, Sowjanya Komatineni пишет:
>>>>> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>>>>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>>>>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>>>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>>>>>> clocks.
>>>>>>>>>>
>>>>>>>>>> All the CAR controller settings are lost on suspend when core
>>>>>>>>>> power goes off.
>>>>>>>>>>
>>>>>>>>>> This patch has implementation for saving and restoring all
>>>>>>>>>> the PLLs and clocks context during system suspend and resume
>>>>>>>>>> to have the clocks back to same state for normal operation.
>>>>>>>>>>
>>>>>>>>>> Acked-by: Thierry Reding <treding@...dia.com>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@...dia.com>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/clk/tegra/clk-tegra210.c | 115
>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>>>>>>      drivers/clk/tegra/clk.c          |  14 +++++
>>>>>>>>>>      drivers/clk/tegra/clk.h          |   1 +
>>>>>>>>>>      3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> @@ -9,10 +9,12 @@
>>>>>>>>>>      #include <linux/clkdev.h>
>>>>>>>>>>      #include <linux/of.h>
>>>>>>>>>>      #include <linux/of_address.h>
>>>>>>>>>> +#include <linux/of_platform.h>
>>>>>>>>>>      #include <linux/delay.h>
>>>>>>>>>>      #include <linux/export.h>
>>>>>>>>>>      #include <linux/mutex.h>
>>>>>>>>>>      #include <linux/clk/tegra.h>
>>>>>>>>>> +#include <linux/syscore_ops.h>
>>>>>>>>>>      #include <dt-bindings/clock/tegra210-car.h>
>>>>>>>>>>      #include <dt-bindings/reset/tegra210-car.h>
>>>>>>>>>>      #include <linux/iopoll.h>
>>>>>>>>>> @@ -20,6 +22,7 @@
>>>>>>>>>>      #include <soc/tegra/pmc.h>
>>>>>>>>>>        #include "clk.h"
>>>>>>>>>> +#include "clk-dfll.h"
>>>>>>>>>>      #include "clk-id.h"
>>>>>>>>>>        /*
>>>>>>>>>> @@ -225,6 +228,7 @@
>>>>>>>>>>        #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>>>>>>      #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>>>>>>        #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>>>>>>      #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>>>>>>          struct tegra_clk_pll_freq_table *fentry;
>>>>>>>>>>          struct tegra_clk_pll pllu;
>>>>>>>>>>          u32 reg;
>>>>>>>>>> +    int ret;
>>>>>>>>>>            for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>>>>>> fentry++) {
>>>>>>>>>>              if (fentry->input_rate == pll_ref_freq)
>>>>>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>>>>>>          fence_udelay(1, clk_base);
>>>>>>>>>>          reg |= PLL_ENABLE;
>>>>>>>>>>          writel(reg, clk_base + PLLU_BASE);
>>>>>>>>>> +    fence_udelay(1, clk_base);
>>>>>>>>>>      -    readl_relaxed_poll_timeout_atomic(clk_base +
>>>>>>>>>> PLLU_BASE, reg,
>>>>>>>>>> -                      reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>>>>> -    if (!(reg & PLL_BASE_LOCK)) {
>>>>>>>>>> +    ret = tegra210_wait_for_mask(&pllu, PLLU_BASE,
>>>>>>>>>> PLL_BASE_LOCK);
>>>>>>>>>> +    if (ret) {
>>>>>>>>>>              pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>>>>>>              return -ETIMEDOUT;
>>>>>>>>>>          }
>>>>>>>>>> @@ -3283,6 +3288,103 @@ static void
>>>>>>>>>> tegra210_disable_cpu_clock(u32 cpu)
>>>>>>>>>>      }
>>>>>>>>>>        #ifdef CONFIG_PM_SLEEP
>>>>>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>>>>>> +static struct platform_device *dfll_pdev;
>>>>>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base +
>>>>>>>>>> (_base) + ((_off) * 4))
>>>>>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>>>>>> +        writel_relaxed(_val, clk_base + (_base) + ((_off) *
>>>>>>>>>> 4)) +
>>>>>>>>>> +static int tegra210_clk_suspend(void)
>>>>>>>>>> +{
>>>>>>>>>> +    unsigned int i;
>>>>>>>>>> +    struct device_node *node;
>>>>>>>>>> +
>>>>>>>>>> +    tegra_cclkg_burst_policy_save_context();
>>>>>>>>>> +
>>>>>>>>>> +    if (!dfll_pdev) {
>>>>>>>>>> +        node = of_find_compatible_node(NULL, NULL,
>>>>>>>>>> +                           "nvidia,tegra210-dfll");
>>>>>>>>>> +        if (node)
>>>>>>>>>> +            dfll_pdev = of_find_device_by_node(node);
>>>>>>>>>> +
>>>>>>>>>> +        of_node_put(node);
>>>>>>>>>> +        if (!dfll_pdev)
>>>>>>>>>> +            pr_err("dfll node not found. no suspend for
>>>>>>>>>> dfll\n");
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    if (dfll_pdev)
>>>>>>>>>> +        tegra_dfll_suspend(dfll_pdev);
>>>>>>>>>> +
>>>>>>>>>> +    /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>>>>>> +    tegra_clk_set_pllp_out_cpu(true);
>>>>>>>>>> +
>>>>>>>>>> +    tegra_sclk_cclklp_burst_policy_save_context();
>>>>>>>>>> +
>>>>>>>>>> +    clk_save_context();
>>>>>>>>>> +
>>>>>>>>>> +    for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>>> +        cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>>>>>> +
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void tegra210_clk_resume(void)
>>>>>>>>>> +{
>>>>>>>>>> +    unsigned int i;
>>>>>>>>>> +    struct clk_hw *parent;
>>>>>>>>>> +    struct clk *clk;
>>>>>>>>>> +
>>>>>>>>>> +    /*
>>>>>>>>>> +     * clk_restore_context restores clocks as per the clock
>>>>>>>>>> tree.
>>>>>>>>>> +     *
>>>>>>>>>> +     * dfllCPU_out is first in the clock tree to get
>>>>>>>>>> restored and it
>>>>>>>>>> +     * involves programming DFLL controller along with
>>>>>>>>>> restoring CPUG
>>>>>>>>>> +     * clock burst policy.
>>>>>>>>>> +     *
>>>>>>>>>> +     * DFLL programming needs dfll_ref and dfll_soc
>>>>>>>>>> peripheral clocks
>>>>>>>>>> +     * to be restores which are part ofthe peripheral
>>>>>>>>>> clocks.
>>>>>>>                                                ^ white-space
>>>>>>>
>>>>>>> Please use spellchecker to avoid typos.
>>>>>>>>>> +     * So, peripheral clocks restore should happen prior to
>>>>>>>>>> dfll clock
>>>>>>>>>> +     * restore.
>>>>>>>>>> +     */
>>>>>>>>>> +
>>>>>>>>>> +    tegra_clk_osc_resume(clk_base);
>>>>>>>>>> +    for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>>> +        car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>>>>>> +
>>>>>>>>>> +    /* restore all plls and peripheral clocks */
>>>>>>>>>> +    tegra210_init_pllu();
>>>>>>>>>> +    clk_restore_context();
>>>>>>>>>> +
>>>>>>>>>> +    fence_udelay(5, clk_base);
>>>>>>>>>> +
>>>>>>>>>> +    /* resume SCLK and CPULP clocks */
>>>>>>>>>> +    tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>>>>>> +
>>>>>>>>>> +    /*
>>>>>>>>>> +     * restore CPUG clocks:
>>>>>>>>>> +     * - enable DFLL in open loop mode
>>>>>>>>>> +     * - switch CPUG to DFLL clock source
>>>>>>>>>> +     * - close DFLL loop
>>>>>>>>>> +     * - sync PLLX state
>>>>>>>>>> +     */
>>>>>>>>>> +    if (dfll_pdev)
>>>>>>>>>> +        tegra_dfll_resume(dfll_pdev, false);
>>>>>>>>>> +
>>>>>>>>>> +    tegra_cclkg_burst_policy_restore_context();
>>>>>>>>>> +    fence_udelay(2, clk_base);
>>>>>>>>>> +
>>>>>>>>>> +    if (dfll_pdev)
>>>>>>>>>> +        tegra_dfll_resume(dfll_pdev, true);
>>>>>>>>>> +
>>>>>>>>>> +    parent =
>>>>>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>>>>>> +    clk = clks[TEGRA210_CLK_PLL_X];
>>>>>>>>>> +    if (parent != __clk_get_hw(clk))
>>>>>>>>>> +        tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>>>>>> +
>>>>>>>>>> +    /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>>>>>> +    tegra_clk_set_pllp_out_cpu(false);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>      static void tegra210_cpu_clock_suspend(void)
>>>>>>>>>>      {
>>>>>>>>>>          /* switch coresite to clk_m, save off original source
>>>>>>>>>> */ @@ -3298,6 +3400,11 @@ static void
>>>>>>>>>> tegra210_cpu_clock_resume(void) }
>>>>>>>>>>      #endif
>>>>>>>>>>      +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>>>>>> +    .suspend = tegra210_clk_suspend,
>>>>>>>>>> +    .resume = tegra210_clk_resume,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>      static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>>>>>>          .wait_for_reset    = tegra210_wait_cpu_in_reset,
>>>>>>>>>>          .disable_clock    = tegra210_disable_cpu_clock,
>>>>>>>>>> @@ -3583,5 +3690,7 @@ static void __init
>>>>>>>>>> tegra210_clock_init(struct device_node *np)
>>>>>>>>>>          tegra210_mbist_clk_init();
>>>>>>>>>>            tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>>>>>> +
>>>>>>>>>> +    register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>>>>>>      }
>>>>>>>>> Is it really worthwhile to use syscore_ops for suspend/resume
>>>>>>>>> given that drivers for
>>>>>>>>> won't resume before the CLK driver anyway? Are there any other
>>>>>>>>> options for CLK
>>>>>>>>> suspend/resume?
>>>>>>>>>
>>>>>>>>> I'm also not sure whether PM runtime API could be used at all
>>>>>>>>> in the context of
>>>>>>>>> syscore_ops ..
>>>>>>>>>
>>>>>>>>> Secondly, what about to use generic clk_save_context() /
>>>>>>>>> clk_restore_context()
>>>>>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>>>>>> essential (and proper)
>>>>>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the
>>>>>>>>> clocks using the generic
>>>>>>>>> CLK Framework API.
>>>>>>>> Clock resume should happen very early to restore peripheral and
>>>>>>>> cpu clocks very early than peripheral drivers resume happens.
>>>>>>> If all peripheral drivers properly requested all of the
>>>>>>> necessary clocks and CLK driver was a platform driver, then I
>>>>>>> guess the probe should have been naturally ordered. But that's
>>>>>>> not very achievable with the currently available infrastructure
>>>>>>> in the kernel, so I'm not arguing that the clocks should be
>>>>>>> explicitly resumed before the users.
>>>>>>>> this patch series uses clk_save_context and clk_restore_context
>>>>>>>> for corresponding divider, pll, pllout.. save and restore
>>>>>>>> context.
>>>>>>> Now I see that indeed this API is utilized in this patch, thank
>>>>>>> you for the clarification.
>>>>>>>> But as there is dependency on dfll resume and cpu and pllx
>>>>>>>> clocks restore, couldnt use clk_save_context and
>>>>>>>> clk_restore_context for dfll.
>>>>>>>>
>>>>>>>> So implemented recommended dfll resume sequence in main
>>>>>>>> Tegra210 clock driver along with invoking
>>>>>>>> clk_save_context/clk_restore_context where all other clocks
>>>>>>>> save/restore happens as per clock tree traversal.
>>>>>>> Could you please clarify what part of peripherals clocks is
>>>>>>> required for DFLL's restore? Couldn't DFLL driver be changed to
>>>>>>> avoid that quirkness and thus to make DFLL driver suspend/resume
>>>>>>> the clock?
>>>>>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>>>>>
>>>>>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>>>>>> dfll_ref and dfll_soc peripheral clocks are not resumed by the
>>>>>> time dfll resume happens first.
>>>>>>
>>>>>> ref_clk and soc_clk source is from pll_p and clock tree has these
>>>>>> registered under pll_p which happens later.
>>>>>>
>>>>>> tegra210_clock_init registers in order plls, peripheral clocks,
>>>>>> super_clk init for cclk_g during clock driver probe and dfll
>>>>>> probe and register happens later.
>>>>> One more thing, CLDVFS peripheral clock enable is also needed to be
>>>>> enabled to program DFLL Controller and all peripheral clock
>>>>> context is restored only after their PLL sources are restored.
>>>>>
>>>>> DFLL restore involves dfll source clock resume along with CLDVFS
>>>>> periheral clock enable and reset
>>>> I don't quite see why you can't simply add suspend/resume callbacks
>>>> to the CPUFreq driver to:
>>>>
>>>> On suspend:
>>>> 1. Switch CPU to PLLP (or whatever "safe" parent)
>>>> 2. Disable/teardown DFLL
>>>>
>>>> On resume:
>>>> 1. Enable/restore DFLL
>>>> 2. Switch CPU back to DFLL
>>> dfll runtime suspend/resume are already part of dfll_pm_ops. Don't we
>>> want to use it for suspend/resume as well?
>> Looks like no. Seems runtime PM of that driver is intended solely for
>> the DFLL's clk management.
>>
>>> currently no APIs are shared b/w clk/tegra driver and CPUFreq driver
>>> to invoke dfll suspend/resume in CPUFreq driver
>>>
>> Just add it. Also, please note that CPUFreq driver is optional and thus
>> you may need to switch CPU to a safe parent on clk-core suspend as
>> well in order to resume properly if CPU was running off unsafe parent
>> during boot and CPUFreq driver is disabled in kernel build (or failed
>> to load).
> 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?

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.

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 part looks good to me. BTW, change the patch subject to "Add 
suspend-resume support" seems more appropriate to me.

Thanks.

Powered by blists - more mailing lists