[PATCH] ARM: mx7: psci: fix suspend/resume e10133 workaround

[PATCH] ARM: mx7: psci: fix suspend/resume e10133 workaround

[Matthias Schiffer]

The e10133 workaround was broken in two places:

- The code intended to temporarily mask all interrupts in GPC_IMRx_CORE0.
  While the old register values were saved, the actual masking was
  missing.
- imx_udelay() expects the system counter to run at its base frequency,
  but the system counter is switched to a lower frequency earlier in
  psci_system_suspend(), leading to a much longer delay than intended.
  Replace the call with an equivalent loop (linux-imx 5.15 does the same)

This fixes the SoC hanging forever when there was already a wakeup IRQ
pending while suspending.

Fixes: 57b620255e ("imx: mx7: add system suspend/resume support")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 arch/arm/mach-imx/mx7/psci-mx7.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index f32945ea37..699a2569cb 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -643,8 +643,10 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
 	/* disable GIC distributor */
 	writel(0, GIC400_ARB_BASE_ADDR + GIC_DIST_OFFSET);
 
-	for (i = 0; i < 4; i++)
+	for (i = 0; i < 4; i++) {
 		gpc_mask[i] = readl(GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
+		writel(~0, GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
+	}
 
 	/*
 	 * enable the RBC bypass counter here
@@ -668,7 +670,7 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
 		writel(gpc_mask[i], GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
 
 	/*
-	 * now delay for a short while (3usec)
+	 * now delay for a short while (~3usec)
 	 * ARM is at 1GHz at this point
 	 * so a short loop should be enough.
 	 * this delay is required to ensure that
@@ -677,7 +679,8 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
 	 * or in case an interrupt arrives just
 	 * as ARM is about to assert DSM_request.
 	 */
-	imx_udelay(3);
+	for (i = 0; i < 2000; i++)
+		asm volatile("");
 
 	/* save resume entry and sp in CPU0 GPR registers */
 	asm volatile("mov %0, sp" : "=r" (val));
-- 
2.25.1

Re: [PATCH] ARM: mx7: psci: fix suspend/resume e10133 workaround

[Matthias Schiffer]

On Mon, 2022-09-26 at 10:31 +0200, Matthias Schiffer wrote:
> The e10133 workaround was broken in two places:
> 
> - The code intended to temporarily mask all interrupts in GPC_IMRx_CORE0.
>   While the old register values were saved, the actual masking was
>   missing.
> - imx_udelay() expects the system counter to run at its base frequency,
>   but the system counter is switched to a lower frequency earlier in
>   psci_system_suspend(), leading to a much longer delay than intended.
>   Replace the call with an equivalent loop (linux-imx 5.15 does the same)
> 
> This fixes the SoC hanging forever when there was already a wakeup IRQ
> pending while suspending.
> 
> Fixes: 57b620255e ("imx: mx7: add system suspend/resume support")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Ping, any feedback on this?

I'm not entirely sure anymore if this solution is adequate, as the
duration of the loop depends on the CPU clock frequency and cache
enable/disable state. Maybe a modified imx_udelay() that accounts for
the changed system counter configuration would be necessary after all?


> ---
>  arch/arm/mach-imx/mx7/psci-mx7.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
> index f32945ea37..699a2569cb 100644
> --- a/arch/arm/mach-imx/mx7/psci-mx7.c
> +++ b/arch/arm/mach-imx/mx7/psci-mx7.c
> @@ -643,8 +643,10 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
>  	/* disable GIC distributor */
>  	writel(0, GIC400_ARB_BASE_ADDR + GIC_DIST_OFFSET);
>  
> -	for (i = 0; i < 4; i++)
> +	for (i = 0; i < 4; i++) {
>  		gpc_mask[i] = readl(GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
> +		writel(~0, GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
> +	}
>  
>  	/*
>  	 * enable the RBC bypass counter here
> @@ -668,7 +670,7 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
>  		writel(gpc_mask[i], GPC_IPS_BASE_ADDR + GPC_IMR1_CORE0 + i * 4);
>  
>  	/*
> -	 * now delay for a short while (3usec)
> +	 * now delay for a short while (~3usec)
>  	 * ARM is at 1GHz at this point
>  	 * so a short loop should be enough.
>  	 * this delay is required to ensure that
> @@ -677,7 +679,8 @@ __secure void psci_system_suspend(u32 __always_unused function_id,
>  	 * or in case an interrupt arrives just
>  	 * as ARM is about to assert DSM_request.
>  	 */
> -	imx_udelay(3);
> +	for (i = 0; i < 2000; i++)
> +		asm volatile("");
>  
>  	/* save resume entry and sp in CPU0 GPR registers */
>  	asm volatile("mov %0, sp" : "=r" (val));

[PATCH] ARM: mx7: psci: fix suspend/resume e10133 workaround

[sbabic]

> The e10133 workaround was broken in two places:
> - The code intended to temporarily mask all interrupts in GPC_IMRx_CORE0.
>   While the old register values were saved, the actual masking was
>   missing.
> - imx_udelay() expects the system counter to run at its base frequency,
>   but the system counter is switched to a lower frequency earlier in
>   psci_system_suspend(), leading to a much longer delay than intended.
>   Replace the call with an equivalent loop (linux-imx 5.15 does the same)
> This fixes the SoC hanging forever when there was already a wakeup IRQ
> pending while suspending.
> Fixes: 57b620255e ("imx: mx7: add system suspend/resume support")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================