Skip to contentSkip to conversation

HPET driver

PATCH In review Robert Picco 7 participants 26 messages May 13, 2004 – May 20, 2004 3 messages not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 HPET driver

Hi:

The driver supports the High Precision Event Timer. The driver has adopted a similar API to the Real Time Clock driver. It can support any number of HPET devices and the maximum number of timers per HPET device. For further information look at the documentation in the patch.

Thanks to Venki at Intel for testing the driver on X86 hardware with HPET.

thanks,

Bob

arch/i386/Kconfig +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/arch/i386/Kconfig linux-2.6.6-hpet/arch/i386/Kconfig
@@ -435,6 +435,7 @@
 

config HPET_EMULATE_RTC

	def_bool HPET_TIMER && RTC=y

+ depends on !HPET_RTC_IRQ

config SMP
bool "Symmetric multi-processing support"

arch/i386/kernel/time_hpet.c +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/arch/i386/kernel/time_hpet.c linux-2.6.6-hpet/arch/i386/kernel/time_hpet.c
@@ -21,6 +21,7 @@

#include <linux/config.h>

#include <asm/hpet.h>
+#include <linux/hpet.h>

unsigned long hpet_period; /* fsecs / HPET clock */
unsigned long hpet_tick; /* hpet clks count per tick */
@@ -135,6 +136,51 @@

	hpet_writel(cfg, HPET_CFG);

	use_hpet = 1;

+
+#ifdef CONFIG_HPET
+ {
+ struct hpet_data hd;
+ unsigned int ntimer;
+
+ memset(&hd, 0, sizeof (hd));
+
+ ntimer = hpet_readl(HPET_ID);
+ ntimer = (ntimer & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT;
+ ntimer++;
+
+ /*
+ * Register with driver. + * Timer0 and Timer1 is used by platform.
+ */
+ hd.hd_address = hpet_virt_address;
+ hd.hd_nirqs = ntimer;
+ hd.hd_flags = HPET_DATA_PLATFORM;
+#ifndef CONFIG_HPET_EMULATE_RTC
+ hd.hd_state = 0x1;
+#else
+ hd.hd_state = 0x3;
+#endif
+ hd.hd_irq[0] = HPET_LEGACY_8254;
+ hd.hd_irq[1] = HPET_LEGACY_RTC;
+ if (ntimer > 2) {
+ struct hpet *hpet;
+ struct hpet_timer *timer;
+ int i;
+
+ hpet = (struct hpet *) hpet_virt_address;
+
+ for (i = 2, timer = &hpet->hpet_timers[2]; i < ntimer;
+ timer++, i++) + hd.hd_irq[i] = (timer->hpet_config &
+ Tn_INT_ROUTE_CNF_MASK) >>
+ Tn_INT_ROUTE_CNF_SHIFT;
+
+ }
+
+ hpet_alloc(&hd);
+ }
+#endif
+

#ifdef CONFIG_X86_LOCAL_APIC
	wait_timer_tick = wait_hpet_tick;

#endif

Documentation/hpet.txt +286 -0

286 lines not on this page · raw message

drivers/char/hpet.c +1116 -0

1,116 lines not on this page · raw message

drivers/char/Kconfig +24 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/drivers/char/Kconfig linux-2.6.6-hpet/drivers/char/Kconfig
@@ -959,6 +959,32 @@
          kernels.  Applications should simply open the device (eg /dev/hda1)
          with the O_DIRECT flag.
 
+config HPET
+	bool "HPET - High Precision Event Timer"
+	default n
+	help
+	  If you say Y here, you will have a device named "/dev/hpet/XX" for
+	  each timer supported by the HPET.  The timers are +	  non-periodioc and/or periodic. +
+config HPET_RTC_IRQ
+	bool "HPET Control RTC IRQ"
+	default n
+	depends on HPET
+	help
+	  If you say Y here, you will disable RTC_IRQ in drivers/char/rtc.c. It
+	  is assumed the platform called hpet_alloc with the RTC IRQ values for
+	  the HPET timers.
+
+config HPET_NOMMAP
+	bool "HPET - Control mmap capability."
+	default n
+	depends on HPET
+	help
+	  If you say Y here, then the mmap interface for the HPET driver returns ENOSYS.
+	  Some hardware implementations might not want all the memory in the page the
+	  HPET control registers reside to be exposed.
+

config MAX_RAW_DEVS
int "Maximum number of RAW devices to support (1-8192)" depends on RAW_DRIVER

drivers/char/Makefile +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/drivers/char/Makefile linux-2.6.6-hpet/drivers/char/Makefile
@@ -55,6 +55,7 @@

obj-$(CONFIG_APPLICOM) += applicom.o
obj-$(CONFIG_SONYPI) += sonypi.o
obj-$(CONFIG_RTC) += rtc.o
+obj-$(CONFIG_HPET) += hpet.o
obj-$(CONFIG_GEN_RTC) += genrtc.o
obj-$(CONFIG_EFI_RTC) += efirtc.o
ifeq ($(CONFIG_GENERIC_NVRAM),y)

drivers/char/rtc.c +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/drivers/char/rtc.c linux-2.6.6-hpet/drivers/char/rtc.c
@@ -97,6 +97,11 @@

static int rtc_irq = PCI_IRQ_NONE;
#endif

+#ifdef CONFIG_HPET_RTC_IRQ
+#undef RTC_IRQ
+#define RTC_IRQ 0
+#endif
+
#if RTC_IRQ
static int rtc_has_irq = 1;
#endif

include/asm-i386/hpet.h +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/include/asm-i386/hpet.h linux-2.6.6-hpet/include/asm-i386/hpet.h
@@ -57,9 +57,12 @@

#define HPET_ID_LEGSUP 0x00008000
#define HPET_ID_NUMBER 0x00001f00
#define HPET_ID_REV 0x000000ff
+#define HPET_ID_NUMBER_SHIFT 8

#define HPET_CFG_ENABLE 0x001
#define HPET_CFG_LEGACY 0x002
+#define HPET_LEGACY_8254 2
+#define HPET_LEGACY_RTC 8

#define HPET_TN_ENABLE 0x004
#define HPET_TN_PERIODIC 0x008

include/linux/hpet.h +140 -0

140 lines not on this page · raw message

include/linux/major.h +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/include/linux/major.h linux-2.6.6-hpet/include/linux/major.h
@@ -165,4 +165,6 @@
 

#define VIOTAPE_MAJOR 230

+#define HPET_MAJOR 229 /* High Precision Event Timer */
+
#endif

#1 lore raw
Quoted 11 lines · Robert Picco

Hi:

The driver supports the High Precision Event Timer. The driver has adopted a similar API to the Real Time Clock driver. It can support any number of HPET devices and the maximum number of timers per HPET device. For further information look at the documentation in the patch.

Thanks to Venki at Intel for testing the driver on X86 hardware with HPET.

From a quick read:

- Impressive lack of code comments!

- The /proc entries seem to be undocumented.

- There are several uses of `volatile' in the driver. Usually this is either unnecessary or indicates a deeper problem. Are they needed?

- Why is mmap setting VM_SHM?

- hpet_alloc() leaks `hpetp' on error paths

#2 lore raw
In reply to Robert Picco

Review

Quoted 6 lines · Robert Picco

diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-orig/drivers/char/hpet.c linux-2.6.6-hpet/drivers/char/hpet.c
--- linux-2.6.6-orig/drivers/char/hpet.c 1969-12-31
19:00:00.000000000 -0500
+++ linux-2.6.6-hpet/drivers/char/hpet.c 2004-05-10
15:43:48.000000000 -0400

(quoted diff) +0 -0
@@ -0,0 +1,1116 @@

run this through Lindent... non-standard indentation

Quoted 59 lines
drivers/char/hpet.c +51 -0
+/*
+ * Intel & MS High Precision Event Timer Implementation.
+ * Contributors:
+ *    Venki Pallipadi
+ *     Bob Picco
+ */
+
+#include <linux/config.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/devfs_fs_kernel.h>
+#include <linux/major.h>
+#include <linux/ioport.h>
+#include <linux/fcntl.h>
+#include <linux/init.h>
+#include <linux/poll.h>
+#include <linux/proc_fs.h>
+#include <linux/spinlock.h>
+#include <linux/sysctl.h>
+#include <linux/wait.h>
+#include <linux/bcd.h>
+#include <linux/seq_file.h>
+
+#include <asm/current.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+#include <asm/io.h>
+#include <asm/irq.h>
+#include <asm/bitops.h>
+#include <asm/div64.h>
+
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <linux/hpet.h>
+
+/*
+ * The High Precision Event Timer driver.
+ * This driver is closely modelled after the rtc.c driver.
+ * http://www.intel.com/labs/platcomp/hpet/hpetspec.htm
+ */
+#define    HPET_USER_FREQ    (64)
+#define    HPET_DRIFT    (500)
+
+static u32 hpet_ntimer, hpet_nhpet, hpet_max_freq = HPET_USER_FREQ;
+static char hpetname[] = "hpet/XX";
+
+/* A lock for concurrent access by app and isr hpet activity. */
+static spinlock_t hpet_lock = SPIN_LOCK_UNLOCKED;
+/* A lock for concurrent intermodule access to hpet and isr hpet 

activity. */

drivers/char/hpet.c +7 -0
+static spinlock_t hpet_task_lock = SPIN_LOCK_UNLOCKED;
+
+struct hpet_dev {
+    struct hpets        *hd_hpets;
+    volatile struct hpet    *hd_hpet;
+    volatile struct hpet_timer
+                *hd_timer;

drivers/char/hpet.c line 68

Quoted 10 lines
drivers/char/hpet.c +10 -0
+    unsigned long        hd_ireqfreq;
+    unsigned long        hd_irqdata;
+    wait_queue_head_t    hd_waitqueue;
+    struct fasync_struct    *hd_async_queue;
+    struct hpet_task    *hd_task;
+    unsigned int        hd_flags;
+    unsigned int        hd_irq;
+    unsigned int        hd_hdwirq;
+    int            hd_minor;
+};

kill the 'volatile', it's preferred to put barriers and such in the code where they are needed.

Quoted 60 lines
drivers/char/hpet.c +60 -0
+struct hpets {
+    struct hpets        *hp_next;
+    volatile struct hpet    *hp_hpet;
+    unsigned long        hp_period;
+    unsigned long        hp_delta;
+    unsigned int        hp_ntimer;
+    unsigned int        hp_which;
+    struct hpet_dev        hp_dev[1];
+};
+
+static struct hpets *hpets;
+
+#define    HPET_OPEN        0x0001
+#define    HPET_IE            0x0002            /* interrupt enabled */
+#define    HPET_PERIODIC        0x0004
+
+static irqreturn_t
+hpet_interrupt (int irq, void *data, struct pt_regs *regs)
+{
+    struct hpet_dev *devp;
+    unsigned long isr;
+
+    devp = data;
+
+    spin_lock(&hpet_lock);
+    devp->hd_irqdata++;
+
+    if ((devp->hd_flags & (HPET_IE | HPET_PERIODIC)) == HPET_IE) {
+        unsigned long m, t;
+
+        t = devp->hd_ireqfreq;
+        m = devp->hd_hpet->hpet_mc;
+        devp->hd_timer->hpet_compare = t + m + devp->hd_hpets->hp_delta;
+    }
+
+    isr = (1 << (devp - devp->hd_hpets->hp_dev));
+    devp->hd_hpet->hpet_isr = isr;
+    spin_unlock(&hpet_lock);
+
+    spin_lock(&hpet_task_lock);
+    if (devp->hd_task)
+        devp->hd_task->ht_func(devp->hd_task->ht_data);
+    spin_unlock(&hpet_task_lock);
+
+    wake_up_interruptible(&devp->hd_waitqueue);
+
+    kill_fasync(&devp->hd_async_queue, SIGIO, POLL_IN);
+
+    return IRQ_HANDLED;
+}
+
+static struct hpet_dev *
+hpet_minor_to_dev (int minor)
+{
+    struct hpets *hpetp;
+    int i;
+
+    for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
+        for (i = 0; i <  hpetp->hp_ntimer; i++)
+            if (hpetp->hp_dev[i].hd_minor == minor)

drivers/char/hpet.c line 139

Quoted 10 lines
drivers/char/hpet.c +10 -0
+                return &hpetp->hp_dev[i];
+    return 0;
+}
+
+static int
+hpet_open (struct inode *inode, struct file *file)
+{
+    struct hpet_dev *devp;
+
+    devp = hpet_minor_to_dev(MINOR(inode->i_rdev));

locking of hpets?

drivers/char/hpet.c line 253

Quoted 3 lines
drivers/char/hpet.c +3 -0
+    vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);
+    vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+    addr = __pa(addr);

where did these flags come from? don't you just want VM_RESERVED?

drivers/char/hpet.c line 258

Quoted 5 lines

+ if (remap_page_range(vma, vma->vm_start, addr, PAGE_SIZE,
vma->vm_page_prot)) {

drivers/char/hpet.c +3 -0
+        printk(KERN_ERR "remap_page_range failed in hpet.c\n");
+        return -EAGAIN;
+    }

why not ->nopage like in sound/oss/via82cxxx_audio.c?

Quoted 168 lines
drivers/char/hpet.c +21 -0
+static int
+hpet_release (struct inode *inode, struct file *file)
+{
+    struct hpet_dev    *devp;
+    volatile struct hpet_timer *timer;
+
+    devp = file->private_data;
+    timer = devp->hd_timer;
+
+    spin_lock_irq(&hpet_lock);
+
+    timer->hpet_config &= ~Tn_INT_ENB_CNF_MASK;
+
+    if (devp->hd_irq) {
+        free_irq(devp->hd_irq, devp);
+        devp->hd_irq = 0;
+    }
+
+    devp->hd_ireqfreq = 0;
+
+    if (devp->hd_flags & HPET_PERIODIC && timer->hpet_config & 

Tn_TYPE_CNF_MASK) {

drivers/char/hpet.c +21 -0
+        unsigned long v;
+
+        v = timer->hpet_config;
+        v ^= Tn_TYPE_CNF_MASK;
+        timer->hpet_config = v;
+    }
+
+    devp->hd_flags &= ~(HPET_OPEN | HPET_IE | HPET_PERIODIC);
+    spin_unlock_irq(&hpet_lock);
+
+    if (file->f_flags & FASYNC)
+        hpet_fasync(-1, file, 0);
+
+    file->private_data = 0;
+    return 0;
+}
+
+static int hpet_ioctl_common(struct hpet_dev *, int, unsigned long, int);
+
+static int
+hpet_ioctl (struct inode *inode, struct file *file, unsigned int cmd, 

unsigned long arg)

drivers/char/hpet.c +41 -0
+{
+    struct hpet_dev    *devp;
+
+    devp = file->private_data;
+    return hpet_ioctl_common(devp, cmd, arg, 0);
+}
+
+static int
+hpet_ioctl_ieon (struct hpet_dev *devp)
+{
+    volatile struct hpet_timer *timer;
+    volatile struct hpet *hpet;
+    struct hpets *hpetp;
+    int irq;
+    unsigned long g, v, t, m;
+    unsigned long flags, isr;
+
+    timer = devp->hd_timer;
+    hpet = devp->hd_hpet;
+    hpetp = devp->hd_hpets;
+
+    v = timer->hpet_config;
+    spin_lock_irq(&hpet_lock);
+
+    if (devp->hd_flags & HPET_IE) {
+        spin_unlock_irq(&hpet_lock);
+        return -EBUSY;
+    }
+
+    devp->hd_flags |= HPET_IE;
+    spin_unlock_irq(&hpet_lock);
+
+    t = timer->hpet_config;
+    irq = devp->hd_hdwirq;
+
+    if (irq) {
+        char name[7];
+
+        sprintf(name, "hpet%d", (int) (devp - hpetp->hp_dev));
+
+        if (request_irq(irq, hpet_interrupt, SA_INTERRUPT, name, (void 

*) devp)) {

drivers/char/hpet.c +53 -0
+            printk(KERN_ERR "hpet: IRQ %d is not free\n", irq);
+            irq = 0;
+        }
+    }
+
+    if (irq == 0) {
+        spin_lock_irq(&hpet_lock);
+        devp->hd_flags ^= HPET_IE;
+        spin_unlock_irq(&hpet_lock);
+        return -EIO;
+    }
+
+    devp->hd_irq = irq;
+    t = devp->hd_ireqfreq;
+    v = timer->hpet_config;
+    g = v | Tn_INT_ENB_CNF_MASK;
+
+    if (devp->hd_flags & HPET_PERIODIC) {
+        timer->hpet_compare = t;
+        g |= Tn_TYPE_CNF_MASK;
+        v |= Tn_TYPE_CNF_MASK;
+        timer->hpet_config = v;
+        v |= Tn_VAL_SET_CNF_MASK;
+        timer->hpet_config = v;
+        local_irq_save(flags);
+        m = hpet->hpet_mc;
+        timer->hpet_compare = t + m + hpetp->hp_delta;
+    } else {
+        local_irq_save(flags);
+        m = hpet->hpet_mc;
+        timer->hpet_compare = t + m + hpetp->hp_delta;
+    }
+
+    isr = (1 << (devp - hpets->hp_dev));
+    hpet->hpet_isr = isr;
+    timer->hpet_config = g;
+    local_irq_restore(flags);
+
+    return 0;
+}
+
+static inline unsigned long
+hpet_time_div (unsigned long dis)
+{
+    unsigned long long m = 1000000000000000ULL;
+
+    do_div(m, dis);
+
+    return (unsigned long) m;
+}
+
+static int
+hpet_ioctl_common (struct hpet_dev *devp, int cmd, unsigned long arg, 

int kernel)

drivers/char/hpet.c +28 -0
+{
+    volatile struct hpet_timer *timer;
+    volatile struct hpet *hpet;
+    struct hpets *hpetp;
+    int err;
+    unsigned long v;
+
+    switch (cmd) {
+    case HPET_IE_OFF:
+    case HPET_INFO:
+    case HPET_EPI:
+    case HPET_DPI:
+    case HPET_IRQFREQ:
+        timer = devp->hd_timer;
+        hpet = devp->hd_hpet;
+        hpetp = devp->hd_hpets;
+        break;
+    case HPET_IE_ON:
+        return hpet_ioctl_ieon(devp);
+    default:
+        return -EINVAL;
+    }
+
+    err = 0;
+
+    switch (cmd) {
+    case HPET_IE_OFF:
+        if ((devp->hd_flags & HPET_IE) == 0)

drivers/char/hpet.c line 450

Quoted 10 lines
drivers/char/hpet.c +10 -0
+            break;
+        v = timer->hpet_config;
+        v &= ~Tn_INT_ENB_CNF_MASK;
+        timer->hpet_config = v;
+        if (devp->hd_irq) {
+            free_irq(devp->hd_irq, devp);
+            devp->hd_irq = 0;
+        }
+        devp->hd_flags ^= HPET_IE;
+        break;

do you really want to be freeing the irq outside of the open-close model?

Quoted 661 lines
drivers/char/hpet.c +93 -0
+    case HPET_INFO:
+    {
+        struct hpet_info        info;
+
+        info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
+            devp->hd_ireqfreq);
+        info.hi_flags = timer->hpet_config & Tn_PER_INT_CAP_MASK;
+        if (copy_to_user((void *) arg, &info , sizeof (info)))
+            err = -EFAULT;
+        break;
+    }
+    case HPET_EPI:
+        v = timer->hpet_config;
+        if ((v & Tn_PER_INT_CAP_MASK) == 0) {
+            err = -ENXIO;
+            break;
+        }
+        devp->hd_flags |= HPET_PERIODIC;
+        break;
+    case HPET_DPI:
+        v = timer->hpet_config;
+        if ((v & Tn_PER_INT_CAP_MASK) == 0) {
+            err = -ENXIO;
+            break;
+        }
+        if (devp->hd_flags & HPET_PERIODIC &&
+            timer->hpet_config & Tn_TYPE_CNF_MASK) {
+            v = timer->hpet_config;
+            v ^= Tn_TYPE_CNF_MASK;
+            timer->hpet_config = v;
+        }
+        devp->hd_flags &= ~HPET_PERIODIC;
+        break;
+    case HPET_IRQFREQ:
+        if (!kernel && (arg > hpet_max_freq) &&
+            !capable(CAP_SYS_RESOURCE)) {
+            err = -EACCES;
+            break;
+        }
+
+        if (arg & (arg - 1)) {
+            err = -EINVAL;
+            break;
+        }
+
+        devp->hd_ireqfreq = hpet_time_div(hpetp->hp_period * arg);
+    }
+
+    return err;
+}
+
+static struct file_operations hpet_fops = {
+    .owner        = THIS_MODULE,
+    .llseek        = no_llseek,
+    .read        = hpet_read,
+    .poll        = hpet_poll,
+    .ioctl        = hpet_ioctl,
+    .open        = hpet_open,
+    .release    = hpet_release,
+    .fasync        = hpet_fasync,
+    .mmap        = hpet_mmap,
+};
+
+EXPORT_SYMBOL(hpet_alloc);
+EXPORT_SYMBOL(hpet_register);
+EXPORT_SYMBOL(hpet_unregister);
+EXPORT_SYMBOL(hpet_control);
+
+int
+hpet_register (struct hpet_task *tp, int periodic)
+{
+    unsigned int i;
+    u64 mask;
+    volatile struct hpet_timer *timer;
+    struct hpet_dev *devp;
+    struct hpets *hpetp;
+
+    switch (periodic) {
+    case 1:
+        mask = Tn_PER_INT_CAP_MASK;
+        break;
+    case 0:
+        mask = 0;
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    spin_lock_irq(&hpet_task_lock);
+    spin_lock(&hpet_lock);
+
+    for (devp = 0, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)
+    for (timer = hpetp->hp_hpet->hpet_timers, i = 0; i < 

hpetp->hp_ntimer; i++, timer++) {

drivers/char/hpet.c +37 -0
+        if ((timer->hpet_config & Tn_PER_INT_CAP_MASK) != mask)
+            continue;
+
+        devp = &hpetp->hp_dev[i];
+
+        if (devp->hd_flags & HPET_OPEN || devp->hd_task) {
+            devp = 0;
+            continue;
+        }
+
+        tp->ht_opaque = devp;
+        devp->hd_task = tp;
+        break;
+    }
+
+    spin_unlock(&hpet_lock);
+    spin_unlock_irq(&hpet_task_lock);
+
+    if (tp->ht_opaque)
+        return 0;
+    else
+        return -EBUSY;
+}
+
+static inline int
+hpet_tpcheck (struct hpet_task *tp)
+{
+    struct hpet_dev    *devp;
+    struct hpets *hpetp;
+
+    devp = tp->ht_opaque;
+
+    if (!devp)
+        return -ENXIO;
+
+    for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
+        if (devp >= hpetp->hp_dev && devp < (hpetp->hp_dev + 

hpetp->hp_ntimer) &&

drivers/char/hpet.c +67 -0
+            devp->hd_hpet == hpetp->hp_hpet)
+            return 0;
+
+    return -ENXIO;
+}
+
+
+int
+hpet_unregister (struct hpet_task *tp)
+{
+    struct hpet_dev    *devp;
+    volatile struct hpet_timer *timer;
+    int err;
+
+    if ((err = hpet_tpcheck(tp)))
+        return err;
+
+    spin_lock_irq(&hpet_task_lock);
+    spin_lock(&hpet_lock);
+
+    devp = tp->ht_opaque;
+    if (devp->hd_task != tp) {
+        spin_unlock(&hpet_lock);
+        spin_unlock_irq(&hpet_task_lock);
+        return -ENXIO;
+    }
+
+    timer = devp->hd_timer;
+    timer->hpet_config &= ~Tn_INT_ENB_CNF_MASK;
+    devp->hd_flags &= ~( HPET_IE | HPET_PERIODIC);
+    devp->hd_task = 0;
+    spin_unlock(&hpet_lock);
+    spin_unlock_irq(&hpet_task_lock);
+
+    return 0;
+}
+
+int
+hpet_control (struct hpet_task *tp, unsigned int cmd, unsigned long arg)
+{
+    struct hpet_dev    *devp;
+    int err;
+
+    if ((err = hpet_tpcheck(tp)))
+        return err;
+
+    spin_lock_irq(&hpet_lock);
+    devp = tp->ht_opaque;
+    if (devp->hd_task != tp) {
+        spin_unlock_irq(&hpet_lock);
+        return -ENXIO;
+    }
+    spin_unlock_irq(&hpet_lock);
+    return hpet_ioctl_common(devp, cmd, arg, 1);
+}
+
+
+
+#ifdef    CONFIG_TIME_INTERPOLATION
+
+static unsigned long hpet_offset, last_wall_hpet;
+static long hpet_nsecs_per_cycle, hpet_cycles_per_sec;
+
+static unsigned long
+hpet_getoffset (void)
+{
+    return hpet_offset + (hpets->hp_hpet->hpet_mc - last_wall_hpet) * 

hpet_nsecs_per_cycle;

drivers/char/hpet.c +119 -0
+}
+
+static void
+hpet_update (long delta)
+{
+    unsigned long mc;
+    unsigned long offset;
+    volatile struct hpet *hpet;
+
+    hpet = hpets->hp_hpet;
+    mc = hpet->hpet_mc;
+    offset = hpet_offset + (mc - last_wall_hpet) * hpet_nsecs_per_cycle;
+
+    if (delta < 0 || (unsigned long) delta < offset)
+        hpet_offset = offset - delta;
+    else
+        hpet_offset = 0;
+    last_wall_hpet = mc;
+}
+
+static void
+hpet_reset (void)
+{
+    volatile struct hpet *hpet;
+
+    hpet = hpets->hp_hpet;
+    hpet_offset = 0;
+    last_wall_hpet = hpet->hpet_mc;
+}
+
+static struct time_interpolator hpet_interpolator = {
+    .get_offset    =    hpet_getoffset,
+    .update        =    hpet_update,
+    .reset        =    hpet_reset
+};
+
+#endif
+
+static ctl_table hpet_table[] = {
+    {
+        .ctl_name    = 1,
+        .procname    = "max-user-freq",
+        .data        = &hpet_max_freq,
+        .maxlen        = sizeof(int),
+        .mode        = 0644,
+        .proc_handler    = &proc_dointvec,
+    },
+    { .ctl_name = 0 }
+};
+
+static ctl_table hpet_root[] = {
+    {
+        .ctl_name    = 1,
+        .procname    = "hpet",
+        .maxlen        = 0,
+        .mode        = 0555,
+        .child        = hpet_table,
+    },
+    { .ctl_name = 0 }
+};
+
+static ctl_table dev_root[] = {
+    {
+        .ctl_name    = CTL_DEV,
+        .procname    = "dev",
+        .maxlen        = 0,
+        .mode        = 0555,
+        .child        = hpet_root,
+    },
+    { .ctl_name = 0 }
+};
+
+static struct ctl_table_header *sysctl_header;
+
+static void *
+hpet_start (struct seq_file *s, loff_t *pos)
+{
+    struct hpets *hpetp;
+    loff_t n;
+
+    for (n = *pos, hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
+        if (!n--)
+            return hpetp;
+
+    return 0;
+}
+
+static void *
+hpet_next (struct seq_file *s, void *v, loff_t *pos)
+{
+    struct hpets *hpetp;
+
+    hpetp = v;
+    ++*pos;
+    return hpetp->hp_next;
+}
+
+static void
+hpet_stop (struct seq_file *s, void *v)
+{
+    return;
+}
+
+static int
+hpet_show (struct seq_file *s, void *v)
+{
+    struct hpets *hpetp;
+    volatile struct hpet *hpet;
+    u64 cap, vendor, period;
+
+    hpetp = v;
+    hpet = hpetp->hp_hpet;
+
+    cap = hpet->hpet_cap;
+    period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
+        HPET_COUNTER_CLK_PERIOD_SHIFT;
+    vendor = (cap & HPET_VENDOR_ID_MASK) >> HPET_VENDOR_ID_SHIFT;
+
+    seq_printf(s, "HPET%d period = %d 10**-15  vendor = 0x%x number 

timer = %d\n",

drivers/char/hpet.c +43 -0
+        hpetp->hp_which, (u32) period, (u32) vendor, hpetp->hp_ntimer);
+
+    return 0;
+}
+
+static struct seq_operations hpet_seq_ops = {
+    .start    =    hpet_start,
+    .next    =    hpet_next,
+    .stop    =    hpet_stop,
+    .show    =    hpet_show
+};
+
+static int
+hpet_proc_open (struct inode *inode, struct file *file)
+{
+    return seq_open(file, &hpet_seq_ops);
+}
+
+static struct file_operations hpet_proc_fops = {
+    .open        =    hpet_proc_open,
+    .read        =    seq_read,
+    .llseek        =    seq_lseek,
+    .release    =    seq_release
+};
+
+
+/*
+ * Adjustment for when arming the timer with
+ * initial conditions.  That is, main counter
+ * ticks expired before interrupts are enabled.
+ */
+#define    TICK_CALIBRATE    (1000UL)
+
+static unsigned long __init
+hpet_calibrate (struct hpets *hpetp)
+{
+    volatile struct hpet_timer *timer;
+    unsigned long t, m, count, i, flags, start;
+    struct hpet_dev *devp;
+    int j;
+    volatile struct hpet *hpet;
+
+    for (timer = 0, j = 0, devp = hpetp->hp_dev;  j < hpetp->hp_ntimer; 

j++, devp++)

drivers/char/hpet.c +49 -0
+        if ((devp->hd_flags & HPET_OPEN) == 0) {
+            timer = devp->hd_timer;
+            break;
+        }
+
+    if (!timer)
+        return 0;
+
+    hpet = hpets->hp_hpet;
+    t = timer->hpet_compare;
+
+    i = 0;
+    count = hpet_time_div(hpetp->hp_period * TICK_CALIBRATE);
+
+    local_irq_save(flags);
+
+    start = hpet->hpet_mc;
+
+    do {
+        m = hpet->hpet_mc;
+        timer->hpet_compare = t + m + hpetp->hp_delta;
+    } while (i++, (m - start) <  count);
+
+    local_irq_restore(flags);
+
+    return (m - start) / i;
+}
+
+static void __init
+hpet_init_chrdev (void)
+{
+    static int once;
+
+    if (!once++ && register_chrdev(HPET_MAJOR, "hpet", &hpet_fops))
+        panic("unable to to major %d for hpet device", HPET_MAJOR);
+
+    return;
+}
+
+static void __init
+hpet_post_platform (void)
+{
+    struct hpets *hpetp;
+    u32 i, ntimer;
+    struct hpet_dev    *devp;
+
+    hpet_init_chrdev();
+
+    for (ntimer = 0, hpetp = hpets; hpetp; hpetp = hpetp->hp_next, 

ntimer++)
+ for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++,
devp++) {

drivers/char/hpet.c +89 -0
+
+            if (devp->hd_flags & HPET_OPEN)
+                continue;
+
+            sprintf(&hpetname[5], "%d", ntimer);
+            devfs_mk_cdev(MKDEV(HPET_MAJOR, ntimer),
+                S_IFCHR|S_IRUSR|S_IWUSR, hpetname);
+            init_waitqueue_head(&devp->hd_waitqueue);
+        }
+
+    return;
+}
+
+int __init
+hpet_alloc (struct hpet_data *hdp)
+{
+    u64 cap, mcfg;
+    struct hpet_dev    *devp;
+    u32 i, ntimer;
+    struct hpets *hpetp;
+    size_t siz;
+    volatile struct hpet *hpet;
+    static struct hpets *last __initdata = (struct hpets *) 0;
+
+    for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
+        if (hpetp->hp_hpet == (struct hpet *) (hdp->hd_address))
+            return 0;
+
+    siz = sizeof (struct hpets) + ((hdp->hd_nirqs - 1) *
+        sizeof (struct hpet_dev));
+
+    hpetp = kmalloc(siz, GFP_KERNEL);
+
+    if (!hpetp)
+        return -ENOMEM;
+
+    memset(hpetp, 0, siz);
+
+    hpetp->hp_which = hpet_nhpet++;
+    hpetp->hp_hpet = (struct hpet *) hdp->hd_address;
+
+    if (last)
+        last->hp_next = hpetp;
+    else
+        hpets = hpetp;
+
+    last = hpetp;
+
+    hpetp->hp_ntimer = hdp->hd_nirqs;
+
+    for (i = 0; i < hdp->hd_nirqs; i++)
+        hpetp->hp_dev[i].hd_hdwirq = hdp->hd_irq[i];
+
+    hpet = hpetp->hp_hpet;
+
+    cap = hpet->hpet_cap;
+
+    ntimer = ((cap & HPET_NUM_TIM_CAP_MASK) >> HPET_NUM_TIM_CAP_SHIFT) 
+ 1;
+
+    if (!ntimer) {
+        printk(KERN_WARNING "hpet: no timers in config data\n");
+        return -ENODEV;
+    }
+    else if (hpetp->hp_ntimer != ntimer) {
+        printk(KERN_WARNING "hpet: number irqs doesn't agree"
+            " with number of timers\n");
+        return -ENODEV;
+    }
+
+    hpetp->hp_period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
+        HPET_COUNTER_CLK_PERIOD_SHIFT;
+
+    mcfg = hpet->hpet_config;
+    if ((mcfg & HPET_ENABLE_CNF_MASK) == 0) {
+        hpet->hpet_mc = 0L;
+        mcfg |= HPET_ENABLE_CNF_MASK;
+        hpet->hpet_config = mcfg;
+    }
+
+    /*
+     * Create character devices and init wait queue.
+     * If this is a platform call, then device initialization
+     * occurs during startup call to hpet_init.
+     */
+    if (hdp->hd_flags ^ HPET_DATA_PLATFORM)
+        hpet_init_chrdev();
+
+    for (i = 0, devp = hpetp->hp_dev;  i < hpetp->hp_ntimer; i++, 

hpet_ntimer++, devp++) {

drivers/char/hpet.c +48 -0
+        unsigned long v;
+        volatile struct hpet_timer *timer;
+
+        timer = &hpet->hpet_timers[devp - hpetp->hp_dev];
+        v = timer->hpet_config;
+
+        devp->hd_hpets = hpetp;
+        devp->hd_hpet = hpet;
+        devp->hd_timer = timer;
+
+        devp->hd_minor = hpet_ntimer;
+
+        if (hdp->hd_state & (1 << i)) {
+            devp->hd_flags = HPET_OPEN;
+            continue;
+        }
+
+        if (hdp->hd_flags & HPET_DATA_PLATFORM)
+            continue;
+
+        sprintf(&hpetname[5], "%d", hpet_ntimer);
+        devfs_mk_cdev(MKDEV(HPET_MAJOR, hpet_ntimer),
+            S_IFCHR|S_IRUSR|S_IWUSR, hpetname);
+        init_waitqueue_head(&devp->hd_waitqueue);
+    }
+
+    hpetp->hp_delta = hpet_calibrate(hpetp);
+
+    return 0;
+}
+
+static acpi_status __init
+hpet_resources (struct acpi_resource *res, void *data)
+{
+    struct hpet_data *hdp;
+    acpi_status status;
+    struct acpi_resource_address64 addr;
+    struct hpets *hpetp;
+
+    hdp = data;
+
+    status = acpi_resource_to_address64(res, &addr);
+
+    if (ACPI_SUCCESS(status)) {
+        unsigned long size;
+
+        size = addr.max_address_range - addr.min_address_range + 1;
+        hdp->hd_address = (unsigned long) 

ioremap(addr.min_address_range, size);

drivers/char/hpet.c +36 -0
+
+        for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
+            if (hpetp->hp_hpet == (struct hpet *) (hdp->hd_address))
+                return -EBUSY;
+    }
+    else if (res->id == ACPI_RSTYPE_EXT_IRQ) {
+        struct acpi_resource_ext_irq *irqp;
+        int i;
+
+        irqp = &res->data.extended_irq;
+
+        if (irqp->number_of_interrupts > 0) {
+            hdp->hd_nirqs = irqp->number_of_interrupts;
+
+            for (i = 0; i < hdp->hd_nirqs; i++)
+#ifdef    CONFIG_IA64
+                hdp->hd_irq[i] = acpi_register_irq(irqp->interrupts[i],
+                            irqp->active_high_low, irqp->edge_level);
+#else
+                hdp->hd_irq[i] = irqp->interrupts[i];
+#endif
+        }
+    }
+
+    return AE_OK;
+}
+
+static int __init
+hpet_acpi_add (struct acpi_device *device)
+{
+    acpi_status result;
+    struct hpet_data data;
+
+    memset(&data, 0, sizeof(data));
+
+    result = acpi_walk_resources(device->handle, METHOD_NAME__CRS, 

hpet_resources, &data);

drivers/char/hpet.c +69 -0
+
+    if (ACPI_FAILURE(result))
+        return -ENODEV;
+
+    if (!data.hd_address || !data.hd_nirqs) {
+        printk("%s: no address or irqs in _CRS\n", __FUNCTION__);
+        return -ENODEV;
+    }
+
+    return hpet_alloc(&data);
+}
+
+static int __init
+hpet_acpi_remove (struct acpi_device *device, int type)
+{
+    return 0;
+}
+
+static struct acpi_driver hpet_acpi_driver __initdata = {
+    .name    =    "hpet",
+    .class    =    "",
+    .ids    =    "PNP0103",
+    .ops = {
+        .add    =    hpet_acpi_add,
+        .remove    =    hpet_acpi_remove,
+    },
+};
+
+
+
+static int __init
+hpet_init (void)
+{
+    struct proc_dir_entry *entry;
+
+    if (hpets)
+        hpet_post_platform();
+
+    (void) acpi_bus_register_driver(&hpet_acpi_driver);
+
+    if (hpets) {
+        entry = create_proc_entry("driver/hpet", 0, 0);
+
+        if (entry)
+            entry->proc_fops = &hpet_proc_fops;
+
+        sysctl_header = register_sysctl_table(dev_root, 0);
+
+#ifdef    CONFIG_TIME_INTERPOLATION
+        {
+            volatile struct hpet    *hpet;
+
+            hpet = hpets->hp_hpet;
+            hpet_cycles_per_sec = hpet_time_div(hpets->hp_period);
+            hpet_interpolator.frequency = hpet_cycles_per_sec;
+            hpet_interpolator.drift = hpet_cycles_per_sec *
+                HPET_DRIFT / 1000000;
+            hpet_nsecs_per_cycle = 1000000000 / hpet_cycles_per_sec;
+            register_time_interpolator(&hpet_interpolator);
+        }
+#endif
+        return 0;
+    }
+    else
+        return -ENODEV;
+}
+
+static void __exit
+hpet_exit (void)

drivers/char/hpet.c line 1110

Quoted 10 lines
drivers/char/hpet.c +10 -0
+{
+    acpi_bus_unregister_driver(&hpet_acpi_driver);
+
+    if (hpets) {
+        unregister_sysctl_table(sysctl_header);
+        remove_proc_entry("driver/hpet", NULL);
+    }
+
+    return;
+}

Finally, would prefer sysfs use to procfs.

No major objections though.

Jeff

#3 lore raw
Quoted 6 lines · Jeff Garzik

drivers/char/hpet.c line 253

Quoted 4 lines
drivers/char/hpet.c +3 -0
+    vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);
+    vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+    addr = __pa(addr);

where did these flags come from? don't you just want VM_RESERVED?

VM_IO is the way to mark mmapped I/O devices.

	vma->vm_flags |= VM_IO;

should be sufficient here.

hm, I'm trying to decrypt how the driver accesses the hardware. It's taking copies of kernel virtual addresses based off hpet_virt_address, but there are no readl's or writel's in there. Is the actual device access done over in time_hpet.c?

#4 lore raw
Quoted 6 lines · Andrew Morton

hm, I'm trying to decrypt how the driver accesses the hardware. It's taking copies of kernel virtual addresses based off hpet_virt_address, but there are no readl's or writel's in there. Is the actual device access done over in time_hpet.c?

Oh. That's why the volatiles are there.

+static int
+hpet_release (struct inode *inode, struct file *file) +{
+ struct hpet_dev *devp;
+ volatile struct hpet_timer *timer;
+
+ devp = file->private_data;
+ timer = devp->hd_timer;
+
+ spin_lock_irq(&hpet_lock);
+
+ timer->hpet_config &= ~Tn_INT_ENB_CNF_MASK;

local variable `timer' points at hardware.

Methinks this all should be converted to readl and friends, no?

#6 lore raw
In reply to Andrew Morton
Quoted 11 lines · Andrew Morton
Quoted 5 lines · Jeff Garzik

drivers/char/hpet.c line 253

Quoted 3 lines
drivers/char/hpet.c +3 -0
+    vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);
+    vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+    addr = __pa(addr);

where did these flags come from? don't you just want VM_RESERVED?

VM_IO is the way to mark mmapped I/O devices.

Quoted 8 lines
	vma->vm_flags |= VM_IO;

should be sufficient here.

hm, I'm trying to decrypt how the driver accesses the hardware. It's taking copies of kernel virtual addresses based off hpet_virt_address, but there are no readl's or writel's in there. Is the actual device access done over in time_hpet.c?

HPET writes into RAM at magic addresses, so it's not really a bus address.

Thus I think only VM_RESERVED is needed...

Jeff

#7 lore raw
Quoted 16 lines · Jeff Garzik
Quoted 15 lines · Andrew Morton
Quoted 5 lines · Jeff Garzik

drivers/char/hpet.c line 253

Quoted 3 lines
drivers/char/hpet.c +3 -0
+    vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);
+    vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+    addr = __pa(addr);

where did these flags come from? don't you just want VM_RESERVED?

VM_IO is the way to mark mmapped I/O devices.

	vma->vm_flags |= VM_IO;

should be sufficient here.

Quoted 7 lines
Quoted 4 lines

hm, I'm trying to decrypt how the driver accesses the hardware. It's taking copies of kernel virtual addresses based off hpet_virt_address, but there are no readl's or writel's in there. Is the actual device access done over in time_hpet.c?

HPET writes into RAM at magic addresses, so it's not really a bus address.

Since it lives in the bridge (either north or south), IMO it is a bus address ... at least on AMD machines it's the same space where MMIO comes from.

Quoted 3 lines

Thus I think only VM_RESERVED is needed...

Jeff

Signature · 2 lines
Vojtech Pavlik
SuSE Labs, SuSE CR
#8 lore raw
Quoted 7 lines · Vojtech Pavlik
Quoted 1 line · Jeff Garzik

HPET writes into RAM at magic addresses, so it's not really a bus address.

Since it lives in the bridge (either north or south), IMO it is a bus address ... at least on AMD machines it's the same space where MMIO comes from.

hmmm, yeah, you're right. I stand corrected.

Jeff

#9 lore raw
In reply to Jeff Garzik

All issues hopefully addressed below. Another patch with diffs against -mm3.

Quoted 2 lines

run this through Lindent... non-standard indentation

Done. ...

Quoted 2 lines

kill the 'volatile', it's preferred to put barriers and such in the code where they are needed.

O.K. Did this but had to add a writeq and readq for i386.
...

Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +6 -0
+static int
+hpet_open (struct inode *inode, struct file *file)
+{
+    struct hpet_dev *devp;
+
+    devp = hpet_minor_to_dev(MINOR(inode->i_rdev));

locking of hpets?

Not sure what you are getting at here. hpets can't really change in number after boot initialization.

Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +3 -0
+    vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);
+    vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+    addr = __pa(addr);

where did these flags come from? don't you just want VM_RESERVED?

Changed to just VM_IO.

Quoted 11 lines

Review

Quoted 8 lines

+ if (remap_page_range(vma, vma->vm_start, addr, PAGE_SIZE,
vma->vm_page_prot)) {

(quoted diff) +3 -0
+        printk(KERN_ERR "remap_page_range failed in hpet.c\n");
+        return -EAGAIN;
+    }

why not ->nopage like in sound/oss/via82cxxx_audio.c?

I just modelled after what is done by other drivers in kernel. ...

Quoted 7 lines

Review

Quoted 4 lines
(quoted diff) +2 -0
+        devp->hd_flags ^= HPET_IE;
+        break;

do you really want to be freeing the irq outside of the open-close model?

Well it seemed complete to allocate the irq and free it when it is no longer required. I can't see how it would be dangerous. Perhaps I'm missing the point.
....

Quoted 2 lines

Finally, would prefer sysfs use to procfs.

I can look to changing it over to sysfs in future. For now I'd like to keep procfs.

Quoted 5 lines

No major objections though.

Jeff

The drivers/char/Kconfig changes will fix the x86_64 build problem with -mm3. Should this patch be acceptable, then I really need to ask Venki to test once more on his i386/HPET hardware to be sure nothing is broken there.

thanks,

Bob

Documentation/filesystems/proc.txt +1 -1
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-mm3/Documentation/filesystems/proc.txt linux-2.6.6-mm3-hpet/Documentation/filesystems/proc.txt
@@ -201,7 +201,7 @@
 devices     Available devices (block and character)           
 dma         Used DMS channels                                 
 filesystems Supported filesystems                             
- driver	     Various drivers grouped here, currently rtc	(2.4)
+ driver	     Various drivers grouped here, currently rtc (2.4) and hpet (2.6)
 execdomains Execdomains, related to security			(2.4)
 fb	     Frame Buffer devices				(2.4)
 fs	     File system parameters, currently nfs/exports	(2.4)
drivers/char/hpet.c +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-mm3/drivers/char/hpet.c linux-2.6.6-mm3-hpet/drivers/char/hpet.c
@@ -52,39 +52,60 @@

static spinlock_t hpet_task_lock = SPIN_LOCK_UNLOCKED;

struct hpet_dev {
- struct hpets *hd_hpets;
- volatile struct hpet *hd_hpet;
- volatile struct hpet_timer
- *hd_timer;
- unsigned long hd_ireqfreq;
- unsigned long hd_irqdata;
- wait_queue_head_t hd_waitqueue;
- struct fasync_struct *hd_async_queue;
- struct hpet_task *hd_task;
- unsigned int hd_flags;
- unsigned int hd_irq;
- unsigned int hd_hdwirq;
- int hd_minor;
+ struct hpets *hd_hpets;
+ struct hpet *hd_hpet;
+ struct hpet_timer *hd_timer;
+ unsigned long hd_ireqfreq;
+ unsigned long hd_irqdata;
+ wait_queue_head_t hd_waitqueue;
+ struct fasync_struct *hd_async_queue;
+ struct hpet_task *hd_task;
+ unsigned int hd_flags;
+ unsigned int hd_irq;
+ unsigned int hd_hdwirq;
+ int hd_minor;
};

struct hpets {
- struct hpets *hp_next;
- volatile struct hpet *hp_hpet;
- unsigned long hp_period;
- unsigned long hp_delta;
- unsigned int hp_ntimer;
- unsigned int hp_which;
- struct hpet_dev hp_dev[1];
+ struct hpets *hp_next;
+ struct hpet *hp_hpet;
+ unsigned long hp_period;
+ unsigned long hp_delta;
+ unsigned int hp_ntimer;
+ unsigned int hp_which;
+ struct hpet_dev hp_dev[1];
};

static struct hpets *hpets;

#define HPET_OPEN 0x0001
-#define HPET_IE 0x0002 /* interrupt enabled */
+#define HPET_IE 0x0002 /* interrupt enabled */
#define HPET_PERIODIC 0x0004

-static irqreturn_t
-hpet_interrupt (int irq, void *data, struct pt_regs *regs)
+#if BITS_PER_LONG == 64
+#define write_counter(V, MC) writeq(V, MC)
+#define read_counter(MC) readq(MC)
+#else
+#define write_counter(V, MC) writel(V, MC)
+#define read_counter(MC) readl(MC)
+#endif
+
+#ifndef readq
+static unsigned long long __inline readq(void *addr)
+{
+ return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
+}
+#endif
+
+#ifndef writeq
+static void __inline writeq(unsigned long long v, void *addr)
+{
+ writel(v & 0xffffffff, addr);
+ writel(v >> 32, addr + 4);
+}
+#endif
+
+static irqreturn_t hpet_interrupt(int irq, void *data, struct pt_regs *regs)

{
	struct hpet_dev *devp;
	unsigned long isr;
@@ -94,16 +115,21 @@
	spin_lock(&hpet_lock);
	devp->hd_irqdata++;

+ /*
+ * For non-periodic timers, increment the accumulator.
+ * This has the effect of treating non-periodic like periodic.
+ */

	if ((devp->hd_flags & (HPET_IE | HPET_PERIODIC)) == HPET_IE) {
		unsigned long m, t;

		t = devp->hd_ireqfreq;

- m = devp->hd_hpet->hpet_mc;
- devp->hd_timer->hpet_compare = t + m + devp->hd_hpets->hp_delta;
+ m = read_counter(&devp->hd_hpet->hpet_mc);
+ write_counter(t + m + devp->hd_hpets->hp_delta,

+			      &devp->hd_timer->hpet_compare);
	}

	isr = (1 << (devp - devp->hd_hpets->hp_dev));
-	devp->hd_hpet->hpet_isr = isr;
+	writeq(isr, &devp->hd_hpet->hpet_isr);
	spin_unlock(&hpet_lock);

	spin_lock(&hpet_task_lock);
@@ -118,21 +144,19 @@
	return IRQ_HANDLED;
}

-static struct hpet_dev *
-hpet_minor_to_dev (int minor)
+static struct hpet_dev *hpet_minor_to_dev(int minor)

{
	struct hpets *hpetp;
	int i;

	for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
-		for (i = 0; i <  hpetp->hp_ntimer; i++)
+		for (i = 0; i < hpetp->hp_ntimer; i++)
			if (hpetp->hp_dev[i].hd_minor == minor)
				return &hpetp->hp_dev[i];
	return 0;
}

-static int
-hpet_open (struct inode *inode, struct file *file) +static int hpet_open(struct inode *inode, struct file *file)

{
	struct hpet_dev *devp;

@@ -142,7 +166,7 @@

		return -ENODEV;

	spin_lock_irq(&hpet_lock);
-	if (devp->hd_flags &  HPET_OPEN || devp->hd_task) {
+	if (devp->hd_flags & HPET_OPEN || devp->hd_task) {
		spin_unlock_irq(&hpet_lock);
		return -EBUSY;
	}

@@ -156,18 +180,18 @@
}

static ssize_t
-hpet_read (struct file *file, char *buf, size_t count, loff_t *ppos) +hpet_read(struct file *file, char *buf, size_t count, loff_t * ppos) {
- DECLARE_WAITQUEUE(wait,current);

+	DECLARE_WAITQUEUE(wait, current);
	unsigned long data;

- ssize_t retval;
- struct hpet_dev *devp;
+ ssize_t retval;
+ struct hpet_dev *devp;

	devp = file->private_data;
	if (!devp->hd_ireqfreq)
		return -EIO;

- if (count < sizeof (unsigned long))
+ if (count < sizeof(unsigned long))

		return -EINVAL;

	add_wait_queue(&devp->hd_waitqueue, &wait);

@@ -192,23 +216,22 @@

		schedule();

- } while(1);
+ } while (1);

- retval = put_user(data, (unsigned long *) buf);
+ retval = put_user(data, (unsigned long *)buf);
if (!retval)
- retval = sizeof (unsigned long);
-out:
+ retval = sizeof(unsigned long);
+ out:

	current->state = TASK_RUNNING;
	remove_wait_queue(&devp->hd_waitqueue, &wait);

	return retval;
}

-static unsigned int
-hpet_poll (struct file *file, poll_table *wait)
+static unsigned int hpet_poll(struct file *file, poll_table * wait)

{
	unsigned long v;

- struct hpet_dev *devp;
+ struct hpet_dev *devp;

	devp = file->private_data;

@@ -227,13 +250,12 @@

	return 0;
}

-static int
-hpet_mmap (struct file *file, struct vm_area_struct *vma) +static int hpet_mmap(struct file *file, struct vm_area_struct *vma) {

#ifdef	CONFIG_HPET_NOMMAP
	return -ENOSYS;
#else
-	struct hpet_dev	*devp;
+	struct hpet_dev *devp;
	unsigned long addr;

	if (((vma->vm_end - vma->vm_start) != PAGE_SIZE) || vma->vm_pgoff)
@@ -243,16 +265,17 @@
		return -EPERM;

	devp = file->private_data;

- addr = (unsigned long) devp->hd_hpet;
+ addr = (unsigned long)devp->hd_hpet;

	if (addr & (PAGE_SIZE - 1))
		return -ENOSYS;

- vma->vm_flags |= (VM_IO | VM_SHM | VM_LOCKED);

+	vma->vm_flags |= VM_IO;
	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
	addr = __pa(addr);

- if (remap_page_range(vma, vma->vm_start, addr, PAGE_SIZE, vma->vm_page_prot)) {
+ if (remap_page_range

+	    (vma, vma->vm_start, addr, PAGE_SIZE, vma->vm_page_prot)) {
		printk(KERN_ERR "remap_page_range failed in hpet.c\n");
		return -EAGAIN;
	}

@@ -261,10 +284,9 @@
#endif
}

-static int
-hpet_fasync (int fd, struct file *file, int on) +static int hpet_fasync(int fd, struct file *file, int on) {
- struct hpet_dev *devp;
+ struct hpet_dev *devp;

	devp = file->private_data;

@@ -274,18 +296,18 @@

		return -EIO;
}

-static int
-hpet_release (struct inode *inode, struct file *file) +static int hpet_release(struct inode *inode, struct file *file) {
- struct hpet_dev *devp;
- volatile struct hpet_timer *timer;
+ struct hpet_dev *devp;
+ struct hpet_timer *timer;

	devp = file->private_data;
	timer = devp->hd_timer;

	spin_lock_irq(&hpet_lock);

- timer->hpet_config &= ~Tn_INT_ENB_CNF_MASK;
+ writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
+ &timer->hpet_config);

	if (devp->hd_irq) {
		free_irq(devp->hd_irq, devp);

@@ -294,12 +316,13 @@

	devp->hd_ireqfreq = 0;

- if (devp->hd_flags & HPET_PERIODIC && timer->hpet_config & Tn_TYPE_CNF_MASK) {
+ if (devp->hd_flags & HPET_PERIODIC

+	    && readq(&timer->hpet_config) & Tn_TYPE_CNF_MASK) {
		unsigned long v;

- v = timer->hpet_config;

+		v = readq(&timer->hpet_config);
		v ^= Tn_TYPE_CNF_MASK;
-		timer->hpet_config = v;
+		writeq(v, &timer->hpet_config);
	}

	devp->hd_flags &= ~(HPET_OPEN | HPET_IE | HPET_PERIODIC);

@@ -315,19 +338,19 @@
static int hpet_ioctl_common(struct hpet_dev *, int, unsigned long, int);

static int
-hpet_ioctl (struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg) +hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+ unsigned long arg)
{
- struct hpet_dev *devp;
+ struct hpet_dev *devp;

	devp = file->private_data;
	return hpet_ioctl_common(devp, cmd, arg, 0);
}

-static int
-hpet_ioctl_ieon (struct hpet_dev *devp) +static int hpet_ioctl_ieon(struct hpet_dev *devp) {
- volatile struct hpet_timer *timer;
- volatile struct hpet *hpet;
+ struct hpet_timer *timer;

+	struct hpet *hpet;
	struct hpets *hpetp;
	int irq;
	unsigned long g, v, t, m;
@@ -337,7 +360,7 @@
	hpet = devp->hd_hpet;
	hpetp = devp->hd_hpets;

- v = timer->hpet_config;

+	v = readq(&timer->hpet_config);
	spin_lock_irq(&hpet_lock);

	if (devp->hd_flags & HPET_IE) {
@@ -348,15 +371,16 @@
	devp->hd_flags |= HPET_IE;
	spin_unlock_irq(&hpet_lock);

- t = timer->hpet_config;

+	t = readq(&timer->hpet_config);
	irq = devp->hd_hdwirq;

	if (irq) {
		char name[7];

- sprintf(name, "hpet%d", (int) (devp - hpetp->hp_dev));
+ sprintf(name, "hpet%d", (int)(devp - hpetp->hp_dev));

- if (request_irq(irq, hpet_interrupt, SA_INTERRUPT, name, (void *) devp)) {
+ if (request_irq

+		    (irq, hpet_interrupt, SA_INTERRUPT, name, (void *)devp)) {
			printk(KERN_ERR "hpet: IRQ %d is not free\n", irq);
			irq = 0;
		}

@@ -371,48 +395,47 @@

	devp->hd_irq = irq;
	t = devp->hd_ireqfreq;
-	v = timer->hpet_config;
+	v = readq(&timer->hpet_config);
	g = v | Tn_INT_ENB_CNF_MASK;

	if (devp->hd_flags & HPET_PERIODIC) {
-		timer->hpet_compare = t;
+		write_counter(t, &timer->hpet_compare);
		g |= Tn_TYPE_CNF_MASK;
		v |= Tn_TYPE_CNF_MASK;
-		timer->hpet_config = v;
+		writeq(v, &timer->hpet_config);
		v |= Tn_VAL_SET_CNF_MASK;
-		timer->hpet_config = v;
+		writeq(v, &timer->hpet_config);
		local_irq_save(flags);
-		m = hpet->hpet_mc;
-		timer->hpet_compare = t + m + hpetp->hp_delta;
+		m = read_counter(&hpet->hpet_mc);
+		write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
	} else {
		local_irq_save(flags);
-		m = hpet->hpet_mc;
-		timer->hpet_compare = t + m + hpetp->hp_delta;
+		m = read_counter(&hpet->hpet_mc);
+		write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
	}

	isr = (1 << (devp - hpets->hp_dev));
-	hpet->hpet_isr = isr;
-	timer->hpet_config = g;
+	writeq(isr, &hpet->hpet_isr);
+	writeq(g, &timer->hpet_config);
	local_irq_restore(flags);

	return 0;
}

-static inline unsigned long
-hpet_time_div (unsigned long dis)
+static inline unsigned long hpet_time_div(unsigned long dis)

{
	unsigned long long m = 1000000000000000ULL;

	do_div(m, dis);

- return (unsigned long) m;
+ return (unsigned long)m;
}

static int
-hpet_ioctl_common (struct hpet_dev *devp, int cmd, unsigned long arg, int kernel) +hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg, int kernel) {
- volatile struct hpet_timer *timer;
- volatile struct hpet *hpet;
+ struct hpet_timer *timer;

+	struct hpet *hpet;
	struct hpets *hpetp;
	int err;
	unsigned long v;
@@ -439,9 +462,9 @@
	case HPET_IE_OFF:
		if ((devp->hd_flags & HPET_IE) == 0)
			break;
-		v = timer->hpet_config;
+		v = readq(&timer->hpet_config);
		v &= ~Tn_INT_ENB_CNF_MASK;
-		timer->hpet_config = v;
+		writeq(v, &timer->hpet_config);
		if (devp->hd_irq) {
			free_irq(devp->hd_irq, devp);
			devp->hd_irq = 0;
@@ -449,18 +472,19 @@
		devp->hd_flags ^= HPET_IE;
		break;
	case HPET_INFO:

- {
- struct hpet_info info;
+ {
+ struct hpet_info info;

- info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
- devp->hd_ireqfreq);
- info.hi_flags = timer->hpet_config & Tn_PER_INT_CAP_MASK;
- if (copy_to_user((void *) arg, &info , sizeof (info)))
- err = -EFAULT;
- break;
- }
+ info.hi_ireqfreq = hpet_time_div(hpetp->hp_period *
+ devp->hd_ireqfreq);
+ info.hi_flags =
+ readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK;
+ if (copy_to_user((void *)arg, &info, sizeof(info)))
+ err = -EFAULT;
+ break;
+ }
case HPET_EPI:
- v = timer->hpet_config;

+		v = readq(&timer->hpet_config);
		if ((v & Tn_PER_INT_CAP_MASK) == 0) {
			err = -ENXIO;
			break;
@@ -468,22 +492,22 @@
		devp->hd_flags |= HPET_PERIODIC;
		break;
	case HPET_DPI:
-		v = timer->hpet_config;
+		v = readq(&timer->hpet_config);
		if ((v & Tn_PER_INT_CAP_MASK) == 0) {
			err = -ENXIO;
			break;
		}
		if (devp->hd_flags & HPET_PERIODIC &&
-			timer->hpet_config & Tn_TYPE_CNF_MASK) {
-			v = timer->hpet_config;
+		    readq(&timer->hpet_config) & Tn_TYPE_CNF_MASK) {
+			v = readq(&timer->hpet_config);
			v ^= Tn_TYPE_CNF_MASK;
-			timer->hpet_config = v;
+			writeq(v, &timer->hpet_config);
		}
		devp->hd_flags &= ~HPET_PERIODIC;
		break;
	case HPET_IRQFREQ:
		if (!kernel && (arg > hpet_max_freq) &&
-			!capable(CAP_SYS_RESOURCE)) {
+		    !capable(CAP_SYS_RESOURCE)) {
			err = -EACCES;
			break;
		}

@@ -500,15 +524,15 @@
}

static struct file_operations hpet_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .read = hpet_read,
- .poll = hpet_poll,
- .ioctl = hpet_ioctl,
- .open = hpet_open,
- .release = hpet_release,
- .fasync = hpet_fasync,
- .mmap = hpet_mmap,
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .read = hpet_read,
+ .poll = hpet_poll,
+ .ioctl = hpet_ioctl,
+ .open = hpet_open,
+ .release = hpet_release,
+ .fasync = hpet_fasync,
+ .mmap = hpet_mmap,
};

EXPORT_SYMBOL(hpet_alloc);
@@ -516,12 +540,11 @@
EXPORT_SYMBOL(hpet_unregister);
EXPORT_SYMBOL(hpet_control);

-int
-hpet_register (struct hpet_task *tp, int periodic) +int hpet_register(struct hpet_task *tp, int periodic)

{
	unsigned int i;
	u64 mask;
-	volatile struct hpet_timer *timer;
+	struct hpet_timer *timer;
	struct hpet_dev *devp;
	struct hpets *hpetp;

@@ -540,21 +563,23 @@

	spin_lock(&hpet_lock);

	for (devp = 0, hpetp = hpets; hpetp && !devp; hpetp = hpetp->hp_next)

- for (timer = hpetp->hp_hpet->hpet_timers, i = 0; i < hpetp->hp_ntimer; i++, timer++) {
- if ((timer->hpet_config & Tn_PER_INT_CAP_MASK) != mask)
- continue;
+ for (timer = hpetp->hp_hpet->hpet_timers, i = 0;
+ i < hpetp->hp_ntimer; i++, timer++) {
+ if ((readq(&timer->hpet_config) & Tn_PER_INT_CAP_MASK)
+ != mask)
+ continue;

- devp = &hpetp->hp_dev[i];
+ devp = &hpetp->hp_dev[i];

- if (devp->hd_flags & HPET_OPEN || devp->hd_task) {
- devp = 0;
- continue;
- }
+ if (devp->hd_flags & HPET_OPEN || devp->hd_task) {
+ devp = 0;
+ continue;
+ }

- tp->ht_opaque = devp;
- devp->hd_task = tp;
- break;
- }
+ tp->ht_opaque = devp;
+ devp->hd_task = tp;
+ break;
+ }

	spin_unlock(&hpet_lock);
	spin_unlock_irq(&hpet_task_lock);
@@ -565,10 +590,9 @@
		return -EBUSY;
}

-static inline int
-hpet_tpcheck (struct hpet_task *tp)
+static inline int hpet_tpcheck(struct hpet_task *tp) {
- struct hpet_dev *devp;

+	struct hpet_dev *devp;
	struct hpets *hpetp;

	devp = tp->ht_opaque;
@@ -577,19 +601,18 @@
		return -ENXIO;

	for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)

- if (devp >= hpetp->hp_dev && devp < (hpetp->hp_dev + hpetp->hp_ntimer) &&
- devp->hd_hpet == hpetp->hp_hpet)
+ if (devp >= hpetp->hp_dev
+ && devp < (hpetp->hp_dev + hpetp->hp_ntimer)
+ && devp->hd_hpet == hpetp->hp_hpet)

			return 0;

	return -ENXIO;
}

-
-int
-hpet_unregister (struct hpet_task *tp)
+int hpet_unregister(struct hpet_task *tp)
{
- struct hpet_dev *devp;
- volatile struct hpet_timer *timer;
+ struct hpet_dev *devp;

+	struct hpet_timer *timer;
	int err;

	if ((err = hpet_tpcheck(tp)))
@@ -606,8 +629,9 @@
	}

	timer = devp->hd_timer;

- timer->hpet_config &= ~Tn_INT_ENB_CNF_MASK;
- devp->hd_flags &= ~( HPET_IE | HPET_PERIODIC);
+ writeq((readq(&timer->hpet_config) & ~Tn_INT_ENB_CNF_MASK),
+ &timer->hpet_config);

+	devp->hd_flags &= ~(HPET_IE | HPET_PERIODIC);
	devp->hd_task = 0;
	spin_unlock(&hpet_lock);
	spin_unlock_irq(&hpet_task_lock);
@@ -615,10 +639,9 @@
	return 0;
}

-int
-hpet_control (struct hpet_task *tp, unsigned int cmd, unsigned long arg) +int hpet_control(struct hpet_task *tp, unsigned int cmd, unsigned long arg) {
- struct hpet_dev *devp;

+	struct hpet_dev *devp;
	int err;

	if ((err = hpet_tpcheck(tp)))
@@ -634,93 +657,83 @@
	return hpet_ioctl_common(devp, cmd, arg, 1);
}

-
-
#ifdef CONFIG_TIME_INTERPOLATION

static unsigned long hpet_offset, last_wall_hpet; static long hpet_nsecs_per_cycle, hpet_cycles_per_sec;

-static unsigned long
-hpet_getoffset (void)
+static unsigned long hpet_getoffset(void)
{
- return hpet_offset + (hpets->hp_hpet->hpet_mc - last_wall_hpet) * hpet_nsecs_per_cycle;
+ return hpet_offset + (read_counter(&hpets->hp_hpet->hpet_mc) -
+ last_wall_hpet) * hpet_nsecs_per_cycle;
}

-static void
-hpet_update (long delta)
+static void hpet_update(long delta)

{
	unsigned long mc;
	unsigned long offset;

- volatile struct hpet *hpet;

- hpet = hpets->hp_hpet;
- mc = hpet->hpet_mc;

+	mc = read_counter(&hpets->hp_hpet->hpet_mc);
	offset = hpet_offset + (mc - last_wall_hpet) * hpet_nsecs_per_cycle;
-	if (delta < 0 || (unsigned long) delta < offset)
+	if (delta < 0 || (unsigned long)delta < offset)
		hpet_offset = offset - delta;
	else
		hpet_offset = 0;
	last_wall_hpet = mc;
}

-static void
-hpet_reset (void)
+static void hpet_reset(void)
{
- volatile struct hpet *hpet;
-

-	hpet = hpets->hp_hpet;
	hpet_offset = 0;

- last_wall_hpet = hpet->hpet_mc;
+ last_wall_hpet = read_counter(&hpets->hp_hpet->hpet_mc);
}

static struct time_interpolator hpet_interpolator = {
- .get_offset = hpet_getoffset,
- .update = hpet_update,
- .reset = hpet_reset
+ .get_offset = hpet_getoffset,
+ .update = hpet_update,
+ .reset = hpet_reset
};

#endif

static ctl_table hpet_table[] = {
	{

- .ctl_name = 1,
- .procname = "max-user-freq",
- .data = &hpet_max_freq,
- .maxlen = sizeof(int),
- .mode = 0644,
- .proc_handler = &proc_dointvec,
- },
- { .ctl_name = 0 }
+ .ctl_name = 1,
+ .procname = "max-user-freq",
+ .data = &hpet_max_freq,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = &proc_dointvec,
+ },
+ {.ctl_name = 0}
};

static ctl_table hpet_root[] = {
	{

- .ctl_name = 1,
- .procname = "hpet",
- .maxlen = 0,
- .mode = 0555,
- .child = hpet_table,
- },
- { .ctl_name = 0 }
+ .ctl_name = 1,
+ .procname = "hpet",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = hpet_table,
+ },
+ {.ctl_name = 0}
};

static ctl_table dev_root[] = {
	{

- .ctl_name = CTL_DEV,
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = hpet_root,
- },
- { .ctl_name = 0 }
+ .ctl_name = CTL_DEV,
+ .procname = "dev",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = hpet_root,
+ },
+ {.ctl_name = 0}
};

static struct ctl_table_header *sysctl_header;

-static void *
-hpet_start (struct seq_file *s, loff_t *pos) +static void *hpet_start(struct seq_file *s, loff_t * pos)

{
	struct hpets *hpetp;
	loff_t n;
@@ -732,8 +745,7 @@
	return 0;
}

-static void *
-hpet_next (struct seq_file *s, void *v, loff_t *pos) +static void *hpet_next(struct seq_file *s, void *v, loff_t * pos)

{
	struct hpets *hpetp;

@@ -742,54 +754,52 @@

	return hpetp->hp_next;
}

-static void
-hpet_stop (struct seq_file *s, void *v) +static void hpet_stop(struct seq_file *s, void *v)

{
	return;
}

-static int
-hpet_show (struct seq_file *s, void *v) +static int hpet_show(struct seq_file *s, void *v)

{
	struct hpets *hpetp;
-	volatile struct hpet *hpet;
+	struct hpet *hpet;
	u64 cap, vendor, period;

	hpetp = v;
	hpet = hpetp->hp_hpet;

- cap = hpet->hpet_cap;
+ cap = readq(&hpet->hpet_cap);
period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
- HPET_COUNTER_CLK_PERIOD_SHIFT;

+	    HPET_COUNTER_CLK_PERIOD_SHIFT;
	vendor = (cap & HPET_VENDOR_ID_MASK) >> HPET_VENDOR_ID_SHIFT;

- seq_printf(s, "HPET%d period = %d 10**-15 vendor = 0x%x number timer = %d\n",
- hpetp->hp_which, (u32) period, (u32) vendor, hpetp->hp_ntimer);
+ seq_printf(s,

+		   "HPET%d period = %d 10**-15  vendor = 0x%x number timer = %d\n",
+		   hpetp->hp_which, (u32) period, (u32) vendor,
+		   hpetp->hp_ntimer);
	return 0;
}

static struct seq_operations hpet_seq_ops = {
- .start = hpet_start,
- .next = hpet_next,
- .stop = hpet_stop,
- .show = hpet_show
+ .start = hpet_start,
+ .next = hpet_next,
+ .stop = hpet_stop,
+ .show = hpet_show
};

-static int
-hpet_proc_open (struct inode *inode, struct file *file) +static int hpet_proc_open(struct inode *inode, struct file *file)

{
	return seq_open(file, &hpet_seq_ops);
}

static struct file_operations hpet_proc_fops = {
- .open = hpet_proc_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = seq_release
+ .open = hpet_proc_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release
};

-
/*
* Adjustment for when arming the timer with
* initial conditions. That is, main counter
@@ -797,16 +807,16 @@
*/
#define TICK_CALIBRATE (1000UL)

-static unsigned long __init
-hpet_calibrate (struct hpets *hpetp)
+static unsigned long __init hpet_calibrate(struct hpets *hpetp) {
- volatile struct hpet_timer *timer;

+	struct hpet_timer *timer;
	unsigned long t, m, count, i, flags, start;
	struct hpet_dev *devp;
	int j;

- volatile struct hpet *hpet;
+ struct hpet *hpet;

- for (timer = 0, j = 0, devp = hpetp->hp_dev; j < hpetp->hp_ntimer; j++, devp++)
+ for (timer = 0, j = 0, devp = hpetp->hp_dev; j < hpetp->hp_ntimer;
+ j++, devp++)

		if ((devp->hd_flags & HPET_OPEN) == 0) {
			timer = devp->hd_timer;
			break;
@@ -816,27 +826,26 @@
		return 0;

	hpet = hpets->hp_hpet;

- t = timer->hpet_compare;
+ t = read_counter(&timer->hpet_compare);

	i = 0;
	count = hpet_time_div(hpetp->hp_period * TICK_CALIBRATE);

	local_irq_save(flags);

- start = hpet->hpet_mc;
+ start = read_counter(&hpet->hpet_mc);

	do {

- m = hpet->hpet_mc;
- timer->hpet_compare = t + m + hpetp->hp_delta;
- } while (i++, (m - start) < count);
+ m = read_counter(&hpet->hpet_mc);
+ write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
+ } while (i++, (m - start) < count);

	local_irq_restore(flags);

	return (m - start) / i;
}

-static void __init
-hpet_init_chrdev (void)
+static void __init hpet_init_chrdev(void)

{
	static int once;

@@ -846,47 +855,51 @@

	return;
}

-static void __init
-hpet_post_platform (void)
+static void __init hpet_post_platform(void)

{
	struct hpets *hpetp;
	u32 i, ntimer;

- struct hpet_dev *devp;
+ struct hpet_dev *devp;

	hpet_init_chrdev();

	for (ntimer = 0, hpetp = hpets; hpetp; hpetp = hpetp->hp_next, ntimer++)

- for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer; i++, devp++) {
+ for (i = 0, devp = hpetp->hp_dev; i < hpetp->hp_ntimer;
+ i++, devp++) {

			if (devp->hd_flags & HPET_OPEN)
				continue;

			sprintf(&hpetname[5], "%d", ntimer);
			devfs_mk_cdev(MKDEV(HPET_MAJOR, ntimer),
-				S_IFCHR|S_IRUSR|S_IWUSR, hpetname);
+				      S_IFCHR | S_IRUSR | S_IWUSR, hpetname);
			init_waitqueue_head(&devp->hd_waitqueue);
		}

	return;
}

-int __init
-hpet_alloc (struct hpet_data *hdp)
+int __init hpet_alloc(struct hpet_data *hdp)

{
	u64 cap, mcfg;
-	struct hpet_dev	*devp;
+	struct hpet_dev *devp;
	u32 i, ntimer;
	struct hpets *hpetp;
	size_t siz;

- volatile struct hpet *hpet;
- static struct hpets *last __initdata = (struct hpets *) 0;
+ struct hpet *hpet;
+ static struct hpets *last __initdata = (struct hpets *)0;

+ /*
+ * hpet_alloc can be called by platform dependent code.
+ * if platform dependent code has allocated the hpet
+ * ACPI also reports hpet, then we catch it here.
+ */

	for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
-		if (hpetp->hp_hpet == (struct hpet *) (hdp->hd_address))
+		if (hpetp->hp_hpet == (struct hpet *)(hdp->hd_address))
			return 0;

- siz = sizeof (struct hpets) + ((hdp->hd_nirqs - 1) *
- sizeof (struct hpet_dev));
+ siz = sizeof(struct hpets) + ((hdp->hd_nirqs - 1) *
+ sizeof(struct hpet_dev));

	hpetp = kmalloc(siz, GFP_KERNEL);

@@ -896,14 +909,7 @@

	memset(hpetp, 0, siz);

	hpetp->hp_which = hpet_nhpet++;

- hpetp->hp_hpet = (struct hpet *) hdp->hd_address;
-
- if (last)
- last->hp_next = hpetp;
- else
- hpets = hpetp;
-
- last = hpetp;
+ hpetp->hp_hpet = (struct hpet *)hdp->hd_address;

	hpetp->hp_ntimer = hdp->hd_nirqs;

@@ -912,28 +918,32 @@

	hpet = hpetp->hp_hpet;

- cap = hpet->hpet_cap;
+ cap = readq(&hpet->hpet_cap);

	ntimer = ((cap & HPET_NUM_TIM_CAP_MASK) >> HPET_NUM_TIM_CAP_SHIFT) + 1;

- if (!ntimer) {
- printk(KERN_WARNING "hpet: no timers in config data\n");
- return -ENODEV;
- }
- else if (hpetp->hp_ntimer != ntimer) {
+ if (hpetp->hp_ntimer != ntimer) {
printk(KERN_WARNING "hpet: number irqs doesn't agree"
- " with number of timers\n");
+ " with number of timers\n");

+		kfree(hpetp);
		return -ENODEV;
	}

+ if (last)
+ last->hp_next = hpetp;
+ else
+ hpets = hpetp;
+
+ last = hpetp;
+

	hpetp->hp_period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>

- HPET_COUNTER_CLK_PERIOD_SHIFT;
+ HPET_COUNTER_CLK_PERIOD_SHIFT;

- mcfg = hpet->hpet_config;

+	mcfg = readq(&hpet->hpet_config);
	if ((mcfg & HPET_ENABLE_CNF_MASK) == 0) {
-		hpet->hpet_mc = 0L;
+		write_counter(0L, &hpet->hpet_mc);
		mcfg |= HPET_ENABLE_CNF_MASK;
-		hpet->hpet_config = mcfg;
+		writeq(mcfg, &hpet->hpet_config);
	}

	/*
@@ -944,12 +954,13 @@
	if (hdp->hd_flags ^ HPET_DATA_PLATFORM)
		hpet_init_chrdev();

- 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++, hpet_ntimer++, devp++) {
		unsigned long v;

- volatile struct hpet_timer *timer;
+ struct hpet_timer *timer;

		timer = &hpet->hpet_timers[devp - hpetp->hp_dev];

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

		devp->hd_hpets = hpetp;
		devp->hd_hpet = hpet;

@@ -957,6 +968,10 @@

		devp->hd_minor = hpet_ntimer;

+ /*
+ * If the timer was reserved by platform code,
+ * then make timer unavailable for opens.
+ */

		if (hdp->hd_state & (1 << i)) {
			devp->hd_flags = HPET_OPEN;
			continue;

@@ -967,7 +982,7 @@

		sprintf(&hpetname[5], "%d", hpet_ntimer);
		devfs_mk_cdev(MKDEV(HPET_MAJOR, hpet_ntimer),
-			S_IFCHR|S_IRUSR|S_IWUSR, hpetname);
+			      S_IFCHR | S_IRUSR | S_IWUSR, hpetname);
		init_waitqueue_head(&devp->hd_waitqueue);
	}

@@ -976,8 +991,7 @@

	return 0;
}

-static acpi_status __init
-hpet_resources (struct acpi_resource *res, void *data)
+static acpi_status __init hpet_resources(struct acpi_resource *res, void *data)

{
	struct hpet_data *hdp;
	acpi_status status;
@@ -992,13 +1006,13 @@
		unsigned long size;

		size = addr.max_address_range - addr.min_address_range + 1;

- hdp->hd_address = (unsigned long) ioremap(addr.min_address_range, size);
+ hdp->hd_address =
+ (unsigned long)ioremap(addr.min_address_range, size);

		for (hpetp = hpets; hpetp; hpetp = hpetp->hp_next)
-			if (hpetp->hp_hpet == (struct hpet *) (hdp->hd_address))
+			if (hpetp->hp_hpet == (struct hpet *)(hdp->hd_address))
				return -EBUSY;
-	}
-	else if (res->id == ACPI_RSTYPE_EXT_IRQ) {
+	} else if (res->id == ACPI_RSTYPE_EXT_IRQ) {
		struct acpi_resource_ext_irq *irqp;
		int i;

@@ -1009,8 +1023,10 @@

			for (i = 0; i < hdp->hd_nirqs; i++)

#ifdef CONFIG_IA64
- hdp->hd_irq[i] = acpi_register_irq(irqp->interrupts[i],
- irqp->active_high_low, irqp->edge_level);
+ hdp->hd_irq[i] =
+ acpi_register_irq(irqp->interrupts[i],
+ irqp->active_high_low,
+ irqp->edge_level);

#else
				hdp->hd_irq[i] = irqp->interrupts[i];
#endif
@@ -1020,15 +1036,16 @@
	return AE_OK;
}

-static int __init
-hpet_acpi_add (struct acpi_device *device) +static int __init hpet_acpi_add(struct acpi_device *device)

{
	acpi_status result;
	struct hpet_data data;

	memset(&data, 0, sizeof(data));

- result = acpi_walk_resources(device->handle, METHOD_NAME__CRS, hpet_resources, &data);
+ result =
+ acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+ hpet_resources, &data);

	if (ACPI_FAILURE(result))
		return -ENODEV;
@@ -1041,33 +1058,34 @@
	return hpet_alloc(&data);
}

-static int __init
-hpet_acpi_remove (struct acpi_device *device, int type) +static int __init hpet_acpi_remove(struct acpi_device *device, int type)

{
	return 0;
}

static struct acpi_driver hpet_acpi_driver __initdata = {
- .name = "hpet",
- .class = "",
- .ids = "PNP0103",
+ .name = "hpet",
+ .class = "",
+ .ids = "PNP0103",

	.ops = {

- .add = hpet_acpi_add,
- .remove = hpet_acpi_remove,
- },
+ .add = hpet_acpi_add,
+ .remove = hpet_acpi_remove,
+ },
};

-
-
-static int __init
-hpet_init (void)
+static int __init hpet_init(void)

{
	struct proc_dir_entry *entry;

+ /*
+ * If platform dependent code allocated hpet,
+ * then do the rest of post boot initialization
+ * of these hpets.
+ */

	if (hpets)
		hpet_post_platform();

- (void) acpi_bus_register_driver(&hpet_acpi_driver);
+ (void)acpi_bus_register_driver(&hpet_acpi_driver);

	if (hpets) {
		entry = create_proc_entry("driver/hpet", 0, 0);

@@ -1079,25 +1097,23 @@

#ifdef	CONFIG_TIME_INTERPOLATION
		{

- volatile struct hpet *hpet;
+ struct hpet *hpet;

			hpet = hpets->hp_hpet;
			hpet_cycles_per_sec = hpet_time_div(hpets->hp_period);
			hpet_interpolator.frequency = hpet_cycles_per_sec;
			hpet_interpolator.drift = hpet_cycles_per_sec *
-				HPET_DRIFT / 1000000;
+			    HPET_DRIFT / 1000000;
			hpet_nsecs_per_cycle = 1000000000 / hpet_cycles_per_sec;
			register_time_interpolator(&hpet_interpolator);
		}
#endif
		return 0;
-	}
-	else
+	} else
		return -ENODEV;
}

-static void __exit
-hpet_exit (void)
+static void __exit hpet_exit(void)

{
	acpi_bus_unregister_driver(&hpet_acpi_driver);

@@ -1109,7 +1125,6 @@

	return;
}

-
module_init(hpet_init);
module_exit(hpet_exit);
MODULE_AUTHOR("Bob Picco <[email protected]>");

drivers/char/Kconfig +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-mm3/drivers/char/Kconfig linux-2.6.6-mm3-hpet/drivers/char/Kconfig
@@ -938,15 +938,16 @@
          with the O_DIRECT flag.
 

config HPET
- bool "HPET - High Precision Event Timer"
+ bool "HPET - High Precision Event Timer" if (X86 || IA64)
default n
+ depends on ACPI
help
If you say Y here, you will have a device named "/dev/hpet/XX" for

-	  each timer supported by the HPET.  The timers are
-	  non-periodioc and/or periodic.
+	  each timer supported by the HPET.  The timers are +	  non-periodioc and/or periodic. 

config HPET_RTC_IRQ
- bool "HPET Control RTC IRQ"
+ bool "HPET Control RTC IRQ" if !HPET_EMULATE_RTC
default n
depends on HPET
help

include/linux/hpet.h +0 -0
diff -ruN -X /home/picco/losl/dontdiff linux-2.6.6-mm3/include/linux/hpet.h linux-2.6.6-mm3-hpet/include/linux/hpet.h
@@ -6,26 +6,26 @@
 */
 

struct hpet {
- u64 hpet_cap; /* capabilities */
- u64 res0; /* reserved */
- u64 hpet_config; /* configuration */
- u64 res1; /* reserved */
- u64 hpet_isr; /* interrupt status reg */
- u64 res2[25]; /* reserved */
- union { /* main counter */
- u64 _hpet_mc64;
- u32 _hpet_mc32;
- unsigned long _hpet_mc;
+ u64 hpet_cap; /* capabilities */
+ u64 res0; /* reserved */
+ u64 hpet_config; /* configuration */
+ u64 res1; /* reserved */
+ u64 hpet_isr; /* interrupt status reg */
+ u64 res2[25]; /* reserved */
+ union { /* main counter */
+ u64 _hpet_mc64;
+ u32 _hpet_mc32;

+		unsigned long _hpet_mc;
	} _u0;
-	u64	res3;			/* reserved */
+	u64 res3;		/* reserved */
	struct hpet_timer {

- u64 hpet_config; /* configuration/cap */
- union { /* timer compare register */
- u64 _hpet_hc64;
- u32 _hpet_hc32;
- unsigned long _hpet_compare;
+ u64 hpet_config; /* configuration/cap */
+ union { /* timer compare register */
+ u64 _hpet_hc64;
+ u32 _hpet_hc32;

+			unsigned long _hpet_compare;
		} _u1;
-		u64	hpet_fsb[2];	/* FSB route */
+		u64 hpet_fsb[2];	/* FSB route */
	} hpet_timers[1];
};

@@ -47,12 +47,10 @@
#define HPET_NUM_TIM_CAP_MASK (0x1f00)
#define HPET_NUM_TIM_CAP_SHIFT (8ULL)

-
/*
* HPET general configuration register
*/

-
#define HPET_LEG_RT_CNF_MASK (2UL)
#define HPET_ENABLE_CNF_MASK (1UL)

@@ -63,9 +61,6 @@

#define	HPET_ISR_CLEAR(HPET, TIMER)				\
		(HPET)->hpet_isr |= (1UL << TIMER)

-
-
-
/*
* Timer configuration register
*/
@@ -95,38 +90,35 @@
#define Tn_FSB_INT_VAL_MASK (0x00000000ffffffffULL)

struct hpet_info {
- unsigned long hi_ireqfreq; /* Hz */
- unsigned long hi_flags; /* information */
+ unsigned long hi_ireqfreq; /* Hz */
+ unsigned long hi_flags; /* information */
};

-#define HPET_INFO_PERIODIC 0x0001 /* timer is periodic */
+#define HPET_INFO_PERIODIC 0x0001 /* timer is periodic */

-
-#define HPET_IE_ON _IO('h', 0x01) /* interrupt on */
-#define HPET_IE_OFF _IO('h', 0x02) /* interrupt off */
+#define HPET_IE_ON _IO('h', 0x01) /* interrupt on */
+#define HPET_IE_OFF _IO('h', 0x02) /* interrupt off */
#define HPET_INFO _IOR('h', 0x03, struct hpet_info)
-#define HPET_EPI _IO('h', 0x04) /* enable periodic */
-#define HPET_DPI _IO('h', 0x05) /* disable periodic */
+#define HPET_EPI _IO('h', 0x04) /* enable periodic */
+#define HPET_DPI _IO('h', 0x05) /* disable periodic */
#define HPET_IRQFREQ _IOW('h', 0x6, unsigned long) /* IRQFREQ usec */

-
-
/*
* exported interfaces
*/

struct hpet_task {
- void (*ht_func)(void *);
- void *ht_data;
- void *ht_opaque;
+ void (*ht_func) (void *);
+ void *ht_data;
+ void *ht_opaque;
};

struct hpet_data {
- unsigned long hd_address;
- unsigned short hd_nirqs;
- unsigned short hd_flags;
- unsigned int hd_state; /* timer allocated */
- unsigned int hd_irq[HPET_MAX_TIMERS];
+ unsigned long hd_address;
+ unsigned short hd_nirqs;
+ unsigned short hd_flags;
+ unsigned int hd_state; /* timer allocated */
+ unsigned int hd_irq[HPET_MAX_TIMERS];
};

#define HPET_DATA_PLATFORM 0x0001 /* platform call to hpet_alloc */
@@ -136,5 +128,4 @@
int hpet_unregister(struct hpet_task *);
int hpet_control(struct hpet_task *, unsigned int, unsigned long);

-
-#endif /* !__HPET__ */
+#endif /* !__HPET__ */

#11 lore raw
In reply to Robert Picco
Quoted 1 line · Robert Picco

O.K. Did this but had to add a writeq and readq for i386.

Let me be the first to say: yay!

IMO, readq and writeq are not only needed, but an implementation should be added to 2.4.x kernels as well.

The amount of hardware (and thus drivers) that will use 64-bit memory IOs can and will increase, as time passes.

Jeff

#12 lore raw
In reply to Robert Picco
Quoted 2 lines · Robert Picco

O.K. Did this but had to add a writeq and readq for i386.

You implementation of these is private to hpet.c. From what Jeff is saying, it looks like it should be in include/asm-i386/io.h?

#ifndef readq
static unsigned long long __inline readq(void *addr)

{
	return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
}

#endif

#ifndef writeq
static void __inline writeq(unsigned long long v, void *addr)

{
	writel(v & 0xffffffff, addr);
	writel(v >> 32, addr + 4);
}

#endif

#13 lore raw
Quoted 7 lines · Andrew Morton
Quoted 1 line · Robert Picco

O.K. Did this but had to add a writeq and readq for i386.

You implementation of these is private to hpet.c. From what Jeff is saying, it looks like it should be in include/asm-i386/io.h?

Yep.

Jeff

#14 lore raw
In reply to Andrew Morton
Quoted 7 lines · Andrew Morton
Quoted 2 lines · Robert Picco

O.K. Did this but had to add a writeq and readq for i386.

You implementation of these is private to hpet.c. From what Jeff is saying, it looks like it should be in include/asm-i386/io.h?

This look OK?

readq()/writeq() are supposed to be defined in terms of u64's.

---

 25-akpm/drivers/char/hpet.c   |   15 ---------------
 25-akpm/include/asm-i386/io.h |   11 +++++++++++
 2 files changed, 11 insertions(+), 15 deletions(-)
drivers/char/hpet.c renamed from drivers/char/hpet.c~hpet-driver-updates-move-readq +0 -15
diff -puN drivers/char/hpet.c~hpet-driver-updates-move-readq drivers/char/hpet.c
@@ -90,21 +90,6 @@ static struct hpets *hpets;
 #define	read_counter(MC)	readl(MC)
 #endif
 
-#ifndef readq
-static unsigned long long __inline readq(void *addr)
-{
-	return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
-}
-#endif
-
-#ifndef writeq
-static void __inline writeq(unsigned long long v, void *addr)
-{
-	writel(v & 0xffffffff, addr);
-	writel(v >> 32, addr + 4);
-}
-#endif
-
 static irqreturn_t hpet_interrupt(int irq, void *data, struct pt_regs *regs)
 {
 	struct hpet_dev *devp;
include/asm-i386/io.h renamed from include/asm-i386/io.h~hpet-driver-updates-move-readq +11 -0
diff -puN include/asm-i386/io.h~hpet-driver-updates-move-readq include/asm-i386/io.h
@@ -364,4 +364,15 @@ BUILDIO(b,b,char)
 BUILDIO(w,w,short)
 BUILDIO(l,,int)
 
+static inline u64 readq(void *addr)
+{
+	return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
+static inline void writeq(u64 v, void *addr)
+{
+	writel(v & 0xffffffff, addr);
+	writel(v >> 32, addr + 4);
+}
+
 #endif

_

#15 lore raw

include/asm-i386/io.h BUILDIO(b,b,char) · line 376

Quoted 10 lines · Andrew Morton
include/asm-i386/io.h renamed from include/asm-i386/io.h~hpet-driver-updates-move-readq +10 -0
+static inline u64 readq(void *addr)
+{
+	return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
+static inline void writeq(u64 v, void *addr)
+{
+	writel(v & 0xffffffff, addr);
+	writel(v >> 32, addr + 4);
+}

Seems sane, though I wonder about two things:

* better home is probably asm-generic

* It seems to me that a poorly-written writel() macro might prefer some guarantee that it's argument is pre-cast to u32. I dunno if this is just paranoia or not.

#16 lore raw
Quoted 3 lines · Jeff Garzik

Seems sane, though I wonder about two things:

* better home is probably asm-generic

hmm, I wonder about endian issues tho (and please remember that ARM can be either BE or LE depending on the machine we're building for...)

Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
#17 lore raw
Quoted 9 lines · Russell King
Quoted 3 lines · Jeff Garzik

Seems sane, though I wonder about two things:

* better home is probably asm-generic

hmm, I wonder about endian issues tho (and please remember that ARM can be either BE or LE depending on the machine we're building for...)

As long as writeq() is implemented wholly in terms of writel(), that's fine...

Jeff

#18 lore raw
In reply to Jeff Garzik
Quoted 16 lines · Jeff Garzik

include/asm-i386/io.h BUILDIO(b,b,char) · line 376

Quoted 10 lines · Andrew Morton
include/asm-i386/io.h renamed from include/asm-i386/io.h~hpet-driver-updates-move-readq +10 -0
+static inline u64 readq(void *addr)
+{
+	return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
+static inline void writeq(u64 v, void *addr)
+{
+	writel(v & 0xffffffff, addr);
+	writel(v >> 32, addr + 4);
+}

Seems sane, though I wonder about two things:

Quoted 1 line

* better home is probably asm-generic

It's only applicable to 32-bit machines. I thik I'd prefer to let the various arch maintainers decide if this is an appropriate implementation.

Quoted 3 lines

* It seems to me that a poorly-written writel() macro might prefer some guarantee that it's argument is pre-cast to u32. I dunno if this is just paranoia or not.

That could be an issue if other architectures were to use this particular implementation. I'll stick the typecasts in there anyway.

#19 lore raw
Quoted 2 lines · Andrew Morton

It's only applicable to 32-bit machines. I thik I'd prefer to let the various arch maintainers decide if this is an appropriate implementation.

Agreed, though I observe it's mostly 32-bit architectures that are missing readq() and writeq() implementations...

Jeff

#20 lore raw
Quoted 9 lines · Jeff Garzik
Quoted 2 lines · Andrew Morton

It's only applicable to 32-bit machines. I thik I'd prefer to let the various arch maintainers decide if this is an appropriate implementation.

Agreed, though I observe it's mostly 32-bit architectures that are missing readq() and writeq() implementations...

s2io.h has a private readq/writeq implementation, which I'm removing. There are probably others around the place (haven't looked).

This means that architecture implementation of readq()/writeq() becomes non-optional.

#21 lore raw
Quoted 8 lines · Andrew Morton
Quoted 6 lines · Jeff Garzik
Quoted 2 lines · Andrew Morton

It's only applicable to 32-bit machines. I thik I'd prefer to let the various arch maintainers decide if this is an appropriate implementation.

Quoted 8 lines
Quoted 2 lines

Agreed, though I observe it's mostly 32-bit architectures that are missing readq() and writeq() implementations...

s2io.h has a private readq/writeq implementation, which I'm removing. There are probably others around the place (haven't looked).

This means that architecture implementation of readq()/writeq() becomes non-optional.

It should be non-optional, IMO.

The standard {read,write}[bwl] functions are certainly required if you need to care about the PCI or ISA busses, at least. So {read,write}q should follow suit.

Jeff

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#24 lore raw

Re: readq/writeq on 32bit machines

Quoted 40 lines · Roland Dreier

Thanks for posting this Eric... I sent a less detailed reply yesterday (pointing out that atomic writeq is needed sometimes) but it seems to have gotten eaten.

    Eric> This issue came last night on the openib list.  The driver
    Eric> currently rolls it's own version of writeq and in the case
    Eric> where there is not an atomic 64bit write it needs to a
    Eric> spinlock to make certain things don't get out of order.  The
    Eric> driver fails with the current 2 writel() version.

    Eric> Here is an SSE version, that should not be to intrusive.
    Eric> According to intel's docs a 64bit aligned 64bit write is
    Eric> atomic all of the way back to the Pentium.  If
    Eric> kernel_fpu_begin/kernel_fpu_end are safe in interrupt
    Eric> context we can do an atomic/correct version of writeq for
    Eric> x86 processors that don't support sse as well.  Although I
    Eric> don't know if we want to.

	static inline void __raw_writeq(u64 val, unsigned long dest)
	{
		unsigned long cr0;
		u64 xmmsave __attribute__((aligned(8));
		preempt_disable();
	        cr0 = read_cr0();
		clts();
	        asm volatile (
			"movlps %%xmm0,(%0); \n\t"
			"movlps (%2),%%xmm0; \n\t"
			"movlps %%xmm0,(%1); \n\t"
			"movlps (%0),%%xmm0; \n\t"
			: =m (&xmmsave), "=m" ((void *)dest)
			: "m" (&val)
			);
		write_cr0(cr0);
		preempt_enable();
	}

This is pretty much what I wrote in the above-mentioned openib driver. However I'm worried about using

Quoted 7 lines
        u64 xmmsave __attribute__((aligned(8));

for a stack variable. I don't think gcc respects the alignment attribute for stack variables (I've had a problem in the past using movdqa to a stack variable, even if I do __attribute__((aligned(16))). If we're sure gcc aligns xmmsave properly, stick a comment in and leave out the __attribute__; if not then I think we have to do

I picked that up out of xor.h where the raid code does something similar, so if there is a problem it needs to be fixed there as well.

Quoted 17 lines
        u8 xmmsave[8 + 7];

and then use ~7 & (xmmsave + 7).

    Eric> Thinking about this a little more we might be able to get
    Eric> away with.

	static inline void __raw_writeq(u64 val, unsigned long dest)
	{
		unsigned long flags;
	        local_irq_save(flags);
		writel(val & 0xffffffff, addr);
		writel(val >> 32, addr + 4);
	        irq_restore(flags);
	}
Quoted 7 lines

I don't think this is good enough on SMP. In the openib case, it's entirely possible for one CPU to be ringing a (64-bit) work queue doorbell at the same time as another CPU is ringing a (64-bit) completion queue doorbell, and if the 32-bit halves of those doorbells get interleaved, the hardware gets confused. Maybe there's some magic aspect of the PC hardware that ensures this can't happen but I'd hate to count on it without some very good documentation.

Right. It does make the window incredibly small though. I am even nervous that the version with a spinlock might break, if something really needs an atomic guarantee.

Eric

#25 lore raw

Re: readq/writeq on 32bit machines

Eric> I picked that up out of xor.h where the raid code does Eric> something similar, so if there is a problem it needs to be Eric> fixed there as well.

OK, I found this in raid6x86.h:

/* On i386, the stack is only 8-byte aligned, but SSE requires 16-byte
alignment. The +3 is so we have the slack space to manually align a properly-sized area correctly. */

So I guess __attribute__((aligned(8))) is OK on i386, but 16-byte alignment needs to be handled manually.

By the way, I haven't seen my mails on this thread make it to lkml (despite having [email protected] in my Cc: line). Obviously they're going out, since you replied to one, but they seem to be getting eaten somewhere.

Thanks,
Roland

#26 lore raw

Re: readq/writeq on 32bit machines

Quoted 4 lines · Roland Dreier

By the way, I haven't seen my mails on this thread make it to lkml (despite having [email protected] in my Cc: line). Obviously they're going out, since you replied to one, but they seem to be getting eaten somewhere.

Very odd. My home address is in the CC list "[email protected]" and it has not received any of your messages. And it has been in both the To: and the Cc: line. There does seem to be a partial network outage on your side.

Eric

A message that is not in the archive (sent off-list or not yet received): [email protected]

#28 lore raw
Quoted 3 lines · David Mosberger

What about atomicity? Are there any platforms where {read,write}[bwl]() don't translate into a single bus-transaction? I didn't think so, but I could well be wrong.

This is a good point, as the ensuing thread indicates.

Current usage by drivers doesn't require atomicity, so the proposed implementation is fine.

Thinking about the atomicity issues now is definitely something that should be done, though...

Jeff