[PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

On many new phone/tablet platforms like Baytrail/Merrifield etc,
the HPET are either defeatured or has some problem to be used
as a reliable timer. As these platforms also have X86_64, we
should not make HPET_TIMER default y for all X86_64.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 arch/x86/Kconfig |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0af5250..7f2fb4b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -695,8 +695,7 @@ config X86_CYCLONE_TIMER
 source "arch/x86/Kconfig.cpu"
 
 config HPET_TIMER
-	def_bool X86_64
-	prompt "HPET Timer Support" if X86_32
+	prompt "HPET Timer Support"
 	---help---
 	  Use the IA-PC HPET (High Precision Event Timer) to manage
 	  time in preference to the PIT and RTC, if a HPET is
@@ -709,9 +708,9 @@ config HPET_TIMER
 
 	  You can safely choose Y here.  However, HPET will only be
 	  activated if the platform and the BIOS support this feature.
-	  Otherwise the 8254 will be used for timing services.
 
-	  Choose N to continue using the legacy 8254 timer.
+	  Choose N to use the legacy 8254 timer or the latest
+	  lapic_timer+tsc solution.
 
 config HPET_EMULATE_RTC
 	def_bool y
-- 
1.7.0.4

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> On many new phone/tablet platforms like Baytrail/Merrifield etc, the 
> HPET are either defeatured or has some problem to be used as a 
> reliable timer. As these platforms also have X86_64, we should not 
> make HPET_TIMER default y for all X86_64.

NAK!

If the HPET is unreliable on a specific platform then any of the 
following solutions would address the problem (in order of 
preference):

 - the hardware should not expose it. Why waste silicon on something 
   that does not work?

 - or the firmware should not expose it. Why expose something that 
   does not work?

 - or the kernel should have a quirk to reliably disable it. Why 
   should we crash or misbehave if a driver is built into the
   kernel?

tweaking a default is _NOT_ a solution for an unreliable hpet.

Thanks,

	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

Hi Ingo,

Thanks for reviewing this

On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> 
> * Feng Tang <feng.tang@intel.com> wrote:
> 
> > On many new phone/tablet platforms like Baytrail/Merrifield etc, the 
> > HPET are either defeatured or has some problem to be used as a 
> > reliable timer. As these platforms also have X86_64, we should not 
> > make HPET_TIMER default y for all X86_64.
> 
> NAK!
> 
> If the HPET is unreliable on a specific platform then any of the 
> following solutions would address the problem (in order of 
> preference):
> 
>  - the hardware should not expose it. Why waste silicon on something 
>    that does not work?
> 
>  - or the firmware should not expose it. Why expose something that 
>    does not work?

Agreed, I've raised problem to BIOS vendor, but the response is very slow,
and those hardware/BIOS may already hit the market as a product.

> 
>  - or the kernel should have a quirk to reliably disable it. Why 
>    should we crash or misbehave if a driver is built into the
>    kernel?

I thought about this before, HPET doesn't have PCI ID like stuff, only
thing I can think of to identify them may be the CPU family/ID. 
Runtime check the reliability of HPET may be difficult, as we don't
know if the 8254 or the TSC are the golden timer to check HPET.

Thanks,
Feng

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Clemens Ladisch]

Feng Tang wrote:
> On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
>> * Feng Tang <feng.tang@intel.com> wrote:
>>  - or the kernel should have a quirk to reliably disable it. Why
>>    should we crash or misbehave if a driver is built into the
>>    kernel?
>
> I thought about this before, HPET doesn't have PCI ID like stuff,

HPET does have the PCI vendor ID in the first register.

> only thing I can think of to identify them may be the CPU family/ID.

The HPET is implemented by some actual chip, and that chip also has lots
of PCI devices.  (In the case of a SoC, the CPU ID would work, too).


Regards,
Clemens

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Ingo Molnar]

* Clemens Ladisch <clemens@ladisch.de> wrote:

> Feng Tang wrote:
> > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> >> * Feng Tang <feng.tang@intel.com> wrote:
> >>  - or the kernel should have a quirk to reliably disable it. Why
> >>    should we crash or misbehave if a driver is built into the
> >>    kernel?
> >
> > I thought about this before, HPET doesn't have PCI ID like stuff,
> 
> HPET does have the PCI vendor ID in the first register.
> 
> > only thing I can think of to identify them may be the CPU family/ID.
> 
> The HPET is implemented by some actual chip, and that chip also has lots
> of PCI devices.  (In the case of a SoC, the CPU ID would work, too).

Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
quirks keyed off chipset PCI IDs:

  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
                           ich_force_enable_hpet);
  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
                           ich_force_enable_hpet);
  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
                           ich_force_enable_hpet);
  [...]

Thanks,

	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> 
> * Clemens Ladisch <clemens@ladisch.de> wrote:
> 
> > Feng Tang wrote:
> > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > >> * Feng Tang <feng.tang@intel.com> wrote:
> > >>  - or the kernel should have a quirk to reliably disable it. Why
> > >>    should we crash or misbehave if a driver is built into the
> > >>    kernel?
> > >
> > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > 
> > HPET does have the PCI vendor ID in the first register.
> > 
> > > only thing I can think of to identify them may be the CPU family/ID.
> > 
> > The HPET is implemented by some actual chip, and that chip also has lots
> > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> 
> Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> quirks keyed off chipset PCI IDs:
> 
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
>                            ich_force_enable_hpet);
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
>                            ich_force_enable_hpet);
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
>                            ich_force_enable_hpet);
>   [...]

Got it, thanks Ingo and Clemens for the pointer. Will try to figure it out.

- Feng

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

Hi Ingo,

On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> 
> * Clemens Ladisch <clemens@ladisch.de> wrote:
> 
> > Feng Tang wrote:
> > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > >> * Feng Tang <feng.tang@intel.com> wrote:
> > >>  - or the kernel should have a quirk to reliably disable it. Why
> > >>    should we crash or misbehave if a driver is built into the
> > >>    kernel?
> > >
> > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > 
> > HPET does have the PCI vendor ID in the first register.
> > 
> > > only thing I can think of to identify them may be the CPU family/ID.
> > 
> > The HPET is implemented by some actual chip, and that chip also has lots
> > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> 
> Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> quirks keyed off chipset PCI IDs:
> 
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
>                            ich_force_enable_hpet);
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
>                            ich_force_enable_hpet);
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
>                            ich_force_enable_hpet);
>   [...]

I just gave it another thought, that the HPET on our platform currently
do have some problem to be used as clocksource/clockevent, but it may
get fixed in future version (by Silicon or BIOS). 

If I add quirk to block it now, I may revert this code in future when
it get fixed, same problem applis for the future generation of platform.

So can we do a small change like below, so that we are able to disable
the HPET_TIMER in our own x86_64 .config, while all exising distributions
are not affected as it is still default "y" .

---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f67e839..a1027a5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -711,7 +711,7 @@ source "arch/x86/Kconfig.cpu"
 
 config HPET_TIMER
 	def_bool X86_64
-	prompt "HPET Timer Support" if X86_32
+	prompt "HPET Timer Support"
 	---help---
 	  Use the IA-PC HPET (High Precision Event Timer) to manage
 	  time in preference to the PIT and RTC, if a HPET is


Thanks,
Feng

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> Hi Ingo,
> 
> On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> > 
> > * Clemens Ladisch <clemens@ladisch.de> wrote:
> > 
> > > Feng Tang wrote:
> > > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > > >> * Feng Tang <feng.tang@intel.com> wrote:
> > > >>  - or the kernel should have a quirk to reliably disable it. Why
> > > >>    should we crash or misbehave if a driver is built into the
> > > >>    kernel?
> > > >
> > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > 
> > > HPET does have the PCI vendor ID in the first register.
> > > 
> > > > only thing I can think of to identify them may be the CPU family/ID.
> > > 
> > > The HPET is implemented by some actual chip, and that chip also has lots
> > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > 
> > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > quirks keyed off chipset PCI IDs:
> > 
> >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> >                            ich_force_enable_hpet);
> >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> >                            ich_force_enable_hpet);
> >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> >                            ich_force_enable_hpet);
> >   [...]
> 
> I just gave it another thought, that the HPET on our platform 
> currently do have some problem to be used as clocksource/clockevent, 
> but it may get fixed in future version (by Silicon or BIOS).
> 
> If I add quirk to block it now, I may revert this code in future 
> when it get fixed, same problem applis for the future generation of 
> platform.

If the hardware or BIOS gets fixed then that will be visible in the 
revision level of the hardware, right?

Such kind of revision level, once it is known, can then be used to 
turn the quirk on/off precisely.

Thanks,

	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

On Tue, Apr 15, 2014 at 11:00:25AM +0200, Ingo Molnar wrote:
> 
> * Feng Tang <feng.tang@intel.com> wrote:
> 
> > Hi Ingo,
> > 
> > On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> > > 
> > > * Clemens Ladisch <clemens@ladisch.de> wrote:
> > > 
> > > > Feng Tang wrote:
> > > > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > > > >> * Feng Tang <feng.tang@intel.com> wrote:
> > > > >>  - or the kernel should have a quirk to reliably disable it. Why
> > > > >>    should we crash or misbehave if a driver is built into the
> > > > >>    kernel?
> > > > >
> > > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > > 
> > > > HPET does have the PCI vendor ID in the first register.
> > > > 
> > > > > only thing I can think of to identify them may be the CPU family/ID.
> > > > 
> > > > The HPET is implemented by some actual chip, and that chip also has lots
> > > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > > 
> > > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > > quirks keyed off chipset PCI IDs:
> > > 
> > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> > >                            ich_force_enable_hpet);
> > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> > >                            ich_force_enable_hpet);
> > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> > >                            ich_force_enable_hpet);
> > >   [...]
> > 
> > I just gave it another thought, that the HPET on our platform 
> > currently do have some problem to be used as clocksource/clockevent, 
> > but it may get fixed in future version (by Silicon or BIOS).
> > 
> > If I add quirk to block it now, I may revert this code in future 
> > when it get fixed, same problem applis for the future generation of 
> > platform.
> 
> If the hardware or BIOS gets fixed then that will be visible in the 
> revision level of the hardware, right?

AFAIK, if it is fixed in a new silicon version, we should be able to
detect it by the "stepping", but the PCI DEV ID is not likely to change. 

If it is fixed by BIOS or PUNIT FW, then we may go through the DMI
info to check the BIOS version, One problem is there is some Baytrail
tablet platforms that doesn't provide DMI info as its FW is not
UEFI/Legacy BIOS compatible version.

Thanks,
Feng

> 
> Such kind of revision level, once it is known, can then be used to 
> turn the quirk on/off precisely.
> 
> Thanks,
> 
> 	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> On Tue, Apr 15, 2014 at 11:00:25AM +0200, Ingo Molnar wrote:
> > 
> > * Feng Tang <feng.tang@intel.com> wrote:
> > 
> > > Hi Ingo,
> > > 
> > > On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Clemens Ladisch <clemens@ladisch.de> wrote:
> > > > 
> > > > > Feng Tang wrote:
> > > > > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > > > > >> * Feng Tang <feng.tang@intel.com> wrote:
> > > > > >>  - or the kernel should have a quirk to reliably disable it. Why
> > > > > >>    should we crash or misbehave if a driver is built into the
> > > > > >>    kernel?
> > > > > >
> > > > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > > > 
> > > > > HPET does have the PCI vendor ID in the first register.
> > > > > 
> > > > > > only thing I can think of to identify them may be the CPU family/ID.
> > > > > 
> > > > > The HPET is implemented by some actual chip, and that chip also has lots
> > > > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > > > 
> > > > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > > > quirks keyed off chipset PCI IDs:
> > > > 
> > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> > > >                            ich_force_enable_hpet);
> > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> > > >                            ich_force_enable_hpet);
> > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> > > >                            ich_force_enable_hpet);
> > > >   [...]
> > > 
> > > I just gave it another thought, that the HPET on our platform 
> > > currently do have some problem to be used as clocksource/clockevent, 
> > > but it may get fixed in future version (by Silicon or BIOS).
> > > 
> > > If I add quirk to block it now, I may revert this code in future 
> > > when it get fixed, same problem applis for the future generation of 
> > > platform.
> > 
> > If the hardware or BIOS gets fixed then that will be visible in the 
> > revision level of the hardware, right?
> 
> AFAIK, if it is fixed in a new silicon version, we should be able to 
> detect it by the "stepping", but the PCI DEV ID is not likely to 
> change.
> 
> If it is fixed by BIOS or PUNIT FW, then we may go through the DMI 
> info to check the BIOS version, One problem is there is some 
> Baytrail tablet platforms that doesn't provide DMI info as its FW is 
> not UEFI/Legacy BIOS compatible version.

So just turn it off for the current hardware, via PCI ID. When you get 
a fix, you can turn it back on again by refining the quirk check, for 
that specific hardware.

Thanks,

	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

On Tue, Apr 15, 2014 at 12:42:06PM +0200, Ingo Molnar wrote:
> 
> * Feng Tang <feng.tang@intel.com> wrote:
> 
> > On Tue, Apr 15, 2014 at 11:00:25AM +0200, Ingo Molnar wrote:
> > > 
> > > * Feng Tang <feng.tang@intel.com> wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> > > > > 
> > > > > * Clemens Ladisch <clemens@ladisch.de> wrote:
> > > > > 
> > > > > > Feng Tang wrote:
> > > > > > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > > > > > >> * Feng Tang <feng.tang@intel.com> wrote:
> > > > > > >>  - or the kernel should have a quirk to reliably disable it. Why
> > > > > > >>    should we crash or misbehave if a driver is built into the
> > > > > > >>    kernel?
> > > > > > >
> > > > > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > > > > 
> > > > > > HPET does have the PCI vendor ID in the first register.
> > > > > > 
> > > > > > > only thing I can think of to identify them may be the CPU family/ID.
> > > > > > 
> > > > > > The HPET is implemented by some actual chip, and that chip also has lots
> > > > > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > > > > 
> > > > > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > > > > quirks keyed off chipset PCI IDs:
> > > > > 
> > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> > > > >                            ich_force_enable_hpet);
> > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> > > > >                            ich_force_enable_hpet);
> > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> > > > >                            ich_force_enable_hpet);
> > > > >   [...]
> > > > 
> > > > I just gave it another thought, that the HPET on our platform 
> > > > currently do have some problem to be used as clocksource/clockevent, 
> > > > but it may get fixed in future version (by Silicon or BIOS).
> > > > 
> > > > If I add quirk to block it now, I may revert this code in future 
> > > > when it get fixed, same problem applis for the future generation of 
> > > > platform.
> > > 
> > > If the hardware or BIOS gets fixed then that will be visible in the 
> > > revision level of the hardware, right?
> > 
> > AFAIK, if it is fixed in a new silicon version, we should be able to 
> > detect it by the "stepping", but the PCI DEV ID is not likely to 
> > change.
> > 
> > If it is fixed by BIOS or PUNIT FW, then we may go through the DMI 
> > info to check the BIOS version, One problem is there is some 
> > Baytrail tablet platforms that doesn't provide DMI info as its FW is 
> > not UEFI/Legacy BIOS compatible version.
> 
> So just turn it off for the current hardware, via PCI ID. When you get 
> a fix, you can turn it back on again by refining the quirk check, for 
> that specific hardware.

got it, thanks!

- Feng

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> On Tue, Apr 15, 2014 at 12:42:06PM +0200, Ingo Molnar wrote:
> > 
> > * Feng Tang <feng.tang@intel.com> wrote:
> > 
> > > On Tue, Apr 15, 2014 at 11:00:25AM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Feng Tang <feng.tang@intel.com> wrote:
> > > > 
> > > > > Hi Ingo,
> > > > > 
> > > > > On Fri, Mar 28, 2014 at 09:11:17AM +0100, Ingo Molnar wrote:
> > > > > > 
> > > > > > * Clemens Ladisch <clemens@ladisch.de> wrote:
> > > > > > 
> > > > > > > Feng Tang wrote:
> > > > > > > > On Fri, Mar 28, 2014 at 08:17:16AM +0100, Ingo Molnar wrote:
> > > > > > > >> * Feng Tang <feng.tang@intel.com> wrote:
> > > > > > > >>  - or the kernel should have a quirk to reliably disable it. Why
> > > > > > > >>    should we crash or misbehave if a driver is built into the
> > > > > > > >>    kernel?
> > > > > > > >
> > > > > > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > > > > > 
> > > > > > > HPET does have the PCI vendor ID in the first register.
> > > > > > > 
> > > > > > > > only thing I can think of to identify them may be the CPU family/ID.
> > > > > > > 
> > > > > > > The HPET is implemented by some actual chip, and that chip also has lots
> > > > > > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > > > > > 
> > > > > > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > > > > > quirks keyed off chipset PCI IDs:
> > > > > > 
> > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> > > > > >                            ich_force_enable_hpet);
> > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> > > > > >                            ich_force_enable_hpet);
> > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> > > > > >                            ich_force_enable_hpet);
> > > > > >   [...]
> > > > > 
> > > > > I just gave it another thought, that the HPET on our platform 
> > > > > currently do have some problem to be used as clocksource/clockevent, 
> > > > > but it may get fixed in future version (by Silicon or BIOS).
> > > > > 
> > > > > If I add quirk to block it now, I may revert this code in future 
> > > > > when it get fixed, same problem applis for the future generation of 
> > > > > platform.
> > > > 
> > > > If the hardware or BIOS gets fixed then that will be visible in the 
> > > > revision level of the hardware, right?
> > > 
> > > AFAIK, if it is fixed in a new silicon version, we should be able to 
> > > detect it by the "stepping", but the PCI DEV ID is not likely to 
> > > change.
> > > 
> > > If it is fixed by BIOS or PUNIT FW, then we may go through the 
> > > DMI info to check the BIOS version, One problem is there is some 
> > > Baytrail tablet platforms that doesn't provide DMI info as its 
> > > FW is not UEFI/Legacy BIOS compatible version.
> > 
> > So just turn it off for the current hardware, via PCI ID. When you 
> > get a fix, you can turn it back on again by refining the quirk 
> > check, for that specific hardware.
> 
> got it, thanks!

Btw., note that such narrow quirk refinings are pretty uncontroversial 
and have a pretty fast path upstream, once you have the initial quirk 
in there.

Thanks,

	Ingo

Re: [PATCH v2] x86: hpet: Don't default CONFIG_HPET_TIMER to be y for X86_64

[Feng Tang]

Hi All,

On Wed, Apr 16, 2014 at 08:51:48AM +0200, Ingo Molnar wrote:
> 
> * Feng Tang <feng.tang@intel.com> wrote:
 > > > > > > > >
> > > > > > > > > I thought about this before, HPET doesn't have PCI ID like stuff,
> > > > > > > > 
> > > > > > > > HPET does have the PCI vendor ID in the first register.
> > > > > > > > 
> > > > > > > > > only thing I can think of to identify them may be the CPU family/ID.
> > > > > > > > 
> > > > > > > > The HPET is implemented by some actual chip, and that chip also has lots
> > > > > > > > of PCI devices.  (In the case of a SoC, the CPU ID would work, too).
> > > > > > > 
> > > > > > > Correct. See arch/x86/kernel/hpet.c, which has a large number of HPET 
> > > > > > > quirks keyed off chipset PCI IDs:
> > > > > > > 
> > > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_0,
> > > > > > >                            ich_force_enable_hpet);
> > > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0,
> > > > > > >                            ich_force_enable_hpet);
> > > > > > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1,
> > > > > > >                            ich_force_enable_hpet);
> > > > > > >   [...]
> > > > > > 
> > > > > > I just gave it another thought, that the HPET on our platform 
> > > > > > currently do have some problem to be used as clocksource/clockevent, 
> > > > > > but it may get fixed in future version (by Silicon or BIOS).
> > > > > > 
> > > > > > If I add quirk to block it now, I may revert this code in future 
> > > > > > when it get fixed, same problem applis for the future generation of 
> > > > > > platform.
> > > > > 
> > > > > If the hardware or BIOS gets fixed then that will be visible in the 
> > > > > revision level of the hardware, right?
> > > > 
> > > > AFAIK, if it is fixed in a new silicon version, we should be able to 
> > > > detect it by the "stepping", but the PCI DEV ID is not likely to 
> > > > change.
> > > > 
> > > > If it is fixed by BIOS or PUNIT FW, then we may go through the 
> > > > DMI info to check the BIOS version, One problem is there is some 
> > > > Baytrail tablet platforms that doesn't provide DMI info as its 
> > > > FW is not UEFI/Legacy BIOS compatible version.
> > > 
> > > So just turn it off for the current hardware, via PCI ID. When you 
> > > get a fix, you can turn it back on again by refining the quirk 
> > > check, for that specific hardware.
> > 
> > got it, thanks!
> 
> Btw., note that such narrow quirk refinings are pretty uncontroversial 
> and have a pretty fast path upstream, once you have the initial quirk 
> in there.

During implementing, I found the arch/x86/kerne/quirks.c doesn't fit
well as the DECLARE_PCI_FIXUP_HEADER/EARLY/FINAL() way of quirk happens
inside the pci subsystem init phase, while the hpet_enable() will be
called before that.

Then by searching the arch/x86/, the early-quirks.c can be used to
add such a quirk as the early quirks are checked earlier than
hpet_enable()

I'll post a RFC patch soon.

Thanks,
Feng

[RFC PATCH] x86, hpet: Add quirk to disable HPET for baytrail platform

[Feng Tang]

Hi All,

Please help to review this RFC patch, if you are ok with it. I'll separte them into
2 and repost.
	1. make the boot_hpet_disable extern
	2. add quirk for baytrail HPET.
thanks!

- Feng



>From 4b8c370649f1d2b320aa46a005b8b61eb59a539e Mon Sep 17 00:00:00 2001
From: Feng Tang <feng.tang@intel.com>
Date: Fri, 18 Apr 2014 15:43:22 +0800
Subject: [PATCH] x86, hpet: Add quirk to disable HPET for baytrail platform

HPET on current baytrail platform has accuracy problem to be used as
reliable clocksource/clockevent, so add a early quirk to disable it.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 arch/x86/include/asm/hpet.h    |    1 +
 arch/x86/kernel/early-quirks.c |   14 ++++++++++++++
 arch/x86/kernel/hpet.c         |    2 +-
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index b18df57..36f7125 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -63,6 +63,7 @@
 /* hpet memory map physical address */
 extern unsigned long hpet_address;
 extern unsigned long force_hpet_address;
+extern int boot_hpet_disable;
 extern u8 hpet_blockid;
 extern int hpet_force_user;
 extern u8 hpet_msi_disable;
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index bc4a088..1e5f6bc 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -17,6 +17,7 @@
 #include <asm/dma.h>
 #include <asm/io_apic.h>
 #include <asm/apic.h>
+#include <asm/hpet.h>
 #include <asm/iommu.h>
 #include <asm/gart.h>
 #include <asm/irq_remapping.h>
@@ -380,6 +381,13 @@ static void __init intel_graphics_stolen(int num, int slot, int func)
 	}
 }
 
+static void __init force_disable_hpet(int num, int slot, int func)
+{
+	boot_hpet_disable = 1;
+	pr_info("Will Disable HPET for this platform\n");
+}
+
+
 #define QFLAG_APPLY_ONCE 	0x1
 #define QFLAG_APPLIED		0x2
 #define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
@@ -417,6 +425,12 @@ static struct chipset early_qrk[] __initdata = {
 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
 	  QFLAG_APPLY_ONCE, intel_graphics_stolen },
+	/*
+	 * HPET on current version of Baytrail platform has accuracy
+	 * problem, disable it for now
+	 */
+	{ PCI_VENDOR_ID_INTEL, 0x0f00,
+		PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet},
 	{}
 };
 
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index da85a8e..913e207 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -88,7 +88,7 @@ static inline void hpet_clear_mapping(void)
 /*
  * HPET command line enable / disable
  */
-static int boot_hpet_disable;
+int boot_hpet_disable;
 int hpet_force_user;
 static int hpet_verbose;
 
-- 
1.7.0.4

Re: [RFC PATCH] x86, hpet: Add quirk to disable HPET for baytrail platform

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> Hi All,
> 
> Please help to review this RFC patch, if you are ok with it. I'll separte them into
> 2 and repost.
> 	1. make the boot_hpet_disable extern
> 	2. add quirk for baytrail HPET.
> thanks!
> 
> - Feng
> 
> 
> 
> From 4b8c370649f1d2b320aa46a005b8b61eb59a539e Mon Sep 17 00:00:00 2001
> From: Feng Tang <feng.tang@intel.com>
> Date: Fri, 18 Apr 2014 15:43:22 +0800
> Subject: [PATCH] x86, hpet: Add quirk to disable HPET for baytrail platform
> 
> HPET on current baytrail platform has accuracy problem to be used as
> reliable clocksource/clockevent, so add a early quirk to disable it.
> 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  arch/x86/include/asm/hpet.h    |    1 +
>  arch/x86/kernel/early-quirks.c |   14 ++++++++++++++
>  arch/x86/kernel/hpet.c         |    2 +-
>  3 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
> index b18df57..36f7125 100644
> --- a/arch/x86/include/asm/hpet.h
> +++ b/arch/x86/include/asm/hpet.h
> @@ -63,6 +63,7 @@
>  /* hpet memory map physical address */
>  extern unsigned long hpet_address;
>  extern unsigned long force_hpet_address;
> +extern int boot_hpet_disable;
>  extern u8 hpet_blockid;
>  extern int hpet_force_user;
>  extern u8 hpet_msi_disable;
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index bc4a088..1e5f6bc 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -17,6 +17,7 @@
>  #include <asm/dma.h>
>  #include <asm/io_apic.h>
>  #include <asm/apic.h>
> +#include <asm/hpet.h>
>  #include <asm/iommu.h>
>  #include <asm/gart.h>
>  #include <asm/irq_remapping.h>
> @@ -380,6 +381,13 @@ static void __init intel_graphics_stolen(int num, int slot, int func)
>  	}
>  }
>  
> +static void __init force_disable_hpet(int num, int slot, int func)
> +{
> +	boot_hpet_disable = 1;
> +	pr_info("Will Disable HPET for this platform\n");
> +}
> +
> +
>  #define QFLAG_APPLY_ONCE 	0x1
>  #define QFLAG_APPLIED		0x2
>  #define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
> @@ -417,6 +425,12 @@ static struct chipset early_qrk[] __initdata = {
>  	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
>  	  QFLAG_APPLY_ONCE, intel_graphics_stolen },
> +	/*
> +	 * HPET on current version of Baytrail platform has accuracy
> +	 * problem, disable it for now
> +	 */
> +	{ PCI_VENDOR_ID_INTEL, 0x0f00,
> +		PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet},
>  	{}
>  };
>  
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index da85a8e..913e207 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -88,7 +88,7 @@ static inline void hpet_clear_mapping(void)
>  /*
>   * HPET command line enable / disable
>   */
> -static int boot_hpet_disable;
> +int boot_hpet_disable;
>  int hpet_force_user;
>  static int hpet_verbose;

As long as all those PCI IDs have a buggy HPET, this looks good to me.

hpa, tglx, any preferences?

Thanks,

	Ingo