[PATCH v1] bsps/i386: Update calibration of TSC to be more accurate

[Jan.Sommer at dlr.de]

> -----Original Message-----
> From: Gedare Bloom <gedare at rtems.org>
> Sent: Tuesday, June 15, 2021 5:01 PM
> To: Sommer, Jan <Jan.Sommer at dlr.de>
> Cc: devel at rtems.org
> Subject: Re: [PATCH v1] bsps/i386: Update calibration of TSC to be more
> accurate
> 
> On Fri, Jun 11, 2021 at 1:44 AM Jan Sommer <jan.sommer at dlr.de> wrote:
> >
> > Closes #4455
> > ---
> >  bsps/i386/pc386/clock/ckinit.c | 72
> > ++++++++++++++++++----------------
> >  1 file changed, 39 insertions(+), 33 deletions(-)
> >
> > diff --git a/bsps/i386/pc386/clock/ckinit.c
> > b/bsps/i386/pc386/clock/ckinit.c index 09afe73cde..cbd2360fde 100644
> > --- a/bsps/i386/pc386/clock/ckinit.c
> > +++ b/bsps/i386/pc386/clock/ckinit.c
> > @@ -104,48 +104,61 @@ static uint32_t pc386_get_timecount_i8254(struct
> > timecounter *tc)
> >
> >  /*
> >   * Calibrate CPU cycles per tick. Interrupts should be disabled.
> > + * Will also set the PIT, so call this before registering the
> > + * periodic timer for rtems tick generation
> >   */
> >  static void calibrate_tsc(void)
> >  {
> >    uint64_t              begin_time;
> > -  uint8_t               then_lsb, then_msb, now_lsb, now_msb;
> > -  uint32_t              i;
> > -
> > -  /*
> > -   * We just reset the timer, so we know we're at the beginning of a tick.
> > -   */
> > -
> > -  /*
> > -   * Count cycles. Watching the timer introduces a several microsecond
> > -   * uncertaintity, so let it cook for a while and divide by the number of
> > -   * ticks actually executed.
> > -   */
> > +  uint8_t               lsb, msb;
> > +  uint32_t              max_timer_value;
> > +  uint32_t              last_tick, cur_tick;
> > +  int32_t               diff, remaining;
> > +
> > +  /* Set the timer to free running mode */  outport_byte(TIMER_MODE,
> > + TIMER_SEL0|TIMER_16BIT|TIMER_INTTC);
> Please add spaces between the OR'd values |.
> 

Was just moved from the existing code, but you are right that I can fix it, if I touch it.
Will send an update with this and your other formatting suggestions.

> > +  outport_byte(TIMER_CNTR0, 0);
> > +  outport_byte(TIMER_CNTR0, 0);
> I suppose it is needed to write two 0's, but it may be worth a comment why.
> 

Yes, you need to first set the lsb and then the msb of the 16bit reload register.
I will add a comment.

> > +  /* 16 bit counter */
> > +  max_timer_value = 0xffff;
> > +  /* Calibrate for 1s */
> > +  remaining = TIMER_TICK;
> >
> >    begin_time = rdtsc();
> > -
> > -  for (i = rtems_clock_get_ticks_per_second() * pc386_isrs_per_tick;
> > -       i != 0; --i ) {
> > -    /* We know we've just completed a tick when timer goes from low to
> high */
> > -    then_lsb = then_msb = 0xff;
> > -    do {
> > -      READ_8254(now_lsb, now_msb);
> > -      if ((then_msb < now_msb) ||
> > -          ((then_msb == now_msb) && (then_lsb < now_lsb)))
> > -        break;
> > -      then_lsb = now_lsb;
> > -      then_msb = now_msb;
> > -    } while (1);
> > +  READ_8254(lsb,msb);
> space after comma
> 
> > +  last_tick = (msb << 8) | lsb;
> > +  while(remaining > 0) {
> > +    READ_8254(lsb,msb);
> > +    cur_tick = (msb << 8) | lsb;
> > +    /* PIT counts down, so subtract cur from last */
> > +    diff = last_tick - cur_tick;
> > +    last_tick = cur_tick;
> > +    if (diff < 0) {
> > +        diff += max_timer_value;
> > +    }
> > +    remaining -= diff;
> >    }
> >
> >    pc586_tsc_frequency = rdtsc() - begin_time;
> >
> >  #if 0
> > -  printk( "CPU clock at %u MHz\n", (uint32_t)(pc586_tsc_frequency /
> > 1000000));
> > +  printk( "CPU clock at %u Hz\n", (uint32_t)(pc586_tsc_frequency ));
> >  #endif
> >  }
> >
> >  static void clockOn(void)
> >  {
> > +
> > +  /*
> > +   * First calibrate the TSC. Do this every time we
> > +   * turn the clock on in case the CPU clock speed has changed.
> > +   */
> > +  for (int i = 0; i<5; ++i)
> > +  {
> > +  if ( x86_has_tsc() )
> > +    calibrate_tsc();
> 
> What's the reason to do this 5 times? I don't see any averaging, so just the
> last one is being taken as "correct"
> 

Left overs from debugging.
I removed the print, but forgot to remove the loop as well...

Best regards,

    Jan

> > +  }
> > +
> >    rtems_interrupt_lock_context lock_context;
> >    pc386_isrs_per_tick        = 1;
> >    pc386_microseconds_per_isr =
> > rtems_configuration_get_microseconds_per_tick();
> > @@ -171,13 +184,6 @@ static void clockOn(void)
> >    rtems_interrupt_lock_release(&rtems_i386_i8254_access_lock,
> > &lock_context);
> >
> >    bsp_interrupt_vector_enable( BSP_PERIODIC_TIMER );
> > -
> > -  /*
> > -   * Now calibrate cycles per tick. Do this every time we
> > -   * turn the clock on in case the CPU clock speed has changed.
> > -   */
> > -  if ( x86_has_tsc() )
> > -    calibrate_tsc();
> >  }
> >
> >  bool Clock_isr_enabled = false;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel