Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46428 )
Change subject: soc/intel/skylake/cpu.c: Fix comment coding style
......................................................................
soc/intel/skylake/cpu.c: Fix comment coding style
This comment does not follow any of the styles outlined in the coding
style page of the documentation. Adjust it to match the preferred style.
Change-Id: Idf6d0ea69a08e378266b4256c476580889adfca8
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M src/soc/intel/skylake/cpu.c
1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/46428/1
diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c
index 79fcda1..f1b40f6 100644
--- a/src/soc/intel/skylake/cpu.c
+++ b/src/soc/intel/skylake/cpu.c
@@ -34,10 +34,10 @@
if (conf->speed_shift_enable) {
/*
- * Kernel driver checks CPUID.06h:EAX[Bit 7] to determine if HWP
- is supported or not. coreboot needs to configure MSR 0x1AA
- which is then reflected in the CPUID register.
- */
+ * Kernel driver checks CPUID.06h:EAX[Bit 7] to determine if HWP
+ * is supported or not. coreboot needs to configure MSR 0x1AA
+ * which is then reflected in the CPUID register.
+ */
msr = rdmsr(MSR_MISC_PWR_MGMT);
msr.lo |= MISC_PWR_MGMT_ISST_EN; /* Enable Speed Shift */
msr.lo |= MISC_PWR_MGMT_ISST_EN_INT; /* Enable Interrupt */
--
To view, visit https://review.coreboot.org/c/coreboot/+/46428
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idf6d0ea69a08e378266b4256c476580889adfca8
Gerrit-Change-Number: 46428
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46301 )
Change subject: soc/intel/common: drop default to enable the PIT if SeaBIOS is chosen
......................................................................
Patch Set 1:
> Well. Sure that actually works?
>
> The pmtimer is used in case seabios runs on qemu and initializes the (virtual) acpi pci device.
> I doubt this also happens when running as coreboot payload.
I have reason to assume it does:
platform_hardware_setup() calls coreboot_platform_setup() calls
find_acpi_features() calls pmtimer_setup(). Didn't test it though,
but I assume the PM timer would be used.
And the PM timer always works as it's emulated in the worst case :)
--
To view, visit https://review.coreboot.org/c/coreboot/+/46301
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifd75800678707a92110682347c7cfb93e25109a4
Gerrit-Change-Number: 46301
Gerrit-PatchSet: 1
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: Gerd Hoffmann <kraxel(a)redhat.com>
Gerrit-Reviewer: Kevin O'Connor <kevin(a)koconnor.net>
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-Comment-Date: Wed, 14 Oct 2020 20:33:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Angel Pons 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:
(1 comment)
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 176: _rdmsr(MSR_PLATFORM_INFO) >> 8
> lol, I can definitely drop those below. But would really not like to add […]
reminds me I first thought of replying "to worsen readability?" but thought it would be a tad bit too rude.
--
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: Wed, 14 Oct 2020 20:03:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45826 )
Change subject: soc/intel/icl: enable common CPU code
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45826/2/src/soc/intel/icelake/roms…
File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45826/2/src/soc/intel/icelake/roms…
PS2, Line 59: m_cfg->VmxEnable = CONFIG(ENABLE_VMX);
> This already configures VMX in FSP and set the feature lock bit (FC_LOCK MSR). […]
That's a bit annoying. I would prefer to mimic that in Kconfig then, i.e.
what you proposed
config ENABLE_VMX
select ...LOCK
and always call the lock function in coreboot.
--
To view, visit https://review.coreboot.org/c/coreboot/+/45826
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I58e86021687fc0a836324f70071f7ea80242b3cb
Gerrit-Change-Number: 45826
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
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: 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-Comment-Date: Wed, 14 Oct 2020 19:37:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Nico Huber 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:
(5 comments)
> > 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
It's all about using the TSC, but you need to know it's rate. Still
wonder how you avoid calling get_cpu_speed().
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
> yeah I guess it wouldn't work b/c of the macro.
Thanks Angel. Indeed, I just wanted to avoid the cast. Doesn't matter much
though I guess, could change it?
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
I like that!
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 172: /*
> newline above
Is that mandatory? Currently, the whole function does either what is above
the empty line or what is below... Adding another empty line would make me
want to split the second part into another function. Could do that ofc.
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x…
PS4, Line 176: _rdmsr(MSR_PLATFORM_INFO) >> 8
> well, maybe even both
lol, I can definitely drop those below. But would really not like to add
unneeded ones. Unneeded brackets => people don't learn C => people make
mistakes.
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 […]
The whole point of this file is to discover the TSC rate. So you can later
do rdtsc()*x/cpu_khz to get a monotonic time in s/ms/us/ns.
--
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: Wed, 14 Oct 2020 19:16:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment