On 07/26/2013 03:15 AM, Joseph Lo wrote: > The LP1 suspend mode will power off the CPU, clock gated the PLLs and put > SDRAM to self-refresh mode. Any interrupt can wake up device from LP1. The > sequence when LP1 suspending: > diff --git a/arch/arm/mach-tegra/pm-tegra30.c b/arch/arm/mach-tegra/pm-tegra30.c > +void tegra30_lp1_iram_hook(void) > +{ > + tegra30_lp1_iram.start_addr = &tegra30_iram_start; > + tegra30_lp1_iram.end_addr = &tegra30_iram_end; If you need to fill in the values in that struct dynamically anyway, why not make tegra_lp1_iram be a struct rather than a pointer, and write directly to it? That said, aren't tegra30_iram_start constants that the linker can work out, so I think you can just initialize the structure at compile-time, and save some code. > + tegra_lp1_iram = &tegra30_lp1_iram; > +} > + > +void tegra30_sleep_core_init(void) > +{ > + tegra_sleep_core_finish = tegra30_sleep_core_finish; > +} Is there a need to have separate iram_hook()/sleep_core_init() functions? Perhaps they can be combined into a single function for simplicity. > diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h > +#ifdef CONFIG_ARCH_TEGRA_3x_SOC > +void tegra30_lp1_iram_hook(void); > +void tegra30_sleep_core_init(void); > +#else > +static inline void tegra30_lp1_iram_hook(void) {} > +static inline void void tegra30_sleep_core_init(void) {} > +#endif It'd be nice to be consistent re: whether we define dummy static inlines, or use IS_ENABLED() at the call-site. IIRC, there's lots of use of IS_ENABLED() in the Tegra PM code now, so perhaps these patches should use that instead? > diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > +/* waits until the microsecond counter (base) ticks, for exact timing loops */ > +.macro wait_for_us, rd, base, tmp > + ldr \rd, [\base] > +1001: ldr \tmp, [\base] > + cmp \rd, \tmp > + beq 1001b > +.endm Doesn't this wait any amount of time from 0..1uS, and hence it actually /isn't/ very exact? > +/* waits until the microsecond counter (base) is > rn */ > +.macro wait_until, rn, base, tmp > + add \rn, \rn, #1 > +1002: ldr \tmp, [\base] > + cmp \tmp, \rn > + bmi 1002b > +.endm Parameter "rn" could be renamed to make its purpose clear from the name. How about target_us or wait_until_this_time? "wait_until" is also rather a generic name; wait_until_us would be much better, which would require renaming wait_until_us above, which I think might be better deleted, or renamed to wait_until_next_us? > diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S > +ENTRY(tegra30_sleep_core_finish) > + /* > + * Preload all the address literals that are needed for the > + * CPU power-gating process, to avoid loading from SDRAM which > + * are not supported once SDRAM is put into self-refresh. > + * LP0 / LP1 use physical address, since the MMU needs to be > + * disalbed before putting SDRAM into self-refresh to avoid s/disalbed/disabled/ > +/* > + * tegra30_lp1_reset > + * > + * reset vector for LP1 restore; copied into IRAM during suspend. > + * Brings the system back up to a safe staring point (SDRAM out of > + * self-refresh, PLLC, PLLM and PLLP reenabled, CPU running on PLLX, > + * system clock running on the same PLL that it suspended at), and > + * jumps to tegra_resume to restore virtual addressing. > + * The physical address of tegra_resume expected to be stored in > + * PMC_SCRATCH41. > + * > + * NOTE: THIS *MUST* BE RELOCATED TO TEGRA_IRAM_CODE_AREA AND MUST BE FIRST. What does "AND MUST BE FIRST" mean? > +ENTRY(tegra30_lp1_reset) > + /* > + * The CPU and system bus are running at 32KHz and executing from > + * IRAM when this code is executed; immediately switch to CLKM and > + * enable PLLP, PLLM, PLLC, PLLA and PLLX. > + */ > + mov32 r0, TEGRA_CLK_RESET_BASE > + > + mov r1, #(1 << 28) Some #defines for the various magic values used in this code would be useful. > +tegra30_sdram_pad_save: > + .word 0 > + .word 0 > + .word 0 > + .word 0 > + .word 0 > + .word 0 > + .word 0 > + .word 0 This might be simpler, and easier to validate it's the right length: .rept 8 .long 0 .endr > +tegra30_sdram_pad_address: > + .word TEGRA_EMC_BASE + EMC_CFG @0x0 > + .word TEGRA_EMC_BASE + EMC_ZCAL_INTERVAL @0x4 > + .word TEGRA_EMC_BASE + EMC_AUTO_CAL_INTERVAL @0x8 > + .word TEGRA_EMC_BASE + EMC_XM2VTTGENPADCTRL @0xc > + .word TEGRA_EMC_BASE + EMC_XM2VTTGENPADCTRL2 @0x10 > + .word TEGRA_PMC_BASE + PMC_IO_DPD_STATUS @0x14 > + .word TEGRA_CLK_RESET_BASE + CLK_RESET_CLK_SOURCE_MSELECT @0x18 > + .word TEGRA_CLK_RESET_BASE + CLK_RESET_SCLK_BURST @0x1c > + > +tegra30_sdram_pad_size: > + .word tegra30_sdram_pad_address - tegra30_sdram_pad_save Perhaps if you swapp the order of declaring tegra30_sdram_pad_save and tegra30_sdram_pad_address, you can even do something like: .rept (tegra30_sdram_pad_addr_end - tegra30_sdram_pad_addr) / 4 ? > +tegra30_switch_cpu_to_clk32k: > + /* > + * start by jumping to CLKM to safely disable PLLs, then jump to jumping sounds like a CPU program counter operation. s/jumping/switching/ ? > + /* 2uS delay delay between changing SCLK and CCLK */ > + wait_for_us r1, r7, r9 > + add r1, r1, #2 > + wait_until r1, r7, r9 Ah, I see how wait_for_us is used now. Perhaps rename it wait_for_us_boundary or wait_for_us_tick? Alternatively, perhaps wait_until can just incorporate that logic itself? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html