Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz <dslutz@verizon.com>]

On 04/14/14 11:07, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> This is based on software-developers-hpet-spec-1-0a.pdf saying:
>>
>> When the main counter value matches the value in the timer's
>> comparator register, an interrupt can be generated.  The hardware
>> will then automatically increase the value in the compare register
>> by the last value written to that register.
>>
>> And since interrupt can only be generated when the master clock
>> is enabled.
> This last sentence looks truncated.

It is. I think I had:

And since interrupt can only be generated when the master clock
is enabled, the compare register should also change only then.

Which looks too complex to me now.

 From the spec:


ENABLE_CNF

Overall Enable:

This bit must be set to enable any of the timers to generate interrupts. If
this bit is 0, then the main counter will halt (will not increment) and no interrupts will be
caused by any of these timers.

• 0 – Halt main count and disable all timer interrupts
• 1 – allow main counter to run, and allow timer interrupts if enabled




I think a better statement would be:

When the overall enable is off (the main count is halted), none of
the compare registers should change.


So I will change this in the commit message.

> As to the change - I'm not sure: The quoted description from the
> specification cal also be read to mean that interrupt generation is
> optional, but comparator increment will always happen. As long as
> this can't be clarified, I'd prefer to stay with the code as is.

I think the code needs to change to match the spec.

#define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)

vs

#define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)


The change uses hpet_enabled() (I.E. Overall Enable).

-Don Slutz

>
> Jan
>