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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/06/16 16:19, Jan Beulich wrote:
>>>> 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().

Where? LAPIC initialisation is before this point - its higher up in
init_IRQ() so surely can't depend on this currently working.

As for benign, at the best it is a waste of time, and on reduced
hardware platforms, wrong.  We shouldn't be propagating problems like these.

>
>>> @@ -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.

This is just like any other bit of style - it should be fixed up while
editing even if the rest of the file isn't completely up to scratch.

~Andrew

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