Re: [PATCH v8 14/21] clk: tegra210: Add suspend and resume support

From: Sowjanya Komatineni
Date: Sun Aug 11 2019 - 15:16:23 EST


On 8/11/19 10:39 AM, Dmitry Osipenko wrote:
09.08.2019 21:40, Sowjanya Komatineni ÐÐÑÐÑ:
On 8/9/19 11:18 AM, Dmitry Osipenko wrote:
09.08.2019 19:19, Sowjanya Komatineni ÐÐÑÐÑ:
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@xxxxxxxxxx>
---
 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.
Okay, it should be explained in the comment.

Is it possible to disable trapping of changing the undefined bits?
No its internal to design
Okay.

Also, what about to move the valid_mask into struct tegra_clk_periph_regs?

No, we cannot move to tegra_clk_periph_regs as its in tegra/clk.c and is common for all tegra.

Reserved bits are different on tegra chips so should come from Tegra chip specific clock driver like

clk-tegra210 for Tegra210.

+ÂÂÂ 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.
This should be explained in the comment.

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.
Actually, readl does the rmb() and it should be a more correct variant of fencing because it
actually ensures that the write reached hardware. I suppose that something like fence_udelay
should be used for the pinctrl as well.

+ÂÂÂ 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.
Then why not to implement restore_context for PLLU?
pllu is registered as fixed_rate clock and we using clk core clk_register_fixed_rate which
uses clk_fixed_rate_ops from the same generic clk-fixed-rate driver.

Also pllu init happens in the same clk-tegra210, so invoking it during resume which is the
same sequence needed during resume as well.
Okay.