hpet: hpet driver periodic timer setup bug fixes

PATCH In review Pallipadi, Venkatesh 4 participants 5 messages Aug 10, 2009 – Aug 28, 2009

Subsystems (2)

Char and misc drivers Hpet: High Precision Event Timers driver

lore

Share & tools

lore

Pallipadi, Venkatesh

author

2009-08-10 22:05 UTC #0 lore raw

PATCH 1/1 hpet: hpet driver periodic timer setup bug fixes

From: Nils Carlson <[email protected]>

The periodic interrupt from drivers/char/hpet.c does not work correctly, both when using the periodic capability of the hardware and while emulating the periodic interrupt (when hardware does not support periodic mode).

With timers capable of periodic interrupts, the comparator field is first set with the period value followed by set of hidden accumulator, which has the side effect of overwriting the comparator value. This results in wrong periodicity for the interrupts. For,
periodic interrupts to work, following steps are necessary, in that order.
* Set config with Tn_VAL_SET_CNF bit
* Write to hidden accumulator, the value written is the time when the first interrupt should be generated
* Write compartor with period interval for subsequent interrupts
(http://www.intel.com/hardwaredesign/hpetspec_1.pdf )

When emulating periodic timer with timers not capable of periodic interrupt, driver is adding the period to counter value instead of comparator value, which causes slow drift when using this emulation.

Also, driver seems to add hpetp->hp_delta both while setting up periodic interrupt and while emulating periodic interrupts with timers not capable of doing periodic interrupts. This hp_delta will result in slower than expected interrupt rate and should not be used while setting the interval.

Signed-off-by Venkatesh Pallipadi <[email protected]>

Signed-off-by Nils Carlson <[email protected]>

---
 drivers/char/hpet.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

drivers/char/hpet.c +12 -9

index 4a9f349..70a770a 100644
@@ -166,9 +166,8 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
 		unsigned long m, t;
 
 		t = devp->hd_ireqfreq;
-		m = read_counter(&devp->hd_hpet->hpet_mc);
-		write_counter(t + m + devp->hd_hpets->hp_delta,
-			      &devp->hd_timer->hpet_compare);
+		m = read_counter(&devp->hd_timer->hpet_compare);
+		write_counter(t + m, &devp->hd_timer->hpet_compare);
 	}
 
 	if (devp->hd_flags & HPET_SHARED_IRQ)

@@ -504,21 +503,25 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
 	g = v | Tn_32MODE_CNF_MASK | Tn_INT_ENB_CNF_MASK;
 
 	if (devp->hd_flags & HPET_PERIODIC) {
-		write_counter(t, &timer->hpet_compare);
 		g |= Tn_TYPE_CNF_MASK;
-		v |= Tn_TYPE_CNF_MASK;
-		writeq(v, &timer->hpet_config);
-		v |= Tn_VAL_SET_CNF_MASK;
+		v |= Tn_TYPE_CNF_MASK | Tn_VAL_SET_CNF_MASK;
 		writeq(v, &timer->hpet_config);
 		local_irq_save(flags);
 
-		/* NOTE:  what we modify here is a hidden accumulator
+		/*
+		 * NOTE: First we modify the hidden accumulator
 		 * register supported by periodic-capable comparators.
 		 * We never want to modify the (single) counter; that
-		 * would affect all the comparators.
+		 * would affect all the comparators. The value written
+		 * is the counter value when the first interrupt is due.
 		 */
 		m = read_counter(&hpet->hpet_mc);
 		write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
+		/*
+		 * Then we modify the comparator, indicating the period
+		 * for subsequent interrupt.
+		 */
+		write_counter(t, &timer->hpet_compare);
 	} else {
 		local_irq_save(flags);
 		m = read_counter(&hpet->hpet_mc);

Signature · 1 line

1.6.0.6

Andrew Morton

2009-08-11 11:55 UTC #1 lore raw

Quoted 22 lines · Venkatesh

From: Nils Carlson <[email protected]>
The periodic interrupt from drivers/char/hpet.c does not work correctly, both when using the periodic capability of the hardware and while emulating the periodic interrupt (when hardware does not support periodic mode).
With timers capable of periodic interrupts, the comparator field is first set with the period value followed by set of hidden accumulator, which has the side effect of overwriting the comparator value. This results in wrong periodicity for the interrupts. For,
periodic interrupts to work, following steps are necessary, in that order.
* Set config with Tn_VAL_SET_CNF bit
* Write to hidden accumulator, the value written is the time when the first interrupt should be generated
* Write compartor with period interval for subsequent interrupts
(http://www.intel.com/hardwaredesign/hpetspec_1.pdf )
When emulating periodic timer with timers not capable of periodic interrupt, driver is adding the period to counter value instead of comparator value, which causes slow drift when using this emulation.

Quoted 6 lines

Also, driver seems to add hpetp->hp_delta both while setting up periodic interrupt and while emulating periodic interrupts with timers not capable of doing periodic interrupts. This hp_delta will result in slower than expected interrupt rate and should not be used while setting the interval.

Do you have any thoughts on which kernel versions this fix should be merged into, and when?

Thanks.

Pallipadi, Venkatesh

author

2009-08-11 18:10 UTC #2 lore raw

Quoted 31 lines · Andrew Morton

Quoted 22 lines · Venkatesh
From: Nils Carlson <[email protected]>
The periodic interrupt from drivers/char/hpet.c does not work correctly, both when using the periodic capability of the hardware and while emulating the periodic interrupt (when hardware does not support periodic mode).
With timers capable of periodic interrupts, the comparator field is first set with the period value followed by set of hidden accumulator, which has the side effect of overwriting the comparator value. This results in wrong periodicity for the interrupts. For,
periodic interrupts to work, following steps are necessary, in that order.
* Set config with Tn_VAL_SET_CNF bit
* Write to hidden accumulator, the value written is the time when the first interrupt should be generated
* Write compartor with period interval for subsequent interrupts
(http://www.intel.com/hardwaredesign/hpetspec_1.pdf )
When emulating periodic timer with timers not capable of periodic interrupt, driver is adding the period to counter value instead of comparator value, which causes slow drift when using this emulation.
Quoted 6 lines
Also, driver seems to add hpetp->hp_delta both while setting up periodic interrupt and while emulating periodic interrupts with timers not capable of doing periodic interrupts. This hp_delta will result in slower than expected interrupt rate and should not be used while setting the interval.

Quoted 3 lines

Do you have any thoughts on which kernel versions this fix should be merged into, and when?

Should be OK let this bake for a while and merge this in after .31.

Thanks,
Venki

Goldwyn Rodrigues

2009-08-27 19:25 UTC #3 lore raw

In reply to Pallipadi, Venkatesh

Hi,

One comment on this patch.

Quoted 72 lines · Venkatesh

From: Nils Carlson <[email protected]>
The periodic interrupt from drivers/char/hpet.c does not work correctly, both when using the periodic capability of the hardware and while emulating the periodic interrupt (when hardware does not support periodic mode).
With timers capable of periodic interrupts, the comparator field is first set with the period value followed by set of hidden accumulator, which has the side effect of overwriting the comparator value. This results in wrong periodicity for the interrupts. For,
periodic interrupts to work, following steps are necessary, in that order.
* Set config with Tn_VAL_SET_CNF bit
* Write to hidden accumulator, the value written is the time when the first interrupt should be generated
* Write compartor with period interval for subsequent interrupts
(http://www.intel.com/hardwaredesign/hpetspec_1.pdf )
When emulating periodic timer with timers not capable of periodic interrupt, driver is adding the period to counter value instead of comparator value, which causes slow drift when using this emulation.
Also, driver seems to add hpetp->hp_delta both while setting up periodic interrupt and while emulating periodic interrupts with timers not capable of doing periodic interrupts. This hp_delta will result in slower than expected interrupt rate and should not be used while setting the interval.
Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Nils Carlson <[email protected]>
---
drivers/char/hpet.c | 21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
drivers/char/hpet.c +7 -9
index 4a9f349..70a770a 100644
@@ -166,9 +166,8 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
                unsigned long m, t;
 
                t = devp->hd_ireqfreq;
-               m = read_counter(&devp->hd_hpet->hpet_mc);
-               write_counter(t + m + devp->hd_hpets->hp_delta,
-                             &devp->hd_timer->hpet_compare);
+               m = read_counter(&devp->hd_timer->hpet_compare);
+               write_counter(t + m, &devp->hd_timer->hpet_compare);
        }
 
        if (devp->hd_flags & HPET_SHARED_IRQ)
@@ -504,21 +503,25 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
        g = v | Tn_32MODE_CNF_MASK | Tn_INT_ENB_CNF_MASK;
 
        if (devp->hd_flags & HPET_PERIODIC) {
-               write_counter(t, &timer->hpet_compare);
                g |= Tn_TYPE_CNF_MASK;
-               v |= Tn_TYPE_CNF_MASK;
-               writeq(v, &timer->hpet_config);
-               v |= Tn_VAL_SET_CNF_MASK;
+               v |= Tn_TYPE_CNF_MASK | Tn_VAL_SET_CNF_MASK;
                writeq(v, &timer->hpet_config);
                local_irq_save(flags);
 
-               /* NOTE:  what we modify here is a hidden accumulator
+               /*
+                * NOTE: First we modify the hidden accumulator
                 * register supported by periodic-capable comparators.
                 * We never want to modify the (single) counter; that
-                * would affect all the comparators.
+                * would affect all the comparators. The value written
+                * is the counter value when the first interrupt is due.
                 */

drivers/char/hpet.c static int hpet_ioctl_ieon(struct hpet_dev *devp) · line 527

Quoted 10 lines

drivers/char/hpet.c +5 -0

                m = read_counter(&hpet->hpet_mc);
                write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
+               /*
+                * Then we modify the comparator, indicating the period
+                * for subsequent interrupt.
+                */
+               write_counter(t, &timer->hpet_compare);
        } else {
                local_irq_save(flags);
                m = read_counter(&hpet->hpet_mc);

Shouldn't
write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare); be deleted if we are doing
write_counter(t, &timer->hpet_compare);
?

Regards,

Signature · 1 line

Goldwyn

Nils Carlson

2009-08-28 07:14 UTC #4 lore raw

Hi there,

Answers below.

Quoted 97 lines · Goldwyn Rodrigues

Hi,
One comment on this patch.
Quoted 73 lines · Venkatesh
From: Nils Carlson <[email protected]>
The periodic interrupt from drivers/char/hpet.c does not work correctly, both when using the periodic capability of the hardware and while emulating the periodic interrupt (when hardware does not support periodic mode).
With timers capable of periodic interrupts, the comparator field is first set with the period value followed by set of hidden accumulator, which has the side effect of overwriting the comparator value. This results in wrong periodicity for the interrupts. For,
periodic interrupts to work, following steps are necessary, in that order.
* Set config with Tn_VAL_SET_CNF bit
* Write to hidden accumulator, the value written is the time when the first interrupt should be generated
* Write compartor with period interval for subsequent interrupts
(http://www.intel.com/hardwaredesign/hpetspec_1.pdf )
When emulating periodic timer with timers not capable of periodic interrupt, driver is adding the period to counter value instead of comparator value, which causes slow drift when using this emulation.
Also, driver seems to add hpetp->hp_delta both while setting up periodic interrupt and while emulating periodic interrupts with timers not capable of doing periodic interrupts. This hp_delta will result in slower than expected interrupt rate and should not be used while setting the interval.
Signed-off-by Venkatesh Pallipadi <[email protected]>
Signed-off-by Nils Carlson <[email protected]>
---
 drivers/char/hpet.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
drivers/char/hpet.c +7 -9
index 4a9f349..70a770a 100644
@@ -166,9 +166,8 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
               unsigned long m, t;
 
               t = devp->hd_ireqfreq;
-               m = read_counter(&devp->hd_hpet->hpet_mc);
-               write_counter(t + m + devp->hd_hpets->hp_delta,
-                             &devp->hd_timer->hpet_compare);
+               m = read_counter(&devp->hd_timer->hpet_compare);
+               write_counter(t + m, &devp->hd_timer->hpet_compare);
       }
 
       if (devp->hd_flags & HPET_SHARED_IRQ)
@@ -504,21 +503,25 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
       g = v | Tn_32MODE_CNF_MASK | Tn_INT_ENB_CNF_MASK;
 
       if (devp->hd_flags & HPET_PERIODIC) {
-               write_counter(t, &timer->hpet_compare);
               g |= Tn_TYPE_CNF_MASK;
-               v |= Tn_TYPE_CNF_MASK;
-               writeq(v, &timer->hpet_config);
-               v |= Tn_VAL_SET_CNF_MASK;
+               v |= Tn_TYPE_CNF_MASK | Tn_VAL_SET_CNF_MASK;
               writeq(v, &timer->hpet_config);
               local_irq_save(flags);
 
-               /* NOTE:  what we modify here is a hidden accumulator
+               /*
+                * NOTE: First we modify the hidden accumulator
                * register supported by periodic-capable comparators.
                * We never want to modify the (single) counter; that
-                * would affect all the comparators.
+                * would affect all the comparators. The value written
+                * is the counter value when the first interrupt is due.
                */
drivers/char/hpet.c static int hpet_ioctl_ieon(struct hpet_dev *devp) · line 527
Quoted 10 lines
drivers/char/hpet.c +5 -0
               m = read_counter(&hpet->hpet_mc);
               write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
+               /*
+                * Then we modify the comparator, indicating the period
+                * for subsequent interrupt.
+                */
+               write_counter(t, &timer->hpet_compare);
       } else {
               local_irq_save(flags);
               m = read_counter(&hpet->hpet_mc);
Shouldn't
write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare); be deleted if we are doing
write_counter(t, &timer->hpet_compare);
?

Quoted 3 lines

Regards,

The answer is no. The first write of
write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare) writes directly to the comparator( also reffered to as accumulator), that is, it is the next trigger value. This happens because of the setting of Tn_VAL_SET_CNF_MASK .

The second write, write_counter(t, &timer->hpet_compare), doesn't write to the comparator but instead writes to the adder register, the value of which is added to the comparator every time the timer triggers.

So the first write sets when the FIRST trigger should occur, the second write defines the FREQUENCY. To quoute the intel documentation regarding Tn_VAL_SET_CNF_MASK:

"Timer n Value Set: (where n is the timer number: 00 to 31). Software uses this read/write bit only for timers that have been set to periodic mode. By writing this bit to a 1, the software is then allowed to directly set a periodic timer’s accumulator.
Software does NOT have to write this bit back to 0 (it automatically clears)."

For some nice diagrams see the intel documentation at
http://www.intel.com/hardwaredesign/hpetspec_1.pdf
pages 19 and 22.

Please feel free to ask more questions, I've spent a lot of time lately debugging hpet.

Regards,

Nils