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

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

[Clemens Ladisch]

Randy.Dunlap wrote:

> On Tue, 18 Oct 2005, Clemens Ladisch wrote:
>
> > Randy.Dunlap wrote:
> > > hpet_poll, HPET_IE_ON failed: (5) Input/output error
> >
> > This probably means that the interrupt isn't free.
>
> Isn't free because it is already in use as a system timer
> interrupt, for example?

If it tries to use interrupt 0, yes.

> Does this test work (succeed) for you?  If so, is HPET being
> used as replacement for legacy timer and RTC?  (as it is where I
> am testing)

Yes.  Yes.


However, I've patched my kernel to initialize the HPET manually
because my BIOS doesn't bother to do it at all.  A quick Google search
shows that in most cases where the BIOS _does_ bother, the third timer
(which is the only free one after system timer and RTC have grabbed
theirs) didn't get initialized and is still set to interrupt 0 (which
isn't actually supported by most HPET hardware).

This means that hpet.c must initialize the interrupt routing register
in this case.  I'll write a patch for this.


Regards,
Clemens

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

[Petr Vandrovec]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

Clemens Ladisch wrote:
> Randy.Dunlap wrote:

> However, I've patched my kernel to initialize the HPET manually
> because my BIOS doesn't bother to do it at all.  A quick Google search
> shows that in most cases where the BIOS _does_ bother, the third timer
> (which is the only free one after system timer and RTC have grabbed
> theirs) didn't get initialized and is still set to interrupt 0 (which
> isn't actually supported by most HPET hardware).
> 
> This means that hpet.c must initialize the interrupt routing register
> in this case.  I'll write a patch for this.

I'm using attached diff.  But I gave up on HPET.  On VIA periodic mode
is hopelessly broken, on AMD HPET read takes about 1500ns (23 HPET cycles),
and current Linux RTC emulation has problem that when interrupt is delayed
it stops until counter rollover.  And fixing this would add at least 1.5us
to the interrupt handler, and it seems quite lot to me...
								Petr

[-- Attachment #2: hpet.diff --]
[-- Type: text/x-patch, Size: 3600 bytes --]

diff -urdN linux/drivers/char/hpet.c linux/drivers/char/hpet.c
--- linux/drivers/char/hpet.c	2005-10-08 16:26:51.000000000 +0200
+++ linux/drivers/char/hpet.c	2005-10-08 22:32:43.000000000 +0200
@@ -70,6 +70,7 @@
 	unsigned int hd_flags;
 	unsigned int hd_irq;
 	unsigned int hd_hdwirq;
+	int hd_levelirq;
 	char hd_name[HPET_DEV_NAME];
 };
 
@@ -120,6 +121,15 @@
 	unsigned long isr;
 
 	devp = data;
+	isr = (1 << (devp - devp->hd_hpets->hp_dev));
+	if (devp->hd_levelirq) {
+		unsigned long v;
+		
+		v = readl(&devp->hd_hpet->hpet_isr);
+		if (!(v & isr)) {
+			return IRQ_NONE;
+		}
+	}	
 
 	spin_lock(&hpet_lock);
 	devp->hd_irqdata++;
@@ -137,7 +147,7 @@
 			      &devp->hd_timer->hpet_compare);
 	}
 
-	isr = (1 << (devp - devp->hd_hpets->hp_dev));
+	/* Spec says that only 0 may be written to status bit when corresponding timer is in edge mode! */
 	writeq(isr, &devp->hd_hpet->hpet_isr);
 	spin_unlock(&hpet_lock);
 
@@ -351,6 +361,44 @@
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
 
+static unsigned int hpet_find_irq(struct hpet_dev *devp)
+{
+	unsigned long long v;
+	unsigned int irqmask;
+	struct hpet_timer __iomem *timer;
+	unsigned int irq;
+	unsigned int hpet_id;
+
+	irq = devp->hd_hdwirq;
+	if (irq)
+		return irq;
+
+	hpet_id = devp - devp->hd_hpets->hp_dev;
+	timer = devp->hd_timer;
+	v = readq(&timer->hpet_config);
+	irq = (v & Tn_INT_ROUTE_CNF_MASK) >> Tn_INT_ROUTE_CNF_SHIFT;
+	irqmask = (v & Tn_INT_ROUTE_CAP_MASK) >> Tn_INI_ROUTE_CAP_SHIFT;
+	/* Try already assigned irq first?! */
+	for (irq = 0; irq < 32; irq++) {
+		int acpiirq = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW);
+//		printk(KERN_ERR "Mapping %u => %u\n", irq, acpiirq);
+		/* Skip 0 (no such APIC pin), 2 (8254) and 8 (RTC) */
+		if (irq == 0 || irq == 2 || irq == 8)
+			continue;
+		if (can_request_irq(acpiirq, SA_INTERRUPT)) {
+			devp->hd_levelirq = 1;
+			v = (v & ~Tn_INT_ROUTE_CNF_MASK) | (irq << Tn_INT_ROUTE_CNF_SHIFT) | Tn_INT_TYPE_CNF_MASK;
+			writeq(v, &timer->hpet_config);
+			printk(KERN_DEBUG "hpet%u.%u: Reserved IRQ %u\n", devp->hd_hpets->hp_which, hpet_id, acpiirq);
+			return acpiirq;
+		} else {
+			printk(KERN_DEBUG "hpet%u.%u: IRQ %u is busy\n", devp->hd_hpets->hp_which, hpet_id, acpiirq);
+		}
+	}
+	printk(KERN_DEBUG "hpet%u.%u: No free IRQ found\n", devp->hd_hpets->hp_which, hpet_id);
+	return 0;
+}
+
 static int hpet_ioctl_ieon(struct hpet_dev *devp)
 {
 	struct hpet_timer __iomem *timer;
@@ -375,14 +422,14 @@
 	devp->hd_flags |= HPET_IE;
 	spin_unlock_irq(&hpet_lock);
 
 	t = readq(&timer->hpet_config);
-	irq = devp->hd_hdwirq;
+	irq = hpet_find_irq(devp);
 
 	if (irq) {
-		sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
+		sprintf(devp->hd_name, "hpet%d.%d", hpetp->hp_which, (int)(devp - hpetp->hp_dev));
 
 		if (request_irq
-		    (irq, hpet_interrupt, SA_INTERRUPT, devp->hd_name, (void *)devp)) {
+		    (irq, hpet_interrupt, (devp->hd_levelirq ? SA_SHIRQ : 0) | SA_INTERRUPT, devp->hd_name, (void *)devp)) {
 			printk(KERN_ERR "hpet: IRQ %d is not free\n", irq);
 			irq = 0;
 		}
@@ -477,8 +527,12 @@
 		{
 			struct hpet_info info;
 
-			info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
-							 devp->hd_ireqfreq);
+			if (devp->hd_ireqfreq)
+				info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
+								 devp->hd_ireqfreq);
+			else
+				info.hi_ireqfreq = ~0UL;
+
 			info.hi_flags =
 			    readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
 			info.hi_hpet = devp->hd_hpets->hp_which;
@@ -516,7 +570,7 @@
 			break;
 		}
 
-		if (arg & (arg - 1)) {
+		if (!arg) {
 			err = -EINVAL;
 			break;
 		}

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

[Clemens Ladisch]

Petr Vandrovec wrote:
> Clemens Ladisch wrote:
> > However, I've patched my kernel to initialize the HPET manually
> > because my BIOS doesn't bother to do it at all.  A quick Google search
> > shows that in most cases where the BIOS _does_ bother, the third timer
> > (which is the only free one after system timer and RTC have grabbed
> > theirs) didn't get initialized and is still set to interrupt 0 (which
> > isn't actually supported by most HPET hardware).
> >
> > This means that hpet.c must initialize the interrupt routing register
> > in this case.  I'll write a patch for this.
>
> I'm using attached diff.

The other changes of your patch are already in the -mm kernel.

> But I gave up on HPET.  On VIA periodic mode is hopelessly broken,

I've heard it works with timer 0, and the capability bit on timer 1 is
just wrong.

> on AMD HPET read takes about 1500ns (23 HPET cycles),

It's a round trip to the southbridge.  Intel needs about 500 ns.

> and current Linux RTC emulation has problem that when interrupt is
> delayed it stops until counter rollover.

I'm planning to fix this.

OTOH, the hpet.c implementation has the problem that all interrupt
delays affect the timer, i.e., it isn't precise anymore.

> And fixing this would add at least 1.5us to the interrupt handler,
> and it seems quite lot to me...

I didn't measure how much reading the RTC registers costs us, but
those aren't likely to be faster.

I'm thinking of a different approach:  Assuming that such a big delay
almost never actually does happen, we run a separate watchdog timer
(using a kernel timer that is guaranteed to work) at a much lower
frequency to check whether the real timer got stuck.  This trades off
the HPET register read against the timer_list overhead (and that we
still lose _some_ interrupts when the worst case happens).


Regards,
Clemens

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

[Petr Vandrovec]

Clemens Ladisch wrote:
> Petr Vandrovec wrote:
> 
>>Clemens Ladisch wrote:
>>
>>>However, I've patched my kernel to initialize the HPET manually
>>>because my BIOS doesn't bother to do it at all.  A quick Google search
>>>shows that in most cases where the BIOS _does_ bother, the third timer
>>>(which is the only free one after system timer and RTC have grabbed
>>>theirs) didn't get initialized and is still set to interrupt 0 (which
>>>isn't actually supported by most HPET hardware).
>>>
>>>This means that hpet.c must initialize the interrupt routing register
>>>in this case.  I'll write a patch for this.
>>
>>I'm using attached diff.
> 
> 
> The other changes of your patch are already in the -mm kernel.
> 
> 
>>But I gave up on HPET.  On VIA periodic mode is hopelessly broken,
> 
> 
> I've heard it works with timer 0, and the capability bit on timer 1 is
> just wrong.

Nope.  Periodic mode works (I've made my tests on timer #2), you can just
set only period (through way which sets value according to the spec), and
you cannot set current value (at least I do not know how).  So I can
program VIA hardware to generate periodic interrupt, there is just
unavoidable delay up to 5 minutes. I've worked around by setting period
to 1 tick, so in 5 minutes value and main timer synchronize, and if
timer is not stopped after that then it stays synchronized with main timer.

>>And fixing this would add at least 1.5us to the interrupt handler,
>>and it seems quite lot to me...
> 
> 
> I didn't measure how much reading the RTC registers costs us, but
> those aren't likely to be faster.
> 
> I'm thinking of a different approach:  Assuming that such a big delay
> almost never actually does happen, we run a separate watchdog timer
> (using a kernel timer that is guaranteed to work) at a much lower
> frequency to check whether the real timer got stuck.  This trades off
> the HPET register read against the timer_list overhead (and that we
> still lose _some_ interrupts when the worst case happens).

It would work for VMware's use of /dev/rtc if number of missed interrupts
will be reported on next read.  Otherwise it might be a problem for
keeping time between host and virtual machines in sync.
								Petr

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

[Clemens Ladisch]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 253 bytes --]

I wrote:
> This means that hpet.c must initialize the interrupt routing register
> in this case.  I'll write a patch for this.

Okay, this is a quick hack, untested.  It just tries to set the first
interrupt that the timer could use.


Regards,
Clemens

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1667 bytes --]

Index: linux/arch/i386/kernel/time_hpet.c
===================================================================
--- linux.orig/arch/i386/kernel/time_hpet.c	2005-10-02 17:23:20.000000000 +0200
+++ linux/arch/i386/kernel/time_hpet.c	2005-10-19 14:37:55.000000000 +0200
@@ -196,6 +196,12 @@ int __init hpet_enable(void)
 
 			for (i = 2, timer = &hpet->hpet_timers[2]; i < ntimer;
 				timer++, i++)
+				if (!(timer->hpet_config & Tn_INT_ROUTE_CNF_MASK) &&
+				    !(timer->hpet_config & Tn_FSB_EN_CNF_MASK)) {
+					int irq = ffs(timer->hpet_config >> Tn_INI_ROUTE_CAP_SHIFT);
+					timer->hpet_config |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+					printk(KERN_INFO "HPET: timer %d configured for IRQ %d\n", i, irq);
+				}
 				hd.hd_irq[i] = (timer->hpet_config &
 					Tn_INT_ROUTE_CNF_MASK) >>
 					Tn_INT_ROUTE_CNF_SHIFT;
Index: linux/arch/x86_64/kernel/time.c
===================================================================
--- linux.orig/arch/x86_64/kernel/time.c	2005-10-02 17:23:20.000000000 +0200
+++ linux/arch/x86_64/kernel/time.c	2005-10-19 14:36:23.000000000 +0200
@@ -783,6 +783,12 @@ static __init int late_hpet_init(void)
 
 		for (i = 2, timer = &hpet->hpet_timers[2]; i < ntimer;
 		     timer++, i++)
+			if (!(timer->hpet_config & Tn_INT_ROUTE_CNF_MASK) &&
+			    !(timer->hpet_config & Tn_FSB_EN_CNF_MASK)) {
+				int irq = ffs(timer->hpet_config >> Tn_INI_ROUTE_CAP_SHIFT);
+				timer->hpet_config |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+				printk(KERN_INFO "HPET: timer %d configured for IRQ %d\n", i, irq);
+			}
 			hd.hd_irq[i] = (timer->hpet_config &
 					Tn_INT_ROUTE_CNF_MASK) >>
 				Tn_INT_ROUTE_CNF_SHIFT;

RE: [PATCH 0/7] more HPET fixes and enhancements

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org 
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of 
>Clemens Ladisch
>Sent: Wednesday, October 19, 2005 6:49 AM
>To: Randy.Dunlap
>Cc: linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 0/7] more HPET fixes and enhancements
>
>I wrote:
>> This means that hpet.c must initialize the interrupt routing register
>> in this case.  I'll write a patch for this.
>
>Okay, this is a quick hack, untested.  It just tries to set the first
>interrupt that the timer could use.
>


You will need more changes to make interrupt work. Especially if the
particular IRQ that you are using here is not used by any other PCI
device in the system. Then BIOS won't report anything on that IRQ and
all the things done in setup_IO_APIC_irqs() should be done for this
particular IRQ. And it will depend on whether IOAPIC/PIC is ised for
interrupts and such things as well. Atleast that what I remember from
last time I got HPET to work with IRQs other than IRQ0 and IRQ8.

Thanks,
Venki

[PATCH 0/7] more HPET fixes and enhancements

[Clemens Ladisch]

Another round of HPET bugfixes and cleanups.

 drivers/char/hpet.c |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

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

[Bob Picco]

Clemens Ladisch wrote:	[Tue Oct 04 2005, 08:41:26AM EDT]
> Another round of HPET bugfixes and cleanups.
> 
>  drivers/char/hpet.c |   35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)

Clemens:

These improvements and fixes look good to me.  Thanks for your work.

bob

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

[Randy.Dunlap]

On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:

> Another round of HPET bugfixes and cleanups.
> 
>  drivers/char/hpet.c |   35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)

Hi,

I've applied and tested all of these along with what is
currently in -mm (only -mm hpet + timer patches).

By "tested" I mean that I booted the kernel.  :)

What kind of testing have you done?
Do you have any timer test tools that you use to verify that
timers are actually working as expected?

Or maybe I could/should ask one or all of:
- John Stultz
- Thomas Gleixner
- George Anzinger (HRT project does have some tests)

Thanks,
---
~Randy

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

[Clemens Ladisch]

Randy.Dunlap wrote:

> On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:
>
> > Another round of HPET bugfixes and cleanups.
>
> I've applied and tested all of these along with what is
> currently in -mm (only -mm hpet + timer patches).
>
> By "tested" I mean that I booted the kernel.  :)
>
> What kind of testing have you done?
> Do you have any timer test tools that you use to verify that
> timers are actually working as expected?

Apart from the test program in hpet.txt, I'm using the ALSA HPET
driver (contained in the ALSA 1.0.9 package, but not yet in the kernel
tree) and then just test it using the ALSA API and/or use it as the
MIDI sequencer timer.


HTH
Clemens

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

[Takashi Iwai]

At Mon, 17 Oct 2005 18:30:40 +0200 (METDST),
Clemens Ladisch wrote:
> 
> Randy.Dunlap wrote:
> 
> > On Tue, 04 Oct 2005 14:41:26 +0200 (MEST) Clemens Ladisch wrote:
> >
> > > Another round of HPET bugfixes and cleanups.
> >
> > I've applied and tested all of these along with what is
> > currently in -mm (only -mm hpet + timer patches).
> >
> > By "tested" I mean that I booted the kernel.  :)
> >
> > What kind of testing have you done?
> > Do you have any timer test tools that you use to verify that
> > timers are actually working as expected?
> 
> Apart from the test program in hpet.txt, I'm using the ALSA HPET
> driver (contained in the ALSA 1.0.9 package, but not yet in the kernel
> tree) and then just test it using the ALSA API and/or use it as the
> MIDI sequencer timer.

Let's push it to kernel tree :)


Takashi