coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

Change in coreboot[master]: cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges
by Arthur Heymans (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/63555 to look at the new patch set (#2). Change subject: cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges ...................................................................... cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges Temporary MTRR setup usually cover the memory mapped flash. On recent Intel hardware the mapping is not coherent. It uses an external window for parts of the BIOS region that exceed 16M. To cover both those regions it is necessary to not reset the temporary MTRR setup on each invocation of this function. Change-Id: I23442bd2ab7602e4c5cbd37d187a31413cf27ecc Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 15 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/63555/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/63555 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I23442bd2ab7602e4c5cbd37d187a31413cf27ecc Gerrit-Change-Number: 63555 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: intel/common/block: Add gpmr common driver
by Michael Niewöhner (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Jamie Ryu, Subrata Banik, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Angel Pons, Nick Vaccaro, Tim Wawrzynczak. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63170 ) Change subject: intel/common/block: Add gpmr common driver ...................................................................... Patch Set 14: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/63170/comment/b46bb48d_22e341b7 PS14, Line 9: Move GPMR(General Purpose Memory Range) APIs to gpmr driver from dmi.c : For this, 3 patches are used. : 1. Add GPMR common driver in IA common code(CB:63170) : 2. Migrate all DMI API usage to GPMR(CB:63471) : 3. Drop DMI driver (CB:63472) : > > Agreed. Let me re-arrange patches to utilize rename. […] Thank you, guys! -- To view, visit https://review.coreboot.org/c/coreboot/+/63170 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4d57f4b8bd06e0cf6c9afa4baf4a7bed64ecb56b Gerrit-Change-Number: 63170 Gerrit-PatchSet: 14 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 17:06:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Wonkyu Kim <wonkyu.kim(a)intel.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/fast_spi.c: Use smarter mtrr code in ramstage
by Arthur Heymans (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Subrata Banik. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63550 ) Change subject: soc/intel/fast_spi.c: Use smarter mtrr code in ramstage ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > > > I will test this CL once again. > > > > here is my feedback. > > > > 1. MTRR snapshot with this CL that uses `mtrr_use_temp_range` > > > > `mtrr_use_temp_range` function will clear the previous > > > > Every time BIOS calls mtrr_use_temp_range, it will get a fresh new addr_space see this https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thi… > > > > so `fast_spi_enable_cache_range` tries to allocate BIOS and ext BIOS region by using mtrr_use_temp_range, but eventually, only ext BIOS region is allocated in MTRR since mtrr_use_temp_range doesn't keep the addr_space last time it's called(for BIOS region) > > > > check this, only ext BIOS region is part of MTRR snapshot not the BIOS region unfortunately. > > > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > > [DEBUG] 0x00000000f9000005: PHYBASE3: Address = 0x00000000f9000000, WP > > [DEBUG] 0x00003fffff000800: PHYMASK3: Length = 0x0000000001000000, Valid > > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > > [DEBUG] 0x0000000000000000: PHYBASE7 > > [DEBUG] 0x0000000000000000: PHYMASK7: Disabled > > [DEBUG] 0x0000000000000000: PHYBASE8 > > [DEBUG] 0x0000000000000000: PHYMASK8: Disabled > > [DEBUG] 0x0000000000000000: PHYBASE9 > > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled > > > > 2. MTRR snapshot CB:63492 > > > > Before clearing the temp MTRRs, you can see those rom cache MTRRs are not applied. > > > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > > [DEBUG] 0x0000000090000001: PHYBASE3: Address = 0x0000000090000000, WC > > [DEBUG] 0x00003ffff0000800: PHYMASK3: Length = 0x0000000010000000, Valid > > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > > [DEBUG] 0x00000000ff000005: PHYBASE7: Address = 0x00000000ff000000, WP > > [DEBUG] 0x00003fffff000800: PHYMASK7: Length = 0x0000000001000000, Valid > > [DEBUG] 0x00000000f9000005: PHYBASE8: Address = 0x00000000f9000000, WP > > [DEBUG] 0x00003fffff000800: PHYMASK8: Length = 0x0000000001000000, Valid > > [DEBUG] 0x0000000000000000: PHYBASE9 > > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled > > > > > > After clearing the temp MTRRs (prior to boot to payload) > > > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > > [DEBUG] 0x0000000090000001: PHYBASE3: Address = 0x0000000090000000, WC > > [DEBUG] 0x00003ffff0000800: PHYMASK3: Length = 0x0000000010000000, Valid > > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > > [DEBUG] 0x0000000000000000: PHYBASE7 > > [DEBUG] 0x0000000000000000: PHYMASK7: Disabled > > [DEBUG] 0x0000000000000000: PHYBASE8 > > [DEBUG] 0x0000000000000000: PHYMASK8: Disabled > > [DEBUG] 0x0000000000000000: PHYBASE9 > > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled > > in summary, the boot time increased by several seconds with this CL. You are right. It resets the solution on each invocation. Can you see if CB:63555 fixes that? -- To view, visit https://review.coreboot.org/c/coreboot/+/63550 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I283ba07fc12c410be39dfdc828657598237247c1 Gerrit-Change-Number: 63550 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 17:02:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/amdfwtool: Maintain one copy of PSP Level2 entries
by Karthik Ramasubramanian (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Bao Zheng, Raul Rangel, Felix Held. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63509 ) Change subject: util/amdfwtool: Maintain one copy of PSP Level2 entries ...................................................................... Patch Set 2: (1 comment) File util/amdfwtool/amdfwtool.c: https://review.coreboot.org/c/coreboot/+/63509/comment/923cf7f3_d7dca621 PS1, Line 1519: > Should set the default value here. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/63509 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I06eef8e14b9c14db1d02b621c2f7207188d86326 Gerrit-Change-Number: 63509 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: Avinash Alevoor Gerrit-CC: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-CC: Mohan Viswanathan Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 11 Apr 2022 17:02:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/amdfwtool: Maintain one copy of PSP Level2 entries
by Karthik Ramasubramanian (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Raul Rangel, Karthik Ramasubramanian, Felix Held. Hello Bao Zheng, Raul Rangel, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/63509 to look at the new patch set (#2). Change subject: util/amdfwtool: Maintain one copy of PSP Level2 entries ...................................................................... util/amdfwtool: Maintain one copy of PSP Level2 entries AMDFWtool maintains 2 copies of PSP Level2 entries - one in primary slot A (Type 0x48) and another in backup slot B (Type 0x4A). On boards which use VBOOT with 2 RW firmware slots, maintaining 2 copies of PSP Level2 entries in each FW slot is redundant and space-consuming. Introduce option to maintain only one copy of PSP Level2 entries and point to it from both slots A & B. BUG=None TEST=Build and boot to OS in Skyrim. Ensure that only one copy is added to each FW slot. This achieved a space saving of 1.5 MB in each FW slot. Before: apu/amdfw 0x415fc0 raw 3043328 none After: apu/amdfw 0x415fc0 raw 1556480 none Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I06eef8e14b9c14db1d02b621c2f7207188d86326 --- M util/amdfwtool/amdfwtool.c M util/amdfwtool/amdfwtool.h 2 files changed, 13 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/63509/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/63509 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I06eef8e14b9c14db1d02b621c2f7207188d86326 Gerrit-Change-Number: 63509 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: Avinash Alevoor Gerrit-CC: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-CC: Mohan Viswanathan Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges
by Arthur Heymans (Code Review) 11 Apr '22

11 Apr '22

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63555 ) Change subject: cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges ...................................................................... cpu/x86/mtrr.c: Allow for multiple TEMP MTRR ranges Temporary MTRR setup usually cover the memory mapped flash. On recent Intel hardware the mapping is not coherent. It uses an external window for parts of the BIOS region that exceed 16M. To cover both those regions it is necessary to not reset the temporary MTRR setup on each invocation of this function. Change-Id: I23442bd2ab7602e4c5cbd37d187a31413cf27ecc Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 15 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/63555/1 diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c index 89cac7f..f4cd72a 100644 --- a/src/cpu/x86/mtrr/mtrr.c +++ b/src/cpu/x86/mtrr/mtrr.c @@ -865,24 +865,28 @@ const struct range_entry *r; const struct memranges *orig; struct var_mtrr_solution sol; - struct memranges addr_space; + static struct memranges addr_space; + static first = true; const int above4gb = 1; /* Cover above 4GiB by default. */ int address_bits; /* Make a copy of the original address space and tweak it with the * provided range. */ - memranges_init_empty(&addr_space, NULL, 0); - orig = get_physical_address_space(); - memranges_each_entry(r, orig) { - unsigned long tag = range_entry_tag(r); + if (first) { + memranges_init_empty(&addr_space, NULL, 0); + orig = get_physical_address_space(); + memranges_each_entry(r, orig) { + unsigned long tag = range_entry_tag(r); - /* Remove any write combining MTRRs from the temporary - * solution as it just fragments the address space. */ - if (tag == MTRR_TYPE_WRCOMB) - tag = MTRR_TYPE_UNCACHEABLE; + /* Remove any write combining MTRRs from the temporary + * solution as it just fragments the address space. */ + if (tag == MTRR_TYPE_WRCOMB) + tag = MTRR_TYPE_UNCACHEABLE; - memranges_insert(&addr_space, range_entry_base(r), - range_entry_size(r), tag); + memranges_insert(&addr_space, range_entry_base(r), + range_entry_size(r), tag); + } + first = false; } /* Place new range into the address space. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/63555 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I23442bd2ab7602e4c5cbd37d187a31413cf27ecc Gerrit-Change-Number: 63555 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: cpu/x86/mp_init.c: Add wait_finished_mp_run_on_all_cpus
by Subrata Banik (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Arthur Heymans, Tim Wawrzynczak, Kane Chen. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63553 ) Change subject: cpu/x86/mp_init.c: Add wait_finished_mp_run_on_all_cpus ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: @Kane, can you please pick this CL and check if MTRR snapshot looks good including the boot time. -- To view, visit https://review.coreboot.org/c/coreboot/+/63553 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I70244557bb384739e3bd06de3d8414ec9f4d5f62 Gerrit-Change-Number: 63553 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kane Chen <kane.chen(a)intel.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 16:59:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: arch/x86/acpi_bert_storage.c: Use BOOT_STATE over CBMEM hooks
by Tim Wawrzynczak (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Raul Rangel, Marshall Dawson, Arthur Heymans, Kyösti Mälkki, Felix Held. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63415 ) Change subject: arch/x86/acpi_bert_storage.c: Use BOOT_STATE over CBMEM hooks ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/63415 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I317173d468073906d76228d1c8cc7bc28aae9e75 Gerrit-Change-Number: 63415 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 11 Apr 2022 16:54:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/fast_spi.c: Use smarter mtrr code in ramstage
by Subrata Banik (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Arthur Heymans. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63550 ) Change subject: soc/intel/fast_spi.c: Use smarter mtrr code in ramstage ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > > I will test this CL once again. > > here is my feedback. > > 1. MTRR snapshot with this CL that uses `mtrr_use_temp_range` > > `mtrr_use_temp_range` function will clear the previous > > Every time BIOS calls mtrr_use_temp_range, it will get a fresh new addr_space see this https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thi… > > so `fast_spi_enable_cache_range` tries to allocate BIOS and ext BIOS region by using mtrr_use_temp_range, but eventually, only ext BIOS region is allocated in MTRR since mtrr_use_temp_range doesn't keep the addr_space last time it's called(for BIOS region) > > check this, only ext BIOS region is part of MTRR snapshot not the BIOS region unfortunately. > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > [DEBUG] 0x00000000f9000005: PHYBASE3: Address = 0x00000000f9000000, WP > [DEBUG] 0x00003fffff000800: PHYMASK3: Length = 0x0000000001000000, Valid > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > [DEBUG] 0x0000000000000000: PHYBASE7 > [DEBUG] 0x0000000000000000: PHYMASK7: Disabled > [DEBUG] 0x0000000000000000: PHYBASE8 > [DEBUG] 0x0000000000000000: PHYMASK8: Disabled > [DEBUG] 0x0000000000000000: PHYBASE9 > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled > > 2. MTRR snapshot CB:63492 > > Before clearing the temp MTRRs, you can see those rom cache MTRRs are not applied. > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > [DEBUG] 0x0000000090000001: PHYBASE3: Address = 0x0000000090000000, WC > [DEBUG] 0x00003ffff0000800: PHYMASK3: Length = 0x0000000010000000, Valid > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > [DEBUG] 0x00000000ff000005: PHYBASE7: Address = 0x00000000ff000000, WP > [DEBUG] 0x00003fffff000800: PHYMASK7: Length = 0x0000000001000000, Valid > [DEBUG] 0x00000000f9000005: PHYBASE8: Address = 0x00000000f9000000, WP > [DEBUG] 0x00003fffff000800: PHYMASK8: Length = 0x0000000001000000, Valid > [DEBUG] 0x0000000000000000: PHYBASE9 > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled > > > After clearing the temp MTRRs (prior to boot to payload) > > [DEBUG] 0x0000000000000006: PHYBASE0: Address = 0x0000000000000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK0: Length = 0x0000000080000000, Valid > [DEBUG] 0x0000000077000000: PHYBASE1: Address = 0x0000000077000000, UC > [DEBUG] 0x00003fffff000800: PHYMASK1: Length = 0x0000000001000000, Valid > [DEBUG] 0x0000000078000000: PHYBASE2: Address = 0x0000000078000000, UC > [DEBUG] 0x00003ffff8000800: PHYMASK2: Length = 0x0000000008000000, Valid > [DEBUG] 0x0000000090000001: PHYBASE3: Address = 0x0000000090000000, WC > [DEBUG] 0x00003ffff0000800: PHYMASK3: Length = 0x0000000010000000, Valid > [DEBUG] 0x0000000100000006: PHYBASE4: Address = 0x0000000100000000, WB > [DEBUG] 0x00003fff00000800: PHYMASK4: Length = 0x0000000100000000, Valid > [DEBUG] 0x0000000200000006: PHYBASE5: Address = 0x0000000200000000, WB > [DEBUG] 0x00003fff80000800: PHYMASK5: Length = 0x0000000080000000, Valid > [DEBUG] 0x000000027fc00000: PHYBASE6: Address = 0x000000027fc00000, UC > [DEBUG] 0x00003fffffc00800: PHYMASK6: Length = 0x0000000000400000, Valid > [DEBUG] 0x0000000000000000: PHYBASE7 > [DEBUG] 0x0000000000000000: PHYMASK7: Disabled > [DEBUG] 0x0000000000000000: PHYBASE8 > [DEBUG] 0x0000000000000000: PHYMASK8: Disabled > [DEBUG] 0x0000000000000000: PHYBASE9 > [DEBUG] 0x0000000000000000: PHYMASK9: Disabled in summary, the boot time increased by several seconds with this CL. -- To view, visit https://review.coreboot.org/c/coreboot/+/63550 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I283ba07fc12c410be39dfdc828657598237247c1 Gerrit-Change-Number: 63550 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Mon, 11 Apr 2022 16:52:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/brya0: Change MAX98360 AMP interface to I2S1
by Tim Wawrzynczak (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Amanda Hwang. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63495 ) Change subject: mb/google/brya/var/brya0: Change MAX98360 AMP interface to I2S1 ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/63495 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee45dbceabdedd39a9befffb8002b8bc3d4bfb4 Gerrit-Change-Number: 63495 Gerrit-PatchSet: 6 Gerrit-Owner: Amanda Hwang <amanda_hwang(a)compal.corp-partner.google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Frank Wu <frank_wu(a)compal.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Van Chen <van_chen(a)compal.corp-partner.google.com> Gerrit-Attention: Amanda Hwang <amanda_hwang(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 16:42:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2022