Hi Neri,
While trying out this patch series, I found that it does not work when the HPET
timer
is in periodic mode.
On 2/27/19 11:05 PM, Ricardo Neri wrote:
> ......
> diff --git a/arch/x86/kernel/watchdog_hld_hpet.c
> b/arch/x86/kernel/watchdog_hld_hpet.c
> new file mode 100644
> index 000000000000..cfa284da4bf6
> --- /dev/null
> +++ b/arch/x86/kernel/watchdog_hld_hpet.c
> @@ -0,0 +1,405 @@
> .....
> +
> +/**
> + * set_comparator() - Update the comparator in an HPET timer instance
> + * @hdata: A data structure with the timer instance to update
> + * @cmp: The value to write in the in the comparator registere
> + *
> + * Returns:
> + *
> + * None
> + */
> +static inline void set_comparator(struct hpet_hld_data *hdata,
> + unsigned long cmp)
> +{
> + hpet_writeq(cmp, HPET_Tn_CMP(hdata->num));
> +}
> +
> +/**
> + * kick_timer() - Reprogram timer to expire in the future
> + * @hdata: A data structure with the timer instance to update
> + * @force: Force reprogram. Useful enabling or re-enabling detector.
> + *
> + * Reprogram the timer to expire within watchdog_thresh seconds in the
> future.
> + *
> + * Returns:
> + *
> + * None
> + */
> +static void kick_timer(struct hpet_hld_data *hdata, bool force)
> +{
> + bool kick_needed = force || !(hdata->flags & HPET_DEV_PERI_CAP);
> + unsigned long new_compare, count;
> +
> + /*
> + * Update the comparator in increments of watch_thresh seconds relative
> + * to the current count. Since watch_thresh is given in seconds, we
> + * are able to update the comparator before the counter reaches such new
> + * value.
> + *
> + * Let it wrap around if needed.
> + */
> +
> + if (kick_needed) {
> + count = get_count();
> +
> + new_compare = count + watchdog_thresh * hdata->ticks_per_second;
> +
> + set_comparator(hdata, new_compare);
> + }
> +}
It turns out that the set_comparator() does not seem to support periodic mode,
in which it should have been setting the accumulator/period via the comparator
register.
Instead, what if we re-factor the code in the
arch/x86/kernel/hpet.c:hpet_set_periodic()
to hpet_set_comparator(). Then we can also reuse it in the
arch/x86/kernel/watchdog_hld_pet.c:
kick_timer() as shown below.
With these changes, I can test the series on AMD systems, and see all cores
in /proc/interrupts are receiving NMI interrupts.
Regards,
Suravee
diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index 0976334..f30957f 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -115,6 +115,7 @@ extern int hpet_rtc_timer_init(void);
extern irqreturn_t hpet_rtc_interrupt(int irq, void *dev_id);
extern int hpet_register_irq_handler(rtc_irq_handler handler);
extern void hpet_unregister_irq_handler(rtc_irq_handler handler);
+extern void hpet_set_comparator(int num, unsigned int cmp, unsigned int
period);
#endif /* CONFIG_HPET_EMULATE_RTC */
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 03b05dae..f3b2351 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -328,6 +328,41 @@ static void hpet_legacy_clockevent_register(void)
printk(KERN_DEBUG "hpet clockevent registered\n");
}
+/*
+ * hpet_set_comparator() - Helper function for setting comparator register
+ * @num: The timer ID
+ * @cmp: The value to be written to the comparator/accumulator
+ * @period: The value to be written to the period (0 = oneshot mode)
+ *
+ * Helper function for updating comparator, accumulator and period values.
+ *
+ * In periodic mode, HPET needs HPET_TN_SETVAL to be set before writing
+ * to the Tn_CMP to update the accumulator. Then, HPET needs a second
+ * write (with HPET_TN_SETVAL cleared) to Tn_CMP to set the period.
+ * The HPET_TN_SETVAL bit is automatically cleared after the first write.
+ *
+ * For one-shot mode, HPET_TN_SETVAL does not need to be set.
+ *
+ * See the following documents:
+ * - Intel IA-PC HPET (High Precision Event Timers) Specification
+ * - AMD-8111 HyperTransport I/O Hub Data Sheet, Publication # 24674
+ */
+void hpet_set_comparator(int num, unsigned int cmp, unsigned int period)
+{
+ if (period) {
+ unsigned int v = hpet_readl(HPET_Tn_CFG(num));
+ hpet_writel(v | HPET_TN_SETVAL, HPET_Tn_CFG(num));
+ }
+
+ hpet_writel(cmp, HPET_Tn_CMP(num));
+ if (!period)
+ return;
+
+ udelay(1);
+ hpet_writel(period, HPET_Tn_CMP(num));
+}
+EXPORT_SYMBOL_GPL(hpet_set_comparator);
+
static int hpet_set_periodic(struct clock_event_device *evt, int timer)
{
unsigned int cfg, cmp, now;
@@ -339,19 +374,11 @@ static int hpet_set_periodic(struct clock_event_device
*evt, int timer)
now = hpet_readl(HPET_COUNTER);
cmp = now + (unsigned int)delta;
cfg = hpet_readl(HPET_Tn_CFG(timer));
- cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
- HPET_TN_32BIT;
+ cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_32BIT;
hpet_writel(cfg, HPET_Tn_CFG(timer));
- hpet_writel(cmp, HPET_Tn_CMP(timer));
- udelay(1);
- /*
- * HPET on AMD 81xx needs a second write (with HPET_TN_SETVAL
- * cleared) to T0_CMP to set the period. The HPET_TN_SETVAL
- * bit is automatically cleared after the first write.
- * (See AMD-8111 HyperTransport I/O Hub Data Sheet,
- * Publication # 24674)
- */
- hpet_writel((unsigned int)delta, HPET_Tn_CMP(timer));
+
+ hpet_set_comparator(timer, cmp, (unsigned int)delta);
+
hpet_start_counter();
hpet_print_config();
diff --git a/arch/x86/kernel/watchdog_hld_hpet.c
b/arch/x86/kernel/watchdog_hld_hpet.c
index 4402def..de33c50 100644
--- a/arch/x86/kernel/watchdog_hld_hpet.c
+++ b/arch/x86/kernel/watchdog_hld_hpet.c
@@ -35,21 +35,6 @@ static inline unsigned long get_count(void)
}
/**
- * set_comparator() - Update the comparator in an HPET timer instance
- * @hdata: A data structure with the timer instance to update
- * @cmp: The value to write in the in the comparator registere
- *
- * Returns:
- *
- * None
- */
-static inline void set_comparator(struct hpet_hld_data *hdata,
- unsigned long cmp)
-{
- hpet_writeq(cmp, HPET_Tn_CMP(hdata->num));
-}
-
-/**
* kick_timer() - Reprogram timer to expire in the future
* @hdata: A data structure with the timer instance to update
* @force: Force reprogram. Useful enabling or re-enabling detector.
@@ -68,7 +53,7 @@ static inline void set_comparator(struct hpet_hld_data *hdata,
static void kick_timer(struct hpet_hld_data *hdata, bool force)
{
bool kick_needed = force || !(hdata->flags & HPET_DEV_PERI_CAP);
- unsigned long tsc_curr, tsc_delta, new_compare, count;
+ unsigned long tsc_curr, tsc_delta, new_compare, count, period = 0;
/* Start obtaining the current TSC and HPET counts. */
tsc_curr = rdtsc();
@@ -81,6 +66,9 @@ static void kick_timer(struct hpet_hld_data *hdata, bool
force)
hdata->tsc_next = tsc_curr + tsc_delta;
hdata->tsc_next_error = tsc_delta >> 6;
+ if (!kick_needed)
+ return;
+
/*
* Update the comparator in increments of watch_thresh seconds relative
* to the current count. Since watch_thresh is given in seconds, we
@@ -89,12 +77,11 @@ static void kick_timer(struct hpet_hld_data *hdata, bool
force)
*
* Let it wrap around if needed.
*/
+ if (hdata->flags & HPET_DEV_PERI_CAP)
+ period = watchdog_thresh * hdata->ticks_per_second;
- if (kick_needed) {
- new_compare = count + watchdog_thresh * hdata->ticks_per_second;
-
- set_comparator(hdata, new_compare);
- }
+ new_compare = count + period;
+ hpet_set_comparator(hdata->num, new_compare, period);
}
/**
--
2.7.4
