Re: [PATCH V2 4/8] ARM: tegra: add common LP1 suspend support

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 08/05/2013 05:21 AM, Joseph Lo wrote:
> The LP1 suspending mode on Tegra means CPU rail off, devices and PLLs are
> clock gated and SDRAM in self-refresh mode. That means the low level LP1
> suspending and resuming code couldn't be run on DRAM and the CPU must
> switch to the always on clock domain (a.k.a. CLK_M 12MHz oscillator). And
> the system clock (SCLK) would be switched to CLK_S, a 32KHz oscillator.
> The LP1 low level handling code need to be moved to IRAM area first. And
> marking the LP1 mask for indicating the Tegra device is in LP1. The CPU
> power timer needs to be re-calculated based on 32KHz that was originally
> based on PCLK.
> 
> When resuming from LP1, the LP1 reset handler will resume PLLs and then
> put DRAM to normal mode. Then jumping to the "tegra_resume" that will
> restore full context before back to kernel. The "tegra_resume" handler
> was expected to be found in PMC_SCRATCH41 register.
> 
> This is common LP1 procedures for Tegra, so we do these jobs mainly in
> this patch:
> * moving LP1 low level handling code to IRAM
> * marking LP1 mask
> * copying the physical address of "tegra_resume" to PMC_SCRATCH41
> * re-calculate the CPU power timer based on 32KHz

> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c

> +static void __iomem *iram_code = IO_ADDRESS(TEGRA_IRAM_CODE_AREA);

I repeat: Why store that value in a global variable? It's constant.

> @@ -174,14 +181,75 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
>  				enum tegra_suspend_mode mode)
>  {
>  	/*
> -	 * The Tegra devices only support suspending to LP2 currently.
> +	 * The Tegra devices support suspending to LP1 or lower currently.
>  	 */
> -	if (mode > TEGRA_SUSPEND_LP2)
> -		return TEGRA_SUSPEND_LP2;
> +	if (mode > TEGRA_SUSPEND_LP1)
> +		return TEGRA_SUSPEND_LP1;
>  
>  	return mode;
>  }

I think that change needs to happen after patch 7. At this point in the
series, LP1 doesn't work on any chip. After patch 7, it will work on all
chips.

If you don't make that change, you should move this change into patch 5,
but only enable LP1 for Tegra30, then make patch 6 also enable it for
Tegra20, then make patch 7 also enable it for Tegra114. That's a lot
more complex, so just moving the change above into a new patch after
patch 7 seems better.

Note: You must assume that the DT changes are all checked in before any
of the code changes, so the code needs to work correctly even if the DT
contains the data that allows usage of LP1 before the driver actually
implements LP1.