Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Add new board configuration entry to coreboot tables ......................................................................
[RFC] Add new board configuration entry to coreboot tables
The new entry is intended to be the configuration entry to rule them all, possibly deprecating the three separate "strapping ID" tables with which it is slightly redundant with. In one entry, it should provide enough information to give firmware the information it needs at runtime to make decisions based on what kind of board it is or what is attached to it, all in one place.
The new entry contains: - Board ID - DRAM ID (RAM code) - SKU ID - FW Config
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 4 files changed, 59 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/1
diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index bf8c0d9..fc10031 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -79,6 +79,7 @@ CB_TAG_MMC_INFO = 0x0035, CB_TAG_TCPA_LOG = 0x0036, CB_TAG_FMAP = 0x0037, + CB_TAG_BOARD_CONFIG = 0x0039, CB_TAG_CMOS_OPTION_TABLE = 0x00c8, CB_TAG_OPTION = 0x00c9, CB_TAG_OPTION_ENUM = 0x00ca, @@ -316,6 +317,16 @@ int32_t early_cmd1_status; };
+struct cb_board_config { + uint32_t tag; + uint32_t size; + + struct cbuint64 fw_config; + uint32_t board_id; + uint32_t dram_id; + uint32_t sku_id; +}; + #define CB_MAX_SERIALNO_LENGTH 32
struct cb_cmos_option_table { diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 6393c01..4fb7318 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -80,6 +80,8 @@ LB_TAG_TCPA_LOG = 0x0036, LB_TAG_FMAP = 0x0037, LB_TAG_PLATFORM_BLOB_VERSION = 0x0038, + LB_TAG_BOARD_CONFIG = 0x0039, + /* The following options are CMOS-related */ LB_TAG_CMOS_OPTION_TABLE = 0x00c8, LB_TAG_OPTION = 0x00c9, LB_TAG_OPTION_ENUM = 0x00ca, @@ -415,6 +417,16 @@ struct mac_address mac_addrs[0]; };
+struct lb_board_config { + uint32_t tag; + uint32_t size; + + struct lb_uint64 fw_config; + uint32_t board_id; + uint32_t dram_id; + uint32_t sku_id; +}; + #define MAX_SERIALNO_LENGTH 32
/* The following structures are for the CMOS definitions table */ diff --git a/src/include/fw_config.h b/src/include/fw_config.h index c9ef75f..7eaad7d 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -8,6 +8,8 @@ #include <stdbool.h> #include <stdint.h>
+#define DEFAULT_FW_CONFIG (uint64_t)0 + /** * struct fw_config - Firmware configuration field and option. * @field_name: Name of the field that this option belongs to. diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 9148405..82b718a 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -1,22 +1,23 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <arch/cbconfig.h> -#include <console/console.h> -#include <console/uart.h> -#include <ip_checksum.h> +#include <boardid.h> #include <boot/coreboot_tables.h> #include <boot/tables.h> #include <boot_device.h> -#include <string.h> -#include <version.h> -#include <boardid.h> -#include <device/device.h> -#include <fmap.h> -#include <stdlib.h> -#include <cbfs.h> -#include <cbmem.h> #include <bootmem.h> #include <bootsplash.h> +#include <cbfs.h> +#include <cbmem.h> +#include <console/console.h> +#include <console/uart.h> +#include <device/device.h> +#include <fmap.h> +#include <fw_config.h> +#include <ip_checksum.h> +#include <stdlib.h> +#include <string.h> +#include <version.h> #include <spi_flash.h> #include <security/vboot/misc.h> #include <security/vboot/vbnv_layout.h> @@ -211,6 +212,7 @@ __weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; } __weak uint32_t ram_code(void) { return UNDEFINED_STRAPPING_ID; } __weak uint32_t sku_id(void) { return UNDEFINED_STRAPPING_ID; } +__weak uint64_t fw_config_get(void) { return DEFAULT_FW_CONFIG; }
static void lb_board_id(struct lb_header *header) { @@ -368,6 +370,24 @@ return mainboard; }
+static struct lb_board_config *lb_board_config(struct lb_header *header) +{ + struct lb_record *rec; + struct lb_board_config *config; + rec = lb_new_record(header); + config = (struct lb_board_config *)rec; + + config->tag = LB_TAG_BOARD_CONFIG; + config->size = sizeof(*config); + + config->board_id = board_id(); + config->dram_id = ram_code(); + config->sku_id = sku_id(); + config->fw_config = pack_lb64(fw_config_get()); + + return config; +} + #if CONFIG(USE_OPTION_TABLE) static struct cmos_checksum *lb_cmos_checksum(struct lb_header *header) { @@ -557,6 +577,9 @@
lb_boot_media_params(head);
+ /* Board configuration information */ + lb_board_config(head); + /* Add architecture records. */ lb_arch_add_records(head);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Add new board configuration entry to coreboot tables ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... PS1, Line 83: 0x0039 Could you use 0x0040 instead, please? CB:40520 uses 0x0039 and already had to be bumped once.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Add new board configuration entry to coreboot tables ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/1//COMMIT_MSG@10 PS1, Line 10: possibly deprecating the three separate "strapping ID" tables with : which it is slightly redundant with. Let's do that deprecation right away in the same patch?
https://review.coreboot.org/c/coreboot/+/46605/1/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/46605/1/src/include/fw_config.h@11 PS1, Line 11: #define DEFAULT_FW_CONFIG (uint64_t)0 Can we please use the existing UNDEFINED_STRAPPING_ID (~0, or now better ~(uint64_t)0) for this? For CBI-based fw_config this doesn't matter, but if other boards later use strapping-based fw_config then 0 will be a valid value that they may want to use for a real config (since there's often only very few values to use anyway). ~0 is usually safe because nobody designs a strapping with all 64 pins.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Add new board configuration entry to coreboot tables ......................................................................
Patch Set 1:
(1 comment)
Maybe ask for comments on the mailing list?
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h> Sort it in a separate patch, which can be submitted without discussion and makes the diff smaller.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#2).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/boardid.h M src/include/fw_config.h M src/lib/coreboot_table.c 7 files changed, 64 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46605/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/1//COMMIT_MSG@10 PS1, Line 10: possibly deprecating the three separate "strapping ID" tables with : which it is slightly redundant with.
Let's do that deprecation right away in the same patch?
Ack
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... PS1, Line 83: 0x0039
Could you use 0x0040 instead, please? CB:40520 uses 0x0039 and already had to be bumped once.
Oops sorry about that, will bump up.
https://review.coreboot.org/c/coreboot/+/46605/1/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/46605/1/src/include/fw_config.h@11 PS1, Line 11: #define DEFAULT_FW_CONFIG (uint64_t)0
Can we please use the existing UNDEFINED_STRAPPING_ID (~0, or now better ~(uint64_t)0) for this? For […]
Done
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
Sort it in a separate patch, which can be submitted without discussion and makes the diff smaller.
Ack
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#3).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/boardid.h M src/include/fw_config.h M src/lib/coreboot_table.c 7 files changed, 66 insertions(+), 95 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/3
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#4).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/boardid.h M src/include/fw_config.h M src/lib/coreboot_table.c 7 files changed, 69 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/4
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#5).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/boardid.h M src/include/fw_config.h M src/lib/coreboot_table.c 7 files changed, 65 insertions(+), 92 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 263: struct cb_strapping_id { Remove?
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 326: dram_id Since we're keeping the name ram_code in sysinfo (and we probably should, since external payloads may depend on it), shouldn't we just keep using that name in your new code as well for consistency?
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0) I think you can just change the existing macro to 64-bit, it will auto-convert down to ~0 when assigned to a 32-bit variable.
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
Ack
Paul, just as a general thought, for these little drive-by cleanups I think we should be careful to not give people too much trouble with them or we're just going to discourage them from being done at all. In general you are right that separate concerns belong in separate patches, but I think a small significance threshold needs to be met for that to apply. There certainly is *some* extra effort involved every time you split a patch and if you force people to do that for very trivial things, they may just not do them at all instead. I'd rather have our headers occasionally get sorted in the "wrong" patch than never.
(Case in point, Tim followed your suggestion for this patch but I don't think he actually uploaded a separate one for the headers. ;) )
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
Paul, just as a general thought, for these little drive-by cleanups I think we should be careful to […]
Uh, why sort at all? And why sort only a part of the list?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/1/src/commonlib/include/commo... PS1, Line 83: 0x0039
Oops sorry about that, will bump up.
Done
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#6).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/boardid.h M src/include/fw_config.h M src/lib/coreboot_table.c 7 files changed, 65 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 263: struct cb_strapping_id {
Remove?
Done
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 326: dram_id
Since we're keeping the name ram_code in sysinfo (and we probably should, since external payloads ma […]
Fair point, makes discovery of it easier too.
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0)
I think you can just change the existing macro to 64-bit, it will auto-convert down to ~0 when assig […]
I tried that, gcc wasn't happy: ``` In file included from src/lib/coreboot_table.c:12: src/lib/coreboot_table.c: In function 'board_id': src/include/boardid.h:8:36: error: conversion from 'long long unsigned int' to 'uint32_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Werror=overflow] #define UNDEFINED_STRAPPING_ID ~((uint64_t)0) ^ src/lib/coreboot_table.c:213:41: note: in expansion of macro 'UNDEFINED_STRAPPING_ID' __weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; }
```
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
Uh, why sort at all? And why sort only a part of the list?
If I ever get too annoyed by headers being out-of-order, I sort them beforehand.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 6: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0)
I tried that, gcc wasn't happy: […]
Stupid warnings, why don't we just disable them... okay fair enough. ^^
In that case, maybe it's better to name it UNDEFINED_FW_CONFIG or something similar to what you had earlier? That might be easier to follow than having to keep track of which value is 32-bit and which is 64-bit.
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
If I ever get too annoyed by headers being out-of-order, I sort them beforehand.
I makes it easier to binary search with your eyes and see if a specific header is already in there? Idk, it's something that many people have been implicitly doing and asking for on patches, it's not in the official coding style so if you don't think we should do it we could have that discussion as well. (I agree that the specific order in this patch set was weird, and when we sort we should sort everything other than keeping <> and "" headers separate.)
I wanted to say that in a more general sense though, not just in relation to header sorting. People often mix trivial code cleanups that otherwise nobody would bother to do at all into functional patches that touch the surrounding area, and I think to a certain extent we should avoid discouraging that.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#7).
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
[RFC] Replace strapping_id entries with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 65 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/7
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 7: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG@7 PS7, Line 7: [RFC] Remove?
https://review.coreboot.org/c/coreboot/+/46605/7/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/7/src/lib/coreboot_table.c@40 PS7, Line 40: ?
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#8).
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
lib/libpayload: Replace strapping_ids with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 64 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/8
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 8: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG@7 PS7, Line 7: [RFC]
Remove?
There's an ongoing thread on the ML: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/BNCPZ...
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Angel Pons, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#9).
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
lib/libpayload: Replace strapping_ids with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 64 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/9
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0)
Stupid warnings, why don't we just disable them... okay fair enough. ^^ […]
Done
https://review.coreboot.org/c/coreboot/+/46605/7/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/7/src/lib/coreboot_table.c@40 PS7, Line 40:
?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... PS9, Line 111: UNDEFINED_FW_CONFIG Is the payload expected to test whether the fw_config is all 1s before treating it as a valid value? One special thing about fw_config compared to the other IDs is that it is a bit-field member and so the payload will have to be careful about treating all 1s as all 0s. Should we just set the UNDEFINED_FW_CONFIG as all 0s instead here?
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID) One slight change in behavior with the new code is that all the IDs are now added to coreboot tables even if the value is UNDEFINED_STRAPPING_ID. I am guessing there are no side-effects of doing that?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... PS9, Line 111: UNDEFINED_FW_CONFIG
Is the payload expected to test whether the fw_config is all 1s before treating it as a valid value?
Yep.
One special thing about fw_config compared to the other IDs is that it is a bit-field member and so the payload will have to be careful about treating all 1s as all 0s. Should we just set the UNDEFINED_FW_CONFIG as all 0s instead here?
We could, it means that you can't distinguish between a board that doesn't provide a source for FW_CONFIG (so this code will use UNDEFINED_) versus one that has it, but is defined as all 0s.
In our case, we'll have to check lib_sysinfo.fw_config one way or the other in the payload, to know which case is the "I'm empty, enable everything" default case.
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
One slight change in behavior with the new code is that all the IDs are now added to coreboot tables […]
I don't believe so; libpayload parses the entire table and all of the entries found into fields in the `lib_sysinfo` object. I don't think the payload is supposed to care whether an entry exists or not, only the values in lib_sysinfo.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... PS9, Line 111: UNDEFINED_FW_CONFIG
Is the payload expected to test whether the fw_config is all 1s before treating it as a valid valu […]
Agreed. As long as we have the payload check the invalid/undefined case, it should be fine.
I think it would be good to at least add a comment indicating that the payload is responsible for ensuring that it checks against UNDEFINED_FW_CONFIG before using the bits set in fw_config field.
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
I don't believe so; libpayload parses the entire table and all of the entries found into fields in t […]
Right. Before this change that value in lib_sysinfo would be left as 0 and now it is changed to UNDEFINED_STRAPPING_ID. Probably not an issue, but just pointing out the change in behavior.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Angel Pons, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#10).
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
lib/libpayload: Replace strapping_ids with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Note: If a mainboard never implemented the three weak functions for the current strapping IDs, their values in the lib_sysinfo object will now be UNDEFINED_STRAPPING_ID instead of 0.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 69 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/10
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... PS9, Line 111: UNDEFINED_FW_CONFIG
Agreed. As long as we have the payload check the invalid/undefined case, it should be fine. […]
Sure.
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
Right. […]
I'll add a note in the commit msg; I think if this changes any payload's behavior, then there's a bug in the payload (it was depending on data without explicitly asking for it in coreboot).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/libc/co... File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/libc/co... PS9, Line 283: info->sku_id = UNDEFINED_STRAPPING_ID; nit: Also initialize fw_config here, so things would be sane if we leave out the entry?
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
Right. […]
Libpayload actually initialized all lib_sysinfo strapping fields to UNDEFINED_STRAPPING_ID already in case the coreboot table entries were missing, see near the top of cb_parse_header(). Depthcharge depends on that.
The only thing you could decide here is whether you want to omit the coreboot table entry when all four strappings are UNDEFINED...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
Libpayload actually initialized all lib_sysinfo strapping fields to UNDEFINED_STRAPPING_ID already i […]
Aah. That looks good. I had missed that we already use UNDEFINED_STRAPPING_ID as the default value for these IDs. Ignore my earlier comment.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Duncan Laurie, Julius Werner, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46605
to look at the new patch set (#11).
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
lib/libpayload: Replace strapping_ids with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 70 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46605/11
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/libc/co... File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/libc/co... PS9, Line 283: info->sku_id = UNDEFINED_STRAPPING_ID;
nit: Also initialize fw_config here, so things would be sane if we leave out the entry?
Done
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
Aah. That looks good. […]
Oops I missed that too.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 11: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 11: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG@7 PS7, Line 7: [RFC]
There's an ongoing thread on the ML: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot. […]
I guess it's no longer a blocker
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46605/7//COMMIT_MSG@7 PS7, Line 7: [RFC]
I guess it's no longer a blocker
Actually, I never said resolving the ML thread was a requirement to submit this change. I merely pointed out that discussion was taking place outside of Gerrit.
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
lib/libpayload: Replace strapping_ids with new board configuration entry
There are currently 3 different strapping ID entries in the coreboot table, which adds overhead. The new fw_config field is also desired in the coreboot table, which is another kind of strapping id. Therefore, this patch deprecates the 3 current strapping ID entries (board ID, RAM code, and SKU ID), and adds a new entry ("board_config") which provides board ID, RAM code, SKU ID, as well as FW_CONFIG together.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I1ecec847ee77b72233587c1ad7f124e2027470bf Reviewed-on: https://review.coreboot.org/c/coreboot/+/46605 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/include/fw_config.h M src/lib/coreboot_table.c 6 files changed, 70 insertions(+), 97 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index bfdd21e..64db83b 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -80,6 +80,7 @@ CB_TAG_TCPA_LOG = 0x0036, CB_TAG_FMAP = 0x0037, CB_TAG_SMMSTOREV2 = 0x0039, + CB_TAG_BOARD_CONFIG = 0x0040, CB_TAG_CMOS_OPTION_TABLE = 0x00c8, CB_TAG_OPTION = 0x00c9, CB_TAG_OPTION_ENUM = 0x00ca, @@ -260,12 +261,6 @@ uint32_t index; };
-struct cb_strapping_id { - uint32_t tag; - uint32_t size; - uint32_t id_code; -}; - struct cb_spi_flash { uint32_t tag; uint32_t size; @@ -317,6 +312,16 @@ int32_t early_cmd1_status; };
+struct cb_board_config { + uint32_t tag; + uint32_t size; + + struct cbuint64 fw_config; + uint32_t board_id; + uint32_t ram_code; + uint32_t sku_id; +}; + #define CB_MAX_SERIALNO_LENGTH 32
struct cb_cmos_option_table { diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index a3f61e7..dd739ab 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -107,11 +107,18 @@ uintptr_t mrc_cache; uintptr_t acpi_gnvs;
-#define UNDEFINED_STRAPPING_ID (~0) +#define UNDEFINED_STRAPPING_ID (~0) +#define UNDEFINED_FW_CONFIG ~((uint64_t)0) u32 board_id; u32 ram_code; u32 sku_id;
+ /* + * A payload using this field is responsible for ensuring it checks its + * value against UNDEFINED_FW_CONFIG before using it. + */ + u64 fw_config; + uintptr_t wifi_calibration; uint64_t ramoops_buffer; uint32_t ramoops_buffer_size; diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index c48b6cf..b7d2a53 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -143,22 +143,13 @@ info->acpi_gnvs = get_cbmem_addr(ptr); }
-static void cb_parse_board_id(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_board_config(unsigned char *ptr, struct sysinfo_t *info) { - struct cb_strapping_id *const cbbid = (struct cb_strapping_id *)ptr; - info->board_id = cbbid->id_code; -} - -static void cb_parse_ram_code(unsigned char *ptr, struct sysinfo_t *info) -{ - struct cb_strapping_id *const ram_code = (struct cb_strapping_id *)ptr; - info->ram_code = ram_code->id_code; -} - -static void cb_parse_sku_id(unsigned char *ptr, struct sysinfo_t *info) -{ - struct cb_strapping_id *const sku_id = (struct cb_strapping_id *)ptr; - info->sku_id = sku_id->id_code; + struct cb_board_config *const config = (struct cb_board_config *)ptr; + info->fw_config = cb_unpack64(config->fw_config); + info->board_id = config->board_id; + info->ram_code = config->ram_code; + info->sku_id = config->sku_id; }
#if CONFIG(LP_NVRAM) @@ -290,6 +281,7 @@ info->board_id = UNDEFINED_STRAPPING_ID; info->ram_code = UNDEFINED_STRAPPING_ID; info->sku_id = UNDEFINED_STRAPPING_ID; + info->fw_config = UNDEFINED_FW_CONFIG;
/* Now, walk the tables. */ ptr += header->header_bytes; @@ -381,14 +373,8 @@ case CB_TAG_ACPI_GNVS: cb_parse_acpi_gnvs(ptr, info); break; - case CB_TAG_BOARD_ID: - cb_parse_board_id(ptr, info); - break; - case CB_TAG_RAM_CODE: - cb_parse_ram_code(ptr, info); - break; - case CB_TAG_SKU_ID: - cb_parse_sku_id(ptr, info); + case CB_TAG_BOARD_CONFIG: + cb_parse_board_config(ptr, info); break; case CB_TAG_WIFI_CALIBRATION: cb_parse_wifi_calibration(ptr, info); diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 4406002..3e74e6b 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -62,15 +62,15 @@ LB_TAG_DMA = 0x0022, LB_TAG_RAM_OOPS = 0x0023, LB_TAG_ACPI_GNVS = 0x0024, - LB_TAG_BOARD_ID = 0x0025, + LB_TAG_BOARD_ID = 0x0025, /* deprecated */ LB_TAG_VERSION_TIMESTAMP = 0x0026, LB_TAG_WIFI_CALIBRATION = 0x0027, - LB_TAG_RAM_CODE = 0x0028, + LB_TAG_RAM_CODE = 0x0028, /* deprecated */ LB_TAG_SPI_FLASH = 0x0029, LB_TAG_SERIALNO = 0x002a, LB_TAG_MTC = 0x002b, LB_TAG_VPD = 0x002c, - LB_TAG_SKU_ID = 0x002d, + LB_TAG_SKU_ID = 0x002d, /* deprecated */ LB_TAG_BOOT_MEDIA_PARAMS = 0x0030, LB_TAG_CBMEM_ENTRY = 0x0031, LB_TAG_TSC_INFO = 0x0032, @@ -81,6 +81,8 @@ LB_TAG_FMAP = 0x0037, LB_TAG_PLATFORM_BLOB_VERSION = 0x0038, LB_TAG_SMMSTOREV2 = 0x0039, + LB_TAG_BOARD_CONFIG = 0x0040, + /* The following options are CMOS-related */ LB_TAG_CMOS_OPTION_TABLE = 0x00c8, LB_TAG_OPTION = 0x00c9, LB_TAG_OPTION_ENUM = 0x00ca, @@ -347,12 +349,6 @@ uint32_t index; };
-struct lb_strapping_id { - uint32_t tag; - uint32_t size; - uint32_t id_code; -}; - struct lb_spi_flash { uint32_t tag; uint32_t size; @@ -416,6 +412,16 @@ struct mac_address mac_addrs[0]; };
+struct lb_board_config { + uint32_t tag; + uint32_t size; + + struct lb_uint64 fw_config; + uint32_t board_id; + uint32_t ram_code; + uint32_t sku_id; +}; + #define MAX_SERIALNO_LENGTH 32
/* The following structures are for the CMOS definitions table */ diff --git a/src/include/fw_config.h b/src/include/fw_config.h index c9ef75f..3c87725 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -8,6 +8,8 @@ #include <stdbool.h> #include <stdint.h>
+#define UNDEFINED_FW_CONFIG ~((uint64_t)0) + /** * struct fw_config - Firmware configuration field and option. * @field_name: Name of the field that this option belongs to. @@ -30,8 +32,6 @@ .value = FW_CONFIG_FIELD_##__field##_OPTION_##__option##_VALUE \ })
-#if CONFIG(FW_CONFIG) - /** * fw_config_get() - Provide firmware configuration value. * @@ -39,6 +39,8 @@ */ uint64_t fw_config_get(void);
+#if CONFIG(FW_CONFIG) + /** * fw_config_probe() - Check if field and option matches. * @match: Structure containing field and option to probe. diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 857f5a5..69ded3c 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -12,6 +12,7 @@ #include <boardid.h> #include <device/device.h> #include <fmap.h> +#include <fw_config.h> #include <stdlib.h> #include <cbfs.h> #include <cbmem.h> @@ -213,23 +214,7 @@ __weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; } __weak uint32_t ram_code(void) { return UNDEFINED_STRAPPING_ID; } __weak uint32_t sku_id(void) { return UNDEFINED_STRAPPING_ID; } - -static void lb_board_id(struct lb_header *header) -{ - struct lb_strapping_id *rec; - uint32_t bid = board_id(); - - if (bid == UNDEFINED_STRAPPING_ID) - return; - - rec = (struct lb_strapping_id *)lb_new_record(header); - - rec->tag = LB_TAG_BOARD_ID; - rec->size = sizeof(*rec); - rec->id_code = bid; - - printk(BIOS_INFO, "Board ID: %d\n", bid); -} +__weak uint64_t fw_config_get(void) { return UNDEFINED_FW_CONFIG; }
static void lb_boot_media_params(struct lb_header *header) { @@ -257,40 +242,6 @@ bmp->fmap_offset = get_fmap_flash_offset(); }
-static void lb_ram_code(struct lb_header *header) -{ - struct lb_strapping_id *rec; - uint32_t code = ram_code(); - - if (code == UNDEFINED_STRAPPING_ID) - return; - - rec = (struct lb_strapping_id *)lb_new_record(header); - - rec->tag = LB_TAG_RAM_CODE; - rec->size = sizeof(*rec); - rec->id_code = code; - - printk(BIOS_INFO, "RAM code: %d\n", code); -} - -static void lb_sku_id(struct lb_header *header) -{ - struct lb_strapping_id *rec; - uint32_t sid = sku_id(); - - if (sid == UNDEFINED_STRAPPING_ID) - return; - - rec = (struct lb_strapping_id *)lb_new_record(header); - - rec->tag = LB_TAG_SKU_ID; - rec->size = sizeof(*rec); - rec->id_code = sid; - - printk(BIOS_INFO, "SKU ID: %d\n", sid); -} - static void lb_mmc_info(struct lb_header *header) { struct lb_mmc_info *rec; @@ -370,6 +321,24 @@ return mainboard; }
+static struct lb_board_config *lb_board_config(struct lb_header *header) +{ + struct lb_record *rec; + struct lb_board_config *config; + rec = lb_new_record(header); + config = (struct lb_board_config *)rec; + + config->tag = LB_TAG_BOARD_CONFIG; + config->size = sizeof(*config); + + config->board_id = board_id(); + config->ram_code = ram_code(); + config->sku_id = sku_id(); + config->fw_config = pack_lb64(fw_config_get()); + + return config; +} + #if CONFIG(USE_OPTION_TABLE) static struct cmos_checksum *lb_cmos_checksum(struct lb_header *header) { @@ -536,11 +505,6 @@ lb_vbnv(head); #endif
- /* Add strapping IDs if available */ - lb_board_id(head); - lb_ram_code(head); - lb_sku_id(head); - /* Pass mmc early init status */ lb_mmc_info(head);
@@ -563,6 +527,9 @@
lb_boot_media_params(head);
+ /* Board configuration information (including straps) */ + lb_board_config(head); + /* Add architecture records. */ lb_arch_add_records(head);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/12/payloads/libpayload/includ... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/12/payloads/libpayload/includ... PS12, Line 83: CB_TAG_BOARD_CONFIG = 0x0040, 0x39 + 1 = 0x3a Was 0x40 intentionally chosen?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46605/12/payloads/libpayload/includ... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/12/payloads/libpayload/includ... PS12, Line 83: CB_TAG_BOARD_CONFIG = 0x0040,
0x39 + 1 = 0x3a […]
There were some merge conflicts (other things added), so I just bumped the tag up some. In retrospect, probably should have just added 1 😊
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)