| Message ID | 1421741825-18226-8-git-send-email-wenyou.yang@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/01/2015 at 16:17:00 +0800, Wenyou Yang wrote : > To simply the PM code, the suspend to standby mode uses the same sram function > as the suspend to memory mode, running in the internal SRAM, > instead of the respective code for each mode. > > But for the suspend to standby mode, the master clock doesn't > switch to the slow clock, and the main oscillator doesn't > turn off as well. > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> That looks quite better, thanks. Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > arch/arm/mach-at91/pm.c | 81 ++++++++++++++++--------------------- > arch/arm/mach-at91/pm.h | 6 +++ > arch/arm/mach-at91/pm_slowclock.S | 18 +++++++++ > 3 files changed, 59 insertions(+), 46 deletions(-) > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c > index 691e6db..a1010f0 100644 > --- a/arch/arm/mach-at91/pm.c > +++ b/arch/arm/mach-at91/pm.c > @@ -145,62 +145,51 @@ extern void at91_slow_clock(void __iomem *pmc, void __iomem *ramc0, > void __iomem *ramc1, int memctrl); > extern u32 at91_slow_clock_sz; > > +static void at91_pm_suspend(suspend_state_t state) > +{ > + unsigned int pm_data = at91_pm_data.memctrl; > + > + pm_data |= (state == PM_SUSPEND_MEM) ? > + (AT91_PM_SLOW_CLOCK << AT91_PM_MODE_OFFSET) : 0; > + > + slow_clock(at91_pmc_base, at91_ramc_base[0], > + at91_ramc_base[1], pm_data); > +} > + > static int at91_pm_enter(suspend_state_t state) > { > at91_pinctrl_gpio_suspend(); > > switch (state) { > + /* > + * Suspend-to-RAM is like STANDBY plus slow clock mode, so > + * drivers must suspend more deeply, the master clock switches > + * to the clk32k and turns off the main oscillator > + * > + * STANDBY mode has *all* drivers suspended; ignores irqs not > + * marked as 'wakeup' event sources; and reduces DRAM power. > + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > + * nothing fancy done with main or cpu clocks. > + */ > + case PM_SUSPEND_MEM: > + case PM_SUSPEND_STANDBY: > /* > - * Suspend-to-RAM is like STANDBY plus slow clock mode, so > - * drivers must suspend more deeply: only the master clock > - * controller may be using the main oscillator. > + * Ensure that clocks are in a valid state. > */ > - case PM_SUSPEND_MEM: > - /* > - * Ensure that clocks are in a valid state. > - */ > - if (!at91_pm_verify_clocks()) > - goto error; > - > - /* > - * Enter slow clock mode by switching over to clk32k and > - * turning off the main oscillator; reverse on wakeup. > - */ > - if (slow_clock) { > - slow_clock(at91_pmc_base, at91_ramc_base[0], > - at91_ramc_base[1], > - at91_pm_data.memctrl); > - break; > - } else { > - pr_info("AT91: PM - no slow clock mode enabled ...\n"); > - /* FALLTHROUGH leaving master clock alone */ > - } > + if (!at91_pm_verify_clocks()) > + goto error; > > - /* > - * STANDBY mode has *all* drivers suspended; ignores irqs not > - * marked as 'wakeup' event sources; and reduces DRAM power. > - * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > - * nothing fancy done with main or cpu clocks. > - */ > - case PM_SUSPEND_STANDBY: > - /* > - * NOTE: the Wait-for-Interrupt instruction needs to be > - * in icache so no SDRAM accesses are needed until the > - * wakeup IRQ occurs and self-refresh is terminated. > - * For ARM 926 based chips, this requirement is weaker > - * as at91sam9 can access a RAM in self-refresh mode. > - */ > - if (at91_pm_standby) > - at91_pm_standby(); > - break; > + at91_pm_suspend(state); > > - case PM_SUSPEND_ON: > - cpu_do_idle(); > - break; > + break; > > - default: > - pr_debug("AT91: PM - bogus suspend state %d\n", state); > - goto error; > + case PM_SUSPEND_ON: > + cpu_do_idle(); > + break; > + > + default: > + pr_debug("AT91: PM - bogus suspend state %d\n", state); > + goto error; > } > > error: > diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h > index 7664d39..fbfea42 100644 > --- a/arch/arm/mach-at91/pm.h > +++ b/arch/arm/mach-at91/pm.h > @@ -15,6 +15,12 @@ > > #include <mach/at91_ramc.h> > > +#define AT91_PM_MEMCTRL_MASK 0x0f > +#define AT91_PM_MODE_OFFSET 4 > +#define AT91_PM_MODE_MASK 0x0f > + > +#define AT91_PM_SLOW_CLOCK 0x01 > + > /* > * On all at91 except rm9200 and x40 have the System Controller starts > * at address 0xffffc000 and has a size of 16KiB. > diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S > index 634d819..92d7e63 100644 > --- a/arch/arm/mach-at91/pm_slowclock.S > +++ b/arch/arm/mach-at91/pm_slowclock.S > @@ -15,12 +15,15 @@ > #include <linux/clk/at91_pmc.h> > #include <mach/at91_ramc.h> > > +#include "pm.h" > + > pmc .req r0 > sdramc .req r1 > ramc1 .req r2 > memctrl .req r3 > tmp1 .req r4 > tmp2 .req r5 > +mode .req r6 > > /* > * Wait until master clock is ready (after switching master clock source) > @@ -72,6 +75,13 @@ ENTRY(at91_slow_clock) > mov tmp1, #0 > mcr p15, 0, tmp1, c7, c10, 4 > > + mov tmp1, memctrl > + mov tmp2, tmp1, lsr#AT91_PM_MODE_OFFSET > + and mode, tmp2, #AT91_PM_MODE_MASK > + > + mov tmp1, memctrl > + and memctrl, tmp1, #AT91_PM_MEMCTRL_MASK > + > cmp memctrl, #AT91_MEMCTRL_MC > bne ddr_sr_enable > > @@ -144,6 +154,9 @@ sdr_sr_enable: > str tmp1, [sdramc, #AT91_SDRAMC_LPR] > > sdr_sr_done: > + tst mode, #AT91_PM_SLOW_CLOCK > + beq skip_disable_clock > + > /* Save Master clock setting */ > ldr tmp1, [pmc, #AT91_PMC_MCKR] > str tmp1, .saved_mckr > @@ -169,9 +182,13 @@ sdr_sr_done: > bic tmp1, tmp1, #AT91_PMC_MOSCEN > str tmp1, [pmc, #AT91_CKGR_MOR] > > +skip_disable_clock: > /* Wait for interrupt */ > mcr p15, 0, tmp1, c7, c0, 4 > > + tst mode, #AT91_PM_SLOW_CLOCK > + beq skip_enable_clock > + > /* Turn on the main oscillator */ > ldr tmp1, [pmc, #AT91_CKGR_MOR] > orr tmp1, tmp1, #AT91_PMC_MOSCEN > @@ -199,6 +216,7 @@ sdr_sr_done: > > wait_mckrdy > > +skip_enable_clock: > /* > * at91rm9200 Memory controller > * Do nothing - self-refresh is automatically disabled. > -- > 1.7.9.5 >
Hello Wenyou, On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c > index 691e6db..a1010f0 100644 > --- a/arch/arm/mach-at91/pm.c > +++ b/arch/arm/mach-at91/pm.c > @@ -145,62 +145,51 @@ extern void at91_slow_clock(void __iomem *pmc, void __iomem *ramc0, > void __iomem *ramc1, int memctrl); > extern u32 at91_slow_clock_sz; > > +static void at91_pm_suspend(suspend_state_t state) > +{ (...) > + slow_clock(at91_pmc_base, at91_ramc_base[0], > + at91_ramc_base[1], pm_data); > +} > - if (slow_clock) { > - slow_clock(at91_pmc_base, at91_ramc_base[0], > - at91_ramc_base[1], > - at91_pm_data.memctrl); (...) > + at91_pm_suspend(state); By doing that you removed the condition "if (slow_clock)". But slow_clock can still be NULL, see commit d2e4679, there are multiple reasons which ends up with a NULL slow_clock. Sylvain
Hello Wenyou, On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c > index 691e6db..a1010f0 100644 > --- a/arch/arm/mach-at91/pm.c > +++ b/arch/arm/mach-at91/pm.c (...) > static int at91_pm_enter(suspend_state_t state) > { > at91_pinctrl_gpio_suspend(); > > switch (state) { > + /* > + * Suspend-to-RAM is like STANDBY plus slow clock mode, so > + * drivers must suspend more deeply, the master clock switches > + * to the clk32k and turns off the main oscillator > + * > + * STANDBY mode has *all* drivers suspended; ignores irqs not > + * marked as 'wakeup' event sources; and reduces DRAM power. > + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > + * nothing fancy done with main or cpu clocks. > + */ > + case PM_SUSPEND_MEM: > + case PM_SUSPEND_STANDBY: (...) > - case PM_SUSPEND_MEM: > - /* > - * Ensure that clocks are in a valid state. > - */ > - if (!at91_pm_verify_clocks()) > - goto error; (...) > + if (!at91_pm_verify_clocks()) > + goto error; > (...) > - case PM_SUSPEND_STANDBY: > - /* > - * NOTE: the Wait-for-Interrupt instruction needs to be By doing that at91_pm_verify_clocks() is now called for both MEM and STANDBY targets. In my opinion this function is misnamed and should be called at91_pm_verify_clocks_for_slow_clock_mode(). This function actually checks if we can safely switch to slow clock mode, if some peripherals are still using the master clock, we abort the suspend because we can't suspend in good condition. Hard unclocking peripherals which ask for a soft stop, like USB controllers, is something we should avoid doing. This function checks if USB PLL and PLL B are stopped, if PCK0..PCK3 are stopped too (or just using the 32k clock). If all drivers suspended correctly this is the state we expect and we can suspend in a deep state. Not this is currently not the case in linux-next, suspend/resume support to all Atmel USB drivers (ehci-atmel,ohci-at91,atmel_usba,at91_udc) are in my series: [PATCHv7 0/6] USB: host: Atmel OHCI and EHCI drivers improvements <1421761144-11767-1-git-send-email-sylvain.rochet@finsecur.com> [PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements <1421945805-31129-1-git-send-email-sylvain.rochet@finsecur.com> We are not going to change any clock for STANDBY target, there is no clock to check, so we don't need to call at91_pm_verify_clocks() for this target. Sylvain
On 23/01/2015 at 18:32:34 +0100, Sylvain Rochet wrote : > Hello Wenyou, > > On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c > > index 691e6db..a1010f0 100644 > > --- a/arch/arm/mach-at91/pm.c > > +++ b/arch/arm/mach-at91/pm.c > (...) > > static int at91_pm_enter(suspend_state_t state) > > { > > at91_pinctrl_gpio_suspend(); > > > > switch (state) { > > + /* > > + * Suspend-to-RAM is like STANDBY plus slow clock mode, so > > + * drivers must suspend more deeply, the master clock switches > > + * to the clk32k and turns off the main oscillator > > + * > > + * STANDBY mode has *all* drivers suspended; ignores irqs not > > + * marked as 'wakeup' event sources; and reduces DRAM power. > > + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > > + * nothing fancy done with main or cpu clocks. > > + */ > > + case PM_SUSPEND_MEM: > > + case PM_SUSPEND_STANDBY: > (...) > > - case PM_SUSPEND_MEM: > > - /* > > - * Ensure that clocks are in a valid state. > > - */ > > - if (!at91_pm_verify_clocks()) > > - goto error; > (...) > > + if (!at91_pm_verify_clocks()) > > + goto error; > > > (...) > > - case PM_SUSPEND_STANDBY: > > - /* > > - * NOTE: the Wait-for-Interrupt instruction needs to be > > By doing that at91_pm_verify_clocks() is now called for both MEM and > STANDBY targets. > > In my opinion this function is misnamed and should be called > at91_pm_verify_clocks_for_slow_clock_mode(). This function actually > checks if we can safely switch to slow clock mode, if some peripherals > are still using the master clock, we abort the suspend because we can't > suspend in good condition. Hard unclocking peripherals which ask for a > soft stop, like USB controllers, is something we should avoid doing. > > This function checks if USB PLL and PLL B are stopped, if PCK0..PCK3 are > stopped too (or just using the 32k clock). If all drivers suspended > correctly this is the state we expect and we can suspend in a deep > state. > > Not this is currently not the case in linux-next, suspend/resume support > to all Atmel USB drivers (ehci-atmel,ohci-at91,atmel_usba,at91_udc) are > in my series: > [PATCHv7 0/6] USB: host: Atmel OHCI and EHCI drivers improvements > <1421761144-11767-1-git-send-email-sylvain.rochet@finsecur.com> > [PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements > <1421945805-31129-1-git-send-email-sylvain.rochet@finsecur.com> > > We are not going to change any clock for STANDBY target, there is no > clock to check, so we don't need to call at91_pm_verify_clocks() for > this target. > I think we should actually stop checking those clocks. In the meantime, you are right and at91_pm_verify_clocks must not be called unconditionally.
On 23/01/2015 at 17:50:20 +0100, Sylvain Rochet wrote : > Hello Wenyou, > > > On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c > > index 691e6db..a1010f0 100644 > > --- a/arch/arm/mach-at91/pm.c > > +++ b/arch/arm/mach-at91/pm.c > > > > @@ -145,62 +145,51 @@ extern void at91_slow_clock(void __iomem *pmc, void __iomem *ramc0, > > void __iomem *ramc1, int memctrl); > > extern u32 at91_slow_clock_sz; > > > > +static void at91_pm_suspend(suspend_state_t state) > > +{ > (...) > > + slow_clock(at91_pmc_base, at91_ramc_base[0], > > + at91_ramc_base[1], pm_data); > > +} > > > > - if (slow_clock) { > > - slow_clock(at91_pmc_base, at91_ramc_base[0], > > - at91_ramc_base[1], > > - at91_pm_data.memctrl); > (...) > > + at91_pm_suspend(state); > > > By doing that you removed the condition "if (slow_clock)". > > But slow_clock can still be NULL, see commit d2e4679, there are multiple > reasons which ends up with a NULL slow_clock. > I would fix that by not calling suspend_set_ops(&at91_pm_ops) when slow_clock is NULL in patch 6 (quick and easy) or copying the whole at91_pm_sram_init() in at91_pm_init() and handle failures from there.
Hi Sylvain, Thank you for review. > -----Original Message----- > From: Sylvain Rochet [mailto:sylvain.rochet@finsecur.com] > Sent: Saturday, January 24, 2015 1:33 AM > To: Yang, Wenyou > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-kernel@vger.kernel.org; > alexandre.belloni@free-electrons.com; peda@axentia.se; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH 07/12] pm: at91: the standby mode uses the same sram > function as the suspend to memory mode > > Hello Wenyou, > > On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index > > 691e6db..a1010f0 100644 > > --- a/arch/arm/mach-at91/pm.c > > +++ b/arch/arm/mach-at91/pm.c > (...) > > static int at91_pm_enter(suspend_state_t state) { > > at91_pinctrl_gpio_suspend(); > > > > switch (state) { > > + /* > > + * Suspend-to-RAM is like STANDBY plus slow clock mode, so > > + * drivers must suspend more deeply, the master clock switches > > + * to the clk32k and turns off the main oscillator > > + * > > + * STANDBY mode has *all* drivers suspended; ignores irqs not > > + * marked as 'wakeup' event sources; and reduces DRAM power. > > + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > > + * nothing fancy done with main or cpu clocks. > > + */ > > + case PM_SUSPEND_MEM: > > + case PM_SUSPEND_STANDBY: > (...) > > - case PM_SUSPEND_MEM: > > - /* > > - * Ensure that clocks are in a valid state. > > - */ > > - if (!at91_pm_verify_clocks()) > > - goto error; > (...) > > + if (!at91_pm_verify_clocks()) > > + goto error; > > > (...) > > - case PM_SUSPEND_STANDBY: > > - /* > > - * NOTE: the Wait-for-Interrupt instruction needs to be > > By doing that at91_pm_verify_clocks() is now called for both MEM and STANDBY > targets. > > In my opinion this function is misnamed and should be called > at91_pm_verify_clocks_for_slow_clock_mode(). This function actually checks if > we can safely switch to slow clock mode, if some peripherals are still using the > master clock, we abort the suspend because we can't suspend in good condition. > Hard unclocking peripherals which ask for a soft stop, like USB controllers, is > something we should avoid doing. > > This function checks if USB PLL and PLL B are stopped, if PCK0..PCK3 are > stopped too (or just using the 32k clock). If all drivers suspended correctly this is > the state we expect and we can suspend in a deep state. > > Not this is currently not the case in linux-next, suspend/resume support to all Atmel > USB drivers (ehci-atmel,ohci-at91,atmel_usba,at91_udc) are in my series: > [PATCHv7 0/6] USB: host: Atmel OHCI and EHCI drivers improvements > <1421761144-11767-1-git-send-email-sylvain.rochet@finsecur.com> > [PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements > <1421945805-31129-1-git-send-email-sylvain.rochet@finsecur.com> > > We are not going to change any clock for STANDBY target, there is no clock to > check, so we don't need to call at91_pm_verify_clocks() for this target. I will change in the next version. Thanks. > > Sylvain Best Regards, Wenyou Yang
Hi Alexandre, Thank you for review. > -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > Sent: Saturday, January 24, 2015 7:02 AM > To: Sylvain Rochet > Cc: Yang, Wenyou; Ferre, Nicolas; linux@arm.linux.org.uk; linux- > kernel@vger.kernel.org; peda@axentia.se; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 07/12] pm: at91: the standby mode uses the same sram > function as the suspend to memory mode > > On 23/01/2015 at 18:32:34 +0100, Sylvain Rochet wrote : > > Hello Wenyou, > > > > On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > > > > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index > > > 691e6db..a1010f0 100644 > > > --- a/arch/arm/mach-at91/pm.c > > > +++ b/arch/arm/mach-at91/pm.c > > (...) > > > static int at91_pm_enter(suspend_state_t state) { > > > at91_pinctrl_gpio_suspend(); > > > > > > switch (state) { > > > + /* > > > + * Suspend-to-RAM is like STANDBY plus slow clock mode, so > > > + * drivers must suspend more deeply, the master clock switches > > > + * to the clk32k and turns off the main oscillator > > > + * > > > + * STANDBY mode has *all* drivers suspended; ignores irqs not > > > + * marked as 'wakeup' event sources; and reduces DRAM power. > > > + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and > > > + * nothing fancy done with main or cpu clocks. > > > + */ > > > + case PM_SUSPEND_MEM: > > > + case PM_SUSPEND_STANDBY: > > (...) > > > - case PM_SUSPEND_MEM: > > > - /* > > > - * Ensure that clocks are in a valid state. > > > - */ > > > - if (!at91_pm_verify_clocks()) > > > - goto error; > > (...) > > > + if (!at91_pm_verify_clocks()) > > > + goto error; > > > > > (...) > > > - case PM_SUSPEND_STANDBY: > > > - /* > > > - * NOTE: the Wait-for-Interrupt instruction needs to be > > > > By doing that at91_pm_verify_clocks() is now called for both MEM and > > STANDBY targets. > > > > In my opinion this function is misnamed and should be called > > at91_pm_verify_clocks_for_slow_clock_mode(). This function actually > > checks if we can safely switch to slow clock mode, if some peripherals > > are still using the master clock, we abort the suspend because we > > can't suspend in good condition. Hard unclocking peripherals which ask > > for a soft stop, like USB controllers, is something we should avoid doing. > > > > This function checks if USB PLL and PLL B are stopped, if PCK0..PCK3 > > are stopped too (or just using the 32k clock). If all drivers > > suspended correctly this is the state we expect and we can suspend in > > a deep state. > > > > Not this is currently not the case in linux-next, suspend/resume > > support to all Atmel USB drivers > > (ehci-atmel,ohci-at91,atmel_usba,at91_udc) are in my series: > > [PATCHv7 0/6] USB: host: Atmel OHCI and EHCI drivers improvements > > <1421761144-11767-1-git-send-email-sylvain.rochet@finsecur.com> > > [PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements > > <1421945805-31129-1-git-send-email-sylvain.rochet@finsecur.com> > > > > We are not going to change any clock for STANDBY target, there is no > > clock to check, so we don't need to call at91_pm_verify_clocks() for > > this target. > > > > I think we should actually stop checking those clocks. In the meantime, you are > right and at91_pm_verify_clocks must not be called unconditionally. Thanks. I will change, the at91_pm_verify_clocks is only called for suspend to memory mode, not for the standby. > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering http://free-electrons.com Best Regards, Wenyou Yang
Hi Alexandre, > -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > Sent: Saturday, January 24, 2015 7:13 AM > To: Sylvain Rochet > Cc: Yang, Wenyou; Ferre, Nicolas; linux@arm.linux.org.uk; linux- > kernel@vger.kernel.org; peda@axentia.se; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 07/12] pm: at91: the standby mode uses the same sram > function as the suspend to memory mode > > On 23/01/2015 at 17:50:20 +0100, Sylvain Rochet wrote : > > Hello Wenyou, > > > > > > On Tue, Jan 20, 2015 at 04:17:00PM +0800, Wenyou Yang wrote: > > > > > > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index > > > 691e6db..a1010f0 100644 > > > --- a/arch/arm/mach-at91/pm.c > > > +++ b/arch/arm/mach-at91/pm.c > > > > > > > @@ -145,62 +145,51 @@ extern void at91_slow_clock(void __iomem *pmc, > void __iomem *ramc0, > > > void __iomem *ramc1, int memctrl); extern u32 > > > at91_slow_clock_sz; > > > > > > +static void at91_pm_suspend(suspend_state_t state) { > > (...) > > > + slow_clock(at91_pmc_base, at91_ramc_base[0], > > > + at91_ramc_base[1], pm_data); > > > +} > > > > > > > - if (slow_clock) { > > > - slow_clock(at91_pmc_base, at91_ramc_base[0], > > > - at91_ramc_base[1], > > > - at91_pm_data.memctrl); > > (...) > > > + at91_pm_suspend(state); > > > > > > By doing that you removed the condition "if (slow_clock)". > > > > But slow_clock can still be NULL, see commit d2e4679, there are > > multiple reasons which ends up with a NULL slow_clock. > > > > I would fix that by not calling suspend_set_ops(&at91_pm_ops) when slow_clock > is NULL in patch 6 (quick and easy) or copying the whole > at91_pm_sram_init() in at91_pm_init() and handle failures from there. Thank you for suggestion, I will fix it. > > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering http://free-electrons.com Best Regards, Wenyou Yang
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index 691e6db..a1010f0 100644 --- a/arch/arm/mach-at91/pm.c +++ b/arch/arm/mach-at91/pm.c @@ -145,62 +145,51 @@ extern void at91_slow_clock(void __iomem *pmc, void __iomem *ramc0, void __iomem *ramc1, int memctrl); extern u32 at91_slow_clock_sz; +static void at91_pm_suspend(suspend_state_t state) +{ + unsigned int pm_data = at91_pm_data.memctrl; + + pm_data |= (state == PM_SUSPEND_MEM) ? + (AT91_PM_SLOW_CLOCK << AT91_PM_MODE_OFFSET) : 0; + + slow_clock(at91_pmc_base, at91_ramc_base[0], + at91_ramc_base[1], pm_data); +} + static int at91_pm_enter(suspend_state_t state) { at91_pinctrl_gpio_suspend(); switch (state) { + /* + * Suspend-to-RAM is like STANDBY plus slow clock mode, so + * drivers must suspend more deeply, the master clock switches + * to the clk32k and turns off the main oscillator + * + * STANDBY mode has *all* drivers suspended; ignores irqs not + * marked as 'wakeup' event sources; and reduces DRAM power. + * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and + * nothing fancy done with main or cpu clocks. + */ + case PM_SUSPEND_MEM: + case PM_SUSPEND_STANDBY: /* - * Suspend-to-RAM is like STANDBY plus slow clock mode, so - * drivers must suspend more deeply: only the master clock - * controller may be using the main oscillator. + * Ensure that clocks are in a valid state. */ - case PM_SUSPEND_MEM: - /* - * Ensure that clocks are in a valid state. - */ - if (!at91_pm_verify_clocks()) - goto error; - - /* - * Enter slow clock mode by switching over to clk32k and - * turning off the main oscillator; reverse on wakeup. - */ - if (slow_clock) { - slow_clock(at91_pmc_base, at91_ramc_base[0], - at91_ramc_base[1], - at91_pm_data.memctrl); - break; - } else { - pr_info("AT91: PM - no slow clock mode enabled ...\n"); - /* FALLTHROUGH leaving master clock alone */ - } + if (!at91_pm_verify_clocks()) + goto error; - /* - * STANDBY mode has *all* drivers suspended; ignores irqs not - * marked as 'wakeup' event sources; and reduces DRAM power. - * But otherwise it's identical to PM_SUSPEND_ON: cpu idle, and - * nothing fancy done with main or cpu clocks. - */ - case PM_SUSPEND_STANDBY: - /* - * NOTE: the Wait-for-Interrupt instruction needs to be - * in icache so no SDRAM accesses are needed until the - * wakeup IRQ occurs and self-refresh is terminated. - * For ARM 926 based chips, this requirement is weaker - * as at91sam9 can access a RAM in self-refresh mode. - */ - if (at91_pm_standby) - at91_pm_standby(); - break; + at91_pm_suspend(state); - case PM_SUSPEND_ON: - cpu_do_idle(); - break; + break; - default: - pr_debug("AT91: PM - bogus suspend state %d\n", state); - goto error; + case PM_SUSPEND_ON: + cpu_do_idle(); + break; + + default: + pr_debug("AT91: PM - bogus suspend state %d\n", state); + goto error; } error: diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h index 7664d39..fbfea42 100644 --- a/arch/arm/mach-at91/pm.h +++ b/arch/arm/mach-at91/pm.h @@ -15,6 +15,12 @@ #include <mach/at91_ramc.h> +#define AT91_PM_MEMCTRL_MASK 0x0f +#define AT91_PM_MODE_OFFSET 4 +#define AT91_PM_MODE_MASK 0x0f + +#define AT91_PM_SLOW_CLOCK 0x01 + /* * On all at91 except rm9200 and x40 have the System Controller starts * at address 0xffffc000 and has a size of 16KiB. diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S index 634d819..92d7e63 100644 --- a/arch/arm/mach-at91/pm_slowclock.S +++ b/arch/arm/mach-at91/pm_slowclock.S @@ -15,12 +15,15 @@ #include <linux/clk/at91_pmc.h> #include <mach/at91_ramc.h> +#include "pm.h" + pmc .req r0 sdramc .req r1 ramc1 .req r2 memctrl .req r3 tmp1 .req r4 tmp2 .req r5 +mode .req r6 /* * Wait until master clock is ready (after switching master clock source) @@ -72,6 +75,13 @@ ENTRY(at91_slow_clock) mov tmp1, #0 mcr p15, 0, tmp1, c7, c10, 4 + mov tmp1, memctrl + mov tmp2, tmp1, lsr#AT91_PM_MODE_OFFSET + and mode, tmp2, #AT91_PM_MODE_MASK + + mov tmp1, memctrl + and memctrl, tmp1, #AT91_PM_MEMCTRL_MASK + cmp memctrl, #AT91_MEMCTRL_MC bne ddr_sr_enable @@ -144,6 +154,9 @@ sdr_sr_enable: str tmp1, [sdramc, #AT91_SDRAMC_LPR] sdr_sr_done: + tst mode, #AT91_PM_SLOW_CLOCK + beq skip_disable_clock + /* Save Master clock setting */ ldr tmp1, [pmc, #AT91_PMC_MCKR] str tmp1, .saved_mckr @@ -169,9 +182,13 @@ sdr_sr_done: bic tmp1, tmp1, #AT91_PMC_MOSCEN str tmp1, [pmc, #AT91_CKGR_MOR] +skip_disable_clock: /* Wait for interrupt */ mcr p15, 0, tmp1, c7, c0, 4 + tst mode, #AT91_PM_SLOW_CLOCK + beq skip_enable_clock + /* Turn on the main oscillator */ ldr tmp1, [pmc, #AT91_CKGR_MOR] orr tmp1, tmp1, #AT91_PMC_MOSCEN @@ -199,6 +216,7 @@ sdr_sr_done: wait_mckrdy +skip_enable_clock: /* * at91rm9200 Memory controller * Do nothing - self-refresh is automatically disabled.
To simply the PM code, the suspend to standby mode uses the same sram function as the suspend to memory mode, running in the internal SRAM, instead of the respective code for each mode. But for the suspend to standby mode, the master clock doesn't switch to the slow clock, and the main oscillator doesn't turn off as well. Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> --- arch/arm/mach-at91/pm.c | 81 ++++++++++++++++--------------------- arch/arm/mach-at91/pm.h | 6 +++ arch/arm/mach-at91/pm_slowclock.S | 18 +++++++++ 3 files changed, 59 insertions(+), 46 deletions(-)