coreboot-gerrit November 2023

coreboot-gerrit@coreboot.org

1 participants
3037 discussions

[L] Change in coreboot[main]: arch/x86/mmu: Port armv8 MMU to x86_64
by Jérémy Compostella (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Benjamin Doron. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79161?usp=email ) Change subject: arch/x86/mmu: Port armv8 MMU to x86_64 ...................................................................... Patch Set 1: (7 comments) Patchset: PS1: I did not get the chance to go all of it yet but I dropped a few comments. File src/arch/x86/mmu-ramstage.c: https://review.coreboot.org/c/coreboot/+/79161/comment/1c214851_cf09669a : PS1, Line 13: #include <commonlib/region.h> alphabetical order ? File src/arch/x86/mmu.c: https://review.coreboot.org/c/coreboot/+/79161/comment/80431122_54fb470d : PS1, Line 13: #include <commonlib/region.h> alphabetical order ? https://review.coreboot.org/c/coreboot/+/79161/comment/22bb790d_f82cdcda : PS1, Line 313: GRANULE_SIZE I am not very familiar with arm but it looks this GRANULE word comes from the ARM work. Shouldn't we use PAGE_SIZE here ? https://review.coreboot.org/c/coreboot/+/79161/comment/6b561125_219f6fb7 : PS1, Line 350: pages += PDPTs; /* PDPT pointing to 1 GiB PLEs */ the beginning of both branches is identical, can't we take them out of the condition ? https://review.coreboot.org/c/coreboot/+/79161/comment/a325b7f2_b2252b01 : PS1, Line 357: 256 define a constant ? https://review.coreboot.org/c/coreboot/+/79161/comment/e7b94caf_9f017acd : PS1, Line 364: 4 * KiB There are a lot of `4 * KiB` in this file, could we create a constant ? -- To view, visit https://review.coreboot.org/c/coreboot/+/79161?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6e8b46e65925823a84b8ccd647c7d6848aa20992 Gerrit-Change-Number: 79161 Gerrit-PatchSet: 1 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Comment-Date: Tue, 21 Nov 2023 17:39:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable
by Tim Van Patten (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Karthik Ramasubramanian, Mark Hasemeyer, Matt DeVillier. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79155?usp=email ) Change subject: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Patchset: PS2: LGTM -- To view, visit https://review.coreboot.org/c/coreboot/+/79155?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I166ac7b50b367c067e1a743fc94686e69dd07844 Gerrit-Change-Number: 79155 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 21 Nov 2023 17:29:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable
by Karthik Ramasubramanian (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Mark Hasemeyer, Matt DeVillier, Tim Van Patten. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79155?usp=email ) Change subject: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable ...................................................................... Patch Set 2: (4 comments) File src/soc/amd/common/psp_verstage/Kconfig: https://review.coreboot.org/c/coreboot/+/79155/comment/995cc400_023b688e : PS1, Line 49: default y if SOC_AMD_CEZANNE > Have you tried any earlier boards as well (e.g., Grunt and Zork)? […] Done. Tested on Zork. PSP verstage is not enabled in Grunt and hence it does not apply there. File src/soc/amd/common/psp_verstage/boot_dev.c: https://review.coreboot.org/c/coreboot/+/79155/comment/59610f0b_72b38eb5 : PS1, Line 40: uint32_t ret; > This can be moved inside the `CONFIG` conditional block. Acknowledged. But I updated the condition logic to check for true to be consistent with the rest of the code. File src/soc/amd/common/psp_verstage/fch.c: https://review.coreboot.org/c/coreboot/+/79155/comment/fde98e26_3d621c7f : PS1, Line 101: if (!CONFIG(PSP_VERSTAGE_MAP_ENTIRE_SPIROM)) : return spi.SpiBiosSmnBase; : : if (spi.SpiBiosSmnBase != 0) : if (svc_map_spi_rom(spi.SpiBiosSmnBase, CONFIG_ROM_SIZE, (void **)&addr)) : printk(BIOS_DEBUG, "Error mapping SPI ROM to address.\n"); : : return addr; > Positive logic is easier to read, so I think this should be inverted: […] Done File src/soc/amd/common/psp_verstage/psp_verstage.c: https://review.coreboot.org/c/coreboot/+/79155/comment/a25b28d3_e55fff63 : PS1, Line 355: boot_dev_base = rdev_mmap_full(boot_device_ro()); > Missing the `post_code(POSTCODE_UNMAP_SPI_ROM);` line as well: […] Done. -- To view, visit https://review.coreboot.org/c/coreboot/+/79155?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I166ac7b50b367c067e1a743fc94686e69dd07844 Gerrit-Change-Number: 79155 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 21 Nov 2023 17:25:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: acpi: Enable 64bit ECAM resource
by Martin L Roth (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Angel Pons, Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, Lean Sheng Tan, Maximilian Brune, Naresh Solanki, Nico Huber, Patrick Rudolph, Tim Wawrzynczak. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79096?usp=email ) Change subject: acpi: Enable 64bit ECAM resource ...................................................................... Patch Set 2: Code-Review+2 (1 comment) File src/acpi/dsdt_top.asl: https://review.coreboot.org/c/coreboot/+/79096/comment/4234725d_27d17e97 : PS2, Line 66: 0x0000000000000000, Nit: Maybe comment these? Up to you. // Granularity // _MIN // _MAX // Translation // _LEN -- To view, visit https://review.coreboot.org/c/coreboot/+/79096?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Idb049d848f2311e27df5279a10c33f9fab259c08 Gerrit-Change-Number: 79096 Gerrit-PatchSet: 2 Gerrit-Owner: Naresh Solanki <naresh.solanki(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Naresh Solanki <naresh.solanki(a)9elements.com> Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Comment-Date: Tue, 21 Nov 2023 17:23:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable
by Karthik Ramasubramanian (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Karthik Ramasubramanian, Mark Hasemeyer, Matt DeVillier. Hello Felix Held, Fred Reitberger, Jason Glenesk, Mark Hasemeyer, Matt DeVillier, Tim Van Patten, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/79155?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable ...................................................................... soc/amd/common/psp_verstage: Make SPI ROM mapping configurable Earlier entire SPI ROM was mapped to memory. With limited TLB resources in PSP, this approach hit the limit on systems using 32 MiB SPI ROM. Therefore regions in SPI ROM were mapped on need basis. This works well on Picasso, Mendocino and Phoenix SoCs. But unfortunately this causes boot hangs in Cezanne SoC. Add a configuration to map the entire SPI ROM and enable it in Cezanne SoC. For other SoCs, keep the configuration disabled so that only the required SPI ROM region is mapped. BUG=b:309690716 TEST=Build and boot to OS in both Dewatt and Skyrim. Change-Id: I166ac7b50b367c067e1a743fc94686e69dd07844 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M src/soc/amd/common/psp_verstage/Kconfig M src/soc/amd/common/psp_verstage/boot_dev.c M src/soc/amd/common/psp_verstage/fch.c M src/soc/amd/common/psp_verstage/psp_verstage.c 4 files changed, 30 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/79155/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79155?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I166ac7b50b367c067e1a743fc94686e69dd07844 Gerrit-Change-Number: 79155 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable
by Karthik Ramasubramanian (Code Review) 21 Nov '23

21 Nov '23

Karthik Ramasubramanian has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/79223?usp=email ) Change subject: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable ...................................................................... Abandoned This branch is stuck in my muscle memory. I am typing this branch without even me realizing it. Sorry for the inconvenience. -- To view, visit https://review.coreboot.org/c/coreboot/+/79223?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: I166ac7b50b367c067e1a743fc94686e69dd07844 Gerrit-Change-Number: 79223 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[S] Change in coreboot[master]: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable
by Karthik Ramasubramanian (Code Review) 21 Nov '23

21 Nov '23

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79223?usp=email ) Change subject: soc/amd/common/psp_verstage: Make SPI ROM mapping configurable ...................................................................... soc/amd/common/psp_verstage: Make SPI ROM mapping configurable Earlier entire SPI ROM was mapped to memory. With limited TLB resources in PSP, this approach hit the limit on systems using 32 MiB SPI ROM. Therefore regions in SPI ROM were mapped on need basis. This works well on Mendocino and Phoenix SoCs. But unfortunately this causes boot hangs in Cezanne SoC. Add a configuration to map the entire SPI ROM and enable it in Cezanne SoC. For other SoCs, keep the configuration disabled so that only the required SPI ROM region is mapped. BUG=b:309690716 TEST=Build and boot to OS in both Dewatt and Skyrim. Change-Id: I166ac7b50b367c067e1a743fc94686e69dd07844 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M src/soc/amd/common/psp_verstage/Kconfig M src/soc/amd/common/psp_verstage/boot_dev.c M src/soc/amd/common/psp_verstage/fch.c M src/soc/amd/common/psp_verstage/psp_verstage.c 4 files changed, 30 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/79223/1 diff --git a/src/soc/amd/common/psp_verstage/Kconfig b/src/soc/amd/common/psp_verstage/Kconfig index dc6ea1c..118ef61 100644 --- a/src/soc/amd/common/psp_verstage/Kconfig +++ b/src/soc/amd/common/psp_verstage/Kconfig @@ -43,3 +43,11 @@ help This configuration indicates whether the PSP Verstage stack is mapped to a virtual address space. This has been the case so far only in Picasso SoC. + +config PSP_VERSTAGE_MAP_ENTIRE_SPIROM + bool + default y if SOC_AMD_CEZANNE + default n + help + This configuration indicates whether PSP Verstage needs to map the entire SPI ROM. + This is required only in Cezanne SoC at the moment. diff --git a/src/soc/amd/common/psp_verstage/boot_dev.c b/src/soc/amd/common/psp_verstage/boot_dev.c index c129479..6ff1e2b 100644 --- a/src/soc/amd/common/psp_verstage/boot_dev.c +++ b/src/soc/amd/common/psp_verstage/boot_dev.c @@ -20,6 +20,8 @@ uint32_t ret; mdev = container_of(rd, __typeof__(*mdev), rdev); + if (CONFIG(PSP_VERSTAGE_MAP_ENTIRE_SPIROM)) + return &(mdev->base[offset]); if (mdev->base) { if ((ret = svc_map_spi_rom(&mdev->base[offset], size, (void **)&mapping)) @@ -37,6 +39,9 @@ { uint32_t ret; + if (CONFIG(PSP_VERSTAGE_MAP_ENTIRE_SPIROM)) + return 0; + active_map_count--; if ((ret = svc_unmap_spi_rom(mapping)) != BL_OK) printk(BIOS_ERR, "Failed(%d) to unmap SPI ROM mapping %p\n", ret, mapping); diff --git a/src/soc/amd/common/psp_verstage/fch.c b/src/soc/amd/common/psp_verstage/fch.c index 0517545..5e46e68 100644 --- a/src/soc/amd/common/psp_verstage/fch.c +++ b/src/soc/amd/common/psp_verstage/fch.c @@ -97,6 +97,13 @@ if (svc_get_spi_rom_info(&spi)) printk(BIOS_DEBUG, "Error getting SPI ROM info.\n"); + if (CONFIG(PSP_VERSTAGE_MAP_ENTIRE_SPIROM) && spi.SpiBiosSmnBase != 0) { + uintptr_t *addr = NULL; + + if (svc_map_spi_rom(spi.SpiBiosSmnBase, CONFIG_ROM_SIZE, (void **)&addr)) + printk(BIOS_DEBUG, "Error mapping SPI ROM to address.\n"); + return addr; + } return spi.SpiBiosSmnBase; } diff --git a/src/soc/amd/common/psp_verstage/psp_verstage.c b/src/soc/amd/common/psp_verstage/psp_verstage.c index 87d126f2..7d9ef35 100644 --- a/src/soc/amd/common/psp_verstage/psp_verstage.c +++ b/src/soc/amd/common/psp_verstage/psp_verstage.c @@ -235,6 +235,7 @@ uint32_t retval; struct vb2_context *ctx = NULL; uint32_t bootmode; + void *boot_dev_base; /* * Do not use printk() before console_init() @@ -350,7 +351,16 @@ if (retval) reboot_into_recovery(ctx, retval); + if (CONFIG(PSP_VERSTAGE_MAP_ENTIRE_SPIROM)) { + post_code(POSTCODE_UNMAP_SPI_ROM); + boot_dev_base = rdev_mmap_full(boot_device_ro()); + if (boot_dev_base) { + if (svc_unmap_spi_rom((void *)boot_dev_base)) + printk(BIOS_ERR, "Error unmapping SPI rom\n"); + } + } assert(!boot_dev_get_active_map_count()); + post_code(POSTCODE_UNMAP_FCH_DEVICES); unmap_fch_devices(); -- To view, visit https://review.coreboot.org/c/coreboot/+/79223?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: I166ac7b50b367c067e1a743fc94686e69dd07844 Gerrit-Change-Number: 79223 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[main]: Documentation: Improve x86_64
by Martin L Roth (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Benjamin Doron. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79160?usp=email ) Change subject: Documentation: Improve x86_64 ...................................................................... Patch Set 1: Code-Review+1 (9 comments) Patchset: PS1: Thanks for doing this. If you don't feel like addressing all of the thoughts I've added, that's fine. What you have here is a definite improvement, so I'd rather see it merged and address any issues later if you don't feel like addressing my comments. File Documentation/arch/x86/x86_64.md: https://review.coreboot.org/c/coreboot/+/79160/comment/79b0f00f_fa396e74 : PS1, Line 3: experimental Do you have any thoughts about how long this is going to remain experimental? What's preventing it from not being experimental? If you're not sure, that's fine - we can bring it up to a wider audience to see about creating a plan. https://review.coreboot.org/c/coreboot/+/79160/comment/0260ce68_e96c0b59 : PS1, Line 10: a *"hack"* which places page tables in ROM Add: "This will be explained further below in the section on page tables"? https://review.coreboot.org/c/coreboot/+/79160/comment/8b141c0f_144aaace : PS1, Line 24: : The large memory model causes the compiler to emit 64bit addressing : instructions, which are not only slower, but also increase code size. Should this be moved to a section comparing advantages and disadvantages between 32 & 64-bit modes? A question I have after reading all of this is "Why would anyone use this? What's the advantage?". Currently it seems to only list problems. https://review.coreboot.org/c/coreboot/+/79160/comment/5eafffdd_24b89a4b : PS1, Line 28: must generate page tables at build time Is this really a toolchain requirement, or just an implementation detail? https://review.coreboot.org/c/coreboot/+/79160/comment/d5a763ad_bc3ade38 : PS1, Line 59: . This config option is enabled if the SOC/CPU selects HAVE_EXP_X86_64_SUPPORT. Here's the list of boards currently selecting that feature. src/cpu/qemu-x86/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/cpu/intel/model_206ax/Kconfig: select HAVE_EXP_X86_64_SUPPORT if USE_NATIVE_RAMINIT src/cpu/intel/model_2065x/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/amd/genoa/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/amd/picasso/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/intel/cannonlake/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/northbridge/intel/x4x/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/northbridge/intel/gm45/Kconfig: select HAVE_EXP_X86_64_SUPPORT I'm not sure I'd list the boards, as done below, just because that list is going to expand pretty rapidly at this point and the documentation will be outdated. https://review.coreboot.org/c/coreboot/+/79160/comment/f59255c6_838a8af3 : PS1, Line 133: qemu I think QEMU should be uppercase unless referring to the directory name in coreboot. https://review.coreboot.org/c/coreboot/+/79160/comment/6e8fdcd9_29a0da76 : PS1, Line 140: Here's a list of known issues: Should this be another subsection, or is this still referring to QEMU? https://review.coreboot.org/c/coreboot/+/79160/comment/2f58018e_2d830b1e : PS1, Line 155: * Entering long mode crashes on AMD host platforms. I don't think this is true anymore. There's was an issue with 32-bit FSPs when using 64-bit mode, but I believe Felix has added code to switch back to 32-bit until we have 64-bit FSPs. Of course with openSIL-based platforms, like the GenoaPOC, the openSIL code is built as part of coreboot, thus in the same mode. -- To view, visit https://review.coreboot.org/c/coreboot/+/79160?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ia5ba51be629a8c878aad64d3297176457cf8e855 Gerrit-Change-Number: 79160 Gerrit-PatchSet: 1 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Comment-Date: Tue, 21 Nov 2023 17:00:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: sb/intel/bd82x6x/pch: Add method to identify PCH
by Patrick Rudolph (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Angel Pons, Paul Menzel. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79145?usp=email ) Change subject: sb/intel/bd82x6x/pch: Add method to identify PCH ...................................................................... Patch Set 3: (4 comments) File src/southbridge/intel/bd82x6x/pch.c: https://review.coreboot.org/c/coreboot/+/79145/comment/8d5934a2_e2b9b10c : PS2, Line 47: {0x1C43, 0x1C47, 0x1C49, 0x1C4B, 0x1C4F, 0x1C41, : 0x1C4D, 0x1E42, 0x1E55, 0x1E5D, 0x1E57, 0x1E58, : 0x1E59, 0x1E43, 0x1E56}; > nit: Format these values in 2 lines for CPT/PPT? I also sorted the IDs numerically. […] Done https://review.coreboot.org/c/coreboot/+/79145/comment/7ded8ef0_8de43b14 : PS2, Line 52: 1 > true Done https://review.coreboot.org/c/coreboot/+/79145/comment/bb0f679a_1bf47301 : PS2, Line 53: 0 > false Done https://review.coreboot.org/c/coreboot/+/79145/comment/f6f80160_7fa0199f : PS2, Line 14: int pch_silicon_revision(void) : { : static int pch_revision_id = -1; : : #ifdef __SIMPLE_DEVICE__ : pci_devfn_t dev = PCI_DEV(0, 0x1f, 0); : #else : struct device *dev = pcidev_on_root(0x1f, 0); : #endif : : if (pch_revision_id < 0) : pch_revision_id = pci_read_config8(dev, PCI_REVISION_ID); : return pch_revision_id; : } : : int pch_silicon_type(void) : { : static int pch_type = -1; : : #ifdef __SIMPLE_DEVICE__ : pci_devfn_t dev = PCI_DEV(0, 0x1f, 0); : #else : struct device *dev = pcidev_on_root(0x1f, 0); : #endif : : if (pch_type < 0) : pch_type = pci_read_config8(dev, PCI_DEVICE_ID + 1); : return pch_type; : } : : bool pch_is_mobile(void) : { : const u16 devid = pci_s_read_config16(PCH_LPC_DEV, PCI_DEVICE_ID); : const u16 devids[] = {0x1C43, 0x1C47, 0x1C49, 0x1C4B, 0x1C4F, 0x1C41, : 0x1C4D, 0x1E42, 0x1E55, 0x1E5D, 0x1E57, 0x1E58, : 0x1E59, 0x1E43, 0x1E56}; : for (size_t i = 0; i < ARRAY_SIZE(devids); i++) : if (devid == devids[i]) : return 1; : return 0; : } > Maybe these functions should be in a separate file, so that `__SIMPLE_DEVICE__` is not a problem? My plan was to revive https://review.coreboot.org/c/coreboot/+/49167 after this is merged. -- To view, visit https://review.coreboot.org/c/coreboot/+/79145?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ic3e15c1d8d4b1d1012d6204cc65de92d91431fbe Gerrit-Change-Number: 79145 Gerrit-PatchSet: 3 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 21 Nov 2023 16:29:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: sb/intel/bd82x6x/early_usb: Print error for invalid USB setting
by Patrick Rudolph (Code Review) 21 Nov '23

21 Nov '23

Attention is currently required from: Angel Pons, Paul Menzel. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78827?usp=email ) Change subject: sb/intel/bd82x6x/early_usb: Print error for invalid USB setting ...................................................................... Patch Set 8: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/78827/comment/ae7e45d3_2aa43dff : PS8, Line 15: Tested: Lenovo X220 still boots. > Isn't this a laptop? This change should be a no-op for mobile PCHs. That's correct. File src/southbridge/intel/bd82x6x/early_usb.c: https://review.coreboot.org/c/coreboot/+/78827/comment/3a6a0935_cc9e027a : PS8, Line 28: if (!pch_is_mobile() && portmap[i].current == 0) { > We may want to skip complaining about disabled ports. […] Done https://review.coreboot.org/c/coreboot/+/78827/comment/d439d065_cd8b9a04 : PS8, Line 29: Consult the OEM for correct settings > As most Sandy/Ivy Bridge ports are retrofits (people add coreboot support without OEM involvement), […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/78827?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If76e9126b4aba8e16c1c91dece725aac12e1a7e9 Gerrit-Change-Number: 78827 Gerrit-PatchSet: 8 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 21 Nov 2023 16:25:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2023