Attention is currently required from: Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81958?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: soc/intel/xeon_sp: Add soc_add_dram_resources
......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1:
Please do not submit before the new implementation is under review.
I always find refactorings hard to assess when new implementations
are unknown.
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/0ca9e30c_86eadb34 :
PS1, Line 234: * @param mc_values List of system memory map variables.
Why pass the whole array? This makes the API more complicated than necessary.
AIUI, the purpose of this functions is to add DRAM regions between 4G and
the absolute top. This would only need the top value.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/9b20d96c_8f4cc357 :
PS1, Line 306: } else {
So there were already two different implementations. One without and one
with CXL. IOW, a gen1 and a gen4 implementation. If we extract this into
a separate function, with API and everything. Shouldn't we be consistent
and split this right away?
https://review.coreboot.org/c/coreboot/+/81958/comment/f4b2d25b_ac764074 :
PS1, Line 274: index +=
Returning the count and adding it up is uncommon. And if reading the whole code,
it looks odd: Last thing before the return is to subtract the old index, and the
very next thing here is to add it again. It's more common to return the new value
or passing a pointer to change the variable directly.
It's also odd to have one style for mc_add_dram_resources() (passing a pointer)
and then doing it differently for soc_add_dram_resources() (returning the count).
Please choose one way of doing it.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81958?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: Ie15b11e1f4cdc861324286b1397f9c5e431b74ab
Gerrit-Change-Number: 81958
Gerrit-PatchSet: 1
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 20:35:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Elyes Haouas.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81974?usp=email )
Change subject: drivers/wifi/generic: Drop unused symbol
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
It's a typo, the Makefile.mk was changed late during review, don't know why.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81974?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: Icdd0332ae9c56a54596a775c0a9aa7b9f8d6738c
Gerrit-Change-Number: 81974
Gerrit-PatchSet: 1
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Wed, 17 Apr 2024 18:59:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support
......................................................................
arch/x86: Enable long mode entry into payload for x86_64 support
This patch allows coreboot to enter libpayload in long mode (64-bit)
instead of protected mode (32-bit), enabling support for upcoming Intel
platforms and payloads requiring access to memory beyond 4GB.
This change ensures compatibility between libpayload and depthcharge,
preventing compilation issues and stack misalignment during the
transition.
Introduces a new Kconfig named `PAYLOAD_X86_64_SUPPORT` to ensure user
can specify the intention to enter into payload in 64-bit without
trunking into 32-bit mode.
BUG=b:242829490
TEST=Entered libpayload in long mode, successfully parsed coreboot
table.
Change-Id: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
---
M src/arch/x86/Kconfig
M src/arch/x86/boot.c
2 files changed, 23 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/81960/1
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index 7d25561..aeb2670 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -81,6 +81,18 @@
usual protected flat (32-bit) mode. 64-bit CPUs and OSes can be used
irrespective of whether coreboot runs in 32-bit or 64-bit mode.
+config PAYLOAD_X86_64_SUPPORT
+ bool
+ depends on HAVE_X86_64_SUPPORT
+ help
+ Enable this option to load and run payloads in 64-bit mode (also known as "long mode").
+
+ This has the following effects:
+ * **Libpayload:** Operates in 64-bit mode.
+ * **Payloads:** Allows the use of 64-bit payloads (e.g., deptcharge requires 64-bit mode).
+
+ If unsure, leave this option disabled (default is 32-bit mode).
+
config PAGE_TABLES_IN_CBFS
bool
default n
diff --git a/src/arch/x86/boot.c b/src/arch/x86/boot.c
index 90af84f..d3ae40a 100644
--- a/src/arch/x86/boot.c
+++ b/src/arch/x86/boot.c
@@ -22,11 +22,18 @@
void arch_prog_run(struct prog *prog)
{
#if ENV_RAMSTAGE && ENV_X86_64
- const uint32_t arg = pointer_to_uint32_safe(prog_entry_arg(prog));
- const uint32_t entry = pointer_to_uint32_safe(prog_entry(prog));
+ if (CONFIG(PAYLOAD_X86_64_SUPPORT)) {
+ void (*doit)(void *arg);
+ doit = prog_entry(prog);
+ /* coreboot is loading payload in long mode */
+ doit(prog_entry_arg(prog));
+ } else {
+ const uint32_t arg = pointer_to_uint32_safe(prog_entry_arg(prog));
+ const uint32_t entry = pointer_to_uint32_safe(prog_entry(prog));
- /* On x86 coreboot payloads expect to be called in protected mode */
- protected_mode_jump(entry, arg);
+ /* On x86 coreboot payloads expect to be called in protected mode */
+ protected_mode_jump(entry, arg);
+ }
#else
#if ENV_X86_64
void (*doit)(void *arg);
--
To view, visit https://review.coreboot.org/c/coreboot/+/81960?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: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Gerrit-Change-Number: 81960
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Arthur Heymans, Ashish Kumar Mishra, Jérémy Compostella, Patrick Rudolph, Saurabh Mishra, Subrata Banik.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81959?usp=email )
Change subject: [MTL-x64]arch/x86: Update X86_64 memcpy for 4 byte copy
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> > OK, unless it can be proven that `len` is always a multiple of 4, this is actually quite tricky to […]
Note that the first loop in each function works with DWORDs (32-bit values), which is why I can use `ctx->mmio_base + SPIBAR_FDATA(i)`. The alternative would've been to write `ctx->mmio_base + SPIBAR_FDATA(0) + i * 4`.
All `4` are equivalent to `sizeof(uint32_t)` and might even make the code more readable
--
To view, visit https://review.coreboot.org/c/coreboot/+/81959?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: I1110cc18f5aa59c864e3cc04b9e6b3501ec4d54d
Gerrit-Change-Number: 81959
Gerrit-PatchSet: 3
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 17 Apr 2024 18:26:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions
......................................................................
Patch Set 18:
(1 comment)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/aa8cbce4_6b108535 :
PS18, Line 175: ""
> That would mean that all functions that work on the properties of a node always get the offset to the start of the properties instead of the offset to the node. Seems weird to be honest since you usually traverse all properties from a given node to get the property you want. I am also not sure if the naming scheme is correct then.
I thought you might want to avoid parsing the name twice for each level, because when you have a function that looks for a name the caller isn't interested in reading the name anymore afterwards. If you don't mind that inefficiency it's easy to adapt my suggestion to return the offset to the start of the node instead. (Same for the argument that you pass into `fdt_find_node()`, passing the property offset avoids reading the name in both the caller and callee instance of the function, but if you prefer passing the node offset for aesthetic reasons it's easy to write it that way as well.)
> The idea originally was that you could continue the search from a given node without having to traverse all children nodes again.
Okay, I see. But it sounds like we agree that that doesn't make sense anymore after `fdt_find_subnodes_by_prefix()` became a separate function? So let's design `fdt_find_node()` such that it is as equivalent as possible in what it does to `dt_find_node()`, I think that makes understanding the design much easier.
> Would `skip_name` imply that that the offset given to `fdt_skip_node()` points to the first property of the node instead of the name?
Yes, that was my idea, just make it skip the `fdt_next_node_name()` call. Anyway, I mostly proposed this because it sounded like you were very interested in maximizing performance for this code, but I understand now that you were referring to something else with that. If you don't think a few extra `strlen()` here and there matter, feel free to ignore those microoptimzations, I don't care that much either way.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81081?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: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8
Gerrit-Change-Number: 81081
Gerrit-PatchSet: 18
Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 18:22:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81930?usp=email )
(
1 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: sb/intel/ibexpeak: Drop USB3 settings from devicetree
......................................................................
sb/intel/ibexpeak: Drop USB3 settings from devicetree
ibexpeak has no USB 3 capabilities.
They were kept briefly when its devicetree structure was split from
bd82x6x in commit ab4de83f4330 ("sb/intel/ibexpeak: Sever bd82x6x
source dependency") to verify correctness. With that done, they
can go.
Change-Id: I6b847e1532d2e84a7b408a8858c8613b322d0373
Signed-off-by: Keith Hui <buurin(a)gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81930
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
---
M src/southbridge/intel/ibexpeak/chip.h
1 file changed, 0 insertions(+), 6 deletions(-)
Approvals:
build bot (Jenkins): Verified
Felix Singer: Looks good to me, approved
diff --git a/src/southbridge/intel/ibexpeak/chip.h b/src/southbridge/intel/ibexpeak/chip.h
index aba27cc..dc97da3 100644
--- a/src/southbridge/intel/ibexpeak/chip.h
+++ b/src/southbridge/intel/ibexpeak/chip.h
@@ -68,12 +68,6 @@
bool pcie_hotplug_map[8];
- /* These USB3 fields, copied from bd82x6x, don't apply here,
- * as Ibex Peak doesn't have USB3. */
- uint32_t xhci_switchable_ports;
- uint32_t superspeed_capable_ports;
- uint32_t xhci_overcurrent_mapping;
-
uint32_t spi_uvscc;
uint32_t spi_lvscc;
struct intel_swseq_spi_config spi;
--
To view, visit https://review.coreboot.org/c/coreboot/+/81930?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: I6b847e1532d2e84a7b408a8858c8613b322d0373
Gerrit-Change-Number: 81930
Gerrit-PatchSet: 3
Gerrit-Owner: Keith Hui <buurin(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged