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 <[email protected]>
---
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] = {
+ 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);
+
+ /* wait for all writes to happen to have all the clocks enabled */
+ wmb();
+ fence_udelay(2, clk_base);
+
+ /* restore PLLs and all peripheral clock rates */
+ tegra210_init_pllu();
+ clk_restore_context();
+
+ /* restore all peripheral clocks enable and reset state */
+ tegra_clk_periph_resume();
+}
+
static void tegra210_cpu_clock_suspend(void)
{
/* switch coresite to clk_m, save off original source */
@@ -3303,6 +3391,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,
@@ -3587,5 +3680,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);
}
CLK_OF_DECLARE(tegra210, "nvidia,tegra210-car", tegra210_clock_init);
diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
index eb08047fd02f..368a576132f6 100644
--- a/drivers/clk/tegra/clk.c
+++ b/drivers/clk/tegra/clk.c
@@ -67,6 +67,7 @@ struct tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops;
int *periph_clk_enb_refcnt;
static int periph_banks;
+static u32 *periph_state_ctx;
static struct clk **clks;
static int clk_num;
static struct clk_onecell_data clk_data;
@@ -213,6 +214,61 @@ void tegra_clk_set_pllp_out_cpu(bool enable)
writel_relaxed(val, clk_base + CLK_OUT_ENB_Y);
}
+void tegra_clk_periph_force_on(u32 *clks_rsvd_mask)
+{
+ unsigned int i;
+
+ for (i = 0; i < periph_banks; i++)
+ writel_relaxed(~clks_rsvd_mask[i],
+ clk_base + periph_regs[i].enb_reg);
+}
+
+void tegra_clk_periph_suspend(void)
+{
+ unsigned int i, idx;
+
+ idx = 0;
+ for (i = 0; i < periph_banks; i++, idx++)
+ periph_state_ctx[idx] =
+ readl_relaxed(clk_base + periph_regs[i].enb_reg);
+
+ for (i = 0; i < periph_banks; i++, idx++)
+ periph_state_ctx[idx] =
+ readl_relaxed(clk_base + periph_regs[i].rst_reg);
+}
+
+void tegra_clk_periph_resume(void)
+{
+ unsigned int i, idx;
+
+ idx = 0;
+ for (i = 0; i < periph_banks; i++, idx++)
+ writel_relaxed(periph_state_ctx[idx],
+ clk_base + periph_regs[i].enb_reg);
+ /*
+ * All non-boot peripherals will be in reset state on resume.
+ * Wait for 5us of reset propagation delay before de-asserting
+ * the peripherals based on the saved context.
+ */
+ fence_udelay(5, clk_base);
+
+ for (i = 0; i < periph_banks; i++, idx++)
+ writel_relaxed(periph_state_ctx[idx],
+ clk_base + periph_regs[i].rst_reg);
+
+ fence_udelay(2, clk_base);
+}
+
+static int tegra_clk_periph_ctx_init(int banks)
+{
+ periph_state_ctx = kcalloc(2 * banks, sizeof(*periph_state_ctx),
+ GFP_KERNEL);
+ if (!periph_state_ctx)
+ return -ENOMEM;
+
+ return 0;
+}
+
struct clk ** __init tegra_clk_init(void __iomem *regs, int num, int banks)
{
clk_base = regs;
@@ -234,6 +290,14 @@ struct clk ** __init tegra_clk_init(void __iomem *regs, int num, int banks)
clk_num = num;
+ if (IS_ENABLED(CONFIG_PM_SLEEP)) {
+ if (tegra_clk_periph_ctx_init(banks)) {
+ kfree(periph_clk_enb_refcnt);
+ kfree(clks);
+ return NULL;
+ }
+ }
+
return clks;
}
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 560e2bcb3d7d..9a17cad28d72 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -843,6 +843,9 @@ int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
u8 frac_width, u8 flags);
void tegra_clk_osc_resume(void __iomem *clk_base);
void tegra_clk_set_pllp_out_cpu(bool enable);
+void tegra_clk_periph_force_on(u32 *clks_rsvd_mask);
+void tegra_clk_periph_suspend(void);
+void tegra_clk_periph_resume(void);
/* Combined read fence with delay */
--
2.7.4
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 <[email protected]>
> ---
> 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.
> + 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?
> + /* 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?
> + clk_restore_context();
> +
> + /* restore all peripheral clocks enable and reset state */
> + tegra_clk_periph_resume();
> +}
[snip]
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 <[email protected]>
>> ---
>> 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]
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 <[email protected]>
>>> ---
>>> 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?
>>
>>> + 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?
>>> + clk_restore_context();
>>> +
>>> + /* restore all peripheral clocks enable and reset state */
>>> + tegra_clk_periph_resume();
>>> +}
>> [snip]
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 <[email protected]>
>>>>> ---
>>>>> 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?
>>>>> + 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.
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 <[email protected]>
>>>>>> ---
>>>>>> 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.
>
On Thu, Aug 08, 2019 at 04:46:53PM -0700, Sowjanya Komatineni wrote:
> 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 <[email protected]>
> ---
> 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(-)
Acked-by: Thierry Reding <[email protected]>
11.08.2019 22:15, Sowjanya Komatineni пишет:
>
> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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.
Could you please check whether the reserved bits are RAZ (read as zero)?
[snip]
On 8/12/19 9:25 AM, Dmitry Osipenko wrote:
> 11.08.2019 22:15, Sowjanya Komatineni пишет:
>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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.
> Could you please check whether the reserved bits are RAZ (read as zero)?
>
> [snip]
yes all reserved bits of clk_enb register is 0. This should not be set to 1.
As I will be changing to variable name to valid_mask instead of reserved
mask, will also change values to valid mask so it can be used directly
to write to clk_enb for enabling all peripherals clks.
12.08.2019 20:28, Sowjanya Komatineni пишет:
>
> On 8/12/19 9:25 AM, Dmitry Osipenko wrote:
>> 11.08.2019 22:15, Sowjanya Komatineni пишет:
>>> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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.
>> Could you please check whether the reserved bits are RAZ (read as zero)?
>>
>> [snip]
>
> yes all reserved bits of clk_enb register is 0. This should not be set to 1.
>
> As I will be changing to variable name to valid_mask instead of reserved mask, will also
> change values to valid mask so it can be used directly to write to clk_enb for enabling all
> peripherals clks.
>
It looks to me that the tegra_clk_periph_force_on() could be made local to the
clk-tegra210.c and then the raw clk_enb values could be written directly instead of having
the clk_enb[] array, probably that will be a bit cleaner.
On 8/12/19 11:19 AM, Dmitry Osipenko wrote:
> 12.08.2019 20:28, Sowjanya Komatineni пишет:
>> On 8/12/19 9:25 AM, Dmitry Osipenko wrote:
>>> 11.08.2019 22:15, Sowjanya Komatineni пишет:
>>>> 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 <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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.
>>> Could you please check whether the reserved bits are RAZ (read as zero)?
>>>
>>> [snip]
>> yes all reserved bits of clk_enb register is 0. This should not be set to 1.
>>
>> As I will be changing to variable name to valid_mask instead of reserved mask, will also
>> change values to valid mask so it can be used directly to write to clk_enb for enabling all
>> peripherals clks.
>>
> It looks to me that the tegra_clk_periph_force_on() could be made local to the
> clk-tegra210.c and then the raw clk_enb values could be written directly instead of having
> the clk_enb[] array, probably that will be a bit cleaner
All CLK_OUT_ENB* registers are already defined in clk driver and also
periph_regs includes all of these to use.
To write value to enable all clocks directly without array, it need
total 7 individual register writes for Tegra210. Also when
suspend/resume is implemented for other prior tegras, they need to do
same in tegra clock driver.
Reason I had this in clock driver is, this can be used by all tegra
clock drivers and just can pass valid clocks values.
But doing individual register write with direct hard code values in
corresponding tegra clock driver is preferred still, will update so in
next revision and will move all the CLK_OUT_ENB* register defines to
tegra/clk.h
Currently RST_DEVICES & CLK_OUT_ENB are all in tegra/clk.c
> .
12.08.2019 22:03, Sowjanya Komatineni пишет:
>
> On 8/12/19 11:19 AM, Dmitry Osipenko wrote:
>> 12.08.2019 20:28, Sowjanya Komatineni пишет:
>>> On 8/12/19 9:25 AM, Dmitry Osipenko wrote:
>>>> 11.08.2019 22:15, Sowjanya Komatineni пишет:
>>>>> 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 <[email protected]>
>>>>>>>>>>> ---
>>>>>>>>>>> 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.
>>>> Could you please check whether the reserved bits are RAZ (read as zero)?
>>>>
>>>> [snip]
>>> yes all reserved bits of clk_enb register is 0. This should not be set to 1.
>>>
>>> As I will be changing to variable name to valid_mask instead of reserved mask, will also
>>> change values to valid mask so it can be used directly to write to clk_enb for enabling all
>>> peripherals clks.
>>>
>> It looks to me that the tegra_clk_periph_force_on() could be made local to the
>> clk-tegra210.c and then the raw clk_enb values could be written directly instead of having
>> the clk_enb[] array, probably that will be a bit cleaner
>
> All CLK_OUT_ENB* registers are already defined in clk driver and also periph_regs includes
> all of these to use.
>
> To write value to enable all clocks directly without array, it need total 7 individual
> register writes for Tegra210. Also when suspend/resume is implemented for other prior
> tegras, they need to do same in tegra clock driver.
>
> Reason I had this in clock driver is, this can be used by all tegra clock drivers and just
> can pass valid clocks values.
>
> But doing individual register write with direct hard code values in corresponding tegra
> clock driver is preferred still, will update so in next revision and will move all the
> CLK_OUT_ENB* register defines to tegra/clk.h
>
> Currently RST_DEVICES & CLK_OUT_ENB are all in tegra/clk.c
Yes, it should be a bit more clear to share these defines. Also, please define the "valid"
bitmasks with something like TEGRA210_DEVICES_MASK_L.