Message-ID: <20190619081541.GA3187@ulmo>
Date:   Wed, 19 Jun 2019 10:15:41 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Sowjanya Komatineni <skomatineni@...dia.com>
Cc:     jonathanh@...dia.com, tglx@...utronix.de, jason@...edaemon.net,
        marc.zyngier@....com, linus.walleij@...aro.org, stefan@...er.ch,
        mark.rutland@....com, pdeschrijver@...dia.com, pgaikwad@...dia.com,
        sboyd@...nel.org, linux-clk@...r.kernel.org,
        linux-gpio@...r.kernel.org, jckuo@...dia.com, 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, digetx@...il.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH V3 11/17] clk: tegra210: support for Tegra210 clocks
 suspend and resume

On Tue, Jun 18, 2019 at 10:58:40AM -0700, Sowjanya Komatineni wrote:
> 
> On 6/18/19 5:16 AM, Thierry Reding wrote:
> > On Tue, Jun 18, 2019 at 12:46:25AM -0700, Sowjanya Komatineni wrote:
> > > This patch adds system suspend and resume support for Tegra210
> > > clocks.
> > > 
> > > All the CAR controller settings are lost on suspend when core power
> > > goes off.
> > > 
> > > This patch has implementation for saving and restoring all the PLLs
> > > and clocks context during system suspend and resume to have the
> > > system back to operating state.
> > > 
> > > Signed-off-by: Sowjanya Komatineni <skomatineni@...dia.com>
> > > ---
> > >   drivers/clk/tegra/clk-tegra210.c | 218 +++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 211 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
> > > index e1ba62d2b1a0..c34d92e871f4 100644
> > > --- a/drivers/clk/tegra/clk-tegra210.c
> > > +++ b/drivers/clk/tegra/clk-tegra210.c
> > > @@ -9,10 +9,12 @@
> > >   #include <linux/clkdev.h>
> > >   #include <linux/of.h>
> > >   #include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/export.h>
> > >   #include <linux/mutex.h>
> > >   #include <linux/clk/tegra.h>
> > > +#include <linux/syscore_ops.h>
> > >   #include <dt-bindings/clock/tegra210-car.h>
> > >   #include <dt-bindings/reset/tegra210-car.h>
> > >   #include <linux/iopoll.h>
> > > @@ -20,6 +22,7 @@
> > >   #include <soc/tegra/pmc.h>
> > >   #include "clk.h"
> > > +#include "clk-dfll.h"
> > >   #include "clk-id.h"
> > >   /*
> > > @@ -36,6 +39,8 @@
> > >   #define CLK_SOURCE_LA 0x1f8
> > >   #define CLK_SOURCE_SDMMC2 0x154
> > >   #define CLK_SOURCE_SDMMC4 0x164
> > > +#define CLK_OUT_ENB_Y 0x298
> > > +#define CLK_ENB_PLLP_OUT_CPU BIT(31)
> > >   #define PLLC_BASE 0x80
> > >   #define PLLC_OUT 0x84
> > > @@ -225,6 +230,7 @@
> > >   #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
> > >   #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
> > > +#define CPU_SOFTRST_CTRL 0x380
> > >   #define LVL2_CLK_GATE_OVRA 0xf8
> > >   #define LVL2_CLK_GATE_OVRC 0x3a0
> > > @@ -2820,6 +2826,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)
> > > @@ -2836,7 +2843,7 @@ static int tegra210_enable_pllu(void)
> > >   	reg = readl_relaxed(clk_base + pllu.params->ext_misc_reg[0]);
> > >   	reg &= ~BIT(pllu.params->iddq_bit_idx);
> > >   	writel_relaxed(reg, clk_base + pllu.params->ext_misc_reg[0]);
> > > -	udelay(5);
> > > +	fence_udelay(5, clk_base);
> > >   	reg = readl_relaxed(clk_base + PLLU_BASE);
> > >   	reg &= ~GENMASK(20, 0);
> > > @@ -2844,13 +2851,13 @@ static int tegra210_enable_pllu(void)
> > >   	reg |= fentry->n << 8;
> > >   	reg |= fentry->p << 16;
> > >   	writel(reg, clk_base + PLLU_BASE);
> > > -	udelay(1);
> > > +	fence_udelay(1, clk_base);
> > These udelay() -> fence_udelay() seem like they should be a separate
> > patch.
> > 
> > >   	reg |= PLL_ENABLE;
> > >   	writel(reg, clk_base + PLLU_BASE);
> > > +	fence_udelay(1, clk_base);
> > > -	readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
> > > -					  reg & PLL_BASE_LOCK, 2, 1000);
> > > -	if (!(reg & PLL_BASE_LOCK)) {
> > > +	ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
> > > +	if (ret) {
> > >   		pr_err("Timed out waiting for PLL_U to lock\n");
> > >   		return -ETIMEDOUT;
> > >   	}
> > > @@ -2890,12 +2897,12 @@ static int tegra210_init_pllu(void)
> > >   		reg = readl_relaxed(clk_base + XUSB_PLL_CFG0);
> > >   		reg &= ~XUSB_PLL_CFG0_PLLU_LOCK_DLY_MASK;
> > >   		writel_relaxed(reg, clk_base + XUSB_PLL_CFG0);
> > > -		udelay(1);
> > > +		fence_udelay(1, clk_base);
> > >   		reg = readl_relaxed(clk_base + PLLU_HW_PWRDN_CFG0);
> > >   		reg |= PLLU_HW_PWRDN_CFG0_SEQ_ENABLE;
> > >   		writel_relaxed(reg, clk_base + PLLU_HW_PWRDN_CFG0);
> > > -		udelay(1);
> > > +		fence_udelay(1, clk_base);
> > >   		reg = readl_relaxed(clk_base + PLLU_BASE);
> > >   		reg &= ~PLLU_BASE_CLKENABLE_USB;
> > > @@ -3282,6 +3289,188 @@ static void tegra210_disable_cpu_clock(u32 cpu)
> > >   }
> > >   #ifdef CONFIG_PM_SLEEP
> > > +static u32 cpu_softrst_ctx[3];
> > > +static struct platform_device *dfll_pdev;
> > > +static u32 *periph_clk_src_ctx;
> > > +struct periph_source_bank {
> > Blank line between the above two.
> > 
> > > +	u32 start;
> > > +	u32 end;
> > > +};
> > > +
> > > +static struct periph_source_bank periph_srcs[] = {
> > > +	[0] = {
> > > +		.start = 0x100,
> > > +		.end = 0x198,
> > > +	},
> > > +	[1] = {
> > > +		.start = 0x1a0,
> > > +		.end = 0x1f8,
> > > +	},
> > > +	[2] = {
> > > +		.start = 0x3b4,
> > > +		.end = 0x42c,
> > > +	},
> > > +	[3] = {
> > > +		.start = 0x49c,
> > > +		.end = 0x4b4,
> > > +	},
> > > +	[4] = {
> > > +		.start = 0x560,
> > > +		.end = 0x564,
> > > +	},
> > > +	[5] = {
> > > +		.start = 0x600,
> > > +		.end = 0x678,
> > > +	},
> > > +	[6] = {
> > > +		.start = 0x694,
> > > +		.end = 0x6a0,
> > > +	},
> > > +	[7] = {
> > > +		.start = 0x6b8,
> > > +		.end = 0x718,
> > > +	},
> > > +};
> > > +
> > > +/* This array lists the valid clocks for each periph clk bank */
> > > +static u32 periph_clks_on[] = {
> > > +	0xdcd7dff9,
> > > +	0x87d1f3e7,
> > > +	0xf3fed3fa,
> > > +	0xffc18cfb,
> > > +	0x793fb7ff,
> > > +	0x3fe66fff,
> > > +	0xfc1fc7ff,
> > > +};
> > Hm... this is a bunch of magic. Perhaps replace this by a list of the
> > clock IDs? That's perhaps a little more verbose, but if we ever need to
> > tweak the list of IDs in that periph_clks_on array, that'll be quite the
> > challenge.
> > 
> > Also, is this list a "guess" or are these all guaranteed to be always
> > on? What if some of these ended up getting disabled as part of suspend
> > already (by their users). If we force them on, won't their references
> > become unbalanced if the driver later enables them again on resume?
> 
> Yes, will replace with list of peripheral clock names..
> 
> This list is not a guess. Each entry of this list maps to CLK_ENB set
> register.
> 
> Total 7 registers are available and each bit of these registers is for
> enable/disable clock to corresponding peripheral.
> 
> Some of the bits are off as those peripheral clocks don't need to be enabled
> as we are not changing source or not using them like MIPIBIF, PLLG_REF..
> 
> This list of peripheral clocks are enabled during resume before changing
> clock sources and after clock source update, they are restored back to the
> state they were before suspend. So their references don't become unbalanced.

Okay, good. Can you maybe put a version of that explanation in a comment
on top of the periph_clks_on declaration? And perhaps also describe this
in the commit message.

Or maybe even better, add some comments in the main suspend/resume paths
to sort of "guide" through what's happening.

> > > +
> > > +static struct platform_device *dfll_pdev;
> > I think you already predeclared this one above.
> > 
> > > +#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 * __init tegra210_init_suspend_ctx(void)
> > > +{
> > > +	int i, size = 0;
> > Can both be unsigned int.
> > 
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(periph_srcs); i++)
> > > +		size += periph_srcs[i].end - periph_srcs[i].start + 4;
> > > +
> > > +	periph_clk_src_ctx = kmalloc(size, GFP_KERNEL);
> > > +
> > > +	return periph_clk_src_ctx;
> > It's somewhat wasteful to return a global variable since you can access
> > it anyway. Perhaps it'd be more useful to make the function return a
> > boolean?
> > 
> > > +}
> > > +
> > > +static int tegra210_clk_suspend(void)
> > > +{
> > > +	int i;
> > unsigned int.
> > 
> > > +	unsigned long off;
> > > +	struct device_node *node;
> > > +	u32 *clk_rst_ctx = periph_clk_src_ctx;
> > > +	u32 val;
> > > +
> > > +	tegra_cclkg_burst_policy_save_context();
> > > +
> > > +	if (!dfll_pdev) {
> > > +		node = of_find_compatible_node(NULL, NULL,
> > > +					       "nvidia,tegra210-dfll");
> > > +		if (node)
> > > +			dfll_pdev = of_find_device_by_node(node);
> > > +		of_node_put(node);
> > > +		if (!dfll_pdev)
> > > +			pr_err("dfll node not found. no suspend for dfll\n");
> > > +	}
> > Wouldn't it make sense to run this only once, perhaps as part of
> > tegra210_init_suspend_ctx()?
> > 
> > > +
> > > +	if (dfll_pdev)
> > > +		tegra_dfll_suspend(dfll_pdev);
> > > +
> > > +	/* Enable PLLP_OUT_CPU after dfll suspend */
> > > +	val = car_readl(CLK_OUT_ENB_Y, 0);
> > > +	val |= CLK_ENB_PLLP_OUT_CPU;
> > > +	car_writel(val, CLK_OUT_ENB_Y, 0);
> > > +
> > > +	tegra_clk_periph_suspend(clk_base);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(periph_srcs); i++)
> > > +		for (off = periph_srcs[i].start; off <= periph_srcs[i].end;
> > > +		     off += 4)
> > > +			*clk_rst_ctx++ = car_readl(off, 0);
> > > +
> > > +	tegra_sclk_cclklp_burst_policy_save_context();
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> > > +		cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
> > > +
> > > +	clk_save_context();
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void tegra210_clk_resume(void)
> > > +{
> > > +	int i;
> > > +	unsigned long off;
> > > +	u32 val;
> > > +	u32 *clk_rst_ctx = periph_clk_src_ctx;
> > > +	struct clk_hw *parent;
> > > +	struct clk *clk;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> > > +		car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
> > > +
> > > +	tegra_clk_osc_resume(clk_base);
> > > +
> > > +	/*
> > > +	 * restore all the plls before configuring clocks and resetting
> > > +	 * the devices.
> > > +	 */
> > > +	tegra210_init_pllu();
> > > +	tegra_sclk_cpulp_burst_policy_restore_context();
> > > +	clk_restore_context();
> > > +
> > > +	/* enable all clocks before configuring clock sources */
> > > +	tegra_clk_periph_force_on(periph_clks_on, ARRAY_SIZE(periph_clks_on),
> > > +				  clk_base);
> > > +	/* wait for all writes to happen to have all the clocks enabled */
> > > +	wmb();
> > > +	fence_udelay(2, clk_base);
> > > +
> > > +	/* restore all the devices clock sources */
> > > +	for (i = 0; i < ARRAY_SIZE(periph_srcs); i++)
> > > +		for (off = periph_srcs[i].start; off <= periph_srcs[i].end;
> > > +		     off += 4)
> > > +			car_writel(*clk_rst_ctx++, off, 0);
> > > +
> > > +	/* propagate and restore resets, restore clock state */
> > > +	fence_udelay(5, clk_base);
> > > +	tegra_clk_periph_resume(clk_base);
> > > +
> > > +	/*
> > > +	 * restore CPUG clocks:
> > > +	 * - enable DFLL in open loop mode
> > > +	 * - switch CPUG to DFLL clock source
> > > +	 * - close DFLL loop
> > > +	 * - sync PLLX state
> > > +	 */
> > > +	if (dfll_pdev)
> > > +		tegra_dfll_resume(dfll_pdev, false);
> > > +
> > > +	tegra_cclkg_burst_policy_restore_context();
> > > +	fence_udelay(2, clk_base);
> > > +
> > > +	if (dfll_pdev)
> > > +		tegra_dfll_resume(dfll_pdev, true);
> > > +
> > > +	parent = clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
> > > +	clk = clks[TEGRA210_CLK_PLL_X];
> > > +	if (parent != __clk_get_hw(clk))
> > > +		tegra_clk_sync_state_pll(__clk_get_hw(clk));
> > > +
> > > +	/* Disable PLL_OUT_CPU after DFLL resume */
> > > +	val = car_readl(CLK_OUT_ENB_Y, 0);
> > > +	val &= ~CLK_ENB_PLLP_OUT_CPU;
> > > +	car_writel(val, CLK_OUT_ENB_Y, 0);
> > > +}
> > I'm surprised by the amount of work that we need to do here. I had hoped
> > that the clock framework's save/restore infrastructure would be enough.
> > I suppose you do call clk_restore_context() somewhere in there, so maybe
> > this really is as good as it gets.
> > 
> > Thierry
> 
> Reason is there are dependencies b/w the clocks and DFLL resume and clocks
> resume order needed is not same as clock tree list.
> 
> during resume as per clock tree, CPU clock configs to use DFLL will happen
> first as its first in the clock tree but DFLL resume should be done prior to
> switching CPU to use from DFLL output.
> 
> To resume DFLL, peripheral clocks should be restored.
> 
> Considering these dependencies, performing peripheral and DFLL/CPU resume in
> Tegra210 clock driver rather than in corresponding peripheral clk_ops using
> save and restore context callback.

Okay makes sense. As mentioned above, I think it'd be great if you could
add more comments throughout the tegra210_clk_{suspend,resume}() code to
guide the reader through what you're doing, given that this is far from
obvious. You already do quite a bit of that, but it's perhaps better to
explain more what's going on and, perhaps more importantly, why. You're
currently mostly repeating the code sequence in the code. It'd be great
to have the general suspend/resume sequence detailed and highlight why
the sequence is the way it is and what the dependencies are, etc.

Thierry

> 
> > > +
> > >   static void tegra210_cpu_clock_suspend(void)
> > >   {
> > >   	/* switch coresite to clk_m, save off original source */
> > > @@ -3295,8 +3484,20 @@ static void tegra210_cpu_clock_resume(void)
> > >   	writel(tegra210_cpu_clk_sctx.clk_csite_src,
> > >   				clk_base + CLK_SOURCE_CSITE);
> > >   }
> > > +#else
> > > +#define tegra210_clk_suspend	NULL
> > > +#define tegra210_clk_resume	NULL
> > > +static inline u32 *tegra210_init_suspend_ctx(void)
> > > +{
> > > +	return NULL;
> > > +}
> > >   #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,
> > > @@ -3580,5 +3781,8 @@ static void __init tegra210_clock_init(struct device_node *np)
> > >   	tegra210_mbist_clk_init();
> > >   	tegra_cpu_car_ops = &tegra210_cpu_car_ops;
> > > +
> > > +	if (tegra210_init_suspend_ctx())
> > > +		register_syscore_ops(&tegra_clk_syscore_ops);
> > >   }
> > >   CLK_OF_DECLARE(tegra210, "nvidia,tegra210-car", tegra210_clock_init);
> > > -- 
> > > 2.7.4
> > > 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists