Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
mb/prodrive/hermes: Define board settings in EEPROM
Hermes has an EEPROM with firmware configuration data. Add definitions to read and verify the `board settings` from the EEPROM. Subsequent commits will hook up these EEPROM settings.
Change-Id: Id86632192ae53fd6b0e4df5b26b5a0a81e972818 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 2 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/48806/1
diff --git a/src/mainboard/prodrive/hermes/eeprom.c b/src/mainboard/prodrive/hermes/eeprom.c index d7ecdec..2bfdd2f 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -2,12 +2,17 @@
#include <device/pci_ops.h> #include <console/console.h> +#include <crc_byte.h> #include <device/smbus_host.h> #include <soc/intel/common/block/smbus/smbuslib.h> +#include <types.h> + #include "variants/baseboard/include/eeprom.h"
#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 @@ -28,6 +33,27 @@ return 0; }
+/* + * Read board settings from the EEPROM and verify their checksum. + * If checksum is valid, we assume the settings are sane as well. + */ +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))) { + printk(BIOS_ERR, "CFG EEPROM: Failed to read board settings\n"); + return false; + } + + const uint32_t crc = + CRC(&board_cfg->raw_settings, sizeof(board_cfg->raw_settings), crc32_byte); + + if (crc != board_cfg->signature) { + printk(BIOS_ERR, "CFG EEPROM: Board settings have invalid checksum\n"); + return false; + } + return true; +} + /* Read data from offset and write it to offset in UPD */ bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size) { diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index 95024b6..8abef56 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -18,6 +18,24 @@ .size = member_size(FSP_S_CONFIG, x)} #endif /* ENV_ROMSTAGE */
+__packed struct eeprom_board_settings { + uint32_t signature; + union { + struct { + uint8_t secureboot; + uint8_t primary_video; + uint8_t deep_sx_enabled; + uint8_t wake_on_usb; + uint8_t usb_powered_in_s5; + uint8_t power_state_after_g3; + uint8_t blue_rear_vref; + uint8_t internal_audio_connection; + uint8_t pxe_boot_capability; + }; + uint8_t raw_settings[9]; + }; +}; + typedef struct { size_t offset; size_t size; @@ -25,3 +43,4 @@
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);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... PS1, Line 42: if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) { get_board_settings_from_eeprom should cache board_cfg and CRC valid, not the caller
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... PS1, Line 42: if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) {
get_board_settings_from_eeprom should cache board_cfg and CRC valid, not the caller
Yeah, I agree. However, this file is built for romstage as well. I didn't try, but I might run into trouble with the illegal_globals check.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... PS1, Line 42: if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) {
Yeah, I agree. However, this file is built for romstage as well. […]
static variables are not globals. See CB:48646
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... PS1, Line 42: if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) {
static variables are not globals. […]
Ah wait, let me try something.
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/+/48806
to look at the new patch set (#3).
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
mb/prodrive/hermes: Define board settings in EEPROM
Hermes has an EEPROM with firmware configuration data. Add definitions to read and verify the `board settings` from the EEPROM. Subsequent commits will hook up these EEPROM settings.
Change-Id: Id86632192ae53fd6b0e4df5b26b5a0a81e972818 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 2 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/48806/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... File src/mainboard/prodrive/hermes/eeprom.c:
https://review.coreboot.org/c/coreboot/+/48806/1/src/mainboard/prodrive/herm... PS1, Line 42: if (read_write_config(board_cfg, EEPROM_OFFSET_BOARD_SETTINGS, 0, sizeof(*board_cfg))) {
Ah wait, let me try something.
Done
Attention is currently required from: Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Patchset:
PS3: tested on Prodrive hermes by changing settings in the BMC's webui.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48806 )
Change subject: mb/prodrive/hermes: Define board settings in EEPROM ......................................................................
mb/prodrive/hermes: Define board settings in EEPROM
Hermes has an EEPROM with firmware configuration data. Add definitions to read and verify the `board settings` from the EEPROM. Subsequent commits will hook up these EEPROM settings.
Change-Id: Id86632192ae53fd6b0e4df5b26b5a0a81e972818 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48806 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 2 files changed, 59 insertions(+), 0 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 d7ecdec..bff8340 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -2,12 +2,17 @@
#include <device/pci_ops.h> #include <console/console.h> +#include <crc_byte.h> #include <device/smbus_host.h> #include <soc/intel/common/block/smbus/smbuslib.h> +#include <types.h> + #include "variants/baseboard/include/eeprom.h"
#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 @@ -28,6 +33,41 @@ return 0; }
+/* + * Read board settings from the EEPROM and verify their checksum. + * If checksum is valid, we assume the settings are sane as well. + */ +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))) { + printk(BIOS_ERR, "CFG EEPROM: Failed to read board settings\n"); + return false; + } + + const uint32_t crc = + CRC(&board_cfg->raw_settings, sizeof(board_cfg->raw_settings), crc32_byte); + + if (crc != board_cfg->signature) { + printk(BIOS_ERR, "CFG EEPROM: Board settings have invalid checksum\n"); + return false; + } + return true; +} + +struct eeprom_board_settings *get_board_settings(void) +{ + static struct eeprom_board_settings board_cfg = {0}; + + /* Tri-state: -1: settings are invalid, 0: uninitialized, 1: settings are valid */ + static int checked_valid = 0; + + if (checked_valid == 0) { + const bool success = get_board_settings_from_eeprom(&board_cfg); + checked_valid = success ? 1 : -1; + } + return checked_valid > 0 ? &board_cfg : NULL; +} + /* Read data from offset and write it to offset in UPD */ bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size) { diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index 95024b6..d5a79cd 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -18,6 +18,24 @@ .size = member_size(FSP_S_CONFIG, x)} #endif /* ENV_ROMSTAGE */
+__packed struct eeprom_board_settings { + uint32_t signature; + union { + struct { + uint8_t secureboot; + uint8_t primary_video; + uint8_t deep_sx_enabled; + uint8_t wake_on_usb; + uint8_t usb_powered_in_s5; + uint8_t power_state_after_g3; + uint8_t blue_rear_vref; + uint8_t internal_audio_connection; + uint8_t pxe_boot_capability; + }; + uint8_t raw_settings[9]; + }; +}; + typedef struct { size_t offset; size_t size; @@ -25,3 +43,4 @@
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);
-
Angel Pons (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)