Ingo Molnar wrote:
> * Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> Ingo Molnar wrote:
>>> * Jeff Mahoney <jeffm@suse.com> wrote:
>>>
>>>>> Hmmmmm. That somehow reminds me of what I thought I had to fix in the
>>>>> HPET emulation of QEMU just recently [1] - because of 2.6.30-rc's behavior.
>>>>>
>>>>> Could you try if writing 'delta' a second time makes any difference on
>>>>> that box?
>>>>>
>>>>> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
>>>>> index 648b3a2..523d72b 100644
>>>>> --- a/arch/x86/kernel/hpet.c
>>>>> +++ b/arch/x86/kernel/hpet.c
>>>>> @@ -324,6 +324,7 @@ static void hpet_set_mode(enum clock_event_mode mode,
>>>>>  		       HPET_TN_SETVAL | HPET_TN_32BIT;
>>>>>  		hpet_writel(cfg, HPET_Tn_CFG(timer));
>>>>>  		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
>>>>> +		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
>>>>>  		hpet_start_counter();
>>>>>  		hpet_print_config();
>>>>>  		break;
>>>>>
>>>> Thanks, Jan.
>>>>
>>>> That fixed it for me.
>>> I've queued it up (and i've got a test-system that might be affected 
>>> by a similar problem - it shows a similar crash very rarely), but it 
>>> would be nice to know why this duplicate writeout makes a 
>>> difference. Jan?
>>>
>>> 	Ingo
>> Well, if you look at the HPET spec [1], you first find the explanation
>> of the Tn_VAL_SET_CNF bit (HPET_TN_SETVAL):
>>
>> "[...] By writing this bit to a 1, the software is then allowed to
>> directly set a periodic timer's accumulator."
>>
>> That may sound like "you write to the comparator register if 0, and if
>> 1, you set the accumulator". That's also how HPET was emulated in QEMU
>> so far.
>>
>> But then you read on about changing the period of a running timer:
>>
>> "If the software resets the main counter, the value in the comparator’s
>> value register needs to reset as well. This can be done by setting the
>> Tn_VAL_SET_CNF bit. Again, to avoid race conditions, this should be
>> done with the main counter halted. The following usage model is expected:
>> 1) Software clears the GLOBAL_ENABLE_CNF bit to prevent any interrupts
>> 2) Software Clears the main counter by writing a value of 00000000h to it.
>> 3) Software sets the TIMER0_VAL_SET_CNF bit.
>> 4) Software writes the new value in the TIMER0_COMPARATOR_VAL register
>> 5) Software sets the GLOBAL_ENABLE_CNF bit to enable interrupts."
>>
>> And that somehow sounds like you only need to write the new period once,
>> with Tn_VAL_SET_CNF = 1.
>>
>> I bet now that both interpretations are implemented in silicon somewhere
>> out there - but I'm all ears to learn the right one (and potentially
>> re-fix QEMU).
>>
>> Jan
>>
>> [1] http://www.intel.com/hardwaredesign/hpetspec_1.pdf
> 
> i might be a bit slow today, but how does the above transform into:
> 
>   		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> 		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> 
> ? It sets the same register twice.

No, sorry, I missed to cite also this from the Tn_SET_VAL_CFG
explanation: "Software does NOT have to write this bit back to 0 (it
automatically clears)." So the second write will already take place
without it.

> 
> I'm totally happy if it does transform into that under some quirky 
> interpretation. Since it solved the problem for Jeff, we'll likely 
> add it even if there's no actual explanation ;-) But it would be 
> nice to somehow come up with a line of reasoning that ends with:
> 
>  ... and for that reason, we set the value twice:
> 
>   		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> 		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> 
> right?
> 
> 	Ingo

I'd like to give someone from AMD or Intel or whoever already
implemented such a logic a chance to comment on it. If this doesn't
happen, you may add:

"Some HPET implementations apparently only allow exclusive access to
accumulator and comparator register, instead of writing both in parallel
if HPET_TN_SETVAL is set. In that case we have to write a second time
because HPET_TN_SETVAL will then be cleared again and the write will hit
the comparator register."

Jan