[RFC] OMAP3: PM: Prevent hang in prcm_interrupt_handler

Commit Message

Kevin Hilman June 22, 2009, 11:44 p.m. UTC

Jon Hunter <[e-mail]> writes:

> From: Jon Hunter <[e-mail]>
>
> There are two scenarios where a race condition could result in a hang in the
> prcm_interrupt handler. These are:

IIRC, the RX51 tree has a workaround for some hangs seen in this
interrupt handler as well, so this has definitely been seen.  The fix
there though was just to no loop, I think your fix is more thorough 
and fixes the root cause.  Thanks.

No comments on the funtional changes, but a couple
cosmetic/documentation changes.

1) Would it be possible to summarize the requirements in the function
itself as part of this patch.

2) With the extra indentation, this function is getting too indented.
Looking closer, an abstraction of the 'enable clocks, poll PM_WKST,
disable clocks loop' could be done an called with the various modules
and offsets.  I've attached a hack/patch below to show what I mean.
You could just fold this into your patch.

Kevin

> 1). Waiting for PRM_IRQSTATUS_MPU register to clear.
> Bit 0 of the PRM_IRQSTATUS_MPU register indicates that a wake-up event is
> pending for the MPU. This bit can only be cleared if the all the wake-up events
> latched in the various PM_WKST_x registers have been cleared. If a wake-up event
> occurred during the processing of the prcm interrupt handler, after the
> corresponding PM_WKST_x register was checked but before the PRM_IRQSTATUS_MPU
> was cleared, then the CPU would be stuck forever waiting for bit 0 in
> PRM_IRQSTATUS_MPU to be cleared.
>
> 2). Waiting for the PM_WKST_x register to clear.
> Some power domains have more than one wake-up source. The PM_WKST_x registers
> indicate the source of a wake-up event and need to be cleared after a wake-up
> event occurs. When the PM_WKST_x registers are read and before they are cleared,
> it is possible that another wake-up event could occur causing another bit to be
> set in one of the PM_WKST_x registers. If this did occur after reading a
> PM_WKST_x register then the CPU would miss this event and get stuck forever in
> a loop waiting for that PM_WKST_x register to clear.
>
> This patch address the above race conditions that would result in a hang.
>
> Signed-off-by: Jon Hunter <[e-mail]>

commit 6b95e05225d5f4fa9aaf8400f5b6dd056fd8ce91
Author: Kevin Hilman <[e-mail]>
Date:   Mon Jun 22 16:17:57 2009 -0700

    temp: merge into PRCM interrupt fix

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hunter, Jon June 25, 2009, 6:56 p.m. UTC | #1

Kevin Hilman wrote:
> Jon Hunter <[e-mail]> writes:
> 
>> From: Jon Hunter <[e-mail]>
>>
>> There are two scenarios where a race condition could result in a hang in the
>> prcm_interrupt handler. These are:
> 
> IIRC, the RX51 tree has a workaround for some hangs seen in this
> interrupt handler as well, so this has definitely been seen.  The fix
> there though was just to no loop, I think your fix is more thorough 
> and fixes the root cause.  Thanks.
> 
> No comments on the funtional changes, but a couple
> cosmetic/documentation changes.
> 
> 1) Would it be possible to summarize the requirements in the function
> itself as part of this patch.

Absolutely. I will add some comments.

> 2) With the extra indentation, this function is getting too indented.
> Looking closer, an abstraction of the 'enable clocks, poll PM_WKST,
> disable clocks loop' could be done an called with the various modules
> and offsets.  I've attached a hack/patch below to show what I mean.
> You could just fold this into your patch.

Yes that makes a lot of sense. I did not like the fact it was becoming 
difficult to read with the extra indentation. I will re-work this and 
get you something in the next couple days.

Thanks for the feedback.

Cheers
Jon

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0545262..708816c 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -194,109 +194,40 @@  static void omap3_save_secure_ram_context(u32 target_mpu_state)
 	}
 }
 
+static void prcm_clear_mod_irqs(s16 module, u16 wkst_off,
+				u16 iclk_off, u16 fclk_off) {
+	u32 wkst, fclk, iclk;
+
+	wkst = prm_read_mod_reg(module, wkst_off);
+	if (wkst) {
+		iclk = cm_read_mod_reg(module, iclk_off);
+		fclk = cm_read_mod_reg(module, fclk_off);
+		while (wkst) {
+			cm_set_mod_reg_bits(wkst, module, iclk_off);
+			cm_set_mod_reg_bits(wkst, module, fclk_off);
+			prm_write_mod_reg(wkst, module, wkst_off);
+			wkst = prm_read_mod_reg(module, wkst_off);
+		}
+		cm_write_mod_reg(iclk, module, iclk_off);
+		cm_write_mod_reg(fclk, module, fclk_off);
+	}
+}
+
 /* PRCM Interrupt Handler for wakeups */
 static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 {
-	u32 wkst, irqstatus_mpu;
-	u32 fclk, iclk;
+	u32 irqstatus_mpu;
 
 	do {
-		/* WKUP */
-		wkst = prm_read_mod_reg(WKUP_MOD, PM_WKST);
-		if (wkst) {
-			iclk = cm_read_mod_reg(WKUP_MOD, CM_ICLKEN);
-			fclk = cm_read_mod_reg(WKUP_MOD, CM_FCLKEN);
-			while (wkst) {
-				cm_set_mod_reg_bits(wkst, WKUP_MOD, CM_ICLKEN);
-				cm_set_mod_reg_bits(wkst, WKUP_MOD, CM_FCLKEN);
-				prm_write_mod_reg(wkst, WKUP_MOD, PM_WKST);
-				wkst = prm_read_mod_reg(WKUP_MOD, PM_WKST);
-			}
-			cm_write_mod_reg(iclk, WKUP_MOD, CM_ICLKEN);
-			cm_write_mod_reg(fclk, WKUP_MOD, CM_FCLKEN);
-		}
-
-		/* CORE */
-		wkst = prm_read_mod_reg(CORE_MOD, PM_WKST1);
-		if (wkst) {
-			iclk = cm_read_mod_reg(CORE_MOD, CM_ICLKEN1);
-			fclk = cm_read_mod_reg(CORE_MOD, CM_FCLKEN1);
-			while (wkst) {
-				cm_set_mod_reg_bits(wkst, CORE_MOD, CM_ICLKEN1);
-				cm_set_mod_reg_bits(wkst, CORE_MOD, CM_FCLKEN1);
-				prm_write_mod_reg(wkst, CORE_MOD, PM_WKST1);
-				wkst = prm_read_mod_reg(CORE_MOD, PM_WKST1);
-			}
-			cm_write_mod_reg(iclk, CORE_MOD, CM_ICLKEN1);
-			cm_write_mod_reg(fclk, CORE_MOD, CM_FCLKEN1);
-		}
-		wkst = prm_read_mod_reg(CORE_MOD, OMAP3430ES2_PM_WKST3);
-		if (wkst) {
-			iclk = cm_read_mod_reg(CORE_MOD, CM_ICLKEN3);
-			fclk = cm_read_mod_reg(CORE_MOD,
-						OMAP3430ES2_CM_FCLKEN3);
-			while (wkst) {
-				cm_set_mod_reg_bits(wkst, CORE_MOD, CM_ICLKEN3);
-				cm_set_mod_reg_bits(wkst, CORE_MOD,
-							OMAP3430ES2_CM_FCLKEN3);
-				prm_write_mod_reg(wkst, CORE_MOD,
-							OMAP3430ES2_PM_WKST3);
-				wkst = prm_read_mod_reg(CORE_MOD,
-							OMAP3430ES2_PM_WKST3);
-			}
-			cm_write_mod_reg(iclk, CORE_MOD, CM_ICLKEN3);
-			cm_write_mod_reg(fclk, CORE_MOD,
-						OMAP3430ES2_CM_FCLKEN3);
-		}
-
-		/* PER */
-		wkst = prm_read_mod_reg(OMAP3430_PER_MOD, PM_WKST);
-		if (wkst) {
-			iclk = cm_read_mod_reg(OMAP3430_PER_MOD, CM_ICLKEN);
-			fclk = cm_read_mod_reg(OMAP3430_PER_MOD, CM_FCLKEN);
-			while (wkst) {
-				cm_set_mod_reg_bits(wkst, OMAP3430_PER_MOD,
-							CM_ICLKEN);
-				cm_set_mod_reg_bits(wkst, OMAP3430_PER_MOD,
-							CM_FCLKEN);
-				prm_write_mod_reg(wkst, OMAP3430_PER_MOD,
-							PM_WKST);
-				wkst = prm_read_mod_reg(OMAP3430_PER_MOD,
-							PM_WKST);
-			}
-			cm_write_mod_reg(iclk, OMAP3430_PER_MOD, CM_ICLKEN);
-			cm_write_mod_reg(fclk, OMAP3430_PER_MOD, CM_FCLKEN);
-		}
-
-		if (omap_rev() > OMAP3430_REV_ES1_0) {
-			/* USBHOST */
-			wkst = prm_read_mod_reg(OMAP3430ES2_USBHOST_MOD,
-							PM_WKST);
-			if (wkst) {
-				iclk = cm_read_mod_reg(OMAP3430ES2_USBHOST_MOD,
-					       CM_ICLKEN);
-				fclk = cm_read_mod_reg(OMAP3430ES2_USBHOST_MOD,
-					       CM_FCLKEN);
-				while (wkst) {
-					cm_set_mod_reg_bits(wkst,
-							OMAP3430ES2_USBHOST_MOD,
-							CM_ICLKEN);
-					cm_set_mod_reg_bits(wkst,
-							OMAP3430ES2_USBHOST_MOD,
-							CM_FCLKEN);
-					prm_write_mod_reg(wkst,
-							OMAP3430ES2_USBHOST_MOD,
-							PM_WKST);
-					wkst = prm_read_mod_reg(
-							OMAP3430ES2_USBHOST_MOD,
-							PM_WKST);
-				}
-				cm_write_mod_reg(iclk, OMAP3430ES2_USBHOST_MOD,
-					 CM_ICLKEN);
-				cm_write_mod_reg(fclk, OMAP3430ES2_USBHOST_MOD,
-					 CM_FCLKEN);
-			}
-		}
+		prcm_clear_mod_irqs(WKUP_MOD, PM_WKST, CM_ICLKEN, CM_FCLKEN);
+		prcm_clear_mod_irqs(CORE_MOD, PM_WKST1, CM_ICLKEN1, CM_ICLKEN1);
+		prcm_clear_mod_irqs(CORE_MOD, OMAP3430ES2_PM_WKST3,
+				    CM_ICLKEN3, OMAP3430ES2_CM_FCLKEN3);
+		prcm_clear_mod_irqs(OMAP3430_PER_MOD, PM_WKST,
+				    CM_ICLKEN, CM_FCLKEN);
+		if (omap_rev() > OMAP3430_REV_ES1_0)
+			prcm_clear_mod_irqs(OMAP3430ES2_USBHOST_MOD, PM_WKST,
+					    CM_ICLKEN, CM_FCLKEN);
 
 		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
 					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);