[PATCH 0/7] HPET fixes and enhancements

[PATCH 0/7] HPET fixes and enhancements

[Clemens Ladisch]

These patches remove a bunch of warts and quirks from the HPET drivers.


 arch/i386/kernel/time_hpet.c |   20 +++++++++++---------
 arch/x86_64/kernel/time.c    |   20 +++++++++++---------
 drivers/char/hpet.c          |   39 ++++++++++++++++++++++++---------------
 3 files changed, 46 insertions(+), 33 deletions(-)

[PATCH 1/7] HPET: remove unused variable

[Clemens Ladisch]

The variable hpet_ntimer is never read, so remove it.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c	2005-09-27 21:39:43.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c	2005-09-27 21:42:12.000000000 +0200
@@ -49,7 +49,7 @@
 #define	HPET_USER_FREQ	(64)
 #define	HPET_DRIFT	(500)
 
-static u32 hpet_ntimer, hpet_nhpet, hpet_max_freq = HPET_USER_FREQ;
+static u32 hpet_nhpet, hpet_max_freq = HPET_USER_FREQ;
 
 /* A lock for concurrent access by app and isr hpet activity. */
 static DEFINE_SPINLOCK(hpet_lock);
@@ -854,8 +854,7 @@ int hpet_alloc(struct hpet_data *hdp)
 		writeq(mcfg, &hpet->hpet_config);
 	}
 
-	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer;
-	     i++, hpet_ntimer++, devp++) {
+	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
 		unsigned long v;
 		struct hpet_timer __iomem *timer;
 

[PATCH 2/7] HPET: remove superfluous register reads

[Clemens Ladisch]

This patch removes several reads of a timer's config register that
serve no purpose whatsoever.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c	2005-09-27 21:42:12.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c	2005-09-27 21:44:11.000000000 +0200
@@ -367,7 +367,6 @@ static int hpet_ioctl_ieon(struct hpet_d
 	if (!devp->hd_ireqfreq)
 		return -EIO;
 
-	v = readq(&timer->hpet_config);
 	spin_lock_irq(&hpet_lock);
 
 	if (devp->hd_flags & HPET_IE) {
@@ -378,7 +377,6 @@ static int hpet_ioctl_ieon(struct hpet_d
 	devp->hd_flags |= HPET_IE;
 	spin_unlock_irq(&hpet_lock);
 
-	t = readq(&timer->hpet_config);
 	irq = devp->hd_hdwirq;
 
 	if (irq) {
@@ -855,11 +853,9 @@ int hpet_alloc(struct hpet_data *hdp)
 	}
 
 	for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
-		unsigned long v;
 		struct hpet_timer __iomem *timer;
 
 		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
-		v = readq(&timer->hpet_config);
 
 		devp->hd_hpets = hpetp;
 		devp->hd_hpet = hpet;

[PATCH 3/7] HPET: allow non-power-of-two frequencies

[Clemens Ladisch]

It was only the RTC hardware that restricted interrupt frequencies to a
power of two.  There is no reason to take over this restriction into the
HPET driver, so remove the offending check.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c	2005-09-27 21:44:11.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c	2005-09-27 21:45:12.000000000 +0200
@@ -519,7 +519,7 @@ hpet_ioctl_common(struct hpet_dev *devp,
 			break;
 		}
 
-		if (!arg || (arg & (arg - 1))) {
+		if (!arg) {
 			err = -EINVAL;
 			break;
 		}

[PATCH 4/7] HPET: allow shared interrupts

[Clemens Ladisch]

This patch adds support for shared HPET interrupts.

The driver previously acknowledged interrupts for both edge and level
interrupts, but didn't actually allow a shared interrupt in the latter
case.

We use a new per-timer flag to save whether the timer's interrupt
might be shared, and use it to do the processing required for level
interrupts only if necessary.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/drivers/char/hpet.c
===================================================================
--- linux-2.6.13.orig/drivers/char/hpet.c	2005-09-27 21:45:12.000000000 +0200
+++ linux-2.6.13/drivers/char/hpet.c	2005-09-27 22:21:48.000000000 +0200
@@ -90,6 +90,7 @@ static struct hpets *hpets;
 #define	HPET_OPEN		0x0001
 #define	HPET_IE			0x0002	/* interrupt enabled */
 #define	HPET_PERIODIC		0x0004
+#define	HPET_SHARED_IRQ		0x0008
 
 #if BITS_PER_LONG == 64
 #define	write_counter(V, MC)	writeq(V, MC)
@@ -120,6 +121,11 @@ static irqreturn_t hpet_interrupt(int ir
 	unsigned long isr;
 
 	devp = data;
+	isr = 1 << (devp - devp->hd_hpets->hp_dev);
+
+	if ((devp->hd_flags & HPET_SHARED_IRQ) &&
+	    !(isr & readl(&devp->hd_hpet->hpet_isr)))
+		return IRQ_NONE;
 
 	spin_lock(&hpet_lock);
 	devp->hd_irqdata++;
@@ -137,8 +143,8 @@ static irqreturn_t hpet_interrupt(int ir
 			      &devp->hd_timer->hpet_compare);
 	}
 
-	isr = (1 << (devp - devp->hd_hpets->hp_dev));
-	writeq(isr, &devp->hd_hpet->hpet_isr);
+	if (devp->hd_flags & HPET_SHARED_IRQ)
+		writel(isr, &devp->hd_hpet->hpet_isr);
 	spin_unlock(&hpet_lock);
 
 	spin_lock(&hpet_task_lock);
@@ -375,15 +381,21 @@ static int hpet_ioctl_ieon(struct hpet_d
 	}
 
 	devp->hd_flags |= HPET_IE;
+
+	if (readl(&timer->hpet_config) & Tn_INT_TYPE_CNF_MASK)
+		devp->hd_flags |= HPET_SHARED_IRQ;
 	spin_unlock_irq(&hpet_lock);
 
 	irq = devp->hd_hdwirq;
 
 	if (irq) {
-		sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
+		unsigned long irq_flags;
 
-		if (request_irq
-		    (irq, hpet_interrupt, SA_INTERRUPT, devp->hd_name, (void *)devp)) {
+		sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
+		irq_flags = devp->hd_flags & HPET_SHARED_IRQ
+						? SA_SHIRQ : SA_INTERRUPT;
+		if (request_irq(irq, hpet_interrupt, irq_flags, 
+				devp->hd_name, (void *)devp)) {
 			printk(KERN_ERR "hpet: IRQ %d is not free\n", irq);
 			irq = 0;
 		}
@@ -417,8 +429,10 @@ static int hpet_ioctl_ieon(struct hpet_d
 		write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
 	}
 
-	isr = (1 << (devp - hpets->hp_dev));
-	writeq(isr, &hpet->hpet_isr);
+	if (devp->hd_flags & HPET_SHARED_IRQ) {
+		isr = 1 << (devp - hpets->hp_dev);
+		writel(isr, &hpet->hpet_isr);
+	}
 	writeq(g, &timer->hpet_config);
 	local_irq_restore(flags);
 

[PATCH 5/7] HPET-RTC: disable interrupt when no longer needed

[Clemens Ladisch]

When the emulated RTC interrupt is no longer needed, we better disable
it; otherwise, we get a spurious interrupt whenever the timer has
rolled over and reaches the same comparator value.

Having a superfluous interrupt every five minutes doesn't hurt much,
but it's bad style anyway.  ;-)

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/arch/i386/kernel/time_hpet.c
===================================================================
--- linux-2.6.13.orig/arch/i386/kernel/time_hpet.c	2005-09-27 21:54:12.000000000 +0200
+++ linux-2.6.13/arch/i386/kernel/time_hpet.c	2005-09-27 21:56:38.000000000 +0200
@@ -319,8 +319,12 @@ static void hpet_rtc_timer_reinit(void)
 {
 	unsigned int cfg, cnt;
 
-	if (!(PIE_on | AIE_on | UIE_on))
+	if (unlikely(!(PIE_on | AIE_on | UIE_on))) {
+		cfg = hpet_readl(HPET_T1_CFG);
+		cfg &= ~HPET_TN_ENABLE;
+		hpet_writel(cfg, HPET_T1_CFG);
 		return;
+	}
 
 	if (PIE_on && (PIE_freq > DEFAULT_RTC_INT_FREQ))
 		hpet_rtc_int_freq = PIE_freq;
Index: linux-2.6.13/arch/x86_64/kernel/time.c
===================================================================
--- linux-2.6.13.orig/arch/x86_64/kernel/time.c	2005-09-27 21:54:12.000000000 +0200
+++ linux-2.6.13/arch/x86_64/kernel/time.c	2005-09-27 21:57:27.000000000 +0200
@@ -1149,8 +1149,12 @@ static void hpet_rtc_timer_reinit(void)
 {
 	unsigned int cfg, cnt;
 
-	if (!(PIE_on | AIE_on | UIE_on))
+	if (unlikely(!(PIE_on | AIE_on | UIE_on))) {
+		cfg = hpet_readl(HPET_T1_CFG);
+		cfg &= ~HPET_TN_ENABLE;
+		hpet_writel(cfg, HPET_T1_CFG);
 		return;
+	}
 
 	if (PIE_on && (PIE_freq > DEFAULT_RTC_INT_FREQ))
 		hpet_rtc_int_freq = PIE_freq;

[PATCH 6/7] HPET-RTC: fix timer config register accesses

[Clemens Ladisch]

Make sure that the RTC timer is in non-periodic mode; some stupid BIOS
might have initialized it to periodic mode.

Furthermore, don't set the SETVAL bit in the config register.  This
wouldn't have any effect unless the timer was in period mode (which it
isn't), and then the actual timer frequency would be half that of the
desired one because incrementing the comparator in the interrupt
handler would be done after the hardware has already incremented it
itself.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/arch/i386/kernel/time_hpet.c
===================================================================
--- linux-2.6.13.orig/arch/i386/kernel/time_hpet.c	2005-09-27 21:56:38.000000000 +0200
+++ linux-2.6.13/arch/i386/kernel/time_hpet.c	2005-09-27 21:59:13.000000000 +0200
@@ -309,7 +309,8 @@ int hpet_rtc_timer_init(void)
 	local_irq_restore(flags);
 
 	cfg = hpet_readl(HPET_T1_CFG);
-	cfg |= HPET_TN_ENABLE | HPET_TN_SETVAL | HPET_TN_32BIT;
+	cfg &= ~HPET_TN_PERIODIC;
+	cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
 	hpet_writel(cfg, HPET_T1_CFG);
 
 	return 1;
@@ -335,12 +336,6 @@ static void hpet_rtc_timer_reinit(void)
 	cnt = hpet_readl(HPET_T1_CMP);
 	cnt += hpet_tick*HZ/hpet_rtc_int_freq;
 	hpet_writel(cnt, HPET_T1_CMP);
-
-	cfg = hpet_readl(HPET_T1_CFG);
-	cfg |= HPET_TN_ENABLE | HPET_TN_SETVAL | HPET_TN_32BIT;
-	hpet_writel(cfg, HPET_T1_CFG);
-
-	return;
 }
 
 /*
Index: linux-2.6.13/arch/x86_64/kernel/time.c
===================================================================
--- linux-2.6.13.orig/arch/x86_64/kernel/time.c	2005-09-27 21:57:27.000000000 +0200
+++ linux-2.6.13/arch/x86_64/kernel/time.c	2005-09-27 21:59:13.000000000 +0200
@@ -1139,7 +1139,8 @@ int hpet_rtc_timer_init(void)
 	local_irq_restore(flags);
 
 	cfg = hpet_readl(HPET_T1_CFG);
-	cfg |= HPET_TN_ENABLE | HPET_TN_SETVAL | HPET_TN_32BIT;
+	cfg &= ~HPET_TN_PERIODIC;
+	cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
 	hpet_writel(cfg, HPET_T1_CFG);
 
 	return 1;
@@ -1165,12 +1166,6 @@ static void hpet_rtc_timer_reinit(void)
 	cnt = hpet_readl(HPET_T1_CMP);
 	cnt += hpet_tick*HZ/hpet_rtc_int_freq;
 	hpet_writel(cnt, HPET_T1_CMP);
-
-	cfg = hpet_readl(HPET_T1_CFG);
-	cfg |= HPET_TN_ENABLE | HPET_TN_SETVAL | HPET_TN_32BIT;
-	hpet_writel(cfg, HPET_T1_CFG);
-
-	return;
 }
 
 /*

[PATCH 7/7] HPET-RTC: cache the comparator register

[Clemens Ladisch]

Reads from an HPET register require a round trip to the south bridge
and are almost as slow as PCI reads.  By caching the last value we've
written to the comparator register, we can eliminate all HPET reads
from the fast path in the emulated RTC interrupt handler.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Index: linux-2.6.13/arch/i386/kernel/time_hpet.c
===================================================================
--- linux-2.6.13.orig/arch/i386/kernel/time_hpet.c	2005-09-27 21:59:13.000000000 +0200
+++ linux-2.6.13/arch/i386/kernel/time_hpet.c	2005-09-27 22:01:29.000000000 +0200
@@ -275,6 +275,7 @@ static unsigned long PIE_freq = DEFAULT_
 static unsigned long PIE_count;
 
 static unsigned long hpet_rtc_int_freq; /* RTC interrupt frequency */
+static unsigned int hpet_t1_cmp; /* cached comparator register */
 
 /*
  * Timer 1 for RTC, we do not use periodic interrupt feature,
@@ -306,6 +307,7 @@ int hpet_rtc_timer_init(void)
 	cnt = hpet_readl(HPET_COUNTER);
 	cnt += ((hpet_tick*HZ)/hpet_rtc_int_freq);
 	hpet_writel(cnt, HPET_T1_CMP);
+	hpet_t1_cmp = cnt;
 	local_irq_restore(flags);
 
 	cfg = hpet_readl(HPET_T1_CFG);
@@ -333,9 +335,10 @@ static void hpet_rtc_timer_reinit(void)
 		hpet_rtc_int_freq = DEFAULT_RTC_INT_FREQ;
 
 	/* It is more accurate to use the comparator value than current count.*/
-	cnt = hpet_readl(HPET_T1_CMP);
+	cnt = hpet_t1_cmp;
 	cnt += hpet_tick*HZ/hpet_rtc_int_freq;
 	hpet_writel(cnt, HPET_T1_CMP);
+	hpet_t1_cmp = cnt;
 }
 
 /*
Index: linux-2.6.13/arch/x86_64/kernel/time.c
===================================================================
--- linux-2.6.13.orig/arch/x86_64/kernel/time.c	2005-09-27 21:59:13.000000000 +0200
+++ linux-2.6.13/arch/x86_64/kernel/time.c	2005-09-27 22:01:29.000000000 +0200
@@ -1100,6 +1100,7 @@ static unsigned long PIE_freq = DEFAULT_
 static unsigned long PIE_count;
 
 static unsigned long hpet_rtc_int_freq; /* RTC interrupt frequency */
+static unsigned int hpet_t1_cmp; /* cached comparator register */
 
 int is_hpet_enabled(void)
 {
@@ -1136,6 +1137,7 @@ int hpet_rtc_timer_init(void)
 	cnt = hpet_readl(HPET_COUNTER);
 	cnt += ((hpet_tick*HZ)/hpet_rtc_int_freq);
 	hpet_writel(cnt, HPET_T1_CMP);
+	hpet_t1_cmp = cnt;
 	local_irq_restore(flags);
 
 	cfg = hpet_readl(HPET_T1_CFG);
@@ -1163,9 +1165,10 @@ static void hpet_rtc_timer_reinit(void)
 		hpet_rtc_int_freq = DEFAULT_RTC_INT_FREQ;
 
 	/* It is more accurate to use the comparator value than current count.*/
-	cnt = hpet_readl(HPET_T1_CMP);
+	cnt = hpet_t1_cmp;
 	cnt += hpet_tick*HZ/hpet_rtc_int_freq;
 	hpet_writel(cnt, HPET_T1_CMP);
+	hpet_t1_cmp = cnt;
 }
 
 /*

Re: [PATCH 5/7] HPET-RTC: disable interrupt when no longer needed

[Venkatesh Pallipadi]

On Wed, Sep 28, 2005 at 09:12:26AM +0200, Clemens Ladisch wrote:
> When the emulated RTC interrupt is no longer needed, we better disable
> it; otherwise, we get a spurious interrupt whenever the timer has
> rolled over and reaches the same comparator value.
> 
> Having a superfluous interrupt every five minutes doesn't hurt much,
> but it's bad style anyway.  ;-)
> 

Do you really see the interrupt every five minutes once RTC is disabled.
Or is this to prevent a possible interrupt?
I had assumed while in one-shot interrupt mode, HPET would automatically unarm
after generating the interrupt, so that we won't get interrupts any more.

Thanks,
Venki

Re: [PATCH 6/7] HPET-RTC: fix timer config register accesses

[Venkatesh Pallipadi]

On Wed, Sep 28, 2005 at 09:12:31AM +0200, Clemens Ladisch wrote:
> Make sure that the RTC timer is in non-periodic mode; some stupid BIOS
> might have initialized it to periodic mode.
> 
> Furthermore, don't set the SETVAL bit in the config register.  This
> wouldn't have any effect unless the timer was in period mode (which it
> isn't), and then the actual timer frequency would be half that of the
> desired one because incrementing the comparator in the interrupt
> handler would be done after the hardware has already incremented it
> itself.
> 
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Ack on these two patches.
[PATCH 7/7] HPET-RTC: cache the comparator register
[PATCH 6/7] HPET-RTC: fix timer config register accesses

Andrew: Please apply.

Thanks,
Venki

Re: [PATCH 0/7] HPET fixes and enhancements

[Bob Picco]

Clemens Ladisch wrote:	[Wed Sep 28 2005, 03:11:55AM EDT]
> These patches remove a bunch of warts and quirks from the HPET drivers.
> 
> 
>  arch/i386/kernel/time_hpet.c |   20 +++++++++++---------
>  arch/x86_64/kernel/time.c    |   20 +++++++++++---------
>  drivers/char/hpet.c          |   39 ++++++++++++++++++++++++---------------
>  3 files changed, 46 insertions(+), 33 deletions(-)
> -
Ack on patches [1-4].

For x86_64 you can try Andi Kleen.  Venki and I weren't responsible
for x86_64 HPET.

thanks,

bob

Re: [PATCH 5/7] HPET-RTC: disable interrupt when no longer needed

[Clemens Ladisch]

Venkatesh Pallipadi wrote:
> On Wed, Sep 28, 2005 at 09:12:26AM +0200, Clemens Ladisch wrote:
> > When the emulated RTC interrupt is no longer needed, we better disable
> > it; otherwise, we get a spurious interrupt whenever the timer has
> > rolled over and reaches the same comparator value.
> >
> > Having a superfluous interrupt every five minutes doesn't hurt much,
> > but it's bad style anyway.  ;-)
>
> Do you really see the interrupt every five minutes once RTC is disabled.

Yes; at least on my Intel chipset.  ;-)

> I had assumed while in one-shot interrupt mode, HPET would automatically unarm
> after generating the interrupt, so that we won't get interrupts any more.

The spec never mentions this.  What it mentions is that it was
designed so that it can be implemented in as few gates as possible.


Regards,
Clemens