[PATCH] x86, tsc: Add missing Baytrail frequency to the table

[PATCH] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means that
the CPU reference clock runs at 83.3MHz. Without this we crash a bit later
with backtrace looking like:

 divide error: 0000 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W    3.13.0+ #47
 task: ffff880075508000 ti: ffff880075506000 task.ti: ffff880075506000
 RIP: 0010:[<ffffffff810aec14>]  [<ffffffff810aec14>] clockevents_config.part.3+0x24/0xa0
 RSP: 0000:ffff880075507e58  EFLAGS: 00010246
 RAX: ffffffffffffffff RBX: ffff880079c0cd80 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff
 RBP: ffff880075507e70 R08: 0000000000000001 R09: 00000000000000be
 R10: 00000000000000bd R11: 0000000000000003 R12: 000000000000b008
 R13: 0000000000000008 R14: 000000000000b010 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff880079c00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: ffff880079fff000 CR3: 0000000001c0b000 CR4: 00000000001006f0
 Stack:
  ffff880079c0cd80 000000000000b008 0000000000000008 ffff880075507e88
  ffffffff810aecb0 ffff880079c0cd80 ffff880075507e98 ffffffff81030168
  ffff880075507ed8 ffffffff81d1104f 00000000000000c3 0000000000000000
 Call Trace:
  [<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
  [<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
  [<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
  [<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
  [<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
  [<ffffffff8177c910>] ? rest_init+0x90/0x90
  [<ffffffff8177c91e>] kernel_init+0xe/0x120
  [<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
  [<ffffffff8177c910>] ? rest_init+0x90/0x90

This is due the fact that the returned zero TSC value is propagated further
to clockevents code resulting division by zero.

Fix this by adding the missing frequency to freq_desc_tables for Baytrail.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Bin Gao <bin.gao@intel.com>
---
 arch/x86/kernel/tsc_msr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 8b5434f4389f..10a278661fe1 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -53,7 +53,7 @@ static struct freq_desc freq_desc_tables[] = {
 	/* TNG */
 	{ 6, 0x4a, 1, { 0, FREQ_100, FREQ_133, 0, 0, 0, 0, 0 } },
 	/* VLV2 */
-	{ 6, 0x37, 1, { 0, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
+	{ 6, 0x37, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
 	/* ANN */
 	{ 6, 0x5a, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_100, 0, 0, 0, 0 } },
 };
-- 
1.8.5.2

Re: [PATCH] x86, tsc: Add missing Baytrail frequency to the table

[One Thousand Gnomes]

On Thu, 23 Jan 2014 13:04:03 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means that
> the CPU reference clock runs at 83.3MHz. Without this we crash a bit later
> with backtrace looking like:

Would it not be wise to also make the code robust against future failures
of this kind and at least bitch rather than divide by 0 ?

Alan

Re: [PATCH] x86, tsc: Add missing Baytrail frequency to the table

[H. Peter Anvin]

On 01/23/2014 07:09 AM, One Thousand Gnomes wrote:
> On Thu, 23 Jan 2014 13:04:03 +0200
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
>> Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means that
>> the CPU reference clock runs at 83.3MHz. Without this we crash a bit later
>> with backtrace looking like:
> 
> Would it not be wise to also make the code robust against future failures
> of this kind and at least bitch rather than divide by 0 ?
> 

Chicken!  ;)

	-hpa

Re: [PATCH] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

On Thu, Jan 23, 2014 at 03:09:12PM +0000, One Thousand Gnomes wrote:
> On Thu, 23 Jan 2014 13:04:03 +0200
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means that
> > the CPU reference clock runs at 83.3MHz. Without this we crash a bit later
> > with backtrace looking like:
> 
> Would it not be wise to also make the code robust against future failures
> of this kind and at least bitch rather than divide by 0 ?

Yes, it makes sense. I wasn't sure how to do that properly, though.

The following patch seems to work in that case.

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index a3acbac2ee72..1e54deb56b33 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
 	local_irq_save(flags);
 	i = try_msr_calibrate_tsc(&fast_calibrate);
 	local_irq_restore(flags);
-	if (i >= 0) {
-		if (i == 0)
-			pr_warn("Fast TSC calibration using MSR failed\n");
+	if (i > 0) {
 		return fast_calibrate;
+	} else if (i == 0) {
+		pr_warn("Fast TSC calibration using MSR failed\n");
+		/* Continue with the normal calibration */
 	}
 
 	local_irq_save(flags);

[PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means that
the CPU reference clock runs at 83.3MHz. Without this we crash a bit later
with backtrace looking like:

 divide error: 0000 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W    3.13.0+ #47
 task: ffff880075508000 ti: ffff880075506000 task.ti: ffff880075506000
 RIP: 0010:[<ffffffff810aec14>]  [<ffffffff810aec14>] clockevents_config.part.3+0x24/0xa0
 RSP: 0000:ffff880075507e58  EFLAGS: 00010246
 RAX: ffffffffffffffff RBX: ffff880079c0cd80 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff
 RBP: ffff880075507e70 R08: 0000000000000001 R09: 00000000000000be
 R10: 00000000000000bd R11: 0000000000000003 R12: 000000000000b008
 R13: 0000000000000008 R14: 000000000000b010 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff880079c00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: ffff880079fff000 CR3: 0000000001c0b000 CR4: 00000000001006f0
 Stack:
  ffff880079c0cd80 000000000000b008 0000000000000008 ffff880075507e88
  ffffffff810aecb0 ffff880079c0cd80 ffff880075507e98 ffffffff81030168
  ffff880075507ed8 ffffffff81d1104f 00000000000000c3 0000000000000000
 Call Trace:
  [<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
  [<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
  [<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
  [<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
  [<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
  [<ffffffff8177c910>] ? rest_init+0x90/0x90
  [<ffffffff8177c91e>] kernel_init+0xe/0x120
  [<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
  [<ffffffff8177c910>] ? rest_init+0x90/0x90

This is due the fact that the returned zero TSC value is propagated further
to clockevents code resulting division by zero.

Fix this by adding the missing frequency to freq_desc_tables for Baytrail.

In addition make sure that this won't happen again by checking the return
value of try_msr_calibrate_tsc() and in case of zero or error we fallback
to the non-MSR TSC calibration.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Bin Gao <bin.gao@intel.com>
---
v2: Added fallback to non-TSC calibration.

 arch/x86/kernel/tsc.c     | 7 ++++---
 arch/x86/kernel/tsc_msr.c | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index a3acbac2ee72..1e54deb56b33 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
 	local_irq_save(flags);
 	i = try_msr_calibrate_tsc(&fast_calibrate);
 	local_irq_restore(flags);
-	if (i >= 0) {
-		if (i == 0)
-			pr_warn("Fast TSC calibration using MSR failed\n");
+	if (i > 0) {
 		return fast_calibrate;
+	} else if (i == 0) {
+		pr_warn("Fast TSC calibration using MSR failed\n");
+		/* Continue with the normal calibration */
 	}
 
 	local_irq_save(flags);
diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 8b5434f4389f..10a278661fe1 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -53,7 +53,7 @@ static struct freq_desc freq_desc_tables[] = {
 	/* TNG */
 	{ 6, 0x4a, 1, { 0, FREQ_100, FREQ_133, 0, 0, 0, 0, 0 } },
 	/* VLV2 */
-	{ 6, 0x37, 1, { 0, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
+	{ 6, 0x37, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
 	/* ANN */
 	{ 6, 0x5a, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_100, 0, 0, 0, 0 } },
 };
-- 
1.8.5.2

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Bin Gao]

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index a3acbac2ee72$
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
>       local_irq_save(flags);
>       i = try_msr_calibrate_tsc(&fast_calibrate);
>       local_irq_restore(flags);
> -     if (i >= 0) {
> -             if (i == 0)
> -                     pr_warn("Fast TSC calibration using MSR failed\n");
> +     if (i > 0) {
>               return fast_calibrate;
> +     } else if (i == 0) {
> +             pr_warn("Fast TSC calibration using MSR failed\n");
> +             /* Continue with the normal calibration */
>       }
>
>       local_irq_save(flags);

The original design is to avoid trying PIC because touching PIC on
a non-PIC SoC will simply cause system hang. Returning 0 will cause
kernel to mark TSC unstable(rating 0). Since PM timer and HPET are
typically not available on non-PIC SoC, kernal falls back to jiffies
as clock source. Developers will start looking into the TSC clibrating
failure once they notice the jiffies clock source.

>[<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
>[<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
>[<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
>[<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
>[<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
>[<ffffffff8177c910>] ? rest_init+0x90/0x90
>[<ffffffff8177c91e>] kernel_init+0xe/0x120
>[<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
>[<ffffffff8177c910>] ? rest_init+0x90/0x90

This is because kernel is trying to use tsc-deadline mode for lapic
timer. I think the right fix is to unset X86_FEATURE_TSC_DEADLINE_TIMER
when try_msr_calibrate_tsc() returns 0.

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

On Sat, Jan 25, 2014 at 01:09:23AM -0800, Bin Gao wrote:
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index a3acbac2ee72$
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
> >       local_irq_save(flags);
> >       i = try_msr_calibrate_tsc(&fast_calibrate);
> >       local_irq_restore(flags);
> > -     if (i >= 0) {
> > -             if (i == 0)
> > -                     pr_warn("Fast TSC calibration using MSR failed\n");
> > +     if (i > 0) {
> >               return fast_calibrate;
> > +     } else if (i == 0) {
> > +             pr_warn("Fast TSC calibration using MSR failed\n");
> > +             /* Continue with the normal calibration */
> >       }
> >
> >       local_irq_save(flags);
> 
> The original design is to avoid trying PIC because touching PIC on
> a non-PIC SoC will simply cause system hang. Returning 0 will cause
> kernel to mark TSC unstable(rating 0). Since PM timer and HPET are
> typically not available on non-PIC SoC, kernal falls back to jiffies
> as clock source. Developers will start looking into the TSC clibrating
> failure once they notice the jiffies clock source.
> 
> >[<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
> >[<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
> >[<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
> >[<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
> >[<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
> >[<ffffffff8177c910>] ? rest_init+0x90/0x90
> >[<ffffffff8177c91e>] kernel_init+0xe/0x120
> >[<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
> >[<ffffffff8177c910>] ? rest_init+0x90/0x90
> 
> This is because kernel is trying to use tsc-deadline mode for lapic
> timer. I think the right fix is to unset X86_FEATURE_TSC_DEADLINE_TIMER
> when try_msr_calibrate_tsc() returns 0.

You are right, that works too:

[    0.000000] Resolved frequency ID: 0, frequency: 0 KHz
[    0.000000] tsc: Fast TSC calibration using MSR failed
[    0.000000] tsc: Fast TSC calibration using PIT
[    0.000000] tsc: Detected 1833.334 MHz processor

If there are no objections, I can send a new version of the patch with this
fixed.

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

On Mon, Jan 27, 2014 at 12:20:02PM +0200, Mika Westerberg wrote:
> On Sat, Jan 25, 2014 at 01:09:23AM -0800, Bin Gao wrote:
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index a3acbac2ee72$
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
> > >       local_irq_save(flags);
> > >       i = try_msr_calibrate_tsc(&fast_calibrate);
> > >       local_irq_restore(flags);
> > > -     if (i >= 0) {
> > > -             if (i == 0)
> > > -                     pr_warn("Fast TSC calibration using MSR failed\n");
> > > +     if (i > 0) {
> > >               return fast_calibrate;
> > > +     } else if (i == 0) {
> > > +             pr_warn("Fast TSC calibration using MSR failed\n");
> > > +             /* Continue with the normal calibration */
> > >       }
> > >
> > >       local_irq_save(flags);
> > 
> > The original design is to avoid trying PIC because touching PIC on
> > a non-PIC SoC will simply cause system hang. Returning 0 will cause
> > kernel to mark TSC unstable(rating 0). Since PM timer and HPET are
> > typically not available on non-PIC SoC, kernal falls back to jiffies
> > as clock source. Developers will start looking into the TSC clibrating
> > failure once they notice the jiffies clock source.
> > 
> > >[<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
> > >[<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
> > >[<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
> > >[<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
> > >[<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
> > >[<ffffffff8177c910>] ? rest_init+0x90/0x90
> > >[<ffffffff8177c91e>] kernel_init+0xe/0x120
> > >[<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
> > >[<ffffffff8177c910>] ? rest_init+0x90/0x90
> > 
> > This is because kernel is trying to use tsc-deadline mode for lapic
> > timer. I think the right fix is to unset X86_FEATURE_TSC_DEADLINE_TIMER
> > when try_msr_calibrate_tsc() returns 0.
> 
> You are right, that works too:
> 
> [    0.000000] Resolved frequency ID: 0, frequency: 0 KHz
> [    0.000000] tsc: Fast TSC calibration using MSR failed
> [    0.000000] tsc: Fast TSC calibration using PIT
> [    0.000000] tsc: Detected 1833.334 MHz processor
> 
> If there are no objections, I can send a new version of the patch with this
> fixed.

Actually it does not work. I had the fallback path still in the patch when
I tried.

If I clear X86_FEATURE_TSC_DEADLINE_TIMER like you suggested, I get this:

[    0.093333] ------------[ cut here ]------------
[    0.096666] WARNING: CPU: 0 PID: 1 at arch/x86/kernel/apic/apic.c:1397 setup_local_APIC+0x27c/0x330()
[    0.099999] Modules linked in:
[    0.103333] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0+ #115
[    0.106666]  0000000000000009 ffff880075507e40 ffffffff817953a1 0000000000000000
[    0.113333]  ffff880075507e78 ffffffff810473ed 0000000000000000 00000000000000f0
[    0.123333]  0000000000000000 00000000ffffffff 0000000000000001 ffff880075507e88
[    0.129999] Call Trace:
[    0.133333]  [<ffffffff817953a1>] dump_stack+0x4f/0x7b
[    0.136666]  [<ffffffff810473ed>] warn_slowpath_common+0x7d/0xa0
[    0.139999]  [<ffffffff810474ca>] warn_slowpath_null+0x1a/0x20
[    0.143333]  [<ffffffff8103102c>] setup_local_APIC+0x27c/0x330
[    0.146666]  [<ffffffff81d1053a>] native_smp_prepare_cpus+0x32e/0x3f0
[    0.149999]  [<ffffffff81d03ef0>] kernel_init_freeable+0xc3/0x205
[    0.153333]  [<ffffffff8178d100>] ? rest_init+0x90/0x90
[    0.156666]  [<ffffffff8178d10e>] kernel_init+0xe/0x120
[    0.159999]  [<ffffffff8179e4ec>] ret_from_fork+0x7c/0xb0
[    0.163333]  [<ffffffff8178d100>] ? rest_init+0x90/0x90
[    0.166666] ---[ end trace 0e1b24064a81bb0a ]---
[    0.173333] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
[    0.209999] smpboot: CPU0: Intel(R) Celeron(R) CPU  N2930  @ 1.83GHz (fam: 06, model: 37, stepping: 08)
[    0.216666] divide error: 0000 [#1] PREEMPT SMP 
[    0.216666] Modules linked in:
[    0.216666] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W    3.13.0+ #115
[    0.216666] task: ffff880075508000 ti: ffff880075506000 task.ti: ffff880075506000
[    0.216666] RIP: 0010:[<ffffffff810aebe4>]  [<ffffffff810aebe4>] clockevents_config.part.3+0x24/0xa0
[    0.216666] RSP: 0000:ffff880075507e58  EFLAGS: 00010246
[    0.216666] RAX: ffffffffffffffff RBX: ffff880079c0cd80 RCX: 0000000000000000
[    0.216666] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff
[    0.216666] RBP: ffff880075507e70 R08: 0000000000000001 R09: 00000000000000be
[    0.216666] R10: 00000000000000bd R11: 0000000000000003 R12: 000000000000b008
[    0.216666] R13: 0000000000000008 R14: 000000000000b010 R15: 0000000000000000
[    0.216666] FS:  0000000000000000(0000) GS:ffff880079c00000(0000) knlGS:0000000000000000
[    0.216666] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    0.216666] CR2: ffff880079fff000 CR3: 0000000001c0b000 CR4: 00000000001006f0
[    0.216666] Stack:
[    0.216666]  ffff880079c0cd80 000000000000b008 0000000000000008 ffff880075507e88
[    0.216666]  ffffffff810aec80 ffff880079c0cd80 ffff880075507e98 ffffffff81030168
[    0.216666]  ffff880075507ed8 ffffffff81d1205a 00000000000000c5 0000000000000000
[    0.216666] Call Trace:
[    0.216666]  [<ffffffff810aec80>] clockevents_config_and_register+0x20/0x30
[    0.216666]  [<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
[    0.216666]  [<ffffffff81d1205a>] setup_boot_APIC_clock+0x4cc/0x4d8
[    0.216666]  [<ffffffff81d105e9>] native_smp_prepare_cpus+0x3dd/0x3f0
[    0.216666]  [<ffffffff81d03ef0>] kernel_init_freeable+0xc3/0x205
[    0.216666]  [<ffffffff8178d100>] ? rest_init+0x90/0x90
[    0.216666]  [<ffffffff8178d10e>] kernel_init+0xe/0x120
[    0.216666]  [<ffffffff8179e4ec>] ret_from_fork+0x7c/0xb0
[    0.216666]  [<ffffffff8178d100>] ? rest_init+0x90/0x90
[    0.216666] Code: c3 66 0f 1f 44 00 00 0f 1f 44 00 00 55 31 d2 89 f1 48 89 e5 41 55 89 f6 41 54 41 b8 01 00 00 00 53 48 89 fb 48 8b 7f 70 48 89 f8 <48> f7 f6 48 85 c0 74 0b 48 3d 58 02 00 00 41 89 c0 77 51 4c 8d 
[    0.216666] RIP  [<ffffffff810aebe4>] clockevents_config.part.3+0x24/0xa0
[    0.216666]  RSP <ffff880075507e58>
[    0.219999] ---[ end trace 0e1b24064a81bb0b ]---

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Bin Gao]

On Tue, Jan 28, 2014 at 02:04:07PM +0200, Mika Westerberg wrote:
> Actually it does not work. I had the fallback path still in the patch when
> I tried.

Let's merge the ptach v2 then(continue on msr calibration retuning 0).
All supported SoCs now have the correct frequency ID coding
so continuing on non-PIC SoCs has very very small possibility.

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

On Wed, Jan 29, 2014 at 01:47:54PM -0800, Bin Gao wrote:
> On Tue, Jan 28, 2014 at 02:04:07PM +0200, Mika Westerberg wrote:
> > Actually it does not work. I had the fallback path still in the patch when
> > I tried.
> 
> Let's merge the ptach v2 then(continue on msr calibration retuning 0).
> All supported SoCs now have the correct frequency ID coding
> so continuing on non-PIC SoCs has very very small possibility.

x86 maintainers, any comments on this patch (v2)? Without the patch some
Baytrail systems fail to boot 3.14-rc1.

Re: [PATCH v2] x86, tsc: Add missing Baytrail frequency to the table

[Ingo Molnar]

* Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, Jan 29, 2014 at 01:47:54PM -0800, Bin Gao wrote:
> > On Tue, Jan 28, 2014 at 02:04:07PM +0200, Mika Westerberg wrote:
> > > Actually it does not work. I had the fallback path still in the patch when
> > > I tried.
> > 
> > Let's merge the ptach v2 then(continue on msr calibration retuning 0).
> > All supported SoCs now have the correct frequency ID coding
> > so continuing on non-PIC SoCs has very very small possibility.
> 
> x86 maintainers, any comments on this patch (v2)? Without the patch some
> Baytrail systems fail to boot 3.14-rc1.

It should be split in two: first a patch to fix the divide by zero 
bug, second the Baytrail addition.

Thanks,

	Ingo

[PATCH v3 1/2] x86, tsc: Fallback to normal calibration if fast MSR calibration fails

[Mika Westerberg]

If we cannot calibrate TSC via MSR based calibration try_msr_calibrate_tsc()
stores zero to fast_calibrate and returns that to the caller. This value
gets then propagated further to clockevents code resulting division by zero
oops like the one below:

 divide error: 0000 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W    3.13.0+ #47
 task: ffff880075508000 ti: ffff880075506000 task.ti: ffff880075506000
 RIP: 0010:[<ffffffff810aec14>]  [<ffffffff810aec14>] clockevents_config.part.3+0x24/0xa0
 RSP: 0000:ffff880075507e58  EFLAGS: 00010246
 RAX: ffffffffffffffff RBX: ffff880079c0cd80 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffffffffff
 RBP: ffff880075507e70 R08: 0000000000000001 R09: 00000000000000be
 R10: 00000000000000bd R11: 0000000000000003 R12: 000000000000b008
 R13: 0000000000000008 R14: 000000000000b010 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff880079c00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: ffff880079fff000 CR3: 0000000001c0b000 CR4: 00000000001006f0
 Stack:
  ffff880079c0cd80 000000000000b008 0000000000000008 ffff880075507e88
  ffffffff810aecb0 ffff880079c0cd80 ffff880075507e98 ffffffff81030168
  ffff880075507ed8 ffffffff81d1104f 00000000000000c3 0000000000000000
 Call Trace:
  [<ffffffff810aecb0>] clockevents_config_and_register+0x20/0x30
  [<ffffffff81030168>] setup_APIC_timer+0xc8/0xd0
  [<ffffffff81d1104f>] setup_boot_APIC_clock+0x4cc/0x4d8
  [<ffffffff81d0f5de>] native_smp_prepare_cpus+0x3dd/0x3f0
  [<ffffffff81d02ee9>] kernel_init_freeable+0xc3/0x205
  [<ffffffff8177c910>] ? rest_init+0x90/0x90
  [<ffffffff8177c91e>] kernel_init+0xe/0x120
  [<ffffffff8178deec>] ret_from_fork+0x7c/0xb0
  [<ffffffff8177c910>] ? rest_init+0x90/0x90

Prevent this from happening by falling back to normal TSC calibration if MSR
based one fails.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Changes to v2: split the patch into two as suggested by Ingo.

 arch/x86/kernel/tsc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 19e5adb49a27..56facfc54575 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
 	local_irq_save(flags);
 	i = try_msr_calibrate_tsc(&fast_calibrate);
 	local_irq_restore(flags);
-	if (i >= 0) {
-		if (i == 0)
-			pr_warn("Fast TSC calibration using MSR failed\n");
+	if (i > 0) {
 		return fast_calibrate;
+	} else if (i == 0) {
+		pr_warn("Fast TSC calibration using MSR failed\n");
+		/* Continue with the normal calibration */
 	}
 
 	local_irq_save(flags);
-- 
1.8.5.2

[PATCH v3 2/2] x86, tsc: Add missing Baytrail frequency to the table

[Mika Westerberg]

Intel Baytrail is based on Silvermont core so MSR_FSB_FREQ[2:0] == 0 means
that the CPU reference clock runs at 83.3MHz. Add this missing frequency to
the table.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 arch/x86/kernel/tsc_msr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 8b5434f4389f..10a278661fe1 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -53,7 +53,7 @@ static struct freq_desc freq_desc_tables[] = {
 	/* TNG */
 	{ 6, 0x4a, 1, { 0, FREQ_100, FREQ_133, 0, 0, 0, 0, 0 } },
 	/* VLV2 */
-	{ 6, 0x37, 1, { 0, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
+	{ 6, 0x37, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_166, 0, 0, 0, 0 } },
 	/* ANN */
 	{ 6, 0x5a, 1, { FREQ_83, FREQ_100, FREQ_133, FREQ_100, 0, 0, 0, 0 } },
 };
-- 
1.8.5.2

Re: [PATCH v3 1/2] x86, tsc: Fallback to normal calibration if fast MSR calibration fails

[Thomas Gleixner]

On Thu, 6 Feb 2014, Mika Westerberg wrote:
>  arch/x86/kernel/tsc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 19e5adb49a27..56facfc54575 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
>  	local_irq_save(flags);
>  	i = try_msr_calibrate_tsc(&fast_calibrate);
>  	local_irq_restore(flags);
> -	if (i >= 0) {
> -		if (i == 0)
> -			pr_warn("Fast TSC calibration using MSR failed\n");
> +	if (i > 0) {
>  		return fast_calibrate;
> +	} else if (i == 0) {
> +		pr_warn("Fast TSC calibration using MSR failed\n");
> +		/* Continue with the normal calibration */
>  	}

Bah. Can we please fix that proper. The whole 0,1,-1 return value from
try_msr_calibrate_tsc() is just horrible. Why not doing the obvious?

Thanks,

	tglx
---
Index: tip/arch/x86/include/asm/tsc.h
===================================================================
--- tip.orig/arch/x86/include/asm/tsc.h
+++ tip/arch/x86/include/asm/tsc.h
@@ -66,6 +66,6 @@ extern void tsc_save_sched_clock_state(v
 extern void tsc_restore_sched_clock_state(void);
 
 /* MSR based TSC calibration for Intel Atom SoC platforms */
-int try_msr_calibrate_tsc(unsigned long *fast_calibrate);
+unsigned long try_msr_calibrate_tsc(void);
 
 #endif /* _ASM_X86_TSC_H */
Index: tip/arch/x86/kernel/tsc.c
===================================================================
--- tip.orig/arch/x86/kernel/tsc.c
+++ tip/arch/x86/kernel/tsc.c
@@ -653,13 +653,10 @@ unsigned long native_calibrate_tsc(void)
 
 	/* Calibrate TSC using MSR for Intel Atom SoCs */
 	local_irq_save(flags);
-	i = try_msr_calibrate_tsc(&fast_calibrate);
+	fast_calibrate = try_msr_calibrate_tsc();
 	local_irq_restore(flags);
-	if (i >= 0) {
-		if (i == 0)
-			pr_warn("Fast TSC calibration using MSR failed\n");
+	if (fast_calibrate)
 		return fast_calibrate;
-	}
 
 	local_irq_save(flags);
 	fast_calibrate = quick_pit_calibrate();
Index: tip/arch/x86/kernel/tsc_msr.c
===================================================================
--- tip.orig/arch/x86/kernel/tsc_msr.c
+++ tip/arch/x86/kernel/tsc_msr.c
@@ -77,21 +77,18 @@ static int match_cpu(u8 family, u8 model
 
 /*
  * Do MSR calibration only for known/supported CPUs.
- * Return values:
- * -1: CPU is unknown/unsupported for MSR based calibration
- *  0: CPU is known/supported, but calibration failed
- *  1: CPU is known/supported, and calibration succeeded
+ *
+ * Returns the calibration value or 0 if MSR calibration failed.
  */
-int try_msr_calibrate_tsc(unsigned long *fast_calibrate)
+unsigned long try_msr_calibrate_tsc(void)
 {
-	int cpu_index;
 	u32 lo, hi, ratio, freq_id, freq;
+	unsigned long res;
+	int cpu_index;
 
 	cpu_index = match_cpu(boot_cpu_data.x86, boot_cpu_data.x86_model);
 	if (cpu_index < 0)
-		return -1;
-
-	*fast_calibrate = 0;
+		return 0;
 
 	if (freq_desc_tables[cpu_index].msr_plat) {
 		rdmsr(MSR_PLATFORM_INFO, lo, hi);
@@ -103,7 +100,7 @@ int try_msr_calibrate_tsc(unsigned long
 	pr_info("Maximum core-clock to bus-clock ratio: 0x%x\n", ratio);
 
 	if (!ratio)
-		return 0;
+		goto fail;
 
 	/* Get FSB FREQ ID */
 	rdmsr(MSR_FSB_FREQ, lo, hi);
@@ -112,16 +109,19 @@ int try_msr_calibrate_tsc(unsigned long
 	pr_info("Resolved frequency ID: %u, frequency: %u KHz\n",
 				freq_id, freq);
 	if (!freq)
-		return 0;
+		goto fail;
 
 	/* TSC frequency = maximum resolved freq * maximum resolved bus ratio */
-	*fast_calibrate = freq * ratio;
-	pr_info("TSC runs at %lu KHz\n", *fast_calibrate);
+	res = freq * ratio;
+	pr_info("TSC runs at %lu KHz\n", res);
 
 #ifdef CONFIG_X86_LOCAL_APIC
 	lapic_timer_frequency = (freq * 1000) / HZ;
 	pr_info("lapic_timer_frequency = %d\n", lapic_timer_frequency);
 #endif
+	return res;
 
-	return 1;
+fail:
+	pr_warn("Fast TSC calibration using MSR failed\n");
+	return 0;
 }

Re: [PATCH v3 1/2] x86, tsc: Fallback to normal calibration if fast MSR calibration fails

[Mika Westerberg]

On Thu, Feb 06, 2014 at 03:34:18PM +0100, Thomas Gleixner wrote:
> On Thu, 6 Feb 2014, Mika Westerberg wrote:
> >  arch/x86/kernel/tsc.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 19e5adb49a27..56facfc54575 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
> >  	local_irq_save(flags);
> >  	i = try_msr_calibrate_tsc(&fast_calibrate);
> >  	local_irq_restore(flags);
> > -	if (i >= 0) {
> > -		if (i == 0)
> > -			pr_warn("Fast TSC calibration using MSR failed\n");
> > +	if (i > 0) {
> >  		return fast_calibrate;
> > +	} else if (i == 0) {
> > +		pr_warn("Fast TSC calibration using MSR failed\n");
> > +		/* Continue with the normal calibration */
> >  	}
> 
> Bah. Can we please fix that proper. The whole 0,1,-1 return value from
> try_msr_calibrate_tsc() is just horrible. Why not doing the obvious?

Yes, that looks better and works fine, thanks.

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v3 1/2] x86, tsc: Fallback to normal calibration if fast MSR calibration fails

[Ingo Molnar]

* Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Thu, Feb 06, 2014 at 03:34:18PM +0100, Thomas Gleixner wrote:
> > On Thu, 6 Feb 2014, Mika Westerberg wrote:
> > >  arch/x86/kernel/tsc.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index 19e5adb49a27..56facfc54575 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -655,10 +655,11 @@ unsigned long native_calibrate_tsc(void)
> > >  	local_irq_save(flags);
> > >  	i = try_msr_calibrate_tsc(&fast_calibrate);
> > >  	local_irq_restore(flags);
> > > -	if (i >= 0) {
> > > -		if (i == 0)
> > > -			pr_warn("Fast TSC calibration using MSR failed\n");
> > > +	if (i > 0) {
> > >  		return fast_calibrate;
> > > +	} else if (i == 0) {
> > > +		pr_warn("Fast TSC calibration using MSR failed\n");
> > > +		/* Continue with the normal calibration */
> > >  	}
> > 
> > Bah. Can we please fix that proper. The whole 0,1,-1 return value from
> > try_msr_calibrate_tsc() is just horrible. Why not doing the obvious?
> 
> Yes, that looks better and works fine, thanks.
> 
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Please resend the two updated/fixed patches in a new thread.

Thanks,

	Ingo