On Fri, Apr 05, 2019 at 04:12:56PM +0000, Suthikulpanit, Suravee wrote: > Hi Neri,
Hi Suravee, Many thanks for testing the patches! > > While trying out this patch series, I found that it does not work when the > HPET timer > is in periodic mode. I should have tested this better. I'll double check my setup. > > 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. These changes look OK to me. Thanks! I'll incorporate them in my next version. > > 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