Moorestown changes in arch/x86 for 35 merge window

From: Jacob Pan <[email protected]>

check hlt requires external timer interrupt to wake up the cpu. for platforms equipped with only per cpu timers, we don't have external interrupts during check hlt.
this patch checks such condition to avoid kernel hang at hlt. it also saves boot time in that hlt four times in the current code requires four ticks to break out of them.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/include/asm/bugs.h |    1 +
 arch/x86/kernel/cpu/bugs.c  |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

arch/x86/include/asm/bugs.h +1 -0

index 08abf63..1e0cef8 100644
@@ -2,6 +2,7 @@
 #define _ASM_X86_BUGS_H
 
 extern void check_bugs(void);
+extern struct clock_event_device *global_clock_event;
 
 #if defined(CONFIG_CPU_SUP_INTEL) && defined(CONFIG_X86_32)
 int ppro_with_ram_bug(void);

arch/x86/kernel/cpu/bugs.c +0 -0

index 01a2652..c0d9688 100644

I really wish I knew the exact systems affected by the HLT bug. If I remember correctly, it was some 386 systems -- or possibly 486 systems as well -- a very long time ago. This test just provides a diagnosis if the system really is bad (it hangs with an obvious message) at the cost of some 40 ms to the system boot time. I suspect C1 (HLT) being broken is not anywhere close to the predominant power management problem in the current day, and as such I'm wondering if this particular test hasn't outlived its usefulness.

Thoughts?

Quoted 10 lines · H. Peter Anvin

I really wish I knew the exact systems affected by the HLT bug. If I remember correctly, it was some 386 systems -- or possibly 486 systems as well -- a very long time ago. This test just provides a diagnosis if the system really is bad (it hangs with an obvious message) at the cost of some 40 ms to the system boot time. I suspect C1 (HLT) being broken is not anywhere close to the predominant power management problem in the current day, and as such I'm wondering if this particular test hasn't outlived its usefulness.

Quoted 1 line

Thoughts?

we could at least hide it behind the "don't run on pentium or newer" config options..

I'd be cool skipping it for family 5 or newer. I'm just wondering if we should kill it completely -- IIRC it was only a handful of 386/486 systems which had problems, usually due to marginal power supplies which couldn't handle the noise of a variable load (DOS not having any power management would run at a reliable 100% load) -- that's not exactly the type of systems which would have survived to modern day.

Quoted 6 lines

I'd be cool skipping it for family 5 or newer. I'm just wondering if we should kill it completely -- IIRC it was only a handful of 386/486 systems which had problems, usually due to marginal power supplies which couldn't handle the noise of a variable load (DOS not having any power management would run at a reliable 100% load) -- that's not exactly the type of systems which would have survived to modern day.

Also SMM and hardware bugs on some platforms - Cyrix MediaGX 5510 for example where a hlt at the wrong moment during ATA transfers hung the box until power cycle. But all old old stuff.

Quoted 7 lines · Alan Cox

Quoted 6 lines

I'd be cool skipping it for family 5 or newer. I'm just wondering if we should kill it completely -- IIRC it was only a handful of 386/486 systems which had problems, usually due to marginal power supplies which couldn't handle the noise of a variable load (DOS not having any power management would run at a reliable 100% load) -- that's not exactly the type of systems which would have survived to modern day.

Quoted 3 lines

Also SMM and hardware bugs on some platforms - Cyrix MediaGX 5510 for example where a hlt at the wrong moment during ATA transfers hung the box until power cycle. But all old old stuff.

I think family < 5 seems a reasonable cutoff.

Note that the ATA transfer bug you describe above would not be caught by the existing check.

Quoted 6 lines

I'd be cool skipping it for family 5 or newer. I'm just wondering if we should kill it completely -- IIRC it was only a handful of 386/486 systems which had problems, usually due to marginal power supplies which couldn't handle the noise of a variable load (DOS not having any power management would run at a reliable 100% load) -- that's not exactly the type of systems which would have survived to modern day.

Also SMM and hardware bugs on some platforms - Cyrix MediaGX 5510 for example where a hlt at the wrong moment during ATA transfers hung the box until power cycle. But all old old stuff.

Quoted 10 lines · H. Peter Anvin

I really wish I knew the exact systems affected by the HLT bug. If I remember correctly, it was some 386 systems -- or possibly 486 systems as well -- a very long time ago. This test just provides a diagnosis if the system really is bad (it hangs with an obvious message) at the cost of some 40 ms to the system boot time. I suspect C1 (HLT) being broken is not anywhere close to the predominant power management problem in the current day, and as such I'm wondering if this particular test hasn't outlived its usefulness.

Quoted 1 line

Thoughts?

we could at least hide it behind the "don't run on pentium or newer" config options..

Quoted 11 lines · Arjan van de Ven

Quoted 10 lines · H. Peter Anvin

I really wish I knew the exact systems affected by the HLT bug. If I remember correctly, it was some 386 systems -- or possibly 486 systems as well -- a very long time ago. This test just provides a diagnosis if the system really is bad (it hangs with an obvious message) at the cost of some 40 ms to the system boot time. I suspect C1 (HLT) being broken is not anywhere close to the predominant power management problem in the current day, and as such I'm wondering if this particular test hasn't outlived its usefulness.

Quoted 4 lines

Quoted 1 line

Thoughts?

we could at least hide it behind the "don't run on pentium or newer" config options..

Ack. That would take care of all relevant machines.

Sounds like a plan. Jacob, do you want to submit a new patch (bypassing this check if boot_cpu_info.x86 >= 5)?

-hpa

From: Jacob Pan <[email protected]>

lapic timer calibration can be combined with tsc in platform specific calibration functions. if such calibration result is obtained early, we can skip the redundent calibration loops.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/kernel/apic/apic.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

arch/x86/kernel/apic/apic.c +0 -0

index e5a4a1e..8ef56ac 100644

arch/x86/kernel/apic/apic.c +6 -0

 static int lapic_next_event(unsigned long delta,
 			    struct clock_event_device *evt);

@@ -597,6 +597,25 @@ static int __init calibrate_APIC_clock(void)
 	long delta, deltatsc;
 	int pm_referenced = 0;
 
+	/**
+	 * check if lapic timer has already been calibrated by platform
+	 * specific routine, such as tsc calibration code. if so, we just fill
+	 * in the clockevent structure and return.
+	 */
+	if (calibration_result) {

arch/x86/kernel/apic/apic.c +2 -0

+	}
+
 	local_irq_disable();
 
 	/* Replace the global interrupt handler */

Signature · 1 line

1.6.3.3

Quoted 4 lines

lapic timer calibration can be combined with tsc in platform specific calibration functions. if such calibration result is obtained early, we can skip the redundent calibration loops.

I'd rather move lapic calibration into TSC calibration in general as we do the same thing twice for no good reason.

That needs some code restructuring, but that's worth it.

Quoted 2 lines

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

Hehe. So you handed the patch to yourself :)

Review

Quoted 5 lines

---
 arch/x86/kernel/apic/apic.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/arch/x86/kernel/apic/apic.c

Quoted 7 lines

index e5a4a1e..8ef56ac 100644

arch/x86/kernel/apic/apic.c +0 -0

@@ -175,7 +175,7 @@ static struct resource lapic_resource = {
 	.flags = IORESOURCE_MEM | IORESOURCE_BUSY,
 };
 

arch/x86/kernel/apic/apic.c static struct resource lapic_resource = { · line 178

Quoted 2 lines

arch/x86/kernel/apic/apic.c +1 -1

-static unsigned int calibration_result;
+unsigned int calibration_result;

Aside of my general objection it'd be not a good idea to make this global w/o renaming it to something sensible like lapic_timer_frequency.

arch/x86/kernel/apic/apic.c static int __init calibrate_APIC_clock(void) · line 602

Quoted 9 lines

arch/x86/kernel/apic/apic.c +3 -0

 static int lapic_next_event(unsigned long delta,
 			    struct clock_event_device *evt);

@@ -597,6 +597,25 @@ static int __init calibrate_APIC_clock(void)
 	long delta, deltatsc;
 	int pm_referenced = 0;
 
+	/**
+	 * check if lapic timer has already been calibrated by platform
+	 * specific routine, such as tsc calibration code. if so, we just

fill

arch/x86/kernel/apic/apic.c static int __init calibrate_APIC_clock(void)

Quoted 4 lines

arch/x86/kernel/apic/apic.c +4 -0

+	 * in the clockevent structure and return.
+	 */
+	if (calibration_result) {
+		apic_printk(APIC_VERBOSE, "lapic timer already calibrated

%d\n",

arch/x86/kernel/apic/apic.c static int __init calibrate_APIC_clock(void) · line 608

Quoted 2 lines

arch/x86/kernel/apic/apic.c +2 -0

+				calibration_result);
+		lapic_clockevent.mult =

div_sc(calibration_result/APIC_DIVISOR,

arch/x86/kernel/apic/apic.c static int __init calibrate_APIC_clock(void) · line 616

Quoted 7 lines

arch/x86/kernel/apic/apic.c +7 -0

+					TICK_NSEC, lapic_clockevent.shift);
+		lapic_clockevent.max_delta_ns =
+			clockevent_delta2ns(0x7FFFFF, &lapic_clockevent);
+		lapic_clockevent.min_delta_ns =
+			clockevent_delta2ns(0xF, &lapic_clockevent);
+		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_DUMMY;
+		return 0;

Grr. I hate duplicated code.

Quoted 8 lines

arch/x86/kernel/apic/apic.c +2 -1

+	}
+
 	local_irq_disable();
 
 	/* Replace the global interrupt handler */
--

1.6.3.3

Thanks for the review.

Quoted 8 lines

Quoted 4 lines

lapic timer calibration can be combined with tsc in platform specific calibration functions. if such calibration result is obtained early, we can skip the redundent calibration loops.

I'd rather move lapic calibration into TSC calibration in general as we do the same thing twice for no good reason.

Quoted 2 lines

That needs some code restructuring, but that's worth it.

I am trying to avoid the risks of completely remove the current lapic calibration code since there are so many platforms with different timer options. And I don't understand things like why pm timer is preferred. Why not use the rating in clocksource?

Quoted 4 lines

Aside of my general objection it'd be not a good idea to make this global w/o renaming it to something sensible like lapic_timer_frequency.

perhaps, the calibration data can directly be assigned to lapic timer clock_event_device.mult? There is no need for the device specific result scale (e.g. bus clocks per tick)

We do not have access to the clocksource at that point. But we do a calibration loop for TSC and for lapic timer. There is no reason why we can't do that in one go.

Quoted 6 lines

Quoted 4 lines

Aside of my general objection it'd be not a good idea to make this global w/o renaming it to something sensible like lapic_timer_frequency.

perhaps, the calibration data can directly be assigned to lapic timer clock_event_device.mult? There is no need for the device specific

result

Quoted 1 line

scale (e.g. bus clocks per tick)

No. That needs an accessor function.

The question I have is the reference clock used for calibrating those local timers, PIT, HPET, PM timer how should they be ranked. Can we make those known freq clocksource devices available at this point so that we can use the clocksource abstraction and its ranking automatically?

From: Jacob Pan <[email protected]>

Penwell has added always on lapic timers and tsc, we want to treat it as a variant of moorestown so that one binary kernel can boot on both. this patch added clock selction logic so that platform code can set up the optimal configuration for both moorestown and medfield.

This patch will also mark Penwell TSC reliable, thus no need for watchdog clocksource to monitor it.

Signed-off-by Alek Du <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/include/asm/mrst.h |   30 +++++++++++
 arch/x86/kernel/mrst.c      |  119 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 137 insertions(+), 12 deletions(-)

arch/x86/include/asm/mrst.h +0 -0

index 451d30e..3054407 100644

arch/x86/include/asm/mrst.h +8 -0

+/**
+ * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,
+ * cmdline option x86_mrst_timer can be used to override the configuration
+ * to prefer one or the other.
+ * at runtime, there are basically three timer configurations:
+ * 1. per cpu apbt clock only
+ * 2. per cpu always-on lapic clocks only, this is Penwell/Medfield only
+ * 3. per cpu lapic clock (C3STOP) and one apbt clock, with broadcast.

arch/x86/include/asm/mrst.h +6 -4

+ * lapic (always-on,ARAT) ------ 150
+ */
+
+int mrst_timer_options __cpuinitdata;
+
 static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
 static struct sfi_timer_table_entry sfi_mtimer_array[SFI_MTMR_MAX_NUM];
+static u32 mrst_cpu_chip;
 int sfi_mtimer_num;
 
 struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];

@@ -167,15 +191,16 @@ int __init sfi_parse_mrtc(struct sfi_table_header *table)
 	return 0;
 }
 
-/*
- * the secondary clock in Moorestown can be APBT or LAPIC clock, default to
- * APBT but cmdline option can also override it.
- */
 static void __cpuinit mrst_setup_secondary_clock(void)
 {

arch/x86/include/asm/mrst.h +3 -3

+	}
 	apbt_setup_secondary_clock();
 }
 

@@ -183,9 +208,45 @@ static unsigned long __init mrst_calibrate_tsc(void)
 {
 	unsigned long flags, fast_calibrate;
 
-	local_irq_save(flags);
-	fast_calibrate = apbt_quick_calibrate();
-	local_irq_restore(flags);
+	if (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) {
+		u32 lo, hi, ratio, fsb;

arch/x86/include/asm/mrst.h +33 -2

+		fast_calibrate = ratio * fsb;
+		pr_debug("read penwell tsc %lu khz\n", fast_calibrate);
+		calibration_result = fsb * 1000 / HZ;
+		/* mark tsc clocksource as reliable */
+		set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE);
+	} else {
+		/**
+		 * TODO: calibrate lapic timer with apbt, if we use apbt only,
+		 * there is no need to calibrate lapic timer, since they are
+		 * not used.
+		 * if we use lapic timers and apbt, the default calibration
+		 * should work, since we have the global clockevent setup.
+		 * but it would be more efficient if we combine the lapic timer
+		 * with tsc calibration.
+		 */
+		local_irq_save(flags);
+		fast_calibrate = apbt_quick_calibrate();
+		local_irq_restore(flags);
+	}
+
+	pr_info("tsc lapic calibration results %lu %d\n",
+			fast_calibrate, calibration_result);
 
 	if (fast_calibrate)
 		return fast_calibrate;

@@ -195,6 +256,11 @@ static unsigned long __init mrst_calibrate_tsc(void)
 
 void __init mrst_time_init(void)
 {
+	/* if cpu is penwell, lapic timer will be used by default */
+	if ((mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) &&
+		(mrst_timer_options == MRST_TIMER_DEFAULT))
+		return;
+
 	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
 	pre_init_apic_IRQ0();
 	apbt_time_init();

@@ -211,11 +277,38 @@ void __init mrst_rtc_init(void)
  */
 static void __init mrst_setup_boot_clock(void)
 {
-	pr_info("%s: per cpu apbt flag %d \n", __func__, disable_apbt_percpu);
-	if (disable_apbt_percpu)
+	if (mrst_timer_options == MRST_TIMER_APBT_ONLY)
+		return;
+	if ((mrst_timer_options == MRST_TIMER_LAPIC_APBT)
+		|| (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL))
 		setup_boot_APIC_clock();
 };
 
+enum cpuid_regs {
+	CR_EAX = 0,

arch/x86/include/asm/mrst.h +5 -0

+	return mrst_cpu_chip;
+}
+
 /*
  * Moorestown specific x86_init function overrides and early setup
  * calls.

@@ -237,4 +330,6 @@ void __init x86_mrst_early_setup(void)
 	x86_init.pci.fixup_irqs = x86_init_noop;
 
 	legacy_pic = &null_legacy_pic;
+
+	mrst_identify_cpu();
 }

Signature · 1 line

1.6.3.3

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 307

Quoted 4 lines

arch/x86/kernel/mrst.c +4 -0

+	pr_debug("cpuid result %x\n", regs[CR_EAX]);
+	pr_info("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");

Are we going to add one of those for each new family ? This is really redundant bloat with no value.

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 309

Quoted 1 line

arch/x86/kernel/mrst.c +1 -0

+	return mrst_cpu_chip;

From the driver side we need this value determined and visible to the

Quoted 8 lines

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 307

Quoted 4 lines

arch/x86/kernel/mrst.c +4 -0

+	pr_debug("cpuid result %x\n", regs[CR_EAX]);
+	pr_info("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");

Are we going to add one of those for each new family ? This is really redundant bloat with no value.

Quoted 1 line

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 309

Quoted 1 line

arch/x86/kernel/mrst.c +1 -0

+	return mrst_cpu_chip;

From the driver side we need this value determined and visible to the drivers because some things like the IPC interface need to know rather than getting the cpuid magic replicated around.

Quoted 8 lines · Alan Cox

Quoted 8 lines

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 307

Quoted 4 lines

arch/x86/kernel/mrst.c +4 -0

+	pr_debug("cpuid result %x\n", regs[CR_EAX]);
+	pr_info("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");

Are we going to add one of those for each new family ? This is really redundant bloat with no value.

Quoted 5 lines

Quoted 1 line

arch/x86/kernel/mrst.c void __init mrst_rtc_init(void) · line 309

Quoted 1 line

arch/x86/kernel/mrst.c +1 -0

+	return mrst_cpu_chip;

From the driver side we need this value determined and visible to the drivers because some things like the IPC interface need to know rather than getting the cpuid magic replicated around.

Ok. That makes sense. Is there more what MRST drivers need to know ?

apbt sucks performance wise, so why do you consider it a better choice than lapic + broadcast ?

arch/x86/kernel/mrst.c

Quoted 7 lines

+ * lapic (always-on,ARAT) ------ 150
+ */

arch/x86/kernel/mrst.c +3 -0

+
+int mrst_timer_options __cpuinitdata;
+
 static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
 static struct sfi_timer_table_entry

sfi_mtimer_array[SFI_MTMR_MAX_NUM];

arch/x86/kernel/mrst.c int __init sfi_parse_mrtc(struct

Quoted 5 lines

arch/x86/kernel/mrst.c +1 -0

+static u32 mrst_cpu_chip;
 int sfi_mtimer_num;
 
 struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];

@@ -167,15 +191,16 @@ int __init sfi_parse_mrtc(struct

sfi_table_header *table)

arch/x86/kernel/mrst.c int __init sfi_parse_mrtc(struct sfi_table_header *table)

Quoted 5 lines

arch/x86/kernel/mrst.c +0 -2

 	return 0;
 }
 
-/*
- * the secondary clock in Moorestown can be APBT or LAPIC clock,

default to

Quoted 4 lines

arch/x86/kernel/mrst.c +0 -2

- * APBT but cmdline option can also override it.
- */
 static void __cpuinit mrst_setup_secondary_clock(void)
 {

arch/x86/kernel/mrst.c int __init sfi_parse_mrtc(struct sfi_table_header *table) · line 202

Quoted 10 lines

arch/x86/kernel/mrst.c +7 -3

-	/* restore default lapic clock if disabled by cmdline */
-	if (disable_apbt_percpu)
-		return setup_secondary_APIC_clock();
+	if ((mrst_timer_options == MRST_TIMER_APBT_ONLY))
+		return apbt_setup_secondary_clock();
+	if (cpu_has(&current_cpu_data, X86_FEATURE_ARAT)
+		||  (mrst_timer_options == MRST_TIMER_LAPIC_APBT)) {
+		pr_info("using lapic timers for secondary clock\n");
+		setup_secondary_APIC_clock();
+		return;

The logic is confusing.

I guess the 111 is Penwell/MRST specific, right ?

According to SDM we have anyway different results for the various CPU families, but we should utilize this in a generic way and have the translation tables for the various CPUs in one place.

Yikes. From which Intel cookbook is this ?

Aside of that we have that info in boot_cpu_info already, don't we ? So there is neither a requirement for the extra cpuid call nor for the extra mrst_cpu_chip id magic.

From 5ae648b2f18778df4eb3f1916a98971332482544 Mon Sep 17 00:00:00 2001

Quoted 23 lines · Jacob Pan

From: Jacob Pan <[email protected]>

Penwell has added always on lapic timers and tsc, we want to treat it as a variant of moorestown so that one binary kernel can boot on both. this patch added clock selction logic so that platform code can set up the optimal configuration for both moorestown and medfield.

This patch will also mark Penwell TSC reliable, thus no need for watchdog clocksource to monitor it.

Signed-off-by Alek Du <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/include/asm/mrst.h |   30 +++++++++++
 arch/x86/kernel/mrst.c      |  119 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 137 insertions(+), 12 deletions(-)

arch/x86/include/asm/mrst.h +0 -0

index 451d30e..3054407 100644

arch/x86/include/asm/mrst.h line 14

Quoted 4 lines

arch/x86/include/asm/mrst.h +1 -0

@@ -11,7 +11,37 @@
 #ifndef _ASM_X86_MRST_H
 #define _ASM_X86_MRST_H
 extern int pci_mrst_init(void);
+extern unsigned int calibration_result;

Yuck, why is this in a mrst specific header ?

arch/x86/include/asm/mrst.h line 18

Quoted 4 lines

arch/x86/include/asm/mrst.h +4 -0

+
+#define MRST_TIMER_DEFAULT	0
+#define MRST_TIMER_APBT_ONLY	1
+#define MRST_TIMER_LAPIC_APBT	2

enum please, also

Quoted 8 lines

arch/x86/include/asm/mrst.h +8 -0

+/**
+ * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,
+ * cmdline option x86_mrst_timer can be used to override the configuration
+ * to prefer one or the other.
+ * at runtime, there are basically three timer configurations:
+ * 1. per cpu apbt clock only
+ * 2. per cpu always-on lapic clocks only, this is Penwell/Medfield only
+ * 3. per cpu lapic clock (C3STOP) and one apbt clock, with broadcast.

arch/x86/include/asm/mrst.h line 36

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+ *
+ * by default (without cmdline option), platform code first detects cpu type
+ * to see if we are on lincroft or penwell, then set up both lapic or apbt
+ * clocks accordingly.
+ * i.e. by default, medfield uses configuration #2, moorestown uses #1.
+ * config #3 is supported but not recommended on medfield.
+ *
+ * rating and feature summary:
+ * lapic (with C3STOP) --------- 100
+ * apbt (always-on) ------------ 110

apbt sucks performance wise, so why do you consider it a better choice than lapic + broadcast ?

Quoted 21 lines

arch/x86/include/asm/mrst.h +6 -4

+ * lapic (always-on,ARAT) ------ 150
+ */
+
+int mrst_timer_options __cpuinitdata;
+
 static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
 static struct sfi_timer_table_entry sfi_mtimer_array[SFI_MTMR_MAX_NUM];
+static u32 mrst_cpu_chip;
 int sfi_mtimer_num;
 
 struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];

@@ -167,15 +191,16 @@ int __init sfi_parse_mrtc(struct sfi_table_header *table)
 	return 0;
 }
 
-/*
- * the secondary clock in Moorestown can be APBT or LAPIC clock, default to
- * APBT but cmdline option can also override it.
- */
 static void __cpuinit mrst_setup_secondary_clock(void)
 {

arch/x86/include/asm/mrst.h int __init sfi_parse_mrtc(struct sfi_table_header *table) · line 202

Quoted 10 lines

arch/x86/include/asm/mrst.h +7 -3

-	/* restore default lapic clock if disabled by cmdline */
-	if (disable_apbt_percpu)
-		return setup_secondary_APIC_clock();
+	if ((mrst_timer_options == MRST_TIMER_APBT_ONLY))
+		return apbt_setup_secondary_clock();
+	if (cpu_has(&current_cpu_data, X86_FEATURE_ARAT)
+		||  (mrst_timer_options == MRST_TIMER_LAPIC_APBT)) {
+		pr_info("using lapic timers for secondary clock\n");
+		setup_secondary_APIC_clock();
+		return;

The logic is confusing.

Quoted 13 lines

arch/x86/include/asm/mrst.h +3 -3

+	}
 	apbt_setup_secondary_clock();
 }
 

@@ -183,9 +208,45 @@ static unsigned long __init mrst_calibrate_tsc(void)
 {
 	unsigned long flags, fast_calibrate;
 
-	local_irq_save(flags);
-	fast_calibrate = apbt_quick_calibrate();
-	local_irq_restore(flags);
+	if (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) {
+		u32 lo, hi, ratio, fsb;

arch/x86/include/asm/mrst.h static unsigned long __init mrst_calibrate_tsc(void) · line 222

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+
+		rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
+		pr_debug("IA32 perf status is 0x%x, 0x%0x\n", lo, hi);
+		ratio = (hi >> 8) & 0x1f;
+		pr_debug("ratio is %d\n", ratio);
+		if (!ratio) {
+			pr_err("read a zero ratio, should be incorrect!\n");
+			pr_err("force tsc ratio to 16 ...\n");
+			ratio = 16;
+		}

This is not Penwell specific at all. The ratio can be read out on all Core based CPUs either from MSR_PLATFORM_ID[12:8] or MSR_PERF_STAT[44:40] depending on XE operation enabled (MSR_PERF_STAT[31] == 1)

This should be made general available and not burried into the mrst code.

arch/x86/include/asm/mrst.h static unsigned long __init mrst_calibrate_tsc(void) · line 227

Quoted 5 lines

arch/x86/include/asm/mrst.h +5 -0

+		rdmsr(MSR_FSB_FREQ, lo, hi);
+		if ((lo & 0x7) == 0x7)
+			fsb = PENWELL_FSB_FREQ_83SKU;
+		else
+			fsb = PENWELL_FSB_FREQ_100SKU;

I guess the 111 is Penwell/MRST specific, right ?

According to SDM we have anyway different results for the various CPU families, but we should utilize this in a generic way and have the translation tables for the various CPUs in one place.

Quoted 52 lines

arch/x86/include/asm/mrst.h +33 -2

+		fast_calibrate = ratio * fsb;
+		pr_debug("read penwell tsc %lu khz\n", fast_calibrate);
+		calibration_result = fsb * 1000 / HZ;
+		/* mark tsc clocksource as reliable */
+		set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE);
+	} else {
+		/**
+		 * TODO: calibrate lapic timer with apbt, if we use apbt only,
+		 * there is no need to calibrate lapic timer, since they are
+		 * not used.
+		 * if we use lapic timers and apbt, the default calibration
+		 * should work, since we have the global clockevent setup.
+		 * but it would be more efficient if we combine the lapic timer
+		 * with tsc calibration.
+		 */
+		local_irq_save(flags);
+		fast_calibrate = apbt_quick_calibrate();
+		local_irq_restore(flags);
+	}
+
+	pr_info("tsc lapic calibration results %lu %d\n",
+			fast_calibrate, calibration_result);
 
 	if (fast_calibrate)
 		return fast_calibrate;

@@ -195,6 +256,11 @@ static unsigned long __init mrst_calibrate_tsc(void)
 
 void __init mrst_time_init(void)
 {
+	/* if cpu is penwell, lapic timer will be used by default */
+	if ((mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) &&
+		(mrst_timer_options == MRST_TIMER_DEFAULT))
+		return;
+
 	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
 	pre_init_apic_IRQ0();
 	apbt_time_init();

@@ -211,11 +277,38 @@ void __init mrst_rtc_init(void)
  */
 static void __init mrst_setup_boot_clock(void)
 {
-	pr_info("%s: per cpu apbt flag %d \n", __func__, disable_apbt_percpu);
-	if (disable_apbt_percpu)
+	if (mrst_timer_options == MRST_TIMER_APBT_ONLY)
+		return;
+	if ((mrst_timer_options == MRST_TIMER_LAPIC_APBT)
+		|| (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL))
 		setup_boot_APIC_clock();
 };
 
+enum cpuid_regs {
+	CR_EAX = 0,

arch/x86/include/asm/mrst.h void __init mrst_rtc_init(void) · line 298

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+	CR_ECX,
+	CR_EDX,
+	CR_EBX
+};
+
+int mrst_identify_cpu(void)
+{
+	u32 regs[4];
+
+	cpuid(1, &regs[CR_EAX], &regs[CR_EBX], &regs[CR_ECX], &regs[CR_EDX]);

Yikes. From which Intel cookbook is this ?

Aside of that we have that info in boot_cpu_info already, don't we ? So there is neither a requirement for the extra cpuid call nor for the extra mrst_cpu_chip id magic.

arch/x86/include/asm/mrst.h void __init mrst_rtc_init(void) · line 308

Quoted 10 lines

arch/x86/include/asm/mrst.h +8 -0

+	if ((regs[CR_EAX] & PENWELL_FAMILY) == PENWELL_FAMILY)
+		mrst_cpu_chip = MRST_CPU_CHIP_PENWELL;
+	else
+		mrst_cpu_chip = MRST_CPU_CHIP_LINCROFT;
 
 
+	pr_debug("cpuid result %x\n", regs[CR_EAX]);
+	pr_info("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");

Are we going to add one of those for each new family ? This is really redundant bloat with no value.

Quoted 16 lines

arch/x86/include/asm/mrst.h +5 -0

+	return mrst_cpu_chip;
+}
+
 /*
  * Moorestown specific x86_init function overrides and early setup
  * calls.

@@ -237,4 +330,6 @@ void __init x86_mrst_early_setup(void)
 	x86_init.pci.fixup_irqs = x86_init_noop;
 
 	legacy_pic = &null_legacy_pic;
+
+	mrst_identify_cpu();
 }

Signature · 1 line

1.6.3.3

From 4103dc6165530a490c12a6177d916f2c2e012436 Mon Sep 17 00:00:00 2001

Quoted 23 lines · Jacob Pan

From: Jacob Pan <[email protected]>

Penwell has added always on lapic timers and tsc, we want to treat it as a variant of moorestown so that one binary kernel can boot on both. this patch added clock selction logic so that platform code can set up the optimal configuration for both moorestown and medfield.

This patch will also mark Penwell TSC reliable, thus no need for watchdog clocksource to monitor it.

Signed-off-by Alek Du <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/include/asm/mrst.h |   30 +++++++++++
 arch/x86/kernel/mrst.c      |  119 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 137 insertions(+), 12 deletions(-)

arch/x86/include/asm/mrst.h +0 -0

index 451d30e..3054407 100644

arch/x86/include/asm/mrst.h line 14

Quoted 4 lines

arch/x86/include/asm/mrst.h +1 -0

@@ -11,7 +11,37 @@
 #ifndef _ASM_X86_MRST_H
 #define _ASM_X86_MRST_H
 extern int pci_mrst_init(void);
+extern unsigned int calibration_result;

Yuck, why is this in a mrst specific header ?

arch/x86/include/asm/mrst.h line 18

Quoted 4 lines

arch/x86/include/asm/mrst.h +4 -0

+
+#define MRST_TIMER_DEFAULT	0
+#define MRST_TIMER_APBT_ONLY	1
+#define MRST_TIMER_LAPIC_APBT	2

enum please, also

Quoted 8 lines

arch/x86/include/asm/mrst.h +8 -0

+/**
+ * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,
+ * cmdline option x86_mrst_timer can be used to override the configuration
+ * to prefer one or the other.
+ * at runtime, there are basically three timer configurations:
+ * 1. per cpu apbt clock only
+ * 2. per cpu always-on lapic clocks only, this is Penwell/Medfield only
+ * 3. per cpu lapic clock (C3STOP) and one apbt clock, with broadcast.

arch/x86/include/asm/mrst.h line 36

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+ *
+ * by default (without cmdline option), platform code first detects cpu type
+ * to see if we are on lincroft or penwell, then set up both lapic or apbt
+ * clocks accordingly.
+ * i.e. by default, medfield uses configuration #2, moorestown uses #1.
+ * config #3 is supported but not recommended on medfield.
+ *
+ * rating and feature summary:
+ * lapic (with C3STOP) --------- 100
+ * apbt (always-on) ------------ 110

apbt sucks performance wise, so why do you consider it a better choice than lapic + broadcast ?

Quoted 21 lines

arch/x86/include/asm/mrst.h +6 -4

+ * lapic (always-on,ARAT) ------ 150
+ */
+
+int mrst_timer_options __cpuinitdata;
+
 static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
 static struct sfi_timer_table_entry sfi_mtimer_array[SFI_MTMR_MAX_NUM];
+static u32 mrst_cpu_chip;
 int sfi_mtimer_num;
 
 struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];

@@ -167,15 +191,16 @@ int __init sfi_parse_mrtc(struct sfi_table_header *table)
 	return 0;
 }
 
-/*
- * the secondary clock in Moorestown can be APBT or LAPIC clock, default to
- * APBT but cmdline option can also override it.
- */
 static void __cpuinit mrst_setup_secondary_clock(void)
 {

arch/x86/include/asm/mrst.h int __init sfi_parse_mrtc(struct sfi_table_header *table) · line 202

Quoted 10 lines

arch/x86/include/asm/mrst.h +7 -3

-	/* restore default lapic clock if disabled by cmdline */
-	if (disable_apbt_percpu)
-		return setup_secondary_APIC_clock();
+	if ((mrst_timer_options == MRST_TIMER_APBT_ONLY))
+		return apbt_setup_secondary_clock();
+	if (cpu_has(&current_cpu_data, X86_FEATURE_ARAT)
+		||  (mrst_timer_options == MRST_TIMER_LAPIC_APBT)) {
+		pr_info("using lapic timers for secondary clock\n");
+		setup_secondary_APIC_clock();
+		return;

The logic is confusing.

Quoted 13 lines

arch/x86/include/asm/mrst.h +3 -3

+	}
 	apbt_setup_secondary_clock();
 }
 

@@ -183,9 +208,45 @@ static unsigned long __init mrst_calibrate_tsc(void)
 {
 	unsigned long flags, fast_calibrate;
 
-	local_irq_save(flags);
-	fast_calibrate = apbt_quick_calibrate();
-	local_irq_restore(flags);
+	if (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) {
+		u32 lo, hi, ratio, fsb;

arch/x86/include/asm/mrst.h static unsigned long __init mrst_calibrate_tsc(void) · line 222

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+
+		rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
+		pr_debug("IA32 perf status is 0x%x, 0x%0x\n", lo, hi);
+		ratio = (hi >> 8) & 0x1f;
+		pr_debug("ratio is %d\n", ratio);
+		if (!ratio) {
+			pr_err("read a zero ratio, should be incorrect!\n");
+			pr_err("force tsc ratio to 16 ...\n");
+			ratio = 16;
+		}

This is not Penwell specific at all. The ratio can be read out on all Core based CPUs either from MSR_PLATFORM_ID[12:8] or MSR_PERF_STAT[44:40] depending on XE operation enabled (MSR_PERF_STAT[31] == 1)

This should be made general available and not burried into the mrst code.

arch/x86/include/asm/mrst.h static unsigned long __init mrst_calibrate_tsc(void) · line 227

Quoted 5 lines

arch/x86/include/asm/mrst.h +5 -0

+		rdmsr(MSR_FSB_FREQ, lo, hi);
+		if ((lo & 0x7) == 0x7)
+			fsb = PENWELL_FSB_FREQ_83SKU;
+		else
+			fsb = PENWELL_FSB_FREQ_100SKU;

I guess the 111 is Penwell/MRST specific, right ?

According to SDM we have anyway different results for the various CPU families, but we should utilize this in a generic way and have the translation tables for the various CPUs in one place.

Quoted 52 lines

arch/x86/include/asm/mrst.h +33 -2

+		fast_calibrate = ratio * fsb;
+		pr_debug("read penwell tsc %lu khz\n", fast_calibrate);
+		calibration_result = fsb * 1000 / HZ;
+		/* mark tsc clocksource as reliable */
+		set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE);
+	} else {
+		/**
+		 * TODO: calibrate lapic timer with apbt, if we use apbt only,
+		 * there is no need to calibrate lapic timer, since they are
+		 * not used.
+		 * if we use lapic timers and apbt, the default calibration
+		 * should work, since we have the global clockevent setup.
+		 * but it would be more efficient if we combine the lapic timer
+		 * with tsc calibration.
+		 */
+		local_irq_save(flags);
+		fast_calibrate = apbt_quick_calibrate();
+		local_irq_restore(flags);
+	}
+
+	pr_info("tsc lapic calibration results %lu %d\n",
+			fast_calibrate, calibration_result);
 
 	if (fast_calibrate)
 		return fast_calibrate;

@@ -195,6 +256,11 @@ static unsigned long __init mrst_calibrate_tsc(void)
 
 void __init mrst_time_init(void)
 {
+	/* if cpu is penwell, lapic timer will be used by default */
+	if ((mrst_cpu_chip == MRST_CPU_CHIP_PENWELL) &&
+		(mrst_timer_options == MRST_TIMER_DEFAULT))
+		return;
+
 	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
 	pre_init_apic_IRQ0();
 	apbt_time_init();

@@ -211,11 +277,38 @@ void __init mrst_rtc_init(void)
  */
 static void __init mrst_setup_boot_clock(void)
 {
-	pr_info("%s: per cpu apbt flag %d \n", __func__, disable_apbt_percpu);
-	if (disable_apbt_percpu)
+	if (mrst_timer_options == MRST_TIMER_APBT_ONLY)
+		return;
+	if ((mrst_timer_options == MRST_TIMER_LAPIC_APBT)
+		|| (mrst_cpu_chip == MRST_CPU_CHIP_PENWELL))
 		setup_boot_APIC_clock();
 };
 
+enum cpuid_regs {
+	CR_EAX = 0,

arch/x86/include/asm/mrst.h void __init mrst_rtc_init(void) · line 298

Quoted 10 lines

arch/x86/include/asm/mrst.h +10 -0

+	CR_ECX,
+	CR_EDX,
+	CR_EBX
+};
+
+int mrst_identify_cpu(void)
+{
+	u32 regs[4];
+
+	cpuid(1, &regs[CR_EAX], &regs[CR_EBX], &regs[CR_ECX], &regs[CR_EDX]);

Yikes. From which Intel cookbook is this ?

Aside of that we have that info in boot_cpu_info already, don't we ? So there is neither a requirement for the extra cpuid call nor for the extra mrst_cpu_chip id magic.

arch/x86/include/asm/mrst.h void __init mrst_rtc_init(void) · line 308

Quoted 10 lines

arch/x86/include/asm/mrst.h +8 -0

+	if ((regs[CR_EAX] & PENWELL_FAMILY) == PENWELL_FAMILY)
+		mrst_cpu_chip = MRST_CPU_CHIP_PENWELL;
+	else
+		mrst_cpu_chip = MRST_CPU_CHIP_LINCROFT;
 
 
+	pr_debug("cpuid result %x\n", regs[CR_EAX]);
+	pr_info("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");

Are we going to add one of those for each new family ? This is really redundant bloat with no value.

Quoted 16 lines

arch/x86/include/asm/mrst.h +5 -0

+	return mrst_cpu_chip;
+}
+
 /*
  * Moorestown specific x86_init function overrides and early setup
  * calls.

@@ -237,4 +330,6 @@ void __init x86_mrst_early_setup(void)
 	x86_init.pci.fixup_irqs = x86_init_noop;
 
 	legacy_pic = &null_legacy_pic;
+
+	mrst_identify_cpu();
 }

Signature · 1 line

1.6.3.3

From: Feng Tang <[email protected]>

Some wall clock devices use MMIO based HW register, this new function will give them a chance to do some initialization work before their get/set_time service get called.

Signed-off-by Feng Tang <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 arch/x86/include/asm/x86_init.h |    2 ++
 arch/x86/kernel/setup.c         |    2 ++
 arch/x86/kernel/x86_init.c      |    2 ++
 3 files changed, 6 insertions(+), 0 deletions(-)

arch/x86/include/asm/x86_init.h +0 -0

index 519b543..be027a8 100644

arch/x86/include/asm/x86_init.h +1 -0

  * @get_wallclock:		get time from HW clock like RTC etc.
  * @set_wallclock:		set time back to HW clock
  * @is_untracked_pat_range	exclude from PAT logic

@@ -145,6 +146,7 @@ struct x86_cpuinit_ops {
  */
 struct x86_platform_ops {
 	unsigned long (*calibrate_tsc)(void);
+	void (*wallclock_init)(void);
 	unsigned long (*get_wallclock)(void);
 	int (*set_wallclock)(unsigned long nowtime);
 	void (*iommu_shutdown)(void);

arch/x86/kernel/setup.c +2 -0

index c4851ef..d001d8c 100644
@@ -1050,6 +1050,8 @@ void __init setup_arch(char **cmdline_p)
 #endif
 	x86_init.oem.banner();
 
+	x86_platform.wallclock_init();
+
 	mcheck_init();
 }
 

arch/x86/kernel/x86_init.c +0 -0

index 61a1e8c..ee00d76 100644

arch/x86/kernel/vrtc.c line 79

Quoted 3 lines · Jacob Pan

arch/x86/kernel/vrtc.c new file +3 -0

+	printk(KERN_INFO "vRTC: sec: %d min: %d hour: %d day: %d "
+	"mon: %d year: %d\n", sec, min, hour, mday, mon, year);
+

Even though many of the rtc drivers print this way, it seems a very backwards way of presenting time to me.

Quoted 5 lines · On Fri, 07 May 2010 11:51:06 -0700 Joe Perches <[email protected]> wrote:

arch/x86/kernel/vrtc.c line 78

Quoted 2 lines · Jacob Pan

arch/x86/kernel/vrtc.c new file +2 -0

+	printk(KERN_INFO "vRTC: sec: %d min: %d hour: %d day: %d "
+	"mon: %d year: %d\n", sec, min, hour, mday, mon, year);

Even though many of the rtc drivers print this way, it seems a very backwards way of presenting time to me.

Consistency is really more important here IMHO - lots of drivers have set an existing policy.

Quoted 9 lines · Alan Cox

Quoted 5 lines · On Fri, 07 May 2010 11:51:06 -0700 Joe Perches <[email protected]> wrote:

arch/x86/kernel/vrtc.c line 78

Quoted 2 lines · Jacob Pan

arch/x86/kernel/vrtc.c new file +2 -0

+	printk(KERN_INFO "vRTC: sec: %d min: %d hour: %d day: %d "
+	"mon: %d year: %d\n", sec, min, hour, mday, mon, year);

Even though many of the rtc drivers print this way, it seems a very backwards way of presenting time to me.

Consistency is really more important here IMHO - lots of drivers have set an existing policy.

(added Alessandro Zummo to cc's)

look at drivers/rtc.

All of them seem to use a templated copy/paste dev_dbg, which seems to point to a use for a possible common rtc_util.c or some such where this could be standardized.

Is there somewhere else this style is used?

Quoted 1 line

Probably the right thing to do is to (a) move all this printing to common code; (b) change to ISO 8601 format.

-hpa

Thank you all for the comments. please review this follow-on patch.

- Feng

Quoted 4 lines · Feng Tang

Thank you all for the comments. please review this follow-on patch.

- Feng

That doesn't move it to common code, though. I'd rather see existing common style used, then merged (centralized) and *then* the style corrected.

-hpa

arch/x86/include/asm/vrtc.h new file +3 -0

@@ -0,0 +1,27 @@
+#ifndef _MRST_VRTC_H
+#define _MRST_VRTC_H
+

arch/x86/include/asm/vrtc.h new file +6 -0

+static inline unsigned char vrtc_cmos_read(unsigned char reg)
+{
+	return 0xff;
+}
+
 
 /**
  * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,

@@ -268,7 +269,24 @@ void __init mrst_time_init(void)
 
 void __init mrst_rtc_init(void)
 {
+	unsigned long rtc_paddr;

arch/x86/include/asm/vrtc.h new file +7 -0

+
+static unsigned char __iomem *vrtc_virt_base;
+
+void vrtc_set_base(void __iomem *base)
+{
+	vrtc_virt_base = base;
+}

arch/x86/include/asm/vrtc.h new file +27 -0

+	retval = __raw_readb(vrtc_virt_base + (reg << 2));
+	lock_cmos_suffix(reg);
+	return retval;
+}
+EXPORT_SYMBOL(vrtc_cmos_read);
+
+void vrtc_cmos_write(unsigned char val, unsigned char reg)
+{
+	if (reg > 0xd || !vrtc_virt_base)
+		return;
+
+	lock_cmos_prefix(reg);
+	__raw_writeb(val, vrtc_virt_base + (reg << 2));
+	lock_cmos_suffix(reg);
+}
+EXPORT_SYMBOL(vrtc_cmos_write);
+
+unsigned long vrtc_get_time(void)
+{
+	u8 sec, min, hour, mday, mon;
+	u32 year;
+
+	while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP))
+		cpu_relax();
+
+	sec = vrtc_cmos_read(RTC_SECONDS);
+	min = vrtc_cmos_read(RTC_MINUTES);

arch/x86/include/asm/vrtc.h line 13

Quoted 6 lines

arch/x86/include/asm/vrtc.h new file +6 -0

+extern int vrtc_set_mmss(unsigned long nowtime);
+extern void vrtc_set_base(void __iomem *base);
+
+#define MRST_VRTC_PGOFFSET  (0xc00)
+
+#else

Errm. That's a MRST specific header and nothing outside of MRST is using it. So why the #ifdef CONFIG_X86_MRST and the inline functions in the #else path ?

Quoted 8 lines

void __init mrst_rtc_init(void)
{

arch/x86/kernel/mrst.c +5 -0

+	unsigned long rtc_paddr;
+	void __iomem *virt_base;
+
 	sfi_table_parse(SFI_SIG_MRTC, NULL, NULL, sfi_parse_mrtc);
+	if (!sfi_mrtc_num)
+		return;

arch/x86/kernel/mrst.c void __init mrst_time_init(void) · line 282

Quoted 5 lines

arch/x86/kernel/mrst.c +5 -0

+
+	rtc_paddr = sfi_mrtc_array[0].phys_addr;
+
+	/* vRTC's register address may not be page aligned */
+	set_fixmap_nocache(FIX_LNW_VRTC, rtc_paddr);

Why do we need a fixmap for that ? There is no need to setup RTC that early. The first call is from timekeeping_init()

Also this RTC init code should be in vrtc.c

Quoted 13 lines

arch/x86/kernel/vrtc.c new file +12 -0

+
+static unsigned char __iomem *vrtc_virt_base;
+
+void vrtc_set_base(void __iomem *base)
+{
+	vrtc_virt_base = base;
+}
+
+unsigned char vrtc_cmos_read(unsigned char reg)
+{
+	unsigned char retval;
+

arch/x86/kernel/vrtc.c line 41

Quoted 5 lines

arch/x86/kernel/vrtc.c new file +5 -0

+	/* vRTC's registers range from 0x0 to 0xD */
+	if (reg > 0xd || !vrtc_virt_base)
+		return 0xff;
+
+	lock_cmos_prefix(reg);

This lock_cmos magic should just die. I have no idea why something wants or wanted to access the RTC from an NMI.

Quoted 6 lines

arch/x86/kernel/vrtc.c new file +5 -0

+	/* vRTC YEAR reg contains the offset to 1960 */
+	year += 1960;
+
+	printk(KERN_INFO "vRTC: sec: %d min: %d hour: %d day: %d "
+		"mon: %d year: %d\n", sec, min, hour, mday, mon,

year);

Please remove the debug noise

Quoted 8 lines

Quoted 8 lines

void __init mrst_rtc_init(void)
{

arch/x86/kernel/mrst.c +5 -0

+	unsigned long rtc_paddr;
+	void __iomem *virt_base;
+
 	sfi_table_parse(SFI_SIG_MRTC, NULL, NULL, sfi_parse_mrtc);
+	if (!sfi_mrtc_num)
+		return;

Quoted 8 lines

arch/x86/kernel/mrst.c void __init mrst_time_init(void) · line 282

Quoted 5 lines

arch/x86/kernel/mrst.c +5 -0

+
+	rtc_paddr = sfi_mrtc_array[0].phys_addr;
+
+	/* vRTC's register address may not be page aligned */
+	set_fixmap_nocache(FIX_LNW_VRTC, rtc_paddr);

Why do we need a fixmap for that ? There is no need to setup RTC that early. The first call is from timekeeping_init()

Actually when to init the vrtc register is a big problem for me, vrtc need be inited before timekeeping_init(), and I thought better to put it somewhere in setup_arch(), as it is architecture specific, and ioremap is not working at that time. Also that's the reason I created a new wallclock_init func for x86_platforms, I could not find a better way to do the vrtc init.

Quoted 1 line

Also this RTC init code should be in vrtc.c

I agree I should move this init code to vrtc.c, but still think it should be called in the setup_arch() than in start_kernel()

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	lock_cmos_prefix(reg);

This lock_cmos magic should just die. I have no idea why something wants or wanted to access the RTC from an NMI.

I will try to reuse the rtc_lock defined in rtc.c whose get/set_time service won't be called with vrtc's at the same time.

Quoted 7 lines

Actually when to init the vrtc register is a big problem for me, vrtc need be inited before timekeeping_init(), and I thought better to put it somewhere in setup_arch(), as it is architecture specific, and ioremap is not working at that time. Also that's the reason I created a new wallclock_init func for x86_platforms, I could not find a better way to do the vrtc init.

There is no particular reason why we need to read it in timekeeping_init(). Nothing in the kernel needs the correct wall time at that point. So we can safely move the setting of xtime to rtc wall clock time to a separate timekeeping_late_init() function.

John ???

Quoted 7 lines

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	lock_cmos_prefix(reg);

This lock_cmos magic should just die. I have no idea why
something wants or wanted to access the RTC from an NMI.

I will try to reuse the rtc_lock defined in rtc.c whose get/set_time service won't be called with vrtc's at the same time.

Please don't create artifical dependencies. Use a separate vrtc_lock to serialize the access to vrtc.

Thanks,

tglx

Hi Thomas,

Quoted 13 lines · On Mon, 17 May 2010 17:15:55 +0800 Thomas Gleixner <[email protected]> wrote:

Quoted 7 lines

Actually when to init the vrtc register is a big problem for me, vrtc need be inited before timekeeping_init(), and I thought better to put it somewhere in setup_arch(), as it is architecture specific, and ioremap is not working at that time. Also that's the reason I created a new wallclock_init func for x86_platforms, I could not find a better way to do the vrtc init.

There is no particular reason why we need to read it in timekeeping_init(). Nothing in the kernel needs the correct wall time at that point. So we can safely move the setting of xtime to rtc wall clock time to a separate timekeeping_late_init() function.

Quoted 2 lines

John ???

Yeah, good suggestion, if xtime init is moved to a later time in kernel init flow, then vrtc's init function can be set a arch_initcall()

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	lock_cmos_prefix(reg);

This lock_cmos magic should just die. I have no idea why
something wants or wanted to access the RTC from an NMI.

Quoted 5 lines

Quoted 2 lines

I will try to reuse the rtc_lock defined in rtc.c whose get/set_time service won't be called with vrtc's at the same time.

Please don't create artifical dependencies. Use a separate vrtc_lock to serialize the access to vrtc.

I just checked the code, when wall clock's get/set_time service is called, it is always protected by rtc_lock(code in arch/x86/kernel/rtc.c), then no need to add the lock for each individual register read/write operation.

There is no particular reason why we need to read it in timekeeping_init(). Nothing in the kernel needs the correct wall time at that point. So we can safely move the setting of xtime to rtc wall clock time to a separate timekeeping_late_init() function.

John ???

Quoted 7 lines · Thomas Gleixner

There is no particular reason why we need to read it in timekeeping_init(). Nothing in the kernel needs the correct wall time at that point. So we can safely move the setting of xtime to rtc wall clock time to a separate timekeeping_late_init() function.

Quoted 1 line

John ???

No big objections here. Still would like to keep the amount of time that the kernel is up without xtime being initialized to a minimum. However

the generic RTC code already have this issue since some of them require interrupts to be enabled to do a read, so pushing it off into a _late_init() function is probably just a short term fix until we figure out how to get the generic RTC code working better with the timekeeping code.

Does the delayed init required by vrtc cause any trouble with suspend/resume?

From 2451f205e29be57b30f0c50759c1b05a67bb97d1 Mon Sep 17 00:00:00 2001

From 9ba7685bfdcaa2a5c40eade388902c478bc46071 Mon Sep 17 00:00:00 2001

From 14760d3bf6f77a17400c30258e365c06cbc36661 Mon Sep 17 00:00:00 2001