June 2023 - coreboot-gerrit - mail.coreboot.org

[S] Change in coreboot[master]: mb/google/rex: Update GPIO PAD IO Standby State
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Bora Guvendik, Kapil Porwal, Tarun Tuli. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75555?usp=email ) Change subject: mb/google/rex: Update GPIO PAD IO Standby State ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/75555/comment/e9c1fea5_e016b64c : PS3, Line 9: Type-c Type-C -- To view, visit https://review.coreboot.org/c/coreboot/+/75555?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If96c1e71fdde784a55fe079875915ffa5a4f548a Gerrit-Change-Number: 75555 Gerrit-PatchSet: 3 Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:46:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: mb/google/rex/variants/ovis: Add override devicetree
by Jakub Czapiga (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Jan Dabros, Kapil Porwal, Subrata Banik, Tarun Tuli. Hello Kapil Porwal, Subrata Banik, Tarun Tuli, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/75503?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: mb/google/rex/variants/ovis: Add override devicetree ...................................................................... mb/google/rex/variants/ovis: Add override devicetree BUG=b:274421383 TEST=util/abuild/abuild -p none -t google/rex -x -a -b ovis Change-Id: I1644b1d8f49accbb2ea68e236534df80a5151360 Signed-off-by: Jakub Czapiga <jacz(a)semihalf.com> --- M src/mainboard/google/rex/variants/ovis/overridetree.cb 1 file changed, 394 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/75503/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/75503?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1644b1d8f49accbb2ea68e236534df80a5151360 Gerrit-Change-Number: 75503 Gerrit-PatchSet: 4 Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jan Dabros <dabros(a)google.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jan Dabros <dabros(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/google/rex: Update GPIO PAD IO Standby State
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Bora Guvendik, Kapil Porwal, Tarun Tuli. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75555?usp=email ) Change subject: mb/google/rex: Update GPIO PAD IO Standby State ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: we need to apply this on all other variants as well ? -- To view, visit https://review.coreboot.org/c/coreboot/+/75555?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If96c1e71fdde784a55fe079875915ffa5a4f548a Gerrit-Change-Number: 75555 Gerrit-PatchSet: 3 Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:45:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: include/cpu/x86: Simplify en/dis cache functions
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Felix Singer, Himanshu Sahdev, Jérémy Compostella, Lean Sheng Tan. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75552?usp=email ) Change subject: include/cpu/x86: Simplify en/dis cache functions ...................................................................... Patch Set 2: (1 comment) File src/include/cpu/x86/cache.h: https://review.coreboot.org/c/coreboot/+/75552/comment/1ce14ce9_22f45617 : PS2, Line 48: CR0_CacheDisable > want me to change it to screaming snake case -> CR0_CACHE_DISABLE? yup, this is what make sense. > > Also, these macros being in use already, better to do it in different CL to change everywhere. sure -- To view, visit https://review.coreboot.org/c/coreboot/+/75552?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81688e8bbb073e1d09ecf63f3f33e1651dbd778e Gerrit-Change-Number: 75552 Gerrit-PatchSet: 2 Gerrit-Owner: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:43:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: mb/google/rex/variants/screebo: add FW_CONFIG for audio/DB
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Kun Liu, Rui Zhou, Simon Zhou, Tarun Tuli, YH Lin. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75573?usp=email ) Change subject: mb/google/rex/variants/screebo: add FW_CONFIG for audio/DB ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/75573?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I928aae61d4936509a7b68f4041c0cd72f298e83d Gerrit-Change-Number: 75573 Gerrit-PatchSet: 4 Gerrit-Owner: Simon Zhou <zhouguohui(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Kun Liu <liukun11(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Rui Zhou <zhourui(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Rui Zhou <zhourui(a)huaqin.corp-partner.google.com> Gerrit-Attention: Simon Zhou <zhouguohui(a)huaqin.corp-partner.google.com> Gerrit-Attention: Kun Liu <liukun11(a)huaqin.corp-partner.google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:42:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: {cpu, security}: Stitch multiple microcodes per CPUID into CBFS
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Arthur Heymans, Himanshu Sahdev, Julius Werner, Lean Sheng Tan, Rizwan Qureshi, Tarun Tuli, Yu-Ping Wu. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75357?usp=email ) Change subject: {cpu, security}: Stitch multiple microcodes per CPUID into CBFS ...................................................................... Patch Set 14: (1 comment) File src/cpu/Makefile.inc: https://review.coreboot.org/c/coreboot/+/75357/comment/055f239a_22e2c916 : PS10, Line 74: microcode-params := $(shell find "$(microcode-params-dir)" -type f | sed -e 's/.*\///') > > > > > > On lighter note, ~10ms is still a big number, we are looking to optimize 2-3ms even. :P > > > > > > > > > > yeah on Chromebook every ms counts, I think this is also a very distinct differentiator of having coreboot instead of edk2 on Laptops ;) > > > > > > > > @Arthur, as i have moved the FIT microcode entry here for RO CBFS. should we need to add support for Top Swap 2nd FIT like https://review.coreboot.org/c/coreboot/+/75357/14/src/cpu/intel/fit/Makefil… ? > > > > > > fixing typo > > > > > > "don't we need to add support for Top Swap 2nd FIT as well like it had exited in other makefile https://review.coreboot.org/c/coreboot/+/75357/14/src/cpu/intel/fit/Makefil… ?" > > > > ping to get attention on the open question. > > Hmm the topswap FIT points to a separate FMAP region iirc which has only once MCU in there that the RW-A/B writes to. I believe it should be unaffected. okay, so we don't need to port those changes here under newly added Kconfig. -- To view, visit https://review.coreboot.org/c/coreboot/+/75357?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic7db73335ffa25399869cfb0d59129ee118f1012 Gerrit-Change-Number: 75357 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:41:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: {cpu, security}: Stitch multiple microcodes per CPUID into CBFS
by Arthur Heymans (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Himanshu Sahdev, Julius Werner, Lean Sheng Tan, Rizwan Qureshi, Subrata Banik, Tarun Tuli, Yu-Ping Wu. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75357?usp=email ) Change subject: {cpu, security}: Stitch multiple microcodes per CPUID into CBFS ...................................................................... Patch Set 14: (1 comment) File src/cpu/Makefile.inc: https://review.coreboot.org/c/coreboot/+/75357/comment/288b2a88_e849d593 : PS10, Line 74: microcode-params := $(shell find "$(microcode-params-dir)" -type f | sed -e 's/.*\///') > > > > > On lighter note, ~10ms is still a big number, we are looking to optimize 2-3ms even. :P > > > > > > > > yeah on Chromebook every ms counts, I think this is also a very distinct differentiator of having coreboot instead of edk2 on Laptops ;) > > > > > > @Arthur, as i have moved the FIT microcode entry here for RO CBFS. should we need to add support for Top Swap 2nd FIT like https://review.coreboot.org/c/coreboot/+/75357/14/src/cpu/intel/fit/Makefil… ? > > > > fixing typo > > > > "don't we need to add support for Top Swap 2nd FIT as well like it had exited in other makefile https://review.coreboot.org/c/coreboot/+/75357/14/src/cpu/intel/fit/Makefil… ?" > > ping to get attention on the open question. Hmm the topswap FIT points to a separate FMAP region iirc which has only once MCU in there that the RW-A/B writes to. I believe it should be unaffected. -- To view, visit https://review.coreboot.org/c/coreboot/+/75357?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic7db73335ffa25399869cfb0d59129ee118f1012 Gerrit-Change-Number: 75357 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 08:22:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: include/cpu/x86: Skip `wbinvd` on CPUs with cache self-snooping (SS)
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75511?usp=email ) Change subject: include/cpu/x86: Skip `wbinvd` on CPUs with cache self-snooping (SS) ...................................................................... include/cpu/x86: Skip `wbinvd` on CPUs with cache self-snooping (SS) This patch refers and backport some of previous work from Linux Kernel (https://lore.kernel.org/all/1561689337-19390-3-git-send-email-ricardo. neri-calderon(a)linux.intel.com/T/#u) that optimizes the MTRR register programming in multi-processor systems by relying on the CPUID (self-snoop feature supported). Refer to the details below: Programming MTRR registers in multi-processor systems is a rather lengthy process as it involves flushing caches. As a result, the process may take a considerable amount of time. Furthermore, all processors must program these registers serially. `wbinvd` instruction is used to invalidate the cache line to ensure that all modified data is written back to memory. All logical processors are stopped from executing until after the write-back and invalidate operation is completed. The amount of time or cycles for WBINVD to complete will vary due to the size of different cache hierarchies and other factors. As a consequence, the use of the WBINVD instruction can have an impact on response time. As per measurements, around 98% of the time needed by the procedure to program MTRRs in multi-processor systems is spent flushing caches with wbinvd(). As per the Section 11.11.8 of the Intel 64 and IA 32 Architectures Software Developer's Manual, it is not necessary to flush caches if the CPU supports cache self-snooping (ss). "Flush all caches using the WBINVD instructions. Note on a processor that supports self-snooping, CPUID feature flag bit 27, this step is unnecessary." Thus, skipping the cache flushes can reduce by several tens of milliseconds the time needed to complete the programming of the MTRR registers: Platform Before After 12-core (14 Threads) MeteorLake 35ms 1ms BUG=b:260455826 TEST=Able to build and boot google/rex. Change-Id: I83cac2b1e1707bbb1bc1bba82cf3073984e9768f Signed-off-by: Subrata Banik <subratabanik(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/75511 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Jérémy Compostella <jeremy.compostella(a)intel.com> Reviewed-by: Lean Sheng Tan <sheng.tan(a)9elements.com> Reviewed-by: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Reviewed-by: Tarun Tuli <taruntuli(a)google.com> --- M src/include/cpu/x86/cache.h 1 file changed, 14 insertions(+), 1 deletion(-) Approvals: Lean Sheng Tan: Looks good to me, approved build bot (Jenkins): Verified Jérémy Compostella: Looks good to me, but someone else must approve Himanshu Sahdev: Looks good to me, but someone else must approve Tarun Tuli: Looks good to me, approved diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h index 4143d97..d4d9160 100644 --- a/src/include/cpu/x86/cache.h +++ b/src/include/cpu/x86/cache.h @@ -9,9 +9,11 @@ #define CR0_NoWriteThrough (CR0_NW) #define CPUID_FEATURE_CLFLUSH_BIT 19 +#define CPUID_FEATURE_SELF_SNOOP_BIT 27 #if !defined(__ASSEMBLER__) +#include <arch/cpuid.h> #include <stdbool.h> #include <stddef.h> @@ -51,6 +53,16 @@ write_cr0(cr0); } +/* + * Cache flushing is the most time-consuming step when programming the MTRRs. + * However, if the processor supports cache self-snooping (ss), we can skip + * this step and save time. + */ +static __always_inline bool self_snooping_supported(void) +{ + return (cpuid_edx(1) >> CPUID_FEATURE_SELF_SNOOP_BIT) & 1; +} + static __always_inline void disable_cache(void) { /* Disable and write back the cache */ @@ -58,7 +70,8 @@ cr0 = read_cr0(); cr0 |= CR0_CD; write_cr0(cr0); - wbinvd(); + if (!self_snooping_supported()) + wbinvd(); } #endif /* !__ASSEMBLER__ */ -- To view, visit https://review.coreboot.org/c/coreboot/+/75511?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I83cac2b1e1707bbb1bc1bba82cf3073984e9768f Gerrit-Change-Number: 75511 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-MessageType: merged

1 0

[XS] Change in coreboot[master]: cpu/x86/cache: Call wbinvd only once CR0.CD is set
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75474?usp=email ) Change subject: cpu/x86/cache: Call wbinvd only once CR0.CD is set ...................................................................... cpu/x86/cache: Call wbinvd only once CR0.CD is set This patch removes the wbinvd call preceding CR0.CD setting in disable_cache() to improve the boot time performances. According to some experimental measurements, the wbinvd execution takes between 1.6 up and 6 milliseconds to complete so it is preferable to call it only when necessary. According to Intel Software Developer Manual Vol 3.A - 12.5.3 Preventing Caching section there is no need to flush and invalidate the cache before settings CR0.CD. The documented sequence consists in setting CR0.CD and then call wbinvd. We also could not find any extra requirements in the AMD64 Architecture Programmer’s Manual - Volume 2 - Memory System chapter. This extra wbinvd in coreboot disable_cache() function does not seem documented and looking into the history of the project got us all the way back to original commit 8ca8d7665d67 ("- Initial checkin of the freebios2 tree") from April 2003. Even the original disable_cache() implementation (see below) is a bit curious as the comment list two actions: 1. Disable cache cover by line 74, 75 and 77 2. Write back the cache and flush TLB - Line 78 But it does not provide any explanation for the wbinvd call line 76. 68 static inline void disable_cache(void) 69 { 70 unsigned int tmp; 71 /* Disable cache */ 72 /* Write back the cache and flush TLB */ 73 asm volatile ( 74 "movl %%cr0, %0\n\t" 75 "orl $0x40000000, %0\n\t" 76 "wbinvd\n\t" 77 "movl %0, %%cr0\n\t" 78 "wbinvd\n\t" 79 :"=r" (tmp) 80 ::"memory"); 81 } BUG=b/260455826 TEST=Successful boot on Skolas and Rex board Change-Id: I08c6486dc93c4d70cadc22a760d1b7e536e85bfa Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/75474 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Subrata Banik <subratabanik(a)google.com> Reviewed-by: Himanshu Sahdev <himanshu.sahdev(a)intel.com> --- M src/include/cpu/x86/cache.h 1 file changed, 0 insertions(+), 1 deletion(-) Approvals: Subrata Banik: Looks good to me, approved Himanshu Sahdev: Looks good to me, but someone else must approve build bot (Jenkins): Verified diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h index d35c4e7..4143d97 100644 --- a/src/include/cpu/x86/cache.h +++ b/src/include/cpu/x86/cache.h @@ -57,7 +57,6 @@ CRx_TYPE cr0; cr0 = read_cr0(); cr0 |= CR0_CD; - wbinvd(); write_cr0(cr0); wbinvd(); } -- To view, visit https://review.coreboot.org/c/coreboot/+/75474?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08c6486dc93c4d70cadc22a760d1b7e536e85bfa Gerrit-Change-Number: 75474 Gerrit-PatchSet: 7 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in coreboot[master]: soc/intel/{alderlake, meteorlake}: Properly assign the FSP ASPM UPD
by Subrata Banik (Code Review) June 1, 2023

June 1, 2023

Attention is currently required from: Felix Singer, Kapil Porwal, Sean Rhodes, Tarun Tuli, Tim Crawford. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75481?usp=email ) Change subject: soc/intel/{alderlake, meteorlake}: Properly assign the FSP ASPM UPD ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/alderlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/75481/comment/afa74fdd_a35daf40 : PS1, Line 899: if (rp_cfg->pcie_rp_aspm) : s_cfg->PcieRpAspm[i] = get_aspm_control(rp_cfg->pcie_rp_aspm); : else if (CONFIG(PCIEXP_ASPM)) /* use auto (FSP default) if PCIEXP_ASPM is enabled */ : s_cfg->PcieRpAspm[i] = ASPM_AUTO; : else : s_cfg->PcieRpAspm[i] = ASPM_DISABLE; > Those aren't disjoint cases. I want both of them. ASPM should be disabled for a *specific device* BUT be "Auto" for *everything else*. > > > we are not relying anywhere on the FSP default value and trying to make sure that coreboot overrides the FSP default as applicable. can you please elaborate > > "Auto" has been the default value used by every version of the FSP I've used for porting boards. So I mean setting it to "Auto" is the same thing as setting it to the value already used by the FSP. > > It seems like to do what I want, I now have to override `PCIEXP_ASPM` to disable it and specify `pcie_rp_aspm` for *every* device (does that even work if the config is disabled; does FSP do it instead?). Compared to the previous behavior where I only needed to specify the device I want to disable. I don't understand the concern here, are you saying that you don't want to make "PCIEXP_ASPM" kconfig default enabled on your platform ? because you want few devices to have ASPM disable vs others enabled ? wondering how having FSP default set to `Auto` was helpful in your case where you wished to make some device ASPM disabled but others enabled ? -- To view, visit https://review.coreboot.org/c/coreboot/+/75481?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib20c7466c79c3e0757ebda98240a529920c32a16 Gerrit-Change-Number: 75481 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Thu, 01 Jun 2023 07:51:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Crawford <tcrawford(a)system76.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0