[PATCH] x86: Get irq for hpet timer

[PATCH] x86: Get irq for hpet timer

[Kevin Hao]

Hi,

x86: get irq for hpet timer
    
HPET timer's IRQ is 0 by default, so we have to select which irq
will be used for these timers. We wait to set the timer's irq until
we really turn on interrupt in order to reduce the chance of
conflicting with some legacy device.
    
Signed-off-by: Kevin Hao <kexin.hao@windriver.com>

---
 drivers/char/hpet.c  |   62 +++++++++++++++++++++++++++++++++++++------------
 include/linux/hpet.h |    3 +-
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e7fb0bc..67714c2 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -383,6 +383,51 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
 
+static int hpet_timer_get_irq(struct hpet_dev *devp)
+{
+	struct hpet_timer __iomem *timer;
+	struct hpet __iomem *hpet;
+	struct hpets *hpetp;
+	unsigned long cap, irq_flags;
+	int irq;
+
+	timer = devp->hd_timer;
+	hpet = devp->hd_hpet;
+	hpetp = devp->hd_hpets;
+
+	irq = devp->hd_hdwirq;
+
+	sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
+	irq_flags = devp->hd_flags & HPET_SHARED_IRQ
+					? IRQF_SHARED : IRQF_DISABLED;
+	if (irq) {
+		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;
+		}
+		return irq;
+	}
+
+	cap = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
+				 >> Tn_INT_ROUTE_CAP_SHIFT;
+
+	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
+		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
+		if (request_irq(irq, hpet_interrupt, irq_flags,
+				devp->hd_name, (void *)devp)) {
+			printk(KERN_WARNING "hpet: IRQ %d is not free\n", irq);
+			continue;
+		}
+		break;
+	}
+
+	if  (irq >= HPET_MAX_IRQ)
+		irq = 0;
+
+	return irq;
+}
+
 static int hpet_ioctl_ieon(struct hpet_dev *devp)
 {
 	struct hpet_timer __iomem *timer;
@@ -412,21 +457,7 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
 		devp->hd_flags |= HPET_SHARED_IRQ;
 	spin_unlock_irq(&hpet_lock);
 
-	irq = devp->hd_hdwirq;
-
-	if (irq) {
-		unsigned long irq_flags;
-
-		sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
-		irq_flags = devp->hd_flags & HPET_SHARED_IRQ
-						? IRQF_SHARED : IRQF_DISABLED;
-		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;
-		}
-	}
-
+	irq = hpet_timer_get_irq(devp);
 	if (irq == 0) {
 		spin_lock_irq(&hpet_lock);
 		devp->hd_flags ^= HPET_IE;
@@ -438,6 +469,7 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
 	t = devp->hd_ireqfreq;
 	v = readq(&timer->hpet_config);
 	g = v | Tn_INT_ENB_CNF_MASK;
+	g |= irq << Tn_INT_ROUTE_CNF_SHIFT;
 
 	if (devp->hd_flags & HPET_PERIODIC) {
 		write_counter(t, &timer->hpet_compare);
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 2dc29ce..440ca72 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -37,6 +37,7 @@ struct hpet {
 #define	hpet_compare	_u1._hpet_compare
 
 #define	HPET_MAX_TIMERS	(32)
+#define HPET_MAX_IRQ	(32)
 
 /*
  * HPET general capabilities register
@@ -64,7 +65,7 @@ struct hpet {
  */
 
 #define	Tn_INT_ROUTE_CAP_MASK		(0xffffffff00000000ULL)
-#define	Tn_INI_ROUTE_CAP_SHIFT		(32UL)
+#define	Tn_INT_ROUTE_CAP_SHIFT		(32UL)
 #define	Tn_FSB_INT_DELCAP_MASK		(0x8000UL)
 #define	Tn_FSB_INT_DELCAP_SHIFT		(15)
 #define	Tn_FSB_EN_CNF_MASK		(0x4000UL)

Re: [PATCH] x86: Get irq for hpet timer

[Balaji Rao R]

On Fri, May 16, 2008 at 11:35 AM, Kevin Hao <kexin.hao@windriver.com> wrote:
>
> Hi,
>
> x86: get irq for hpet timer
>
> HPET timer's IRQ is 0 by default, so we have to select which irq
> will be used for these timers. We wait to set the timer's irq until
> we really turn on interrupt in order to reduce the chance of
> conflicting with some legacy device.
>
Yes, this is alright. But  i've seen some machines where we have a
valid IRQ in 'devp->hd_hdwirq' which you should not ignore. So, first
check if it is zero and only then do a hpet_timer_get_irq.

--
warm regards

Balaji Rao
NITK Surathkal

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Fri, 2008-05-16 at 13:23 +0530, Balaji Rao R wrote:
> On Fri, May 16, 2008 at 11:35 AM, Kevin Hao <kexin.hao@windriver.com> wrote:
> >
> > Hi,
> >
> > x86: get irq for hpet timer
> >
> > HPET timer's IRQ is 0 by default, so we have to select which irq
> > will be used for these timers. We wait to set the timer's irq until
> > we really turn on interrupt in order to reduce the chance of
> > conflicting with some legacy device.
> >
> Yes, this is alright. But  i've seen some machines where we have a
> valid IRQ in 'devp->hd_hdwirq' which you should not ignore. So, first
> check if it is zero and only then do a hpet_timer_get_irq.

Yes, I have checked that case in hpet_timer_get_irq function. See the
following code:

+       irq = devp->hd_hdwirq;


+       if (irq) {
+               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;
+               }
+               return irq;
+       }

Is that right?

Thanks for your comments.

Best Regards,
Kevin

> 
> --
> warm regards
> 
> Balaji Rao
> NITK Surathkal

Re: [PATCH] x86: Get irq for hpet timer

[Balaji Rao]

On Friday 16 May 2008 01:33:25 pm Kevin Hao wrote:
> On Fri, 2008-05-16 at 13:23 +0530, Balaji Rao R wrote:
> > On Fri, May 16, 2008 at 11:35 AM, Kevin Hao <kexin.hao@windriver.com> 
wrote:
> > > Hi,
> > >
> > > x86: get irq for hpet timer
> > >
> > > HPET timer's IRQ is 0 by default, so we have to select which irq
> > > will be used for these timers. We wait to set the timer's irq until
> > > we really turn on interrupt in order to reduce the chance of
> > > conflicting with some legacy device.
> >
> > Yes, this is alright. But  i've seen some machines where we have a
> > valid IRQ in 'devp->hd_hdwirq' which you should not ignore. So, first
> > check if it is zero and only then do a hpet_timer_get_irq.
>
> Yes, I have checked that case in hpet_timer_get_irq function. See the
> following code:
>
> +       irq = devp->hd_hdwirq;
>
>
> +       if (irq) {
> +               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;
> +               }
> +               return irq;
> +       }
>
> Is that right?
>
Yes. You are right. Sorry for not going through the patch fully before making 
noise.

-- 
Warm Regards,

Balaji Rao
Dept. of Mechanical Engineering
NITK

Re: [PATCH] x86: Get irq for hpet timer

[Clemens Ladisch]

Kevin Hao wrote:
> HPET timer's IRQ is 0 by default, so we have to select which irq
> will be used for these timers. We wait to set the timer's irq until
> we really turn on interrupt in order to reduce the chance of
> conflicting with some legacy device.
>
> +	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
> +		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
> +		if (request_irq(irq, hpet_interrupt, irq_flags,
> +				devp->hd_name, (void *)devp)) {
> +			printk(KERN_WARNING "hpet: IRQ %d is not free\n", irq);
> +			continue;

This warning message will be output for every interrupt that is in use
by another device.  I think it would be better to postpone complaining
until after the loop, if no interrupt has been found at all.


Regards,
Clemens

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Fri, 2008-05-16 at 10:46 +0200, Clemens Ladisch wrote:
> Kevin Hao wrote:
> > HPET timer's IRQ is 0 by default, so we have to select which irq
> > will be used for these timers. We wait to set the timer's irq until
> > we really turn on interrupt in order to reduce the chance of
> > conflicting with some legacy device.
> >
> > +	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
> > +		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
> > +		if (request_irq(irq, hpet_interrupt, irq_flags,
> > +				devp->hd_name, (void *)devp)) {
> > +			printk(KERN_WARNING "hpet: IRQ %d is not free\n", irq);
> > +			continue;
> 
> This warning message will be output for every interrupt that is in use
> by another device.  I think it would be better to postpone complaining
> until after the loop, if no interrupt has been found at all.
> 

Ok, I agree with you.
This is the revised version.

Best Regards,
Kevin
---
 drivers/char/hpet.c  |   63 ++++++++++++++++++++++++++++++++++++++------------
 include/linux/hpet.h |    3 +-
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e7fb0bc..0fdc627 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -383,6 +383,52 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 	return hpet_ioctl_common(devp, cmd, arg, 0);
 }
 
+static int hpet_timer_get_irq(struct hpet_dev *devp)
+{
+	struct hpet_timer __iomem *timer;
+	struct hpet __iomem *hpet;
+	struct hpets *hpetp;
+	unsigned long cap, irq_flags;
+	int irq;
+
+	timer = devp->hd_timer;
+	hpet = devp->hd_hpet;
+	hpetp = devp->hd_hpets;
+
+	irq = devp->hd_hdwirq;
+
+	sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
+	irq_flags = devp->hd_flags & HPET_SHARED_IRQ
+					? IRQF_SHARED : IRQF_DISABLED;
+	if (irq) {
+		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;
+		}
+		return irq;
+	}
+
+	cap = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
+				 >> Tn_INT_ROUTE_CAP_SHIFT;
+
+	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
+		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
+		if (request_irq(irq, hpet_interrupt, irq_flags,
+				devp->hd_name, (void *)devp))
+			continue;
+		break;
+	}
+
+	if  (irq >= HPET_MAX_IRQ) {
+		printk(KERN_WARNING "hpet: No available IRQ for %s timer\n",
+			devp->hd_name);
+		irq = 0;
+	}
+
+	return irq;
+}
+
 static int hpet_ioctl_ieon(struct hpet_dev *devp)
 {
 	struct hpet_timer __iomem *timer;
@@ -412,21 +458,7 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
 		devp->hd_flags |= HPET_SHARED_IRQ;
 	spin_unlock_irq(&hpet_lock);
 
-	irq = devp->hd_hdwirq;
-
-	if (irq) {
-		unsigned long irq_flags;
-
-		sprintf(devp->hd_name, "hpet%d", (int)(devp - hpetp->hp_dev));
-		irq_flags = devp->hd_flags & HPET_SHARED_IRQ
-						? IRQF_SHARED : IRQF_DISABLED;
-		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;
-		}
-	}
-
+	irq = hpet_timer_get_irq(devp);
 	if (irq == 0) {
 		spin_lock_irq(&hpet_lock);
 		devp->hd_flags ^= HPET_IE;
@@ -438,6 +470,7 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
 	t = devp->hd_ireqfreq;
 	v = readq(&timer->hpet_config);
 	g = v | Tn_INT_ENB_CNF_MASK;
+	g |= irq << Tn_INT_ROUTE_CNF_SHIFT;
 
 	if (devp->hd_flags & HPET_PERIODIC) {
 		write_counter(t, &timer->hpet_compare);
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 2dc29ce..440ca72 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -37,6 +37,7 @@ struct hpet {
 #define	hpet_compare	_u1._hpet_compare
 
 #define	HPET_MAX_TIMERS	(32)
+#define HPET_MAX_IRQ	(32)
 
 /*
  * HPET general capabilities register
@@ -64,7 +65,7 @@ struct hpet {
  */
 
 #define	Tn_INT_ROUTE_CAP_MASK		(0xffffffff00000000ULL)
-#define	Tn_INI_ROUTE_CAP_SHIFT		(32UL)
+#define	Tn_INT_ROUTE_CAP_SHIFT		(32UL)
 #define	Tn_FSB_INT_DELCAP_MASK		(0x8000UL)
 #define	Tn_FSB_INT_DELCAP_SHIFT		(15)
 #define	Tn_FSB_EN_CNF_MASK		(0x4000UL)

> 
> Regards,
> Clemens

Re: [PATCH] x86: Get irq for hpet timer

[Clemens Ladisch]

Kevin Hao wrote:
> +	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
> +		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
> +		if (request_irq(irq, hpet_interrupt, irq_flags,
> +				devp->hd_name, (void *)devp))

This spams my log with interrupt sharing violations.  As long as we do
not know that the interrupt slot is empty, we need IRQF_PROBE_SHARED
here.

Another problem: the interrupt controller doesn't get correctly
initialized for some interrupt line that didn't already have some
routing:
| $ cat /proc/interrupts
|            CPU0
|   0:         63   IO-APIC-edge      timer
|   1:         96   IO-APIC-edge      i8042
|   2:          0    XT-PIC-XT        hpet2
|   6:          3   IO-APIC-edge      floppy
|   7:          0   IO-APIC-edge      parport0
|   8:          3   IO-APIC-edge      rtc
|   9:          0   IO-APIC-fasteoi   acpi
| ...

Additionally, I vaguely remember that on X86, there is some funny stuff
going on with interrupt lines 0, 2 and 8 which means that the interrupt
number passed to request_irq() is not necessarily identical to the
hardware interrupt line.

I don't know which of these problems is responsible, or if I'm totally
wrong, but on my machine, interrupts from hpet2 do not arrive.


Regards,
Clemens

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Mon, 19 May 2008, Clemens Ladisch wrote:

> Another problem: the interrupt controller doesn't get correctly
> initialized for some interrupt line that didn't already have some
> routing:
> | $ cat /proc/interrupts
> |            CPU0
> |   0:         63   IO-APIC-edge      timer
> |   1:         96   IO-APIC-edge      i8042
> |   2:          0    XT-PIC-XT        hpet2
> |   6:          3   IO-APIC-edge      floppy
> |   7:          0   IO-APIC-edge      parport0
> |   8:          3   IO-APIC-edge      rtc
> |   9:          0   IO-APIC-fasteoi   acpi
> | ...

 IRQ2 cannot be wired to the legacy 8259A chips, because this line is
taken for the cascade from the slave to the master.  If you know for sure
that the HPET2 can be routed to the an otherwise unused input of the I/O
APIC, then you can use add_pin_to_irq() to route the entry manually.

> Additionally, I vaguely remember that on X86, there is some funny stuff
> going on with interrupt lines 0, 2 and 8 which means that the interrupt
> number passed to request_irq() is not necessarily identical to the
> hardware interrupt line.

 You are correct as far as input lines INT0 and INT2 of the I/O APIC are
concerned.  INT8 is not generally a problem except from some systems where
the polarity used to be recorded incorrectly in the BIOS tables (hardware
can be configured for either).  With INT0 and INT2 the following is
usually the case.

 INT0 is wired to the output of the master 8259A legacy controller.  It is
either configured as an ExtINTA interrupt or disabled by default
altogether (and the LINT0 input of the local APIC of the bootstrap
processor used instead).  Even if some additional logic is present in the
system so that another source can be routed to this line and the 8259A is
quiesced, sharing is probably impossible, because the output of the 8259A
is traditionally active high.

 INT2 is wired to the output of the 8254 timer (and in some more recent
systems shared with the HPET0).  This is the line that is referred to as
IRQ0 above.  This is because in an ACPI-based system the IRQ numbers refer
to lines as defined in ACPI tables, which may not necessarily identity-map
to I/O APIC input lines.

 Then the reasons for routing INT0 and INT2 like this are generally
historical and date back to the old Multi-Processor Specification.  There
may be systems that differ, but care must be taken and the correct
configuration table consulted.

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Mon, 2008-05-19 at 18:10 +0200, Clemens Ladisch wrote:
> Kevin Hao wrote:
> > +	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
> > +		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {
> > +		if (request_irq(irq, hpet_interrupt, irq_flags,
> > +				devp->hd_name, (void *)devp))
> 
> This spams my log with interrupt sharing violations.  As long as we do
> not know that the interrupt slot is empty, we need IRQF_PROBE_SHARED
> here.

Ok, added.

> 
> Another problem: the interrupt controller doesn't get correctly
> initialized for some interrupt line that didn't already have some
> routing:
> | $ cat /proc/interrupts
> |            CPU0
> |   0:         63   IO-APIC-edge      timer
> |   1:         96   IO-APIC-edge      i8042
> |   2:          0    XT-PIC-XT        hpet2
> |   6:          3   IO-APIC-edge      floppy
> |   7:          0   IO-APIC-edge      parport0
> |   8:          3   IO-APIC-edge      rtc
> |   9:          0   IO-APIC-fasteoi   acpi
> | ...
> 
> Additionally, I vaguely remember that on X86, there is some funny stuff
> going on with interrupt lines 0, 2 and 8 which means that the interrupt
> number passed to request_irq() is not necessarily identical to the
> hardware interrupt line.
> 
> I don't know which of these problems is responsible, or if I'm totally
> wrong, but on my machine, interrupts from hpet2 do not arrive.
> 

We can simply skip these special IRQ. :-)
Does anyone has a better solution?

----
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 0fdc627..b04a15d 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -390,6 +390,11 @@ static int hpet_timer_get_irq(struct hpet_dev *devp)
 	struct hpets *hpetp;
 	unsigned long cap, irq_flags;
 	int irq;
+	/*
+	 * skip IRQ0, IRQ2, IRQ8 because which is always used by some
+	 * legacy device
+	 */
+	unsigned long skip_irq = (1 << 0) | (1 << 2) | (1 << 8);
 
 	timer = devp->hd_timer;
 	hpet = devp->hd_hpet;
@@ -411,6 +416,9 @@ static int hpet_timer_get_irq(struct hpet_dev *devp)
 
 	cap = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
 				 >> Tn_INT_ROUTE_CAP_SHIFT;
+	cap &= ~skip_irq;
+
+	irq_flags |= IRQF_PROBE_SHARED;
 
 	for (irq = find_first_bit(&cap, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
 		irq = find_next_bit(&cap, HPET_MAX_IRQ, 1 + irq)) {

---
Best Regards,
Kevin

> 
> Regards,
> Clemens

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Tue, 20 May 2008, Kevin Hao wrote:

> We can simply skip these special IRQ. :-)
> Does anyone has a better solution?

 Hmm, you probably want to skip all lines that are edge-triggered.  
Otherwise you may have problems with sharing.  Drivers for devices used
with these edge-triggered lines may have special provisions to permit
sharing in a crafted way in special arrangements (cf. the 8250 serial or
the IDE driver), but that may not work in a generic way with a different
driver in the scenario.  And the polarity may be wrong too --
edge-triggered lines are often active-high, because it's fine with them.

 This driver is quite platform-specific -- how about instead of blindly
probing for interrupt lines, you actually allocate one somehow in platform
code?  For example the x86 platform could select an otherwise unused
interrupt line for you -- having routed all the legacy and PCI devices,
these systems quite frequently are left with a couple of otherwise unused
I/O APIC inputs.  If not, sharing with a PCI interrupt line would be a
good choice.  AFAIK, except from 8254 and RTC emulation the HPET cannot be
used with the legacy 8259A interrupt controllers, so that is a non issue
(but platform will know to disable your driver then).

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Clemens Ladisch]

Maciej W. Rozycki wrote:
> On Tue, 20 May 2008, Kevin Hao wrote:
> > We can simply skip these special IRQ. :-)
> > Does anyone has a better solution?
>
>  Hmm, you probably want to skip all lines that are edge-triggered.
> Otherwise you may have problems with sharing.

HPET interrupts can be either edge or level triggered.  Probably we
should modify it according to the type of the interrupt line we're
trying to grab.

> Drivers for devices used with these edge-triggered lines may have
> special provisions to permit sharing ...

An edge-triggered HPET interrupt line is not shared (we set IRQF_SHARED
only if the register says that it's level triggered).

>  This driver is quite platform-specific -- how about instead of blindly
> probing for interrupt lines, you actually allocate one somehow in platform
> code?

I don't have much of a clue about that "somehow", but this sound like a
good idea.  ;-)

I think hpet_reserve_platform_timers() might be the place for this.

It gets called from hpet_late_init(), which is a fs_initcall, so I think
we should be careful not to grab some unsharable interrupt that might be
needed by some ISA device whose driver is initialized later.
(This was a bug in 2.6.25-rc5: <http://bugzilla.kernel.org/show_bug.cgi?id=10382>)


Regards,
Clemens

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Wed, 21 May 2008, Clemens Ladisch wrote:

> >  Hmm, you probably want to skip all lines that are edge-triggered.
> > Otherwise you may have problems with sharing.
> 
> HPET interrupts can be either edge or level triggered.  Probably we
> should modify it according to the type of the interrupt line we're
> trying to grab.

 Edge-triggered lines are generally associated with legacy devices.  It
may not be possible to grab one without disturbing the other device.

> >  This driver is quite platform-specific -- how about instead of blindly
> > probing for interrupt lines, you actually allocate one somehow in platform
> > code?
> 
> I don't have much of a clue about that "somehow", but this sound like a
> good idea.  ;-)
> 
> I think hpet_reserve_platform_timers() might be the place for this.
> 
> It gets called from hpet_late_init(), which is a fs_initcall, so I think
> we should be careful not to grab some unsharable interrupt that might be
> needed by some ISA device whose driver is initialized later.
> (This was a bug in 2.6.25-rc5: <http://bugzilla.kernel.org/show_bug.cgi?id=10382>)

 I have had a look at the relevant areas of ACPI and HPET specs and it
looks pretty straightforward, although not all the information is recorded
in system tables.  Essentially you are free to choose an arbitrary
interrupt supported by the HPET -- which you can find in the timer's
routing capability (as the proposed patch is doing).

 However you are right you really want to select one which does not
conflict with a legacy device, so it's probably best to avoid the legacy
range altogether (worth noting as for example the system I have handy has
the capability of its timer #2 set to 0x00f00800 -- here IRQ11 may not be
safe to use) and then you have to check how many inputs beyond the legacy
range are supported by the I/O APICs in the system -- you can have a look
at mp_find_ioapic() to see how obtain that information and then you can
call mp_register_gsi() on the interrupt line selected like this to set up
routing in the I/O APIC as necessary.  Level-triggered mode has to be used
as the resulting interrupt entry may happen to be shared with a PCI
interrupt.

 Though I have just noticed there is something wrong with the spec -- it
says that "The interrupts are all active high." which precludes sharing,
hmm... -- broken spec?  If hardware designers actually followed it in this
respect (I wouldn't be surprised as for some of them software is abstract
enough a concept not to be bothered with, and then it is a spec after
all), then I am afraid we need to have a way to get an exclusive
reservation of an I/O APIC line.  It could be tough with a system using
fixed routing and reusing a legacy IRQ might be the only choice -- if
supported by the HPET router, that is.

 Of course if the HPET supports MSI delivery and the kernel configuration
has it enabled, then you can avoid all the hassle with finding an
available IRQ line altogether.

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Thu, 2008-05-22 at 04:47 +0100, Maciej W. Rozycki wrote:
> On Wed, 21 May 2008, Clemens Ladisch wrote:
> 
> > >  Hmm, you probably want to skip all lines that are edge-triggered.
> > > Otherwise you may have problems with sharing.
> > 
> > HPET interrupts can be either edge or level triggered.  Probably we
> > should modify it according to the type of the interrupt line we're
> > trying to grab.
> 
>  Edge-triggered lines are generally associated with legacy devices.  It
> may not be possible to grab one without disturbing the other device.
> 
> > >  This driver is quite platform-specific -- how about instead of blindly
> > > probing for interrupt lines, you actually allocate one somehow in platform
> > > code?
> > 
> > I don't have much of a clue about that "somehow", but this sound like a
> > good idea.  ;-)
> > 
> > I think hpet_reserve_platform_timers() might be the place for this.
> > 
> > It gets called from hpet_late_init(), which is a fs_initcall, so I think
> > we should be careful not to grab some unsharable interrupt that might be
> > needed by some ISA device whose driver is initialized later.
> > (This was a bug in 2.6.25-rc5: <http://bugzilla.kernel.org/show_bug.cgi?id=10382>)
> 
>  I have had a look at the relevant areas of ACPI and HPET specs and it
> looks pretty straightforward, although not all the information is recorded
> in system tables.  Essentially you are free to choose an arbitrary
> interrupt supported by the HPET -- which you can find in the timer's
> routing capability (as the proposed patch is doing).
> 
>  However you are right you really want to select one which does not
> conflict with a legacy device, so it's probably best to avoid the legacy
> range altogether (worth noting as for example the system I have handy has
> the capability of its timer #2 set to 0x00f00800 -- here IRQ11 may not be
> safe to use)

But if we avoid all the legacy range, the hpet timer will have no chance
to work on a host using legacy PIC.

>  and then you have to check how many inputs beyond the legacy
> range are supported by the I/O APICs in the system -- you can have a look
> at mp_find_ioapic() to see how obtain that information and then you can
> call mp_register_gsi() on the interrupt line selected like this to set up
> routing in the I/O APIC as necessary.  Level-triggered mode has to be used
> as the resulting interrupt entry may happen to be shared with a PCI
> interrupt.

Yes, we should setup routing in the I/O APIC and use level-triggered
mode. I think it's better to use acpi_register_gsi than mp_register_gsi.
Is that right?

Best Regards,
Kevin

> 
>  Though I have just noticed there is something wrong with the spec -- it
> says that "The interrupts are all active high." which precludes sharing,
> hmm... -- broken spec?  If hardware designers actually followed it in this
> respect (I wouldn't be surprised as for some of them software is abstract
> enough a concept not to be bothered with, and then it is a spec after
> all), then I am afraid we need to have a way to get an exclusive
> reservation of an I/O APIC line.  It could be tough with a system using
> fixed routing and reusing a legacy IRQ might be the only choice -- if
> supported by the HPET router, that is.
> 
>  Of course if the HPET supports MSI delivery and the kernel configuration
> has it enabled, then you can avoid all the hassle with finding an
> available IRQ line altogether.
> 
>   Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Thu, 22 May 2008, Kevin Hao wrote:

> But if we avoid all the legacy range, the hpet timer will have no chance
> to work on a host using legacy PIC.

 It's not meant to be wired to the legacy PIC anyway (except from the
timers #0 and #1 in some configurations, meant to emulate timer interrupts
of the 8254 and the RTC), though I can imagine such a non-standard
extension.

> Yes, we should setup routing in the I/O APIC and use level-triggered
> mode. I think it's better to use acpi_register_gsi than mp_register_gsi.
> Is that right?

 It sounds right to me.

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

Sorry for the delays.

I have revised this patch according to Maciej & Clemens suggestions.
The modifications include:

1. Assign a irq to the timer when we open the timer device. I think it's
better than do it in platform code. So we will have little impact on
other device.

2. Set IRQ routing entry for the irq we select.

3. If we in PIC mode, we skip all the well known legacy IRQ. And in io
apic we skip all the legacy IRQ.

4. Use level triggered mode by default.

BTW, Maciej. I am not familiar with ACPI subsystem. But I think the 
acpi_register_gsi should return -1 if mp_find_ioapic return error.
Is that right? If you also think so, I will make a patch for that.

---
 drivers/char/hpet.c  |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hpet.h |    3 +-
 2 files changed, 63 insertions(+), 1 deletion(-)
---
commit fa8e14bc3abb93832fbc9b7f86da0d2cf5b3b7a7
Author: Kevin Hao <kexin.hao@windriver.com>
Date:   Wed May 28 13:28:20 2008 +0800

    HPET timer's IRQ is 0 by default. So we have to select which irq
    will be used by these timers. We wait to set the timer's irq until
    we really open it in order to reduce the chance of conflicting with
    other device.
    
    Signed-off-by: Kevin Hao <kexin.hao@windriver.com>

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e7fb0bc..94dbfdc 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -184,6 +184,65 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void hpet_timer_set_irq(struct hpet_dev *devp)
+{
+	unsigned long v;
+	int irq, gsi;
+	struct hpet_timer __iomem *timer;
+
+	spin_lock_irq(&hpet_lock);
+	if (devp->hd_hdwirq) {
+		spin_unlock_irq(&hpet_lock);
+		return;
+	}
+
+	timer = devp->hd_timer;
+
+	/* we prefer level triggered mode */
+	v = readl(&timer->hpet_config);
+	if (!(v & Tn_INT_TYPE_CNF_MASK)) {
+		v |= Tn_INT_TYPE_CNF_MASK;
+		writel(v, &timer->hpet_config);
+	}
+	spin_unlock_irq(&hpet_lock);
+
+	v = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
+				 >> Tn_INT_ROUTE_CAP_SHIFT;
+
+	/*
+	 * In PIC mode, skip IRQ0-4, IRQ6-8, IRQ12-15 which is always used by
+	 * legacy device. In IO APIC mode, we skip all the legacy IRQS.
+	 */
+	if (acpi_irq_model == ACPI_IRQ_MODEL_PIC)
+		v &= ~0xf1df;
+	else
+		v &= ~0xffff;
+
+	for (irq = find_first_bit(&v, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
+		irq = find_next_bit(&v, HPET_MAX_IRQ, 1 + irq)) {
+
+		if (irq >= NR_IRQS) {
+			irq = HPET_MAX_IRQ;
+			break;
+		}
+
+		gsi = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE,
+					ACPI_ACTIVE_LOW);
+		if (gsi > 0)
+			break;
+	}
+
+	if  (irq < HPET_MAX_IRQ) {
+		spin_lock_irq(&hpet_lock);
+		v = readl(&timer->hpet_config);
+		v |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+		writel(v, &timer->hpet_config);
+		devp->hd_hdwirq = gsi;
+		spin_unlock_irq(&hpet_lock);
+	}
+	return;
+}
+
 static int hpet_open(struct inode *inode, struct file *file)
 {
 	struct hpet_dev *devp;
@@ -215,6 +274,8 @@ static int hpet_open(struct inode *inode, struct file *file)
 	devp->hd_flags |= HPET_OPEN;
 	spin_unlock_irq(&hpet_lock);
 
+	hpet_timer_set_irq(devp);
+
 	return 0;
 }
 
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 2dc29ce..440ca72 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -37,6 +37,7 @@ struct hpet {
 #define	hpet_compare	_u1._hpet_compare
 
 #define	HPET_MAX_TIMERS	(32)
+#define HPET_MAX_IRQ	(32)
 
 /*
  * HPET general capabilities register
@@ -64,7 +65,7 @@ struct hpet {
  */
 
 #define	Tn_INT_ROUTE_CAP_MASK		(0xffffffff00000000ULL)
-#define	Tn_INI_ROUTE_CAP_SHIFT		(32UL)
+#define	Tn_INT_ROUTE_CAP_SHIFT		(32UL)
 #define	Tn_FSB_INT_DELCAP_MASK		(0x8000UL)
 #define	Tn_FSB_INT_DELCAP_SHIFT		(15)
 #define	Tn_FSB_EN_CNF_MASK		(0x4000UL)
---

Best Regards,
Kevin


On Thu, 2008-05-22 at 16:25 +0100, Maciej W. Rozycki wrote: 
> On Thu, 22 May 2008, Kevin Hao wrote:
> 
> > But if we avoid all the legacy range, the hpet timer will have no chance
> > to work on a host using legacy PIC.
> 
>  It's not meant to be wired to the legacy PIC anyway (except from the
> timers #0 and #1 in some configurations, meant to emulate timer interrupts
> of the 8254 and the RTC), though I can imagine such a non-standard
> extension.
> 
> > Yes, we should setup routing in the I/O APIC and use level-triggered
> > mode. I think it's better to use acpi_register_gsi than mp_register_gsi.
> > Is that right?
> 
>  It sounds right to me.
> 
>   Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Wed, 28 May 2008, Kevin Hao wrote:

> BTW, Maciej. I am not familiar with ACPI subsystem. But I think the 
> acpi_register_gsi should return -1 if mp_find_ioapic return error.
> Is that right? If you also think so, I will make a patch for that.

 Well, the interface is documented in the sources -- I do not feel like
reading through all the execution paths to see whether the implementation
matches though. ;)

 A few comments below -- I may not have time available to throw your piece 
of code at a piece of hardware though.

> +	/* we prefer level triggered mode */
> +	v = readl(&timer->hpet_config);
> +	if (!(v & Tn_INT_TYPE_CNF_MASK)) {
> +		v |= Tn_INT_TYPE_CNF_MASK;
> +		writel(v, &timer->hpet_config);
> +	}

 Please note that when routing through the 8259A you need to match the
trigger mode set for the respective input.  It is normally set in the ELCR
register in the hardware and should be recorded in the ACPI interrupt
source tables too -- the default for IRQ0-15 is edge-triggered, active
high, unless overridden.

> +	v = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
> +				 >> Tn_INT_ROUTE_CAP_SHIFT;

 The operator should be at the end of the first line IMHO.

> +	/*
> +	 * In PIC mode, skip IRQ0-4, IRQ6-8, IRQ12-15 which is always used by
> +	 * legacy device. In IO APIC mode, we skip all the legacy IRQS.
> +	 */

 Quite reasonable assumption IMO, but again -- I'd expect the information 
about legacy devices present on these lines to be recorded in ACPI tables.  
Note that IRQ9 is used for the SCI -- I am not sure if that's a good 
choice for sharing.

> +		gsi = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE,
> +					ACPI_ACTIVE_LOW);

 I am assuming you have verified ACPI_ACTIVE_LOW works here, contrary to 
the HPET spec.

> +	if  (irq < HPET_MAX_IRQ) {

 Single space here.

> @@ -37,6 +37,7 @@ struct hpet {
>  #define	hpet_compare	_u1._hpet_compare
>  
>  #define	HPET_MAX_TIMERS	(32)
> +#define HPET_MAX_IRQ	(32)

 Tab here for consistency with the others?

 Otherwise no obvious problems.

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Thu, 2008-05-29 at 04:13 +0100, Maciej W. Rozycki wrote: 
> On Wed, 28 May 2008, Kevin Hao wrote:
> 
> > BTW, Maciej. I am not familiar with ACPI subsystem. But I think the 
> > acpi_register_gsi should return -1 if mp_find_ioapic return error.
> > Is that right? If you also think so, I will make a patch for that.
> 
>  Well, the interface is documented in the sources -- I do not feel like
> reading through all the execution paths to see whether the implementation
> matches though. ;)
> 
>  A few comments below -- I may not have time available to throw your piece 
> of code at a piece of hardware though.
> 
> > +	/* we prefer level triggered mode */
> > +	v = readl(&timer->hpet_config);
> > +	if (!(v & Tn_INT_TYPE_CNF_MASK)) {
> > +		v |= Tn_INT_TYPE_CNF_MASK;
> > +		writel(v, &timer->hpet_config);
> > +	}
> 
>  Please note that when routing through the 8259A you need to match the
> trigger mode set for the respective input.  It is normally set in the ELCR
> register in the hardware and should be recorded in the ACPI interrupt
> source tables too -- the default for IRQ0-15 is edge-triggered, active
> high, unless overridden.

The IRQ trigger mode is set to level by acpi_register_gsi in PIC mode.
And I have tested it on a host using legacy PIC. It works well.

Yes, we should record these information in ACPI interrupt source table.
And I have noticed that a patch has been made to update mptable. 
http://lkml.org/lkml/2008/5/25/255

We can use mp_config_acpi_gsi to setup route table. But now can we add
somethings like "FIXME: setup interrupt source table" and wait for that
patch to be merged?

> 
> > +	v = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK)
> > +				 >> Tn_INT_ROUTE_CAP_SHIFT;
> 
>  The operator should be at the end of the first line IMHO.

Ok, fix it.

> 
> > +	/*
> > +	 * In PIC mode, skip IRQ0-4, IRQ6-8, IRQ12-15 which is always used by
> > +	 * legacy device. In IO APIC mode, we skip all the legacy IRQS.
> > +	 */
> 
>  Quite reasonable assumption IMO, but again -- I'd expect the information 
> about legacy devices present on these lines to be recorded in ACPI tables.  
> Note that IRQ9 is used for the SCI -- I am not sure if that's a good 
> choice for sharing.

Add IRQ9 to the skipping IRQS.

> 
> > +		gsi = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE,
> > +					ACPI_ACTIVE_LOW);
> 
>  I am assuming you have verified ACPI_ACTIVE_LOW works here, contrary to 
> the HPET spec.

Yes, It can work with other PCI device in ACPI_ACTIVE_LOW polarity.

> 
> > +	if  (irq < HPET_MAX_IRQ) {
> 
>  Single space here.

Ok, deleted.

> 
> > @@ -37,6 +37,7 @@ struct hpet {
> >  #define	hpet_compare	_u1._hpet_compare
> >  
> >  #define	HPET_MAX_TIMERS	(32)
> > +#define HPET_MAX_IRQ	(32)
> 
>  Tab here for consistency with the others?

Ok, fix it.

Thanks for your comments.

---
    HPET timer's IRQ is 0 by default. So we have to select which irq
    will be used by these timers. We wait to set the timer's irq until
    we really open it in order to reduce the chance of conflicting with
    other device.
    
    Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
---
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e7fb0bc..c9bf5d4 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -184,6 +184,67 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void hpet_timer_set_irq(struct hpet_dev *devp)
+{
+	unsigned long v;
+	int irq, gsi;
+	struct hpet_timer __iomem *timer;
+
+	spin_lock_irq(&hpet_lock);
+	if (devp->hd_hdwirq) {
+		spin_unlock_irq(&hpet_lock);
+		return;
+	}
+
+	timer = devp->hd_timer;
+
+	/* we prefer level triggered mode */
+	v = readl(&timer->hpet_config);
+	if (!(v & Tn_INT_TYPE_CNF_MASK)) {
+		v |= Tn_INT_TYPE_CNF_MASK;
+		writel(v, &timer->hpet_config);
+	}
+	spin_unlock_irq(&hpet_lock);
+
+	v = (readq(&timer->hpet_config) & Tn_INT_ROUTE_CAP_MASK) >>
+				 Tn_INT_ROUTE_CAP_SHIFT;
+
+	/*
+	 * In PIC mode, skip IRQ0-4, IRQ6-9, IRQ12-15 which is always used by
+	 * legacy device. In IO APIC mode, we skip all the legacy IRQS.
+	 */
+	if (acpi_irq_model == ACPI_IRQ_MODEL_PIC)
+		v &= ~0xf3df;
+	else
+		v &= ~0xffff;
+
+	for (irq = find_first_bit(&v, HPET_MAX_IRQ); irq < HPET_MAX_IRQ;
+		irq = find_next_bit(&v, HPET_MAX_IRQ, 1 + irq)) {
+
+		if (irq >= NR_IRQS) {
+			irq = HPET_MAX_IRQ;
+			break;
+		}
+
+		gsi = acpi_register_gsi(irq, ACPI_LEVEL_SENSITIVE,
+					ACPI_ACTIVE_LOW);
+		if (gsi > 0)
+			break;
+
+		/* FIXME: Setup interrupt source table */
+	}
+
+	if (irq < HPET_MAX_IRQ) {
+		spin_lock_irq(&hpet_lock);
+		v = readl(&timer->hpet_config);
+		v |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+		writel(v, &timer->hpet_config);
+		devp->hd_hdwirq = gsi;
+		spin_unlock_irq(&hpet_lock);
+	}
+	return;
+}
+
 static int hpet_open(struct inode *inode, struct file *file)
 {
 	struct hpet_dev *devp;
@@ -215,6 +276,8 @@ static int hpet_open(struct inode *inode, struct file *file)
 	devp->hd_flags |= HPET_OPEN;
 	spin_unlock_irq(&hpet_lock);
 
+	hpet_timer_set_irq(devp);
+
 	return 0;
 }
 
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 2dc29ce..6d2626b 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -37,6 +37,7 @@ struct hpet {
 #define	hpet_compare	_u1._hpet_compare
 
 #define	HPET_MAX_TIMERS	(32)
+#define	HPET_MAX_IRQ	(32)
 
 /*
  * HPET general capabilities register
@@ -64,7 +65,7 @@ struct hpet {
  */
 
 #define	Tn_INT_ROUTE_CAP_MASK		(0xffffffff00000000ULL)
-#define	Tn_INI_ROUTE_CAP_SHIFT		(32UL)
+#define	Tn_INT_ROUTE_CAP_SHIFT		(32UL)
 #define	Tn_FSB_INT_DELCAP_MASK		(0x8000UL)
 #define	Tn_FSB_INT_DELCAP_SHIFT		(15)
 #define	Tn_FSB_EN_CNF_MASK		(0x4000UL)
---

Best Regards,
Kevin

> 
>  Otherwise no obvious problems.
> 
>   Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Maciej W. Rozycki]

On Thu, 29 May 2008, Kevin Hao wrote:

> The IRQ trigger mode is set to level by acpi_register_gsi in PIC mode.
> And I have tested it on a host using legacy PIC. It works well.

 It may work by chance.  All what the code in question guarantees is:

int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
{
	*irq = gsi;
	return 0;
}

int acpi_register_gsi(u32 gsi, int triggering, int polarity)
{
	unsigned int irq;
	unsigned int plat_gsi = gsi;

	acpi_gsi_to_irq(plat_gsi, &irq);
	return irq;
}

CONFIG_PCI is optional for the x86 platform.  You could argue this is a
bug in acpi_register_gsi() and I tend to agree -- I think the existence of
the ELCR is implied for ACPI-compliant systems, though I am happy to be
corrected.  The ACPI spec does not mention the register, but quotes:  
"Existing industry standard register interfaces to: CMOS, PIC, PITs, ..."

 Anyway, blindly attaching to an IRQ line is not a terribly good idea as
there may be an edge-triggered line from a device wired there after all.  
For example many LPC SuperIO chips have configurable IRQ resources -- the
parallel port can use either IRQ5 or IRQ7 (but not both at a time).  OTOH
IRQ6 may be free after all as floppy drives become rare.  Only IRQ10 and
IRQ11 are probably safe to be assumed free in systems with no ISA slots,
but then again, I may have missed something.  Hopefully there are no true
PCI-ISA bridges with the HPET implemented -- I have checked a couple of 
datasheets for ACPI-compliant PCI-ISA bridges (such as the PIIX4) that I 
have handy and none of them implements the HPET.

> >  I am assuming you have verified ACPI_ACTIVE_LOW works here, contrary to 
> > the HPET spec.
> 
> Yes, It can work with other PCI device in ACPI_ACTIVE_LOW polarity.

 Good to know.  Thanks for verifying this.

  Maciej

Re: [PATCH] x86: Get irq for hpet timer

[Kevin Hao]

On Thu, 2008-05-29 at 15:32 +0100, Maciej W. Rozycki wrote:

> CONFIG_PCI is optional for the x86 platform.  You could argue this is a
> bug in acpi_register_gsi() and I tend to agree -- I think the existence of
> the ELCR is implied for ACPI-compliant systems, though I am happy to be
> corrected.  The ACPI spec does not mention the register, but quotes:  
> "Existing industry standard register interfaces to: CMOS, PIC, PITs, ..."

Oh, I overlook the existence of CONFIG_PCI.

After look into the code, I think we should add a acpi_pic_set_trigger
to replace eisa_set_level_irq and remove CONFIG_PCI dependency.

> 
>  Anyway, blindly attaching to an IRQ line is not a terribly good idea as
> there may be an edge-triggered line from a device wired there after all. 

Yes, I agree with you. But at present we have not a better way to
resolve this problem. 

Thanks,
Kevin

Re: [PATCH] x86: Get irq for hpet timer

[Ingo Molnar]

* Kevin Hao <kexin.hao@windriver.com> wrote:

> >  Anyway, blindly attaching to an IRQ line is not a terribly good 
> > idea as there may be an edge-triggered line from a device wired 
> > there after all.
> 
> Yes, I agree with you. But at present we have not a better way to 
> resolve this problem.

i've applied your patch for more testing to tip/timers/hpet. Thanks,

	Ingo