Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48808
to review the following change.
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
mb/prodrive/hermes: Improve board config EEPROM handling
* Check and print errors returned reading from I2C * Rework offset calculation by using more macros * Get rid of stage-specific preprocessor code * Define the EEPROM layout as struct * Make use of the defined EEPROM layout to calculate offsets * Read the UPD to disable VT-d from EEPROM
Change-Id: Iad77811318c7dfd3a3a4f8d523cfa0f457f168b6 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 57 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/48808/1
diff --git a/src/mainboard/prodrive/hermes/eeprom.c b/src/mainboard/prodrive/hermes/eeprom.c index 2bfdd2f..f9631df 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -11,8 +11,6 @@
#define I2C_ADDR_EEPROM 0x57
-#define EEPROM_OFFSET_BOARD_SETTINGS 0x1f00 - /* * Check Signature in EEPROM (M24C32-FMN6TP) * If signature is there we assume that that the content is valid @@ -39,7 +37,9 @@ */ bool get_board_settings_from_eeprom(struct eeprom_board_settings *board_cfg) { - if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) { + const size_t board_settings_offset = offsetof(struct eeprom_layout, BoardSettings); + + if (read_write_config(board_cfg, board_settings_offset, 0, sizeof(*board_cfg))) { printk(BIOS_ERR, "CFG EEPROM: Failed to read board settings\n"); return false; } @@ -87,3 +87,8 @@
return ret; } + +void report_eeprom_error(const size_t off) +{ + printk(BIOS_ERR, "MB: Failed to read from EEPROM at addr. 0x%zx\n", off); +} diff --git a/src/mainboard/prodrive/hermes/ramstage.c b/src/mainboard/prodrive/hermes/ramstage.c index b879be7..917f3b9 100644 --- a/src/mainboard/prodrive/hermes/ramstage.c +++ b/src/mainboard/prodrive/hermes/ramstage.c @@ -4,10 +4,6 @@ #include <variant/gpio.h> #include "variants/baseboard/include/eeprom.h"
-static fsp_params parmas_list[] = { - /* FIXME: Fill with additional options */ -}; - void mainboard_silicon_init_params(FSPS_UPD *supd) { FSP_S_CONFIG *params = &supd->FspsConfig; @@ -19,15 +15,9 @@ params->SataLedEnable = 1;
/* Overwrite params */ - if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(offsetof(struct eeprom_layout, supd), FSPS_UPD_SIGNATURE)) return;
- for (u8 i = 0; i <= ARRAY_SIZE(parmas_list); i++) { - if (ARRAY_SIZE(parmas_list) == 0) - break; - read_write_config(params, EEPROM_OFFSET_FSP_CONFIG + - parmas_list[i].offset, - EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, - parmas_list[i].size); - } + READ_EEPROM_FSP_S(supd, FspsTestConfig.VtdDisableDeprecated); + READ_EEPROM_FSP_S(supd, FspsConfig.PchPmWolEnableOverride); } diff --git a/src/mainboard/prodrive/hermes/romstage.c b/src/mainboard/prodrive/hermes/romstage.c index 0ba9f45..1bb8a7f 100644 --- a/src/mainboard/prodrive/hermes/romstage.c +++ b/src/mainboard/prodrive/hermes/romstage.c @@ -5,12 +5,6 @@ #include <variant/variants.h> #include "variants/baseboard/include/eeprom.h"
-static fsp_params parmas_list[] = { - GET_VALUE(RMT), - GET_VALUE(HyperThreading), - GET_VALUE(BootFrequency) -}; - void mainboard_memory_init_params(FSPM_UPD *memupd) { memupd->FspmConfig.UserBd = 7; @@ -19,13 +13,11 @@ cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config());
/* Overwrite memupd */ - if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(offsetof(struct eeprom_layout, mupd), FSPM_UPD_SIGNATURE)) return;
- for (size_t i = 0; i < ARRAY_SIZE(parmas_list); i++) { - read_write_config(memupd, EEPROM_OFFSET_FSP_CONFIG + - parmas_list[i].offset, - EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, - parmas_list[i].size); - } + READ_EEPROM_FSP_M(memupd, FspmConfig.RMT); + READ_EEPROM_FSP_M(memupd, FspmConfig.HyperThreading); + READ_EEPROM_FSP_M(memupd, FspmConfig.BootFrequency); + READ_EEPROM_FSP_M(memupd, FspmTestConfig.VtdDisable); } diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index 8abef56..bc45fc6 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -2,22 +2,6 @@
#include <soc/ramstage.h>
-#if ENV_ROMSTAGE -#define FSP_UPD_SIGNATURE FSPM_UPD_SIGNATURE -#define EEPROM_OFFSET_FSP_SIGNATURE 0 -#define EEPROM_OFFSET_FSP_CONFIG 0 - -#define GET_VALUE(x) {.offset = sizeof(FSP_UPD_HEADER) + sizeof(FSPM_ARCH_UPD) \ - + offsetof(FSP_M_CONFIG, x), .size = member_size(FSP_M_CONFIG, x)} -#else - -#define FSP_UPD_SIGNATURE FSPS_UPD_SIGNATURE -#define EEPROM_OFFSET_FSP_SIGNATURE 0x0600 -#define EEPROM_OFFSET_FSP_CONFIG (EEPROM_OFFSET_FSP_SIGNATURE + sizeof(FSP_UPD_HEADER)) -#define GET_VALUE(x) {.offset = offsetof(FSP_S_CONFIG, x), \ - .size = member_size(FSP_S_CONFIG, x)} -#endif /* ENV_ROMSTAGE */ - __packed struct eeprom_board_settings { uint32_t signature; union { @@ -36,11 +20,48 @@ }; };
-typedef struct { - size_t offset; - size_t size; -} fsp_params; +/* The EEPROM on address 0x57 has the following vendor defined layout: */ +__packed struct eeprom_layout { + union { + uint8_t RawFSPMUPD[0x600]; + FSPM_UPD mupd; + }; + union { + uint8_t RawFSPSUPD[0xC00]; + FSPS_UPD supd; + }; + uint8_t BoardLayout[0x400]; + uint8_t BootOrder[0x900]; + union { + uint8_t RawBoardSetting[0x100]; + struct eeprom_board_settings BoardSettings; + }; +}; + +_Static_assert(sizeof(FSPM_UPD) <= 0x600, "FSPM_UPD too big"); +_Static_assert(sizeof(FSPS_UPD) <= 0xC00, "FSPS_UPD too big"); +_Static_assert(sizeof(struct eeprom_layout) == 0x2000, "EEPROM layout size mismatch");
bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size); int check_signature(const size_t offset, const uint64_t signature); bool get_board_settings_from_eeprom(struct eeprom_board_settings *board_cfg); +void report_eeprom_error(const size_t off); + +#define READ_EEPROM(section_type, section_name, dest, opt_name) \ + do { \ + typeof(dest->opt_name) __tmp; \ + size_t __off = offsetof(struct eeprom_layout, section_name); \ + bool ret = read_write_config(&__tmp, \ + __off + offsetof(section_type, opt_name), \ + 0, \ + sizeof(__tmp)); \ + if (ret) { \ + report_eeprom_error(__off + offsetof(section_type, opt_name)); \ + } else { \ + dest->opt_name = __tmp; \ + } \ + } while (0) + +#define READ_EEPROM_FSP_M(dest, opt_name) READ_EEPROM(FSPM_UPD, RawFSPMUPD, dest, opt_name) +#define READ_EEPROM_FSP_S(dest, opt_name) READ_EEPROM(FSPS_UPD, RawFSPSUPD, dest, opt_name) +
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48808 )
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Patrick Rudolph, Christian Walter, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48808
to look at the new patch set (#2).
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
mb/prodrive/hermes: Improve board config EEPROM handling
* Check and print errors returned reading from I2C * Rework offset calculation by using more macros * Get rid of stage-specific preprocessor code * Define the EEPROM layout as struct * Make use of the defined EEPROM layout to calculate offsets * Read the UPD to disable VT-d from EEPROM
Change-Id: Iad77811318c7dfd3a3a4f8d523cfa0f457f168b6 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 56 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/48808/2
Hello build bot (Jenkins), Patrick Rudolph, Christian Walter, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48808
to look at the new patch set (#3).
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
mb/prodrive/hermes: Improve board config EEPROM handling
* Check and print errors returned reading from I2C * Rework offset calculation by using more macros * Get rid of stage-specific preprocessor code * Define the EEPROM layout as struct * Make use of the defined EEPROM layout to calculate offsets * Read the UPD to disable VT-d from EEPROM
Change-Id: Iad77811318c7dfd3a3a4f8d523cfa0f457f168b6 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 56 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/48808/3
Attention is currently required from: Patrick Rudolph. Hello build bot (Jenkins), Patrick Rudolph, Christian Walter, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48808
to look at the new patch set (#5).
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
mb/prodrive/hermes: Improve board config EEPROM handling
* Check and print errors returned from reading from I2C * Rework offset calculation by using more macros * Get rid of stage-specific preprocessor code * Define the EEPROM layout as struct * Make use of the defined EEPROM layout to calculate offsets * Read the UPD to disable VT-d from EEPROM
Change-Id: Iad77811318c7dfd3a3a4f8d523cfa0f457f168b6 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 56 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/48808/5
Attention is currently required from: Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48808 )
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5: Patrick, I did a minor adjustment to the commit message so that you can +2 this
Attention is currently required from: Patrick Rudolph, Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48808 )
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48808 )
Change subject: mb/prodrive/hermes: Improve board config EEPROM handling ......................................................................
mb/prodrive/hermes: Improve board config EEPROM handling
* Check and print errors returned from reading from I2C * Rework offset calculation by using more macros * Get rid of stage-specific preprocessor code * Define the EEPROM layout as struct * Make use of the defined EEPROM layout to calculate offsets * Read the UPD to disable VT-d from EEPROM
Change-Id: Iad77811318c7dfd3a3a4f8d523cfa0f457f168b6 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48808 Reviewed-by: Patrick Rudolph siro@das-labor.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 56 insertions(+), 49 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/mainboard/prodrive/hermes/eeprom.c b/src/mainboard/prodrive/hermes/eeprom.c index bff8340..8db63f8 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -11,8 +11,6 @@
#define I2C_ADDR_EEPROM 0x57
-#define EEPROM_OFFSET_BOARD_SETTINGS 0x1f00 - /* * Check Signature in EEPROM (M24C32-FMN6TP) * If signature is there we assume that that the content is valid @@ -39,7 +37,9 @@ */ static bool get_board_settings_from_eeprom(struct eeprom_board_settings *board_cfg) { - if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) { + const size_t board_settings_offset = offsetof(struct eeprom_layout, BoardSettings); + + if (read_write_config(board_cfg, board_settings_offset, 0, sizeof(*board_cfg))) { printk(BIOS_ERR, "CFG EEPROM: Failed to read board settings\n"); return false; } @@ -101,3 +101,8 @@
return ret; } + +void report_eeprom_error(const size_t off) +{ + printk(BIOS_ERR, "MB: Failed to read from EEPROM at addr. 0x%zx\n", off); +} diff --git a/src/mainboard/prodrive/hermes/ramstage.c b/src/mainboard/prodrive/hermes/ramstage.c index b879be7..917f3b9 100644 --- a/src/mainboard/prodrive/hermes/ramstage.c +++ b/src/mainboard/prodrive/hermes/ramstage.c @@ -4,10 +4,6 @@ #include <variant/gpio.h> #include "variants/baseboard/include/eeprom.h"
-static fsp_params parmas_list[] = { - /* FIXME: Fill with additional options */ -}; - void mainboard_silicon_init_params(FSPS_UPD *supd) { FSP_S_CONFIG *params = &supd->FspsConfig; @@ -19,15 +15,9 @@ params->SataLedEnable = 1;
/* Overwrite params */ - if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(offsetof(struct eeprom_layout, supd), FSPS_UPD_SIGNATURE)) return;
- for (u8 i = 0; i <= ARRAY_SIZE(parmas_list); i++) { - if (ARRAY_SIZE(parmas_list) == 0) - break; - read_write_config(params, EEPROM_OFFSET_FSP_CONFIG + - parmas_list[i].offset, - EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, - parmas_list[i].size); - } + READ_EEPROM_FSP_S(supd, FspsTestConfig.VtdDisableDeprecated); + READ_EEPROM_FSP_S(supd, FspsConfig.PchPmWolEnableOverride); } diff --git a/src/mainboard/prodrive/hermes/romstage.c b/src/mainboard/prodrive/hermes/romstage.c index 0ba9f45..1bb8a7f 100644 --- a/src/mainboard/prodrive/hermes/romstage.c +++ b/src/mainboard/prodrive/hermes/romstage.c @@ -5,12 +5,6 @@ #include <variant/variants.h> #include "variants/baseboard/include/eeprom.h"
-static fsp_params parmas_list[] = { - GET_VALUE(RMT), - GET_VALUE(HyperThreading), - GET_VALUE(BootFrequency) -}; - void mainboard_memory_init_params(FSPM_UPD *memupd) { memupd->FspmConfig.UserBd = 7; @@ -19,13 +13,11 @@ cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config());
/* Overwrite memupd */ - if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(offsetof(struct eeprom_layout, mupd), FSPM_UPD_SIGNATURE)) return;
- for (size_t i = 0; i < ARRAY_SIZE(parmas_list); i++) { - read_write_config(memupd, EEPROM_OFFSET_FSP_CONFIG + - parmas_list[i].offset, - EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, - parmas_list[i].size); - } + READ_EEPROM_FSP_M(memupd, FspmConfig.RMT); + READ_EEPROM_FSP_M(memupd, FspmConfig.HyperThreading); + READ_EEPROM_FSP_M(memupd, FspmConfig.BootFrequency); + READ_EEPROM_FSP_M(memupd, FspmTestConfig.VtdDisable); } diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index d5a79cd..cdb5cea 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -2,22 +2,6 @@
#include <soc/ramstage.h>
-#if ENV_ROMSTAGE -#define FSP_UPD_SIGNATURE FSPM_UPD_SIGNATURE -#define EEPROM_OFFSET_FSP_SIGNATURE 0 -#define EEPROM_OFFSET_FSP_CONFIG 0 - -#define GET_VALUE(x) {.offset = sizeof(FSP_UPD_HEADER) + sizeof(FSPM_ARCH_UPD) \ - + offsetof(FSP_M_CONFIG, x), .size = member_size(FSP_M_CONFIG, x)} -#else - -#define FSP_UPD_SIGNATURE FSPS_UPD_SIGNATURE -#define EEPROM_OFFSET_FSP_SIGNATURE 0x0600 -#define EEPROM_OFFSET_FSP_CONFIG (EEPROM_OFFSET_FSP_SIGNATURE + sizeof(FSP_UPD_HEADER)) -#define GET_VALUE(x) {.offset = offsetof(FSP_S_CONFIG, x), \ - .size = member_size(FSP_S_CONFIG, x)} -#endif /* ENV_ROMSTAGE */ - __packed struct eeprom_board_settings { uint32_t signature; union { @@ -36,11 +20,47 @@ }; };
-typedef struct { - size_t offset; - size_t size; -} fsp_params; +/* The EEPROM on address 0x57 has the following vendor defined layout: */ +__packed struct eeprom_layout { + union { + uint8_t RawFSPMUPD[0x600]; + FSPM_UPD mupd; + }; + union { + uint8_t RawFSPSUPD[0xC00]; + FSPS_UPD supd; + }; + uint8_t BoardLayout[0x400]; + uint8_t BootOrder[0x900]; + union { + uint8_t RawBoardSetting[0x100]; + struct eeprom_board_settings BoardSettings; + }; +}; + +_Static_assert(sizeof(FSPM_UPD) <= 0x600, "FSPM_UPD too big"); +_Static_assert(sizeof(FSPS_UPD) <= 0xC00, "FSPS_UPD too big"); +_Static_assert(sizeof(struct eeprom_layout) == 0x2000, "EEPROM layout size mismatch");
bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size); int check_signature(const size_t offset, const uint64_t signature); struct eeprom_board_settings *get_board_settings(void); +void report_eeprom_error(const size_t off); + +#define READ_EEPROM(section_type, section_name, dest, opt_name) \ + do { \ + typeof(dest->opt_name) __tmp; \ + size_t __off = offsetof(struct eeprom_layout, section_name); \ + bool ret = read_write_config(&__tmp, \ + __off + offsetof(section_type, opt_name), \ + 0, \ + sizeof(__tmp)); \ + if (ret) { \ + report_eeprom_error(__off + offsetof(section_type, opt_name)); \ + } else { \ + dest->opt_name = __tmp; \ + } \ + } while (0) + +#define READ_EEPROM_FSP_M(dest, opt_name) READ_EEPROM(FSPM_UPD, RawFSPMUPD, dest, opt_name) +#define READ_EEPROM_FSP_S(dest, opt_name) READ_EEPROM(FSPS_UPD, RawFSPSUPD, dest, opt_name)
-
Angel Pons (Code Review) -
Patrick Rudolph (Code Review)