ish: fixed wrongly entered D0ix states in some times

In __idle() task, when calculate the 'next_delay' value (the
sleep time), in most time, 'next_delay' should be always positive,
but ISH HPET timer HW has some latency for interrupt, so it's
possible in some times when its very close to the expire
time of the event timer, the current time could advance the
'last_deadline' which should be updated in event timer ISR,
in this case, 'next_delay' could be negative.

We calibrated the 'last_deadline' in timer driver for
this interrupt latency impact.

So, the negative case for 'next_delay' should be not happen.
If still happens, its doesn't matter, we can just ignore it
, and if not want to see this case, can adjust the
'HPET_INT_LATENCY_TICKS' for new calibration till its not
happen anymore.

BUG=b:133459192
BRANCH=none
TEST=tested on arcada platform, with 10ms timer loop task, D0i2/D0i3
should not entered.

Change-Id: Ie84fb630900dd7d59a41c98c08da4a71a831c030
Signed-off-by: Hu, Hebo <hebo.hu@intel.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1643247
Tested-by: Hyungwoo Yang <hyungwoo.yang@intel.com>
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-by: Denis Brockus <dbrockus@chromium.org>
Commit-Queue: Jack Rosenthal <jrosenth@chromium.org>

2 files changed, 43 insertions, 7 deletions

diff --git a/chip/ish/hwtimer.c b/chip/ish/hwtimer.c
index d3d78cadd6..2906d79aff 100644
--- a/chip/ish/hwtimer.c
+++ b/chip/ish/hwtimer.c
@@ -32,6 +32,14 @@ static uint32_t last_deadline;
  */
 #define MINIMUM_EVENT_DELAY_US 800
 
+/*
+ * ISH HPET timer HW has latency for interrupt, on ISH5, this latency is about
+ * 3 ticks, defined this configuration to calibrate the 'last_deadline' which is
+ * updated in event timer interrupt ISR. Without this calibration, we could
+ * get negative sleep time in idle task for low power sleep process.
+ */
+#define HPET_INT_LATENCY_TICKS 3
+
 /* Scaling helper methods for different ISH chip variants */
 #ifdef CHIP_FAMILY_ISH3
 #define CLOCK_FACTOR 12
@@ -161,26 +169,45 @@ static inline uint64_t read_main_timer(void)
 void __hw_clock_event_set(uint32_t deadline)
 {
 	uint32_t remaining_us;
+	uint32_t current_us;
+	uint64_t current_ticks;
 
-	last_deadline = deadline;
+	/* 'current_ticks' is the current absolute 64bit HW timer counter */
+	current_ticks = read_main_timer();
 
-	remaining_us = deadline - __hw_clock_source_read();
+	/*
+	 * 'current_us' is the low 32bit part of current time in 64bit micro
+	 * seconds format, it's can express 2^32 micro seconds in maximum.
+	 */
+	current_us = scale_ticks2us(current_ticks);
 
-	/* Ensure HW has enough time to react to new timer value */
+	/*
+	 * To ensure HW has enough time to react to the new timer value,
+	 * we make remaining time not less than 'MINIMUM_EVENT_DELAY_US'
+	 */
+	remaining_us = deadline - current_us;
 	remaining_us = MAX(remaining_us, MINIMUM_EVENT_DELAY_US);
 
 	/*
+	 * Set new 64bit absolute timeout ticks to Timer 1 comparator
+	 * register.
 	 * For ISH3, this assumes that remaining_us is less than 360 seconds
 	 * (2^32 us / 12Mhz), otherwise we would need to handle 32-bit rollover
 	 * of 12Mhz timer comparator value. Watchdog refresh happens at least
 	 * every 10 seconds.
 	 */
 	wait_while_settling(HPET_T1_CMP_SETTLING);
-	HPET_TIMER_COMP(1) = read_main_timer() + scale_us2ticks(remaining_us);
+	HPET_TIMER_COMP(1) = current_ticks + scale_us2ticks(remaining_us);
 
-	wait_while_settling(HPET_T1_SETTLING);
+	/*
+	 * Update 'last_deadline' and add calibrate delta due to HPET timer
+	 * interrupt latency.
+	 */
+	last_deadline = current_us + remaining_us;
+	last_deadline += scale_ticks2us(HPET_INT_LATENCY_TICKS);
 
-	/* Arm timer */
+	/* Enable timer interrupt */
+	wait_while_settling(HPET_T1_SETTLING);
 	HPET_TIMER_CONF_CAP(1) |= HPET_Tn_INT_ENB_CNF;
 }
 
diff --git a/chip/ish/power_mgt.c b/chip/ish/power_mgt.c
index dea5b4cf1c..cd5d17034e 100644
--- a/chip/ish/power_mgt.c
+++ b/chip/ish/power_mgt.c
@@ -585,7 +585,16 @@ void __idle(void)
 		t0 = get_time();
 		next_delay = __hw_clock_event_get() - t0.le.lo;
 
-		pm_process(t0, next_delay);
+		/*
+		 * Most of the time, 'next_delay' will be positive. But, due to
+		 * interrupt latency, it's possible that get_time() returns
+		 * the value bigger than the one from __hw_clock_event_get()
+		 * which is supposed to be updated by ISR before control reaches
+		 * to the get_time().
+		 *
+		 * Here, we handle the delayed update by changing negative to 0.
+		 */
+		pm_process(t0, MAX(0, next_delay));
 	}
 }