Date: Sat, 18 Apr 2009 22:32:27 +0200
From: Ingo Molnar <mingo@elte.hu>
To: Jan Kiszka <jan.kiszka@web.de>
Cc: Jeff Mahoney <jeffm@suse.com>,
       Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Andreas Herrmann <andreas.herrmann3@amd.com>
Subject: Re: [BUG] IO-APIC + timer doesn't work!
Message-ID: <20090418203227.GC30144@elte.hu>
References: <49E89D1B.7060800@suse.com> <49E9904A.5030201@web.de> <49EA2F81.7030302@suse.com> <20090418200623.GA27294@elte.hu> <49EA373A.8060105@web.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <49EA373A.8060105@web.de>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3607
Lines: 94


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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/