Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34170 )
Change subject: libpayload/x86: Try to discover invariant TSC rate
......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
File payloads/libpayload/arch/x86/timer.c:
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 172: /*
newline above
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 176: _rdmsr(MSR_PLATFORM_INFO) >> 8
nit: brackets for readability?
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 186: cpu_khz
hm, somehow I seem to miss what this global variable is useful for (yeah I know, it was there before but...)
--
To view, visit https://review.coreboot.org/c/coreboot/+/34170
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6607ee2a8b41c2be9dc1bb4f1e23e652bb33889
Gerrit-Change-Number: 34170
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 13 Oct 2020 22:46:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Douglas Anderson,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46318
to review the following change.
Change subject: drivers: snsn65dsi86: Fix link rate parsing
......................................................................
drivers: snsn65dsi86: Fix link rate parsing
DP link rates are reported in an array of LE16 values. The current code
tries to parse them as 8-bit which doesn't get very far, causing us to
always drop into the fallback path. This patch should fix the issue
(+minor whitespace cleanup).
BUG=b:170630766
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: I1e03088ee2d3517bdb5dcc4dcc4ac04f8b14a391
---
M src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c
1 file changed, 5 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/46318/1
diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c
index 5a6eb49..44a8088 100644
--- a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c
+++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c
@@ -259,11 +259,11 @@
DP_BRIDGE_DPCD_REV, 1, DPCD_READ, &dpcd_val);
if (dpcd_val >= DP_BRIDGE_14) {
/* eDP 1.4 devices must provide a custom table */
- uint8_t sink_rates[DP_MAX_SUPPORTED_RATES * 2];
+ uint16_t sink_rates[DP_MAX_SUPPORTED_RATES] = {0};
sn65dsi86_bridge_dpcd_request(bus, chip, DP_SUPPORTED_LINK_RATES,
sizeof(sink_rates),
- DPCD_READ, sink_rates);
+ DPCD_READ, (void *)sink_rates);
for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
rate_per_200khz = le16_to_cpu(sink_rates[i]);
@@ -288,14 +288,12 @@
}
/* On older versions best we can do is use DP_MAX_LINK_RATE */
- sn65dsi86_bridge_dpcd_request(bus, chip,
- DP_MAX_LINK_RATE, 1, DPCD_READ, &dpcd_val);
+ sn65dsi86_bridge_dpcd_request(bus, chip, DP_MAX_LINK_RATE, 1, DPCD_READ, &dpcd_val);
switch (dpcd_val) {
default:
- printk(BIOS_ERR,
- "Unexpected max rate (%#x); assuming 5.4 GHz\n",
- (int)dpcd_val);
+ printk(BIOS_ERR, "Unexpected max rate (%#x); assuming 5.4 GHz\n",
+ (int)dpcd_val);
/* fall through */
case DP_LINK_BW_5_4:
rate_valid[7] = 1;
--
To view, visit https://review.coreboot.org/c/coreboot/+/46318
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1e03088ee2d3517bdb5dcc4dcc4ac04f8b14a391
Gerrit-Change-Number: 46318
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org>
Gerrit-MessageType: newchange
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45957 )
Change subject: [UNTESTED] soc/intel/{icl,tgl,jsl,ehl,adl}: set PM ACPI timer state from Kconfig
......................................................................
Patch Set 24:
Subrata, did Intel drop EnableTcoTimer from Alderlake FSP?
--
To view, visit https://review.coreboot.org/c/coreboot/+/45957
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iaf1eee9297034b29b7250f6c752e6f7f52b4b908
Gerrit-Change-Number: 45957
Gerrit-PatchSet: 24
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 13 Oct 2020 22:31:01 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46302 )
Change subject: soc/intel/common: rewrite and clarify the Legacy 8254 Timer Kconfig
......................................................................
soc/intel/common: rewrite and clarify the Legacy 8254 Timer Kconfig
The current Kconfig help text is confusing because it talks about
enabling the Kconfig for disabling a UPD for disabling power gating.
Rewrite and clarify the help text.
Change-Id: I9637c549db1ce29f259708f316852fc2ae9e7c38
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46302
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
---
M src/soc/intel/common/block/timer/Kconfig
1 file changed, 8 insertions(+), 4 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
diff --git a/src/soc/intel/common/block/timer/Kconfig b/src/soc/intel/common/block/timer/Kconfig
index a214ef0..42613a8 100644
--- a/src/soc/intel/common/block/timer/Kconfig
+++ b/src/soc/intel/common/block/timer/Kconfig
@@ -8,7 +8,11 @@
default y if PAYLOAD_SEABIOS || VGA_ROM_RUN
default n
help
- This sets the FSP UPD to enable Legacy 8254 clock gating. As per
- the FSP Integration guide Legacy 8254 timer clock gating UPD needs
- to be disabled in order to boot SeaBIOS or run OpRom,
- but should otherwise be enabled.
+ Setting this makes the Legacy 8254 Timer available by disabling
+ clock gating. This needs to be enabled in order to boot a legacy
+ BIOS or OS not supporting other timers like PM timer or TSC.
+
+ While SeaBIOS does not require this timer anymore, it is needed
+ when OpRoms are being used.
+
+ Disable this setting to save power, when the timer is not needed.
--
To view, visit https://review.coreboot.org/c/coreboot/+/46302
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9637c549db1ce29f259708f316852fc2ae9e7c38
Gerrit-Change-Number: 46302
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46354 )
Change subject: include/cpu/x86: introduce new helper for (un)setting MSRs
......................................................................
Patch Set 2:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/46354
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idfe9b66e7cfe78ec295a44a2a193f530349f7689
Gerrit-Change-Number: 46354
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 13 Oct 2020 22:17:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34170 )
Change subject: libpayload/x86: Try to discover invariant TSC rate
......................................................................
Patch Set 4:
(2 comments)
> Patch Set 4:
>
> This might need checking how it behaves with Depthcharge. I was
> actually wondering how CrOS devices handle it... is the PIT enabled?
> do you have a different timer?
I can confirm in the volteer (TGL) next to me that these two patches allow me to call `get_cpu_speed()` without hanging 😊. Without these, it hangs on the call, presumably b/c we leave the PIT disabled. We haven't used PIT for timing since some old skylake boards I see. Current boards AFAICT just use the timestamp counter
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
File payloads/libpayload/arch/x86/timer.c:
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 90: ecx
this seems unneeded, you could just pass nominal for ecx, and if it's 0, perform the model checks
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 106: switch (cpuid_model()) {
: case 0x4e: /* SKL-U/Y */
: case 0x5e: /* SKL-S/H */
: case 0x8e: /* KBL-U/Y */
: case 0x9e: /* KBL-S/H */
: nominal = 24000000;
: break;
: case 0x5c: /* APL */
: nominal = 19200000;
: break;
: default:
: return -1;
: }
: }
:
: return nominal * num / denom / 1000;
: }
:
: /**
: * @brief Returns three times the bus clock in kHz
: *
: * The result of calculations with the returned value shall be divided by 3.
: * This helps to avoid rounding errors.
: */
: static int get_bus_khz_x3(void)
: {
: if (cpuid_family() != 6)
: return -1;
:
: switch (cpuid_model()) {
: case 0x25: /* Nehalem */
: return 400 * 1000; /* 133 MHz */
: case 0x2a: /* SandyBridge */
: case 0x3a: /* IvyBridge */
: case 0x3c: /* Haswell */
: case 0x3d: /* Broadwell */
: case 0x45: /* Haswell-ULT */
: case 0x46: /* Haswell-GT3e */
: return 300 * 1000; /* 100 MHz */
: default:
: return -1;
: }
: }
suggestion: an enum for the results of cpuid_model() would be more readable than comments
--
To view, visit https://review.coreboot.org/c/coreboot/+/34170
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6607ee2a8b41c2be9dc1bb4f1e23e652bb33889
Gerrit-Change-Number: 34170
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 13 Oct 2020 22:10:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Felix Singer, build bot (Jenkins), Nico Huber, Paul Menzel, Subrata Banik, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46274
to look at the new patch set (#9).
Change subject: {cpu,soc}/intel: deduplicate cpu code
......................................................................
{cpu,soc}/intel: deduplicate cpu code
Move a whole bunch of copy-pasta code from soc/intel/{bdw,skl,cnl,icl,
tgl,ehl,jsl,adl} and cpu/intel/{hsw,model_*} to cpu/intel/common.
Change-Id: Ib0cc834de8492d59c423317598e1c11847a0b1ab
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
---
M src/cpu/intel/common/common.h
M src/cpu/intel/common/common_init.c
M src/cpu/intel/haswell/haswell_init.c
M src/cpu/intel/model_2065x/model_2065x_init.c
M src/cpu/intel/model_206ax/model_206ax_init.c
M src/include/cpu/intel/msr.h
M src/soc/intel/alderlake/cpu.c
M src/soc/intel/broadwell/cpu.c
M src/soc/intel/cannonlake/cpu.c
M src/soc/intel/elkhartlake/cpu.c
M src/soc/intel/icelake/Kconfig
M src/soc/intel/icelake/cpu.c
M src/soc/intel/jasperlake/cpu.c
M src/soc/intel/skylake/cpu.c
M src/soc/intel/tigerlake/cpu.c
15 files changed, 57 insertions(+), 439 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/46274/9
--
To view, visit https://review.coreboot.org/c/coreboot/+/46274
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib0cc834de8492d59c423317598e1c11847a0b1ab
Gerrit-Change-Number: 46274
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Hello Felix Singer, build bot (Jenkins), Nico Huber, Furquan Shaikh, David Guckian, Paul Menzel, Tim Wawrzynczak, Vanessa Eusebio, Subrata Banik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46278
to look at the new patch set (#4).
Change subject: {cpu,soc}/intel: replace AES-NI locking by common implemenation call
......................................................................
{cpu,soc}/intel: replace AES-NI locking by common implemenation call
Deduplicate code by using the new common cpu code implementation of
AES-NI locking.
For model_2065x and model_206ax locking was moved from SMM to core init
because the MSR is core-scoped, not package-scoped.
Change-Id: I7ab2d3839ecb758335ef8cc6a0c0c7103db0fa50
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
---
M src/cpu/intel/common/common_init.c
M src/cpu/intel/model_2065x/Kconfig
M src/cpu/intel/model_2065x/finalize.c
M src/cpu/intel/model_2065x/model_2065x_init.c
M src/cpu/intel/model_206ax/Kconfig
M src/cpu/intel/model_206ax/finalize.c
M src/cpu/intel/model_206ax/model_206ax_init.c
M src/include/cpu/intel/msr.h
M src/soc/intel/common/block/cpu/cpulib.c
M src/soc/intel/common/block/include/intelblocks/msr.h
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/cpu.c
M src/soc/intel/skylake/cpu.c
13 files changed, 17 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/46278/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/46278
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7ab2d3839ecb758335ef8cc6a0c0c7103db0fa50
Gerrit-Change-Number: 46278
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset