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
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
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
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
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
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
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
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
Attention is currently required from: Arthur Heymans, Felix Singer, Mario Scheithauer.
Jan Samek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75364?usp=email )
Change subject: soc/intel/apollolake: Make SATA speed limit configurable
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/75364?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: I9c3eda0649546e3a40eb24a015b7c6efd8f90e0f
Gerrit-Change-Number: 75364
Gerrit-PatchSet: 3
Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-Reviewer: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 01 Jun 2023 07:28:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Felix Singer, Jan Samek.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75364?usp=email )
Change subject: soc/intel/apollolake: Make SATA speed limit configurable
......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/apollolake/chip.h:
https://review.coreboot.org/c/coreboot/+/75364/comment/90c1a433_65d8a08b :
PS2, Line 31: SATA_GEN2
> Hmm I see. If gen 3 and auto are the same, then it doesn't matter I guess. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/75364?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: I9c3eda0649546e3a40eb24a015b7c6efd8f90e0f
Gerrit-Change-Number: 75364
Gerrit-PatchSet: 3
Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-Reviewer: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 07:27:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Comment-In-Reply-To: Mario Scheithauer <mario.scheithauer(a)siemens.com>
Gerrit-MessageType: comment