| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7d101ec9-c559-8b40-1764-6bf67a9c7a7a@nvidia.com>
Date: Fri, 9 Aug 2019 09:19:19 -0700
From: Sowjanya Komatineni <skomatineni@...dia.com>
To: Dmitry Osipenko <digetx@...il.com>, <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>
CC: <pdeschrijver@...dia.com>, <pgaikwad@...dia.com>,
<sboyd@...nel.org>, <linux-clk@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <jckuo@...dia.com>,
<josephl@...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>, <rjw@...ysocki.net>,
<viresh.kumar@...aro.org>, <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v8 14/21] clk: tegra210: Add suspend and resume support
On 8/9/19 6:56 AM, Dmitry Osipenko wrote:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch adds support for clk: tegra210: suspend-resume.
>>
>> All the CAR controller settings are lost on suspend when core
>> power goes off.
>>
>> This patch has implementation for saving and restoring all PLLs
>> and clocks context during system suspend and resume to have the
>> clocks back to same state for normal operation.
>>
>> Clock driver suspend and resume are registered as syscore_ops as clocks
>> restore need to happen before the other drivers resume to have all their
>> clocks back to the same state as before suspend.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@...dia.com>
>> ---
>> drivers/clk/tegra/clk-tegra210.c | 103 +++++++++++++++++++++++++++++++++++++--
>> drivers/clk/tegra/clk.c | 64 ++++++++++++++++++++++++
>> drivers/clk/tegra/clk.h | 3 ++
>> 3 files changed, 166 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
>> index 998bf60b219a..8dd6f4f4debb 100644
>> --- a/drivers/clk/tegra/clk-tegra210.c
>> +++ b/drivers/clk/tegra/clk-tegra210.c
>> @@ -9,13 +9,13 @@
>> #include <linux/clkdev.h>
>> #include <linux/of.h>
>> #include <linux/of_address.h>
>> +#include <linux/syscore_ops.h>
>> #include <linux/delay.h>
>> #include <linux/export.h>
>> #include <linux/mutex.h>
>> #include <linux/clk/tegra.h>
>> #include <dt-bindings/clock/tegra210-car.h>
>> #include <dt-bindings/reset/tegra210-car.h>
>> -#include <linux/iopoll.h>
>> #include <linux/sizes.h>
>> #include <soc/tegra/pmc.h>
>>
>> @@ -220,11 +220,15 @@
>> #define CLK_M_DIVISOR_SHIFT 2
>> #define CLK_M_DIVISOR_MASK 0x3
>>
>> +#define CLK_MASK_ARM 0x44
>> +#define MISC_CLK_ENB 0x48
>> +
>> #define RST_DFLL_DVCO 0x2f4
>> #define DVFS_DFLL_RESET_SHIFT 0
>>
>> #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
>> @@ -2825,6 +2829,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)
>> @@ -2853,9 +2858,14 @@ static int tegra210_enable_pllu(void)
>> reg |= PLL_ENABLE;
>> writel(reg, clk_base + PLLU_BASE);
>>
>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>> - reg & PLL_BASE_LOCK, 2, 1000);
>> - if (!(reg & PLL_BASE_LOCK)) {
>> + /*
>> + * During clocks resume, same PLLU init and enable sequence get
>> + * executed. So, readx_poll_timeout_atomic can't be used here as it
>> + * uses ktime_get() and timekeeping resume doesn't happen by that
>> + * time. So, using tegra210_wait_for_mask for PLL 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;
>> }
>> @@ -3288,6 +3298,84 @@ static void tegra210_disable_cpu_clock(u32 cpu)
>> }
>>
>> #ifdef CONFIG_PM_SLEEP
>> +/*
>> + * This array lists mask values for each peripheral clk bank
>> + * to mask out reserved bits during the clocks state restore
>> + * on SC7 resume to prevent accidental writes to these reserved
>> + * bits.
>> + */
>> +static u32 periph_clk_rsvd_mask[TEGRA210_CAR_BANK_COUNT] = {
> Should be more natural to have a "valid_mask" instead of "rsvd_mask".
>
> What's actually wrong with touching of the reserved bits? They must be NO-OP.. or the
> reserved bits are actually some kind of "secret" bits? If those bits have some use-case
> outside of Silicon HW (like FPGA simulation), then this doesn't matter for upstream and you
> have to keep the workaround locally in the downstream kernel or whatever.
Will rename as valid_mask.
some bits in these registers are undefined and is not good to write to
these bits as they can cause pslverr.
>
>> + 0x23282006,
>> + 0x782e0c18,
>> + 0x0c012c05,
>> + 0x003e7304,
>> + 0x86c04800,
>> + 0xc0199000,
>> + 0x03e03800,
>> +};
>> +
>> +#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 u32 spare_reg_ctx, misc_clk_enb_ctx, clk_msk_arm_ctx;
>> +static u32 cpu_softrst_ctx[3];
>> +
>> +static int tegra210_clk_suspend(void)
>> +{
>> + unsigned int i;
>> +
>> + clk_save_context();
>> +
>> + /*
>> + * Save the bootloader configured clock registers SPARE_REG0,
>> + * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL.
>> + */
>> + spare_reg_ctx = readl_relaxed(clk_base + SPARE_REG0);
>> + misc_clk_enb_ctx = readl_relaxed(clk_base + MISC_CLK_ENB);
>> + clk_msk_arm_ctx = readl_relaxed(clk_base + CLK_MASK_ARM);
>> +
>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>> +
>> + tegra_clk_periph_suspend();
>> + return 0;
>> +}
>> +
>> +static void tegra210_clk_resume(void)
>> +{
>> + unsigned int i;
>> +
>> + tegra_clk_osc_resume(clk_base);
>> +
>> + /*
>> + * Restore the bootloader configured clock registers SPARE_REG0,
>> + * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL from saved context.
>> + */
>> + writel_relaxed(spare_reg_ctx, clk_base + SPARE_REG0);
>> + writel_relaxed(misc_clk_enb_ctx, clk_base + MISC_CLK_ENB);
>> + writel_relaxed(clk_msk_arm_ctx, clk_base + CLK_MASK_ARM);
>> +
>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>> +
>> + fence_udelay(5, clk_base);
>> +
>> + /* enable all the clocks before changing the clock sources */
>> + tegra_clk_periph_force_on(periph_clk_rsvd_mask);
> Why clocks need to be enabled before changing the sources?
To prevent glitchless frequency switch, Tegra clock programming
recommended sequence is to change MUX control or divisor or both with
the clocks running.
Actual state of clocks before suspend are restored later after all PLL's
and peripheral clocks are restored.
>
>> + /* wait for all writes to happen to have all the clocks enabled */
>> + wmb();
> fence_udelay() has exactly the same barrier at the very beginning of readl(), no need to
> duplicate it here.
>
>> + fence_udelay(2, clk_base);
>> +
>> + /* restore PLLs and all peripheral clock rates */
>> + tegra210_init_pllu();
> Why USB PLL need to be restored at first?
USB PLL restore is independent to all other clocks restore. So this can
be done either before clk_restore_context or even after.
>
>> + clk_restore_context();
>> +
>> + /* restore all peripheral clocks enable and reset state */
>> + tegra_clk_periph_resume();
>> +}
> [snip]
Powered by blists - more mailing lists