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(-)
The variable hpet_ntimer is never read, so remove it.
Signed-off-by: Clemens Ladisch <[email protected]>
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;
This patch removes several reads of a timer's config register that
serve no purpose whatsoever.
Signed-off-by: Clemens Ladisch <[email protected]>
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;
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 <[email protected]>
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;
}
/*
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 <[email protected]>
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;
}
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 <[email protected]>
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;
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 <[email protected]>
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);
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 <[email protected]>
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;
}
/*
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
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 <[email protected]>
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
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
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
>-----Original Message-----
>From: [email protected]
>[mailto:[email protected]] On Behalf Of Clemens Ladisch
>Sent: Wednesday, September 28, 2005 11:30 PM
>To: Pallipadi, Venkatesh
>Cc: [email protected]; Bob Picco
>Subject: Re: [PATCH 5/7] HPET-RTC: disable interrupt when no
>longer needed
>
>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.
>
Verified in the latest version of the SPEC. It indeed says that
one shot timer can happen more than once when the 32 bit counter
wraps around. So, this patch is also required. Thanks for all the fixes.
Andrew, Please pick this one as well.
Thanks,
venki