Message-ID: <6b4e42d10610251420x4365b840sa3232010e7bd7f73@mail.gmail.com>
Date:	Wed, 25 Oct 2006 14:20:22 -0700
From:	"Om Narasimhan" <om.turyx@...il.com>
To:	"Pallipadi, Venkatesh" <venkatesh.pallipadi@...el.com>
Cc:	linux-kernel@...r.kernel.org, randy.dunlap@...cle.com,
	clemens@...isch.de, ak@....de, vojtech@...e.cz, bob.picco@...com
Subject: Re: HPET : Legacy Routing Replacement Enable - 3rd try.

On 10/25/06, Pallipadi, Venkatesh <venkatesh.pallipadi@...el.com> wrote:
>
> General comment. I guess this patch will conflict with timer cleanups
> and hrt timer patches. This patch being smaller, it may be easier to
> rebase this against hrt timer patches.
Thanks for comments.
Rebasing against hrt and other cleanups is a good idea. I would wait
till they make it into mainline.
>
> >+       */
> >+      printk(KERN_INFO PREFIX "HPET id: %#x. ACPI LRR bit %s SET\n",
> >+                      hpet_tbl->id, acpi_hpet_lrr ? "": "NOT");
>
> I don't see acpi_hpet_lrr getting used anywhere in the patch? Are you
> planning to change it in any subsequent patches?

No. Let me explain what I observed.

I tested against five different bioses (some with 8132, some with
CK-804 ..etc) and I observed three different patterns.

1. HW is LRR capable, HPET ACPI it is 1, timer interrupt is on INT2.
Before the fix: Linux cannot get timer interrupts on INT0, goes for ACPI timer.
After the fix : Works fine. This is according to hpet spec.

2. HW is LRR capable, HPET ACPI bit is set to 0. timer interrupt is on INT2.
Before the fix :  Linux cannot get timer interrupts on INT0, goes for
ACPI timer.
After the fix: Faulty BIOS behavior. Introduced parameter
hpet_lrr_force to handle this situation.

3. HW is LRR capable, HPET ACPI bit is set to 1. timer interrupt is on INT0.
Before the fix: Linux works fine.
After the fix :  Faulty BIOS. No way to know which INT timer is connected to.

To handle case 3, I removed all references to acpi_hpet_lrr, explained
this case in the code and decided to solely rely on the command line
parameter for LRR capability. Rational for this approach is ,
1. At present, there are not many BIOSes which implement LRR (correctly)
2. People would see the bootup message (MP-BIOS bug...) if LRR is
enabled and no timer interrupt on INT0. They can pass the hpet_lrr=1
to make everything work fine.
Is it the right approach?

>
> >
> > #ifdef        CONFIG_X86_64
> >       vxtime.hpet_address = hpet_tbl->addr.addrl |
> >diff --git a/arch/i386/kernel/time_hpet.c
> >b/arch/i386/kernel/time_hpet.c
> >index 1a2a979..01b2f67 100644
> >--- a/arch/i386/kernel/time_hpet.c
> >+++ b/arch/i386/kernel/time_hpet.c
> >@@ -94,7 +94,8 @@ static int hpet_timer_stop_set_go(unsign
> >        * Go!
> >        */
> >       cfg = hpet_readl(HPET_CFG);
> >-      if (hpet_use_timer)
> >+      /* Ideally the following should be &&(acpi_hpet_lrr ||
> >hpet_lrr_force) */
> >+      if (hpet_use_timer && hpet_lrr_force)
>
> What will be the value of hpet_lrr_force if no boot parameter was used.
zero.
> It will end up coming from uninitialized data section. Right?
Since it is a global variable, I assumed it would be automatically
initialized to zero, and zero is the expected default. Did I miss
something obvious?

>
> So, CFG_LEGACY will not be set on any platforms unless lrr_force
> parameter is used? Is that the intention or am I missing something?
Yes. That is the way I could think of to handle faulty bios implementations.
>
> >-      setup_irq(0, &irq0);
> >+      printk(KERN_WARNING PREFIX "Registering Timer IRQ =
> >%d\n", timer_irq);
>
> Why is this an unconditional warning?
hmm... That is not required. I should have removed it.

Thanks,
Om.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists