ish: solve timer reliability and time jumps

b:129805553 demonstrates that the time jumps by 2^32 us twice during
boot. This appears to have been caused by leftover interrupts that
have not been cleared while the system was booting triggering T0.

In addition, b:124890290 discusses that the LRE bit should not be set
in HPET configuration, and the do-while loop introduced in CL:1457597
is no longer needed if the settling spin-lock is placed before arming
the timer. This resolves that

BRANCH=none
BUG=b:124890290,b:12980553
TEST=timer appears to be working reliably for 24 hours on arcada, no
time jumps

Change-Id: Ib0c6b8c9c7e8fe2111a312b9c48061b296178c72
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1548505
Reviewed-by: Jett Rink <jettrink@chromium.org>
Reviewed-by: Paul Fagerburg <pfagerburg@chromium.org>

2 files changed, 61 insertions, 45 deletions

diff --git a/chip/ish/hpet.h b/chip/ish/hpet.h
index ee26162518..3823a6e858 100644
--- a/chip/ish/hpet.h
+++ b/chip/ish/hpet.h
@@ -15,9 +15,8 @@
 
 
 /* HPET_GENERAL_CONFIG settings  */
-#define HPET_GENERAL_CONFIG	REG32(ISH_HPET_BASE + 0x10)
-#define HPET_ENABLE_CNF			(1<<0)
-#define HPET_LEGACY_RT_CNF		(1<<1)
+#define HPET_GENERAL_CONFIG		REG32(ISH_HPET_BASE + 0x10)
+#define HPET_ENABLE_CNF		BIT(0)
 
 /* Interrupt status acknowledge register */
 #define HPET_INTR_CLEAR		REG32(ISH_HPET_BASE + 0x20)
@@ -29,11 +28,11 @@
 
 /* HPET Timer 0/1/2 configuration*/
 #define HPET_TIMER_CONF_CAP(x)	REG32(ISH_HPET_BASE + 0x100 + ((x) * 0x20))
-#define HPET_Tn_INT_TYPE_CNF		(1<<1)
-#define HPET_Tn_INT_ENB_CNF		(1<<2)
-#define HPET_Tn_TYPE_CNF		(1<<3)
-#define HPET_Tn_VAL_SET_CNF		(1<<6)
-#define HPET_Tn_32MODE_CNF		(1<<8)
+#define HPET_Tn_INT_TYPE_CNF		BIT(1)
+#define HPET_Tn_INT_ENB_CNF		BIT(2)
+#define HPET_Tn_TYPE_CNF		BIT(3)
+#define HPET_Tn_VAL_SET_CNF		BIT(6)
+#define HPET_Tn_32MODE_CNF		BIT(8)
 #define HPET_Tn_INT_ROUTE_CNF_SHIFT	0x9
 #define HPET_Tn_INT_ROUTE_CNF_MASK	(0x1f << 9)
 
@@ -47,12 +46,17 @@
 /* ISH 4/5: Special status register
  * Use this register to see HPET timer are settled after a write.
  */
-#define HPET_CTRL_STATUS	REG32(ISH_HPET_BASE + 0x160)
-#define HPET_T1_CMP_SETTLING		BIT(8)
+#define HPET_CTRL_STATUS		REG32(ISH_HPET_BASE + 0x160)
+#define HPET_T1_CMP_SETTLING		BIT(9)
+#define HPET_T0_CMP_SETTLING		(BIT(7) | BIT(8))
 #define HPET_T1_CAP_SETTLING		BIT(5)
-#define HPET_MAIN_COUNTER_SETTLING	BIT(2)
+#define HPET_T0_CAP_SETTLING		BIT(4)
+#define HPET_MAIN_COUNTER_SETTLING	(BIT(2) | BIT(3))
 #define HPET_T1_SETTLING		(HPET_T1_CAP_SETTLING | \
 					 HPET_T1_CMP_SETTLING)
+#define HPET_T0_SETTLING		(HPET_T0_CAP_SETTLING | \
+					 HPET_T0_CMP_SETTLING)
+#define HPET_ANY_SETTLING		(BIT(12) - 1)
 
 #if defined(CHIP_FAMILY_ISH3)
 #define ISH_HPET_CLK_FREQ		12000000	/* 12 MHz clock */
diff --git a/chip/ish/hwtimer.c b/chip/ish/hwtimer.c
index 79a48783d9..393e1af4af 100644
--- a/chip/ish/hwtimer.c
+++ b/chip/ish/hwtimer.c
@@ -75,6 +75,12 @@ static inline uint32_t scale_ticks2us(uint64_t ticks)
 	return quotient;
 }
 
+static inline void wait_while_settling(uint32_t mask)
+{
+	/* Do nothing on ISH3, only ISH4 and ISH5 need settling */
+	(void) mask;
+}
+
 #elif defined(CHIP_FAMILY_ISH4) || defined(CHIP_FAMILY_ISH5)
 #define CLOCK_SCALE_BITS 15
 BUILD_ASSERT(BIT(CLOCK_SCALE_BITS) == ISH_HPET_CLK_FREQ);
@@ -113,7 +119,23 @@ static inline uint32_t scale_ticks2us(uint64_t ticks)
 
 	return intermediate >> CLOCK_SCALE_BITS;
 }
-#endif /* CHIP_FAMILY_ISH4 || CHIP_FAMILY_ISH5 */
+
+/*
+ * HPET Control & Status register may indicate that a value which has
+ * been written still needs propogated by hardware. Before updating
+ * HPET_TIMER_CONF_CAP(N), be sure to wait on the value settling with
+ * the corresponding mask (see hpet.h).
+ */
+static inline void wait_while_settling(uint32_t mask)
+{
+	/* Wait for timer settings to settle ~ 150us */
+	while (HPET_CTRL_STATUS & mask)
+		continue;
+}
+
+#else
+#error "Must define CHIP_FAMILY_ISH(3|4|5)"
+#endif
 
 /*
  * The 64-bit read on a 32-bit chip can tear during the read. Ensure that the
@@ -152,20 +174,10 @@ void __hw_clock_event_set(uint32_t deadline)
 	 */
 	HPET_TIMER_COMP(1) = read_main_timer() + scale_us2ticks(remaining_us);
 
-	do {
-		/* Arm timer */
-		HPET_TIMER_CONF_CAP(1) |= HPET_Tn_INT_ENB_CNF;
+	wait_while_settling(HPET_T1_SETTLING);
 
-#if defined(CHIP_FAMILY_ISH4) || defined(CHIP_FAMILY_ISH5)
-		/* Wait for timer settings to settle ~ 150us */
-		while (HPET_CTRL_STATUS & HPET_T1_SETTLING)
-			continue;
-#endif
-	/*
-	 * TODO(b/124890290): Remove or update while loop that ensures timer is
-	 * armed once we have a better hardware understanding.
-	 */
-	} while (!(HPET_TIMER_CONF_CAP(1) & HPET_Tn_INT_ENB_CNF));
+	/* Arm timer */
+	HPET_TIMER_CONF_CAP(1) |= HPET_Tn_INT_ENB_CNF;
 }
 
 uint32_t __hw_clock_event_get(void)
@@ -175,6 +187,7 @@ uint32_t __hw_clock_event_get(void)
 
 void __hw_clock_event_clear(void)
 {
+	wait_while_settling(HPET_T1_SETTLING);
 	HPET_TIMER_CONF_CAP(1) &= ~HPET_Tn_INT_ENB_CNF;
 }
 
@@ -186,18 +199,13 @@ uint32_t __hw_clock_source_read(void)
 void __hw_clock_source_set(uint32_t ts)
 {
 	/* Reset both clock and overflow comparators */
-
+	wait_while_settling(HPET_ANY_SETTLING);
 	HPET_GENERAL_CONFIG &= ~HPET_ENABLE_CNF;
 
 	HPET_MAIN_COUNTER_64 = scale_us2ticks(ts);
 	HPET_TIMER0_COMP_64 = ROLLOVER_CMP_VAL;
 
-#if defined(CHIP_FAMILY_ISH4) || defined(CHIP_FAMILY_ISH5)
-	/* Wait for timer to settle. required for ISH 4 */
-	while (HPET_CTRL_STATUS & HPET_MAIN_COUNTER_SETTLING)
-		continue;
-#endif
-
+	wait_while_settling(HPET_ANY_SETTLING);
 	HPET_GENERAL_CONFIG |= HPET_ENABLE_CNF;
 }
 
@@ -227,7 +235,6 @@ DECLARE_IRQ(ISH_HPET_TIMER1_IRQ, __hw_clock_source_irq_1);
 
 int __hw_clock_source_init(uint32_t start_t)
 {
-
 	/*
 	 * The timer can only fire interrupt when its value reaches zero.
 	 * Therefore we need two timers:
@@ -239,9 +246,20 @@ int __hw_clock_source_init(uint32_t start_t)
 	uint32_t timer1_config = 0x00000000;
 
 	/* Disable HPET */
+	wait_while_settling(HPET_ANY_SETTLING);
 	HPET_GENERAL_CONFIG &= ~HPET_ENABLE_CNF;
+
+	/* Disable T0 and T1 until we get them set up (below) */
+	HPET_TIMER_CONF_CAP(0) &= ~HPET_Tn_INT_ENB_CNF;
+	HPET_TIMER_CONF_CAP(1) &= ~HPET_Tn_INT_ENB_CNF;
+
+	/* Initialize main counter */
 	HPET_MAIN_COUNTER_64 = scale_us2ticks(start_t);
 
+	/* Clear any interrupts from previously running image */
+	HPET_INTR_CLEAR = BIT(0);
+	HPET_INTR_CLEAR = BIT(1);
+
 	/* Set comparator value for Timer 0 and enable periodic mode */
 	HPET_TIMER0_COMP_64 = ROLLOVER_CMP_VAL;
 	timer0_config |= HPET_Tn_TYPE_CNF;
@@ -252,7 +270,7 @@ int __hw_clock_source_init(uint32_t start_t)
 	/* Timer 1 - IRQ routing */
 	timer1_config &= ~HPET_Tn_INT_ROUTE_CNF_MASK;
 	timer1_config |= (ISH_HPET_TIMER1_IRQ <<
-				HPET_Tn_INT_ROUTE_CNF_SHIFT);
+			  HPET_Tn_INT_ROUTE_CNF_SHIFT);
 
 	/* Level triggered interrupt */
 	timer0_config |= HPET_Tn_INT_TYPE_CNF;
@@ -261,6 +279,9 @@ int __hw_clock_source_init(uint32_t start_t)
 	/* Enable interrupt */
 	timer0_config |= HPET_Tn_INT_ENB_CNF;
 
+	/* Before enabling, previous values must have settled */
+	wait_while_settling(HPET_ANY_SETTLING);
+
 	/* Unmask HPET IRQ in IOAPIC */
 	task_enable_irq(ISH_HPET_TIMER0_IRQ);
 	task_enable_irq(ISH_HPET_TIMER1_IRQ);
@@ -269,17 +290,8 @@ int __hw_clock_source_init(uint32_t start_t)
 	HPET_TIMER_CONF_CAP(0) |= timer0_config;
 	HPET_TIMER_CONF_CAP(1) |= timer1_config;
 
-#if defined(CHIP_FAMILY_ISH4) || defined(CHIP_FAMILY_ISH5)
-	/* Wait for timer to settle. required for ISH 4 */
-	while (HPET_CTRL_STATUS & HPET_MAIN_COUNTER_SETTLING)
-		continue;
-#endif
-
-	/*
-	 * LEGACY_RT_CNF for HPET1 interrupt routing
-	 * and enable overall HPET counter/interrupts.
-	 */
-	HPET_GENERAL_CONFIG |= (HPET_ENABLE_CNF | HPET_LEGACY_RT_CNF);
+	/* Enable HPET */
+	HPET_GENERAL_CONFIG |= HPET_ENABLE_CNF;
 
 	/* Return IRQ value for OS event timer */
 	return ISH_HPET_TIMER1_IRQ;