Hi Sylvain,

Thank you for review.

-----Original Message-----
From: Sylvain Rochet [mailto:sylvain.rochet at finsecur.com]
Sent: Saturday, January 24, 2015 1:33 AM
To: Yang, Wenyou
Cc: Ferre, Nicolas; linux at arm.linux.org.uk; linux-kernel at vger.kernel.org;
alexandre.belloni at free-electrons.com; peda at axentia.se; linux-arm-
kernel at 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:

quoted

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

  (...)

quoted

 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:

   (...)

quoted

-		case PM_SUSPEND_MEM:
-			/*
-			 * Ensure that clocks are in a valid state.
-			 */
-			if (!at91_pm_verify_clocks())
-				goto error;

   (...)

quoted

+		if (!at91_pm_verify_clocks())
+			goto error;

   (...)

quoted

-		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
   [ref]
 [PATCHv6 0/5] USB: gadget: atmel_usba_udc: Driver improvements
   [ref]

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