broken src/sys/arch/x86/isa/clock.c (i8254_get_timecount)

[Chuck Cranor <chuck%ece.cmu.edu@localhost>]

hi-

    while debugging my 4.0/i386 auich_calibrate() problem, I've
come to the conclusion that i8254_get_timecount() has been broken
since rev 1.98 of the old src/sys/arch/i386/isa/clock.c (2006/09/03)
when "clkintr_pending" was removed.

    the way I understand it, the i8254 provides a countdown timer
that drops by one at 1193182 Hz (TIMER_FREQ).   the driver programs
it to count down from 11932 to zero to produce an interrupt at 
rate "hz" (100 Hz).   The initial value of the timer (11932) is
"rtclock_tval" in the driver.

    the function i8254_get_timecount() is part of the new timecounter
framework.   it is supposed to be a u_int counting up at TIMER_FREQ Hz
(wrapping when it gets to the end, I think).

    the base value for i8254_get_timecount() is "i8254_offset" (a static 
uint32_t).  each time the clock interrupts clockintr() calls tickle_tc()
which would normally just add "rtclock_tval" to "i8254_offset" ...

               /* from hw interrupt func, i8254_ticked normally not true(?) */
               if (i8254_ticked)
                        i8254_ticked    = 0;
                else {
                        i8254_offset   += rtclock_tval;
                        i8254_lastcount = 0;
                }


    however, if the i8254 counter is read using i8254_get_timecount(),
then that function attempts to add in the between-interrupt ticks while
watching for the i8254 counter to "wrap" around like this:

        /* high and low come from the countdown hardware register */
        /* convert countdown to count up to match the timecounter framework */
        count = rtclock_tval - ((high << 8) | low); 

        /* normal case: rtclock_tval != 0 */
        /* count < i8254_lastcount is true if i8254 counter wrapped */
        /* i8254_ticked is cleared by the hardware interrupt */

        if (rtclock_tval && (count < i8254_lastcount || !i8254_ticked)) {
                i8254_ticked = 1;
                i8254_offset += rtclock_tval;
        }
        i8254_lastcount = count;  /* save old val for wrap detection */
        count += i8254_offset;
        /* ... unlock */
        return (count);


the problem with that is that i8254_ticked is 0 to start, so the very
first time you hit the "if" statement it is going to set i8254_ticked to
1 and advance the i8254_offset irrespective of if a wrap is detected
(e.g. count < i8254_lastcount).


The earlier version of the if statement was:

  if (rtclock_tval && 
       (count < i8254_lastcount || (!i8254_ticked && clkintr_pending))) { ... }

where "clkintr_pending" was an int that was always zero.   So this if
only fired when the wrap detection (count < i8254_lastcount) was true.


I looked at the FreeBSD version of this code and it looks like this:

        if (count < i8254_lastcount ||
            (!i8254_ticked && (clkintr_pending ||
            ((count < 20 || (!(eflags & PSL_I) && count < timer0_max_count / 2u)
) &&    
            i8254_pending != NULL && i8254_pending(i8254_intsrc))))) { ... }


not easy to understand (4 line "if" with 4 levels of parens: YUCK!), but 
I believe the idea of the extra stuff in the if statement is to only fire 
the !i8254_ticked case if !i8254_ticked AND an interrupt is pending.   the 
NetBSD version clearly doesn't have the "AND an interrupt is pending" part.
That is what is causing auich_calibrate() to fail!




To fix the problem, I was thinking one could either ignore i8254_ticked 
like we used to:

        if (rtclock_tval && count < i8254_lastcount) 


or alternately, maybe change the "||" to an "&&" (assuming that the
interrupt will run before the counter wraps more than once?).

        if (rtclock_tval && (count < i8254_lastcount && !i8254_ticked)) 


either way makes auich_calibrate() work properly once again!  any
suggestions on what to go with?


chuck