[Qemu-devel] [PATCH v7 08/10] i8254: add comments about fixing timings

[Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>]

There may be risk of problems with migration if these are just
fixed blindly, but at least comment what it ought to be changed to.

Signed-off-by: Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>
---
 hw/i8254.c        | 31 ++++++++++++++++++++++++++++++-
 hw/i8254_common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index bea5f92..982e8e7 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -39,6 +39,15 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
 
 static int pit_get_count(PITChannelState *s)
 {
+    /* FIXME: Add some way to represent a paused timer and return
+     *   the paused-at counter value, to better model:
+     *     - gate-triggered modes (1 and 5),
+     *     - gate-pausable modes,
+     *     - [maybe] the "wait until next CLK pulse to load counter" logic,
+     *     - [maybe/not clear] half-loaded counter logic for mode 0, and
+     *       the "null count" status bit,
+     *     - etc.
+     */
     uint64_t d;
     int counter;
 
@@ -52,7 +61,11 @@ static int pit_get_count(PITChannelState *s)
         counter = (s->count - d) & 0xffff;
         break;
     case 3:
-        /* XXX: may be incorrect for odd counts */
+        /* XXX: may be incorrect for odd counts
+         * FIXME i8254_timing_fixes: fix this by changing it to something
+         * like:
+         *   counter = (s->count - ((2 * d) % s->count)) & 0xfffe;
+         */
         counter = s->count - ((2 * d) % s->count);
         break;
     default:
@@ -98,6 +111,22 @@ static inline void pit_load_count(PITChannelState *s, int val)
     if (val == 0)
         val = 0x10000;
     s->count_load_time = qemu_get_clock_ns(vm_clock);
+
+    /* FIXME i8254_timing_fixes:
+     * In gate-triggered one-shot modes, indirectly model some pit_get_out()
+     * cases by setting the load time way back until gate-triggered,
+     * using code such as:
+     *
+     * if (s->mode == 1 || s->mode == 5)
+     *     s->count_load_time -= muldiv64(val+2, get_ticks_per_sec(), PIT_FREQ);
+     *
+     * (Generally only affects reading status from channel 2 speaker,
+     * due to hard-wired gates on other channels.)
+     *
+     * FIXME Or this might be redesigned if a paused timer state is added
+     * for pic_get_count().
+     */
+
     s->count = val;
     pit_irq_timer_update(s, s->count_load_time);
 }
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index a03d7cd..33f352f 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -53,9 +53,30 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
         out = (d >= s->count);
         break;
     case 1:
+        /* FIXME i8254_timing_fixes: There are various problems
+         *  with qemu's 8254 model (especially when IRQ0's trailing
+         *  edge occurs), but fixing them directly might cause
+         *  problems with migration.  So just comment the fixes for
+         *  now.
+         *    (Based on reading timing diagrams in Intel's 8254 spec
+         *  sheet, downloadable from
+         *  http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
+         *  or other places.)
+         * FIXME i8254_timing_fixes: Both mode 0 and mode 1 should use
+         *    out = (d >= s->count);
+         *  So mode 0 can fall-through into a fixed mode 1.
+         *  The difference between modes 0 and 1 is in the gate triggering.
+         *  See also an adjustment to make to count_load_time
+         *  when count is loaded for mode 1.
+         */
         out = (d < s->count);
         break;
     case 2:
+        /* FIXME i8254_timing_fixes: This should simply be:
+         *    out = (d % s->count) != (s->count - 1) || s->gate == 0;
+         *  Gate is hard-wired 1 for channel 0 (IRQ0), but it can be
+         *  adjusted in software for channel 2 (PC speaker).
+         */
         if ((d % s->count) == 0 && d != 0) {
             out = 1;
         } else {
@@ -63,10 +84,21 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
         }
         break;
     case 3:
+        /* FIXME i8254_timing_fixes: This should be:
+         *    out = (d % s->count) < ((s->count + 1) >> 1) || s->gate == 0;
+         *  Gate is hard-wired 1 for channel 0 (IRQ0), but it can be
+         *  adjusted in software for channel 2 (PC speaker).
+         */
         out = (d % s->count) < ((s->count + 1) >> 1);
         break;
     case 4:
     case 5:
+        /* FIXME i8254_timing_fixes: The logic is backwards, and should be:
+         *    out = (d != s->count);
+         *  The difference between modes 4 and 5 is in the gate triggering.
+         *  See also an adjustment to make to count_load_time
+         *  when count is loaded for mode 5.
+         */
         out = (d == s->count);
         break;
     }
@@ -93,6 +125,14 @@ int64_t pit_get_next_transition_time(PITChannelState *s, int64_t current_time)
         break;
     case 2:
         base = (d / s->count) * s->count;
+        /* FIXME i8254_timing_fixes: The if condition and else case should
+         *  be changed to:
+         *   if ((d - base) == (s->count - 1)) {
+         *       next_time = base + s->count;
+         *   } else {
+         *       next_time = base + s->count - 1;
+         *   }
+         */
         if ((d - base) == 0 && d != 0) {
             next_time = base + s->count;
         } else {
-- 
1.7.10.2.484.gcd07cc5