RE: i386 HPET code

RE: i386 HPET code

[Pallipadi, Venkatesh]

Hi John, Andrew,


Can you check whether only the following change makes the problem go
away. If yes, then it looks like a hardware issue.

>	hpet_writel(hpet_tick, HPET_T0_CMP);
>+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
>

Thanks,
Venki

>-----Original Message-----
>From: john stultz [mailto:johnstul@us.ibm.com] 
>Sent: Wednesday, February 02, 2005 6:05 PM
>To: Pallipadi, Venkatesh
>Cc: Andi Kleen; lkml; keith maanthey; Max Asbock; Chris McDermott
>Subject: i386 HPET code
>
>Hey Venkatesh,
>	I've been looking into a bug where i386 2.6 kernels do 
>not boot on IBM
>e325s if HPET_TIMER is enabled (hpet=disable works around the issue).
>When running x86-64 kernels, the issue isn't seen. It appears 
>that after
>the hpet is enabled, we stop receiving timer ticks. I've not played on
>any other HPET enabled systems, nor have I looked at the HPET spec, so
>I'm not sure if this is a hardware issue or not.
>
>The following patch, which uses the x86-64 code for
>hpet_timer_stop_set_go() seems to fix the issue.
>
>Your thoughts?
>
>thanks
>-john
>
>
>===== arch/i386/kernel/time_hpet.c 1.10 vs edited =====
>--- 1.10/arch/i386/kernel/time_hpet.c	2004-11-02 06:40:42 -08:00
>+++ edited/arch/i386/kernel/time_hpet.c	2005-02-02 
>17:59:27 -08:00
>@@ -64,29 +64,30 @@
> {
> 	unsigned int cfg;
> 
>-	/*
>-	 * Stop the timers and reset the main counter.
>-	 */
>+/*
>+ * Stop the timers and reset the main counter.
>+ */
>+
> 	cfg = hpet_readl(HPET_CFG);
>-	cfg &= ~HPET_CFG_ENABLE;
>+	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
> 	hpet_writel(cfg, HPET_CFG);
> 	hpet_writel(0, HPET_COUNTER);
> 	hpet_writel(0, HPET_COUNTER + 4);
> 
>-	/*
>-	 * Set up timer 0, as periodic with first interrupt to happen at
>-	 * hpet_tick, and period also hpet_tick.
>-	 */
>-	cfg = hpet_readl(HPET_T0_CFG);
>-	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC |
>-	       HPET_TN_SETVAL | HPET_TN_32BIT;
>-	hpet_writel(cfg, HPET_T0_CFG);
>-	hpet_writel(tick, HPET_T0_CMP);
>+/*
>+ * Set up timer 0, as periodic with first interrupt to happen 
>at hpet_tick,
>+ * and period also hpet_tick.
>+ */
>+
>+	hpet_writel(HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
>+		    HPET_TN_32BIT, HPET_T0_CFG);
>+	hpet_writel(hpet_tick, HPET_T0_CMP);
>+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
>+
>+/*
>+ * Go!
>+ */
> 
>-	/*
>- 	 * Go!
>- 	 */
>-	cfg = hpet_readl(HPET_CFG);
> 	cfg |= HPET_CFG_ENABLE | HPET_CFG_LEGACY;
> 	hpet_writel(cfg, HPET_CFG);
> 
>
>
>

RE: i386 HPET code

[john stultz]

On Thu, 2005-02-03 at 06:28 -0800, Pallipadi, Venkatesh wrote:
> Can you check whether only the following change makes the problem go
> away. If yes, then it looks like a hardware issue.
> 
> >	hpet_writel(hpet_tick, HPET_T0_CMP);
> >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> >

Yep. Adding only the second write seems to make the box boot.

Since this isn't just affecting our hardware (see Andrew Walrond's
comment in the thread), would doing two writes like x86-64 does be
acceptable? 

thanks
-john

Re: i386 HPET code

[Venkatesh Pallipadi]

On Thu, Feb 03, 2005 at 11:30:56AM -0800, john stultz wrote:
> On Thu, 2005-02-03 at 06:28 -0800, Pallipadi, Venkatesh wrote:
> > Can you check whether only the following change makes the problem go
> > away. If yes, then it looks like a hardware issue.
> > 
> > >	hpet_writel(hpet_tick, HPET_T0_CMP);
> > >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> > >
> 
> Yep. Adding only the second write seems to make the box boot.
> 
> Since this isn't just affecting our hardware (see Andrew Walrond's
> comment in the thread), would doing two writes like x86-64 does be
> acceptable? 
> 

Yes. As this is just the initialization code, I think adding second write 
is OK. But, I am not sure why two writes are required and will the two write
be sufficient for all systems. I don't seem to remember anything about this
in HPET specs. I will double check it. 

Basically I am thinking of something like this will be a good generic solution
in place of simple two writes.

for (i = 0 ; i <some number for max retries>; i++) {
	hpet_writel(hpet_tick, HPET_T0_CMP);
	if (hpet_tick == hpet_readl(hpet_tick, HPET_T0_CMP))
		break;
}

I think we can wait for result from Andrew's system and chose either one
of the above approaches.

Thanks,
Venki

Re: i386 HPET code

[Andi Kleen]

> Basically I am thinking of something like this will be a good generic solution
> in place of simple two writes.
> 
> for (i = 0 ; i <some number for max retries>; i++) {
> 	hpet_writel(hpet_tick, HPET_T0_CMP);
> 	if (hpet_tick == hpet_readl(hpet_tick, HPET_T0_CMP))
> 		break;
> }

Makes sense. There were so many bugs in PIT timer access over time,
it would be probably a miracle if the hardware engineers got all
the HPET implementations right ;-) 

If you do a fix like this please change x86-64 too.

-Andi

Re: i386 HPET code

[Andrew Walrond]

On Thursday 03 February 2005 20:02, Venkatesh Pallipadi wrote:
> On Thu, Feb 03, 2005 at 11:30:56AM -0800, john stultz wrote:
> > On Thu, 2005-02-03 at 06:28 -0800, Pallipadi, Venkatesh wrote:
> > > Can you check whether only the following change makes the problem go
> > > away. If yes, then it looks like a hardware issue.
> > >
> > > > hpet_writel(hpet_tick, HPET_T0_CMP);
> > > >+ hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> >
> > Yep. Adding only the second write seems to make the box boot.
> >

Just to confirm that this also fixes the problem for two types of MSI dual 
opteron motherboards. (MSI K8D Master 2/3)

Andrew Walrond

Re: i386 HPET code

[Andi Kleen]

On Thu, Feb 03, 2005 at 06:28:27AM -0800, Pallipadi, Venkatesh wrote:
> 
> Hi John, Andrew,
> 
> 
> Can you check whether only the following change makes the problem go
> away. If yes, then it looks like a hardware issue.
> 
> >	hpet_writel(hpet_tick, HPET_T0_CMP);
> >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */


Ask Vojtech (cced), he wrote the x86-64 HPET code.

-Andi

Re: i386 HPET code

[Vojtech Pavlik]

On Thu, Feb 03, 2005 at 10:30:26PM +0100, Andi Kleen wrote:
> On Thu, Feb 03, 2005 at 06:28:27AM -0800, Pallipadi, Venkatesh wrote:
> > 
> > Hi John, Andrew,
> > 
> > 
> > Can you check whether only the following change makes the problem go
> > away. If yes, then it looks like a hardware issue.
> > 
> > >	hpet_writel(hpet_tick, HPET_T0_CMP);
> > >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> 
> 
> Ask Vojtech (cced), he wrote the x86-64 HPET code.
 
Can you add some background of the thread?

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: i386 HPET code

[Vojtech Pavlik]

On Thu, Feb 03, 2005 at 10:30:26PM +0100, Andi Kleen wrote:
> On Thu, Feb 03, 2005 at 06:28:27AM -0800, Pallipadi, Venkatesh wrote:
> > 
> > Hi John, Andrew,
> > 
> > 
> > Can you check whether only the following change makes the problem go
> > away. If yes, then it looks like a hardware issue.
> > 
> > >	hpet_writel(hpet_tick, HPET_T0_CMP);
> > >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> 
> 
> Ask Vojtech (cced), he wrote the x86-64 HPET code.

It took me a while to remember, but:
 
The first write after writing TN_SETVAL to the config register sets the
counter value, the second write sets the threshold. 

When you only do the first write you never set the threshold and
interrupts won't be generated properly.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: i386 HPET code

[Vojtech Pavlik]

On Fri, Feb 04, 2005 at 09:02:38PM +0100, Vojtech Pavlik wrote:
> On Thu, Feb 03, 2005 at 10:30:26PM +0100, Andi Kleen wrote:
> > On Thu, Feb 03, 2005 at 06:28:27AM -0800, Pallipadi, Venkatesh wrote:
> > > 
> > > Hi John, Andrew,
> > > 
> > > 
> > > Can you check whether only the following change makes the problem go
> > > away. If yes, then it looks like a hardware issue.
> > > 
> > > >	hpet_writel(hpet_tick, HPET_T0_CMP);
> > > >+	hpet_writel(hpet_tick, HPET_T0_CMP); /* AK: why twice? */
> > 
> > 
> > Ask Vojtech (cced), he wrote the x86-64 HPET code.
> 
> It took me a while to remember, but:
>  
> The first write after writing TN_SETVAL to the config register sets the
> counter value, the second write sets the threshold. 
> 
> When you only do the first write you never set the threshold and
> interrupts won't be generated properly.

That means it's not a bug, but a (documented) feature.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

[PATCH][i386] HPET setup, duplicate HPET_T0_CMP needed for some platforms

[Venkatesh Pallipadi]

This patch fixes the issue with HPET on some platforms.

According to Vojtech Pavlik:

The first write after writing TN_SETVAL to the config register sets the
counter value, the second write sets the threshold.

When you only do the first write you never set the threshold and
interrupts won't be generated properly.

Thanks to John Stultz and Andrew Walrond for reporting, root causing 
the issue and verifying this fix.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>


--- linux-2.6.10/arch/i386/kernel/time_hpet.c.org	2005-02-04 12:04:09.000000000 -0800
+++ linux-2.6.10/arch/i386/kernel/time_hpet.c	2005-02-04 18:01:25.000000000 -0800
@@ -81,6 +81,11 @@ static int hpet_timer_stop_set_go(unsign
 	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC |
 	       HPET_TN_SETVAL | HPET_TN_32BIT;
 	hpet_writel(cfg, HPET_T0_CFG);
+	/* 
+	 * Some systems seems to need two writes to HPET_T0_CMP, 
+	 * to get interrupts working
+	 */
+	hpet_writel(tick, HPET_T0_CMP);
 	hpet_writel(tick, HPET_T0_CMP);
 
 	/*

Re: [PATCH][i386] HPET setup, duplicate HPET_T0_CMP needed for some platforms

[Andrew Walrond]

On Friday 04 February 2005 23:41, Venkatesh Pallipadi wrote:
> + /*
> +  * Some systems seems to need two writes to HPET_T0_CMP,
> +  * to get interrupts working
> +  */

I think you should update this comment in light of the information from 
Vojtech:

/*
 * The first write after writing TN_SETVAL to the config register sets the
 * counter value, the second write sets the threshold.
 */

Andrew Walrond

Re: [PATCH][i386] HPET setup, duplicate HPET_T0_CMP needed for some platforms

[Giuseppe Bilotta]

Venkatesh Pallipadi wrote:
> +	/* 
> +	 * Some systems seems to need two writes to HPET_T0_CMP, 
> +	 * to get interrupts working
> +	 */
> +	hpet_writel(tick, HPET_T0_CMP);
>  	hpet_writel(tick, HPET_T0_CMP);

Is it known which platforms require two, and which ones require 
one write? Is it cost-effective to #if CONFIG_ the second 
write?

-- 
Giuseppe "Oblomov" Bilotta

Can't you see
It all makes perfect sense
Expressed in dollar and cents
Pounds shillings and pence
                  (Roger Waters)

RE: [PATCH][i386] HPET setup, duplicate HPET_T0_CMP needed for some platforms

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org 
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of 
>Giuseppe Bilotta
>Sent: Sunday, February 06, 2005 7:59 AM
>To: linux-kernel@vger.kernel.org
>Subject: Re: [PATCH][i386] HPET setup, duplicate HPET_T0_CMP 
>needed for some platforms
>
>Venkatesh Pallipadi wrote:
>> +	/* 
>> +	 * Some systems seems to need two writes to HPET_T0_CMP, 
>> +	 * to get interrupts working
>> +	 */
>> +	hpet_writel(tick, HPET_T0_CMP);
>>  	hpet_writel(tick, HPET_T0_CMP);
>
>Is it known which platforms require two, and which ones require 
>one write? Is it cost-effective to #if CONFIG_ the second 
>write?

Additional write should not be performace critical, as this code is
called
only once, during boot up (and again during system suspend-resume).

Thanks,
Venki