2008-01-11 18:52:53

by Balaji Rao

[permalink] [raw]
Subject: [PATCH] Assign IRQs to HPET Timers

Assign an IRQ to HPET Timer devices when interrupt enable is requested.
This now makes the HPET userspace API work.

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

Signed-off-by: Balaji Rao R <[email protected]>

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 4c16778..92bd889 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -390,7 +390,8 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
struct hpets *hpetp;
int irq;
unsigned long g, v, t, m;
- unsigned long flags, isr;
+ unsigned long flags, isr, irq_bitmap;
+ u64 hpet_config;

timer = devp->hd_timer;
hpet = devp->hd_hpet;
@@ -412,7 +413,29 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
devp->hd_flags |= HPET_SHARED_IRQ;
spin_unlock_irq(&hpet_lock);

- irq = devp->hd_hdwirq;
+ /* Assign an IRQ to the timer */
+ hpet_config = readq(&timer->hpet_config);
+ irq_bitmap =
+ (hpet_config & Tn_INT_ROUTE_CAP_MASK) >> Tn_INT_ROUTE_CAP_SHIFT;
+ if (!irq_bitmap)
+ irq = 0; /* No IRQ Assignable */
+ else {
+ irq = find_first_bit(&irq_bitmap, 32);
+ do {
+ hpet_config |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+ writeq(hpet_config, &timer->hpet_config);
+
+ /* Check whether we wrote a valid IRQ
+ * number by reading back the field
+ */
+ hpet_config = readq(&timer->hpet_config);
+ if (irq == (hpet_config & Tn_INT_ROUTE_CNF_MASK)
+ >> Tn_INT_ROUTE_CNF_SHIFT) {
+ devp->hd_hdwirq = irq;
+ break; /* Success */
+ }
+ } while ((irq = (find_next_bit(&irq_bitmap, 32, irq))));
+ }

if (irq) {
unsigned long irq_flags;
@@ -509,6 +532,10 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
break;
v = readq(&timer->hpet_config);
v &= ~Tn_INT_ENB_CNF_MASK;
+
+ /* Zero out the IRQ field*/
+ v &= ~Tn_INT_ROUTE_CNF_MASK;
+
writeq(v, &timer->hpet_config);
if (devp->hd_irq) {
free_irq(devp->hd_irq, devp);
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 707f7cb..e3c0b2a 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -64,7 +64,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)


2008-01-12 12:03:36

by Balbir Singh

[permalink] [raw]
Subject: Re: [PATCH] Assign IRQs to HPET Timers

* Balaji Rao <[email protected]> [2008-01-12 00:36:11]:

> Assign an IRQ to HPET Timer devices when interrupt enable is requested.
> This now makes the HPET userspace API work.
>

A more detailed changelog will better help understand the nature and
origin of the problem and how to reproduce it.

> drivers/char/hpet.c | 31 +++++++++++++++++++++++++++++--
> include/linux/hpet.h | 2 +-
> 2 files changed, 30 insertions(+), 3 deletions(-)
>
> Signed-off-by: Balaji Rao R <[email protected]>
>
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 4c16778..92bd889 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -390,7 +390,8 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
> struct hpets *hpetp;
> int irq;
> unsigned long g, v, t, m;
> - unsigned long flags, isr;
> + unsigned long flags, isr, irq_bitmap;
> + u64 hpet_config;
>
> timer = devp->hd_timer;
> hpet = devp->hd_hpet;
> @@ -412,7 +413,29 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
> devp->hd_flags |= HPET_SHARED_IRQ;
> spin_unlock_irq(&hpet_lock);
>
> - irq = devp->hd_hdwirq;
> + /* Assign an IRQ to the timer */
> + hpet_config = readq(&timer->hpet_config);
> + irq_bitmap =
> + (hpet_config & Tn_INT_ROUTE_CAP_MASK) >> Tn_INT_ROUTE_CAP_SHIFT;

Should we check if the interrupts are being delivered via FSB, prior
to doing this?

> + if (!irq_bitmap)
> + irq = 0; /* No IRQ Assignable */
> + else {
> + irq = find_first_bit(&irq_bitmap, 32);

> + do {
> + hpet_config |= irq << Tn_INT_ROUTE_CNF_SHIFT;
> + writeq(hpet_config, &timer->hpet_config);
> +
> + /* Check whether we wrote a valid IRQ
> + * number by reading back the field
> + */
> + hpet_config = readq(&timer->hpet_config);
> + if (irq == (hpet_config & Tn_INT_ROUTE_CNF_MASK)
> + >> Tn_INT_ROUTE_CNF_SHIFT) {
> + devp->hd_hdwirq = irq;
> + break; /* Success */
> + }
> + } while ((irq = (find_next_bit(&irq_bitmap, 32, irq))));
> + }

Shouldn't we do this at hpet_alloc() time?


>
> if (irq) {
> unsigned long irq_flags;
> @@ -509,6 +532,10 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel)
> break;
> v = readq(&timer->hpet_config);
> v &= ~Tn_INT_ENB_CNF_MASK;
> +
> + /* Zero out the IRQ field*/
> + v &= ~Tn_INT_ROUTE_CNF_MASK;
> +
> writeq(v, &timer->hpet_config);
> if (devp->hd_irq) {
> free_irq(devp->hd_irq, devp);
> diff --git a/include/linux/hpet.h b/include/linux/hpet.h
> index 707f7cb..e3c0b2a 100644
> --- a/include/linux/hpet.h
> +++ b/include/linux/hpet.h
> @@ -64,7 +64,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)

The patch looks good overall!

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

2008-01-12 20:02:25

by Balaji Rao

[permalink] [raw]
Subject: [PATCH] [RESEND] Assign IRQs to HPET Timers

The userspace API for the HPET (see Documentation/hpet.txt) did not work. The
HPET_IE_ON ioctl was failing as there was no IRQ assigned to the timer
device. This patch fixes it by allocating IRQs to timer blocks in the HPET.

arch/x86/kernel/hpet.c | 13 +++++--------
drivers/char/hpet.c | 45 ++++++++++++++++++++++++++++++++++++++-------
include/linux/hpet.h | 2 +-
3 files changed, 44 insertions(+), 16 deletions(-)

Signed-off-by: Balaji Rao <[email protected]>

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 4a86ffd..08ee998 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -116,8 +116,7 @@ int is_hpet_enabled(void)
static void hpet_reserve_platform_timers(unsigned long id)
{
struct hpet __iomem *hpet = hpet_virt_address;
- struct hpet_timer __iomem *timer = &hpet->hpet_timers[2];
- unsigned int nrtimers, i;
+ unsigned int nrtimers;
struct hpet_data hd;

nrtimers = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT) + 1;
@@ -132,16 +131,14 @@ static void hpet_reserve_platform_timers(unsigned long
id)
#ifdef CONFIG_HPET_EMULATE_RTC
hpet_reserve_timer(&hd, 1);
#endif
-
hd.hd_irq[0] = HPET_LEGACY_8254;
hd.hd_irq[1] = HPET_LEGACY_RTC;

- for (i = 2; i < nrtimers; timer++, i++)
- hd.hd_irq[i] = (timer->hpet_config & Tn_INT_ROUTE_CNF_MASK) >>
- Tn_INT_ROUTE_CNF_SHIFT;
-
+ /*
+ * IRQs for the other timers are assigned dynamically
+ * in hpet_alloc
+ */
hpet_alloc(&hd);
-
}
#else
static void hpet_reserve_platform_timers(unsigned long id) { }
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 4c16778..593b32c 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -806,14 +806,14 @@ static unsigned long hpet_calibrate(struct hpets *hpetp)

int hpet_alloc(struct hpet_data *hdp)
{
- u64 cap, mcfg;
+ u64 cap, mcfg, hpet_config;
struct hpet_dev *devp;
- u32 i, ntimer;
+ u32 i, ntimer, irq;
struct hpets *hpetp;
size_t siz;
struct hpet __iomem *hpet;
static struct hpets *last = NULL;
- unsigned long period;
+ unsigned long period, irq_bitmap;
unsigned long long temp;

/*
@@ -840,11 +840,41 @@ int hpet_alloc(struct hpet_data *hdp)
hpetp->hp_hpet_phys = hdp->hd_phys_address;

hpetp->hp_ntimer = hdp->hd_nirqs;
+ hpet = hpetp->hp_hpet;

- for (i = 0; i < hdp->hd_nirqs; i++)
- hpetp->hp_dev[i].hd_hdwirq = hdp->hd_irq[i];
+ /* Assign IRQs statically for legacy devices */
+ hpetp->hp_dev[0].hd_hdwirq = hdp->hd_irq[0];
+ hpetp->hp_dev[1].hd_hdwirq = hdp->hd_irq[1];

- hpet = hpetp->hp_hpet;
+ /* Assign IRQs dynamically for the others */
+ for (i = 2, devp = &hpetp->hp_dev[2]; i < hdp->hd_nirqs; i++, devp++) {
+ struct hpet_timer __iomem *timer;
+
+ timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
+
+ hpet_config = readq(&timer->hpet_config);
+ irq_bitmap = (hpet_config & Tn_INT_ROUTE_CAP_MASK)
+ >> Tn_INT_ROUTE_CAP_SHIFT;
+ if (!irq_bitmap)
+ irq = 0; /* No valid IRQ Assignable */
+ else {
+ irq = find_first_bit(&irq_bitmap, 32);
+ do {
+ hpet_config |= irq << Tn_INT_ROUTE_CNF_SHIFT;
+ writeq(hpet_config, &timer->hpet_config);
+
+ /*
+ * Verify whether we have written a valid
+ * IRQ number by reading it back again
+ */
+ hpet_config = readq(&timer->hpet_config);
+ if (irq == (hpet_config & Tn_INT_ROUTE_CNF_MASK)
+ >> Tn_INT_ROUTE_CNF_SHIFT)
+ break; /* Success */
+ } while ((irq = (find_next_bit(&irq_bitmap, 32, irq))));
+ }
+ hpetp->hp_dev[i].hd_hdwirq = irq;
+ }

cap = readq(&hpet->hpet_cap);

@@ -875,7 +905,8 @@ int hpet_alloc(struct hpet_data *hdp)
hpetp->hp_which, hdp->hd_phys_address,
hpetp->hp_ntimer > 1 ? "s" : "");
for (i = 0; i < hpetp->hp_ntimer; i++)
- printk("%s %d", i > 0 ? "," : "", hdp->hd_irq[i]);
+ printk("%s %d", i > 0 ? "," : "",
+ hpetp->hp_dev[i].hd_hdwirq);
printk("\n");

printk(KERN_INFO "hpet%u: %u %d-bit timers, %Lu Hz\n",
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 707f7cb..e3c0b2a 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -64,7 +64,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)
---

2008-01-14 15:56:17

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] [RESEND] Assign IRQs to HPET Timers


* Balaji Rao <[email protected]> wrote:

> The userspace API for the HPET (see Documentation/hpet.txt) did not
> work. The HPET_IE_ON ioctl was failing as there was no IRQ assigned to
> the timer device. This patch fixes it by allocating IRQs to timer
> blocks in the HPET.

thanks, applied to x86.git. (sidenote: your patch was linewrapped, fixed
that up. See Documentation/email-clients.txt)

Ingo