Change in coreboot[master]: Rangeley: Fix incorrect BCLK

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Name of user not set (Code Review)

11 Sep 2019 11 Sep '19

7:10 p.m.

hannah.williams@dell.com has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Not all Rangeley SKUs have a fixed 100MHz BCLK. As per BIOS Writer's Guide, BCLK is available in MSR PSB_CLOCK_STS[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz.

Signed-off-by: Hannah Williams hannah.williams@dell.com Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f --- M src/cpu/intel/fsp_model_406dx/acpi.c M src/cpu/intel/fsp_model_406dx/model_406dx.h 2 files changed, 31 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/1

diff --git a/src/cpu/intel/fsp_model_406dx/acpi.c b/src/cpu/intel/fsp_model_406dx/acpi.c index 6672eab..d6177b6 100644 --- a/src/cpu/intel/fsp_model_406dx/acpi.c +++ b/src/cpu/intel/fsp_model_406dx/acpi.c @@ -154,6 +154,33 @@ return (int)power; }

+static int get_core_frequency_mhz(int ratio) +{ + msr_t msr; + unsigned int cpu_bclk_hz[] = { + 83333333, + 100000000, + 133333333, + 116666666 + }; + int core_freq; + int index; + + /* Get BCLK - different SKUs can have different BCLK */ + msr = rdmsr(MSR_PSB_CLOCK_STS); + index = msr.lo & CPU_BCLK_MASK; + if (index >= ARRAY_SIZE(cpu_bclk_hz)) + index = 0; + + printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", + msr.hi, msr.lo, cpu_bclk_hz[index], ratio); + core_freq = (cpu_bclk_hz[index] * ratio) / 1000000; + if ((cpu_bclk_hz[index] * ratio) % 1000000 > 500000) + core_freq++; + printk(BIOS_DEBUG, "core frequency for ratio(%d) %dMHz\n", ratio, core_freq); + return core_freq; +} + static void generate_P_state_entries(int core, int cores_per_package) { int ratio_min, ratio_max, ratio_turbo, ratio_step; @@ -177,7 +204,7 @@ /* Max Non-Turbo Ratio */ ratio_max = (msr.lo >> 8) & 0xff; } - clock_max = ratio_max * RANGELEY_BCLK; + clock_max = get_core_frequency_mhz(ratio_max);

/* Calculate CPU TDP in mW */ msr = rdmsr(MSR_PKG_POWER_SKU_UNIT); @@ -240,7 +267,7 @@

/* Calculate power at this ratio */ power = calculate_power(power_max, ratio_max, ratio); - clock = ratio * RANGELEY_BCLK; + clock = get_core_frequency_mhz(ratio);

acpigen_write_PSS_package( clock, /*MHz*/ diff --git a/src/cpu/intel/fsp_model_406dx/model_406dx.h b/src/cpu/intel/fsp_model_406dx/model_406dx.h index 53a77a9..dcfe914 100644 --- a/src/cpu/intel/fsp_model_406dx/model_406dx.h +++ b/src/cpu/intel/fsp_model_406dx/model_406dx.h @@ -15,8 +15,6 @@ #ifndef _CPU_INTEL_MODEL_406DX_H #define _CPU_INTEL_MODEL_406DX_H

-/* Rangeley bus clock is fixed at 100MHz */ -#define RANGELEY_BCLK 100

#define MSR_FEATURE_CONFIG 0x13c #define MSR_FLEX_RATIO 0x194 @@ -27,6 +25,8 @@

#define MSR_NO_EVICT_MODE 0x2e0 #define MSR_PIC_MSG_CONTROL 0x2e +#define MSR_PSB_CLOCK_STS 0xcd +#define CPU_BCLK_MASK 0x3 #define MSR_PLATFORM_INFO 0xce #define PLATFORM_INFO_SET_TDP (1 << 29) #define MSR_PKG_CST_CONFIG_CONTROL 0xe2

Attachments:

attachment.htm (text/html — 6.7 KB)

Show replies by date

build bot (Jenkins) (Code Review)

11 Sep 11 Sep

7:10 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/1/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/1/src/cpu/intel/fsp_model_406... PS1, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

Name of user not set (Code Review)

7:16 p.m.

Hello Patrick Rudolph, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#2).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/2

build bot (Jenkins) (Code Review)

7:17 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35348/2/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/2/src/cpu/intel/fsp_model_406... PS2, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", trailing whitespace

https://review.coreboot.org/c/coreboot/+/35348/2/src/cpu/intel/fsp_model_406... PS2, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

https://review.coreboot.org/c/coreboot/+/35348/2/src/cpu/intel/fsp_model_406... PS2, Line 180: printk(BIOS_DEBUG, "core frequency for ratio(%d) %dMHz\n", trailing whitespace

Name of user not set (Code Review)

7:20 p.m.

Hello Patrick Rudolph, build bot (Jenkins), David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#3).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/3

build bot (Jenkins) (Code Review)

7:21 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/3/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/3/src/cpu/intel/fsp_model_406... PS3, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

Name of user not set (Code Review)

7:25 p.m.

hannah.williams@dell.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 3:

What am I supposed to do for this error ? Line 175: space required after that ',' (ctx:VxV)

Name of user not set (Code Review)

9:37 p.m.

Hello Patrick Rudolph, HAOUAS Elyes, build bot (Jenkins), Martin Roth, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#4).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/4

build bot (Jenkins) (Code Review)

9:38 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/4/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/4/src/cpu/intel/fsp_model_406... PS4, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

build bot (Jenkins) (Code Review)

12 Sep 12 Sep

5:27 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/5/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/5/src/cpu/intel/fsp_model_406... PS5, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

build bot (Jenkins) (Code Review)

4:45 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/6/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/6/src/cpu/intel/fsp_model_406... PS6, Line 175: printk(BIOS_DEBUG,"MSR_PSB_CLOCK_STS %x:%x BCLK:%dHz ratio:%d\n", space required after that ',' (ctx:VxV)

Name of user not set (Code Review)

4:52 p.m.

Hello Patrick Rudolph, HAOUAS Elyes, build bot (Jenkins), Martin Roth, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#7).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/7

Kyösti Mälkki (Code Review)

5:20 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 7:

(1 comment)

BCLK is also a reference frequency to udelay(), fixing ACPI alone is not enough.

So get_core_fre

https://review.coreboot.org/c/coreboot/+/35348/7/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/7/src/cpu/intel/fsp_model_406... PS7, Line 157: static int get_core_frequency_mhz(int ratio) We need to fix cpu/intel/common/fsb.c get_fsb() instead as different BCLK also affects udelay() implementation in coreboot proper. This acpi code should then call get_ia32_fsb() or get_ia32_fsb_x3().

Name of user not set (Code Review)

7:41 p.m.

hannah.williams@dell.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 7:

...

Patch Set 7:

(1 comment)

BCLK is also a reference frequency to udelay(), fixing ACPI alone is not enough.

So get_core_fre

Agree. I was testing with 4.9 base coreboot so I did not see any other fixed BCLK dependency. Let me fix this also

Name of user not set (Code Review)

8:04 p.m.

hannah.williams@dell.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 7:

...

Patch Set 7:

...
Patch Set 7:

(1 comment)

BCLK is also a reference frequency to udelay(), fixing ACPI alone is not enough.

So get_core_fre

Agree. I was testing with 4.9 base coreboot so I did not see any other fixed BCLK dependency. Let me fix this also

Also, I get more precision by keeping the freq in KHz as I did in this code and multiplying with the ratio and then dividing by 10M. If I used get_fsb, then the return value is in MHz so the freq I would return will be 1494 instead of 1500 for example. So I would prefer to keep this code and also fix the Rangeley code in get_fsb.

Name of user not set (Code Review)

8:49 p.m.

Hello Patrick Rudolph, HAOUAS Elyes, build bot (Jenkins), Martin Roth, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#8).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Signed-off-by: Hannah Williams hannah.williams@dell.com Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f --- M src/cpu/intel/common/fsb.c M src/cpu/intel/fsp_model_406dx/acpi.c M src/cpu/intel/fsp_model_406dx/model_406dx.h 3 files changed, 25 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/8

Name of user not set (Code Review)

8:56 p.m.

Hello Patrick Rudolph, HAOUAS Elyes, build bot (Jenkins), Martin Roth, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#9).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Not all Rangeley SKUs have a fixed 100MHz BCLK. As per BIOS Writer's Guide, BCLK is available in MSR_FSB_FREQ 0xCD[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz. get_fsb that is called from other functions to get the BCLK has also been fixed to return the correct BCLK for Rangeley.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/9

Name of user not set (Code Review)

9:05 p.m.

Hello Patrick Rudolph, HAOUAS Elyes, build bot (Jenkins), Martin Roth, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35348

to look at the new patch set (#10).

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Not all Rangeley SKUs have a fixed 100MHz BCLK. As per BIOS Writer's Guide, BCLK is available in MSR_FSB_FREQ 0xCD[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz. get_fsb and udelay have been fixed to return the correct BCLK for Rangeley.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/10

Martin Roth (Code Review)

16 Sep 16 Sep

5:47 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 10: Code-Review+2

David Guckian (Code Review)

8:18 p.m.

David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 10: Code-Review+1

Kyösti Mälkki (Code Review)

8:35 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 10: Code-Review-2

(3 comments)

A bit attention to details please, with your +1 and +2...

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/common/fsb.c File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/common/fsb.c... PS10, Line 51: ret = rangeley_bclk[rdmsr(MSR_FSB_FREQ).lo & 3]; This changes all the 5 cases that previously returned BCLK=100. Also fix Rangeley comment.

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... PS10, Line 177: I wonder if the value returned here is actually what we would want for tsc_freq_mhz(). If so, find better place for this, outside acpi.c.

https://review.coreboot.org/c/coreboot/+/35348/10/src/northbridge/intel/fsp_... File src/northbridge/intel/fsp_rangeley/udelay.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/northbridge/intel/fsp_... PS10, Line 50: fsb = rangeley_bclk[rdmsr(MSR_FSB_FREQ).lo & 3]; Again, you should not repeat the array and rdmsr, but provide one implementation returning the frequency.

Hannah W (Code Review)

17 Sep 17 Sep

1:10 a.m.

Hannah W has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... PS10, Line 177:

...

I wonder if the value returned here is actually what we would want for tsc_freq_mhz(). […]

I don't see a tsc_freq_mhz inside cpu/intel/fsp_model_406dx/ or inside southbridge/intel/fsp_rangeley. Maybe I should add a tsc_freq_mhz function inside cpu/intel/fsp_model_406dx/ with the old implementation as shown below but with BCLK fix unsigned long tsc_freq_mhz(void) { msr_t msr = rdmsr(MSR_PLATFORM_INFO); return (CONFIG_CPU_BCLK_MHZ * ((msr.lo >> 8) & 0xff)); }

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 10 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-Comment-Date: Tue, 17 Sep 2019 01:10:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

6:25 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 10:

I have rebased timer patchset CB:31340 and I believe it is easier to fix Rangeley after that gets merged.

We cannot introduce Kconfig BCLK for rangeley. Like you noted, it depends of the installed SoC and needs to be detected runtime.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 10 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-Comment-Date: Tue, 17 Sep 2019 18:25:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

2 Nov 2 Nov

8:26 a.m.

Kyösti Mälkki has uploaded a new patch set (#11) to the change originally created by hannah.williams@dell.com. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Not all Rangeley SKUs have a fixed 100MHz BCLK.

As per BIOS Writer's Guide, BCLK is available in MSR_FSB_FREQ 0xCD[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz.

Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Signed-off-by: Hannah Williams hannah.williams@dell.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/intel/common/fsb.c M src/cpu/intel/fsp_model_406dx/acpi.c M src/cpu/intel/fsp_model_406dx/model_406dx.h 3 files changed, 23 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/35348/11

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 11 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-MessageType: newpatchset

Kyösti Mälkki (Code Review)

8:40 a.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 11: -Code-Review

Hannah, sorry for the slow response on the topic. Are you be able to get this tested?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 11 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-Comment-Date: Sat, 02 Nov 2019 08:40:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Name of user not set (Code Review)

5 Nov 5 Nov

4:58 p.m.

hannah.williams@dell.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 12:

...

Patch Set 11: -Code-Review

Hannah, sorry for the slow response on the topic. Are you be able to get this tested?

Would one of you be able to test this? I won't be able to do this at the moment.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 12 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-Reviewer: Дмитрий Понаморев dponamorev@gmail.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 05 Nov 2019 16:58:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Aaron Durbin (Code Review)

5:25 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 12: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 12 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-Reviewer: Дмитрий Понаморев dponamorev@gmail.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 05 Nov 2019 17:25:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

10:17 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Patch Set 12:

(4 comments)

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/common/fsb.c File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/common/fsb.c... PS10, Line 51: ret = rangeley_bclk[rdmsr(MSR_FSB_FREQ).lo & 3];

...

This changes all the 5 cases that previously returned BCLK=100. Also fix Rangeley comment.

Done

https://review.coreboot.org/c/coreboot/+/35348/7/src/cpu/intel/fsp_model_406... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/7/src/cpu/intel/fsp_model_406... PS7, Line 157: static int get_core_frequency_mhz(int ratio)

...

We need to fix cpu/intel/common/fsb. […]

Done

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... File src/cpu/intel/fsp_model_406dx/acpi.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/cpu/intel/fsp_model_40... PS10, Line 177:

...

I don't see a tsc_freq_mhz inside cpu/intel/fsp_model_406dx/ or inside southbridge/intel/fsp_rangele […]

Done

https://review.coreboot.org/c/coreboot/+/35348/10/src/northbridge/intel/fsp_... File src/northbridge/intel/fsp_rangeley/udelay.c:

https://review.coreboot.org/c/coreboot/+/35348/10/src/northbridge/intel/fsp_... PS10, Line 50: fsb = rangeley_bclk[rdmsr(MSR_FSB_FREQ).lo & 3];

...

Again, you should not repeat the array and rdmsr, but provide one implementation returning the frequ […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 12 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-Reviewer: Дмитрий Понаморев dponamorev@gmail.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 05 Nov 2019 22:17:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kyösti Mälkki kyosti.malkki@gmail.com Comment-In-Reply-To: Hannah W tohannahwilliams@gmail.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

7 Nov 7 Nov

2:08 p.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35348 )

Change subject: Rangeley: Fix incorrect BCLK ......................................................................

Rangeley: Fix incorrect BCLK

Not all Rangeley SKUs have a fixed 100MHz BCLK.

As per BIOS Writer's Guide, BCLK is available in MSR_FSB_FREQ 0xCD[1:0]. Using fixed BCLK was causing wrong values of core frequencies in _PSS table for SKUs that do not have BCLK=100MHz.

Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Signed-off-by: Hannah Williams hannah.williams@dell.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35348 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/intel/common/fsb.c M src/cpu/intel/fsp_model_406dx/acpi.c M src/cpu/intel/fsp_model_406dx/model_406dx.h 3 files changed, 23 insertions(+), 5 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved

diff --git a/src/cpu/intel/common/fsb.c b/src/cpu/intel/common/fsb.c index 14dbd60..0004ead 100644 --- a/src/cpu/intel/common/fsb.c +++ b/src/cpu/intel/common/fsb.c @@ -32,6 +32,7 @@ static const short core_fsb[8] = { -1, 133, -1, 166, -1, 100, -1, -1 }; static const short core2_fsb[8] = { 266, 133, 200, 166, 333, 100, 400, -1 }; static const short f2x_fsb[8] = { 100, 133, 200, 166, 333, -1, -1, -1 }; + static const short rangeley_fsb[4] = { 83, 100, 133, 116 }; msr_t msr;

get_fms(&c, cpuid_eax(1)); @@ -56,10 +57,13 @@ case 0x3a: /* IvyBridge BCLK fixed at 100MHz */ case 0x3c: /* Haswell BCLK fixed at 100MHz */ case 0x45: /* Haswell-ULT BCLK fixed at 100MHz */ - case 0x4d: /* Rangeley BCLK fixed at 100MHz */ *fsb = 100; *ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff; break; + case 0x4d: /* Rangeley */ + *fsb = rangeley_fsb[rdmsr(MSR_FSB_FREQ).lo & 3]; + *ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff; + break; default: return -2; } diff --git a/src/cpu/intel/fsp_model_406dx/acpi.c b/src/cpu/intel/fsp_model_406dx/acpi.c index 6672eab..078905d 100644 --- a/src/cpu/intel/fsp_model_406dx/acpi.c +++ b/src/cpu/intel/fsp_model_406dx/acpi.c @@ -13,6 +13,7 @@ */

#include <types.h> +#include <commonlib/helpers.h> #include <console/console.h> #include <arch/acpi.h> #include <arch/acpigen.h> @@ -20,6 +21,7 @@ #include <cpu/x86/msr.h> #include <cpu/intel/speedstep.h> #include <cpu/intel/turbo.h> +#include <delay.h> #include <device/device.h> #include <device/pci.h> #include "model_406dx.h" @@ -154,6 +156,20 @@ return (int)power; }

+static int get_core_frequency_mhz(int ratio) +{ + int fsb, core_freq; + + /* Get BCLK - different SKUs can have different BCLK */ + fsb = get_timer_fsb(); + + printk(BIOS_DEBUG, "BCLK:%d MHz ratio:%d\n", fsb, ratio); + + core_freq = DIV_ROUND_CLOSEST(fsb * ratio, 100) * 100; + printk(BIOS_DEBUG, "core frequency for ratio(%d) %dMHz\n", ratio, core_freq); + return core_freq; +} + static void generate_P_state_entries(int core, int cores_per_package) { int ratio_min, ratio_max, ratio_turbo, ratio_step; @@ -177,7 +193,7 @@ /* Max Non-Turbo Ratio */ ratio_max = (msr.lo >> 8) & 0xff; } - clock_max = ratio_max * RANGELEY_BCLK; + clock_max = get_core_frequency_mhz(ratio_max);

/* Calculate CPU TDP in mW */ msr = rdmsr(MSR_PKG_POWER_SKU_UNIT); @@ -240,7 +256,7 @@

/* Calculate power at this ratio */ power = calculate_power(power_max, ratio_max, ratio); - clock = ratio * RANGELEY_BCLK; + clock = get_core_frequency_mhz(ratio);

acpigen_write_PSS_package( clock, /*MHz*/ diff --git a/src/cpu/intel/fsp_model_406dx/model_406dx.h b/src/cpu/intel/fsp_model_406dx/model_406dx.h index 53a77a9..140dc2e 100644 --- a/src/cpu/intel/fsp_model_406dx/model_406dx.h +++ b/src/cpu/intel/fsp_model_406dx/model_406dx.h @@ -15,8 +15,6 @@ #ifndef _CPU_INTEL_MODEL_406DX_H #define _CPU_INTEL_MODEL_406DX_H

-/* Rangeley bus clock is fixed at 100MHz */ -#define RANGELEY_BCLK 100

#define MSR_FEATURE_CONFIG 0x13c #define MSR_FLEX_RATIO 0x194

-- To view, visit https://review.coreboot.org/c/coreboot/+/35348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8e0244fab0283b74870950cb00a95aab2a7201f Gerrit-Change-Number: 35348 Gerrit-PatchSet: 13 Gerrit-Owner: hannah.williams@dell.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: David Guckian Gerrit-Reviewer: David Guckian david.guckian@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Reviewer: hannah.williams@dell.com Gerrit-Reviewer: Дмитрий Понаморев dponamorev@gmail.com Gerrit-CC: Hannah W tohannahwilliams@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1788

days inactive

1845

days old

coreboot-gerrit@coreboot.org

28 comments

8 participants

tags (0)

participants (8)

Aaron Durbin (Code Review)
build bot (Jenkins) (Code Review)
David Guckian (Code Review)
Hannah W (Code Review)
Kyösti Mälkki (Code Review)
Martin Roth (Code Review)
Name of user not set (Code Review)
Patrick Georgi (Code Review)