Re: [Xen-devel] [PATCH 4/8] x86/time: calibrate TSC against platform timer

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.16 at 16:20, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/06/16 11:28, Jan Beulich wrote:
>> --- a/xen/arch/x86/i8259.c
>> +++ b/xen/arch/x86/i8259.c
>> @@ -359,12 +359,7 @@ void __init init_IRQ(void)
>>  
>>      apic_intr_init();
>>  
>> -    /* Set the clock to HZ Hz */
>> -#define CLOCK_TICK_RATE 1193182 /* crystal freq (Hz) */
>> -#define LATCH (((CLOCK_TICK_RATE)+(HZ/2))/HZ)
>> -    outb_p(0x34, PIT_MODE);        /* binary, mode 2, LSB/MSB, ch 0 */
>> -    outb_p(LATCH & 0xff, PIT_CH0); /* LSB */
>> -    outb(LATCH >> 8, PIT_CH0);     /* MSB */
>> +    preinit_pit();
> 
> This highlights the fact that we have two unconditional calls to
> preinit_pit() during startup, which is one too many.
> 
> init_IRQ() is called rather earlier than early_time_init(), but I can't
> spot anything inbetween the two calls which would require the PIT to be
> set up.  AFAICT, it is safe to just drop the preinit_pit() call here.

LAPIC initialization makes use of the PIT, and I think that would
break when removing it here. And since doing it twice is benign,
I'd also like to not drop it from early_time_init().

>> @@ -340,7 +348,8 @@ static struct platform_timesource __init
>>      .frequency = CLOCK_TICK_RATE,
>>      .read_counter = read_pit_count,
>>      .counter_bits = 32,
>> -    .init = init_pit
>> +    .init = init_pit,
>> +    .resume = resume_pit
> 
> Please add a redundant comma at the end, to reduce the next diff to
> change this block.

Well, I'd like the three instance to remain consistent in this regard
(yet touching the others doesn't seem warranted). And a change
here isn't all that likely.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel