Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46565 )
Change subject: mb/prodrive/hermes/eeprom: Move I2C address into eeprom.c ......................................................................
mb/prodrive/hermes/eeprom: Move I2C address into eeprom.c
There's no need to pass the I2C address, as there's only one eeprom that is being used to store the board settings.
Change-Id: I958304e6ed6df05af923139d44ff4fd1de204738 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/mainboard.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 5 files changed, 19 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46565/1
diff --git a/src/mainboard/prodrive/hermes/eeprom.c b/src/mainboard/prodrive/hermes/eeprom.c index 5aee54a..f371b64 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -9,15 +9,17 @@ #include <string.h> #include "variants/baseboard/include/eeprom.h"
+#define I2C_ADDR_EEPROM 0x57 + /* * Check Signature in EEPROM (M24C32-FMN6TP) * If signature is there we assume that that the content is valid */ -int check_fsp_signature(u8 addr, size_t offset, uint64_t fsp_signature) +int check_fsp_signature(size_t offset, uint64_t fsp_signature) { u8 blob[8] = {0};
- if (!read_write_config(addr, blob, offset, 0, ARRAY_SIZE(blob))) { + if (!read_write_config(blob, offset, 0, ARRAY_SIZE(blob))) { // Check Signature if (*(uint64_t *)blob == fsp_signature) { printk(BIOS_DEBUG, "CFG EEPROM: Signature valid.\n"); @@ -33,13 +35,13 @@ * Check Checksum of board settings in EEPROM (M24C32-FMN6TP) * If signature is valid we assume that the settings are also sane. */ -int check_board_settings_signature(u8 addr) +int check_board_settings_signature(void) { const size_t off = offsetof(struct eeprom_layout, BoardSettings); struct eeprom_board_settings BoardSettings; uint32_t crc;
- if (read_write_config(addr, &BoardSettings, off, 0, sizeof(BoardSettings))) { + if (read_write_config(&BoardSettings, off, 0, sizeof(BoardSettings))) { printk(BIOS_ERR, "CFG EEPROM: Failed to read board settings\n"); return 0; } @@ -54,7 +56,7 @@ }
// Read data from offset and write it to offset in UPD -bool read_write_config(u8 addr, void *blob, size_t read_offset, size_t write_offset, +bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size) { int ret = 0; @@ -75,7 +77,7 @@ for (size_t i = 0; i < size; i = i + 2) { u8 tmp[2] = {0};
- ret = do_smbus_process_call(SMBUS_IO_BASE, addr, 0, + ret = do_smbus_process_call(SMBUS_IO_BASE, I2C_ADDR_EEPROM, 0, swab16(read_offset + i), (uint16_t *)&tmp[0]); if (ret < 0) break; diff --git a/src/mainboard/prodrive/hermes/mainboard.c b/src/mainboard/prodrive/hermes/mainboard.c index 9c263ee..22534c9 100644 --- a/src/mainboard/prodrive/hermes/mainboard.c +++ b/src/mainboard/prodrive/hermes/mainboard.c @@ -75,9 +75,8 @@ const char *method;
/* Disable USB power on S0->S5 transition */ - if (check_board_settings_signature(I2C_ADDR_EEPROM)) { - read_write_config(I2C_ADDR_EEPROM, &value, - offsetof(struct eeprom_layout, BoardSettings) + + if (check_board_settings_signature()) { + read_write_config(&value, offsetof(struct eeprom_layout, BoardSettings) + offsetof(struct eeprom_board_settings, usb_powered_in_s5), 0, sizeof(value)); } @@ -135,10 +134,10 @@ { uint8_t value = 0;
- if (!check_board_settings_signature(I2C_ADDR_EEPROM)) + if (!check_board_settings_signature()) return;
- read_write_config(I2C_ADDR_EEPROM, &value, + read_write_config(&value, offsetof(struct eeprom_layout, BoardSettings) + offsetof(struct eeprom_board_settings, deep_s5), 0, sizeof(value)); diff --git a/src/mainboard/prodrive/hermes/ramstage.c b/src/mainboard/prodrive/hermes/ramstage.c index 2962f58..b713d4a 100644 --- a/src/mainboard/prodrive/hermes/ramstage.c +++ b/src/mainboard/prodrive/hermes/ramstage.c @@ -23,13 +23,12 @@ params->SataLedEnable = 1;
// Overwrite params - if (!check_fsp_signature(I2C_ADDR_EEPROM, - offsetof(struct eeprom_layout, FSPS_UPD), + if (!check_fsp_signature(offsetof(struct eeprom_layout, FSPS_UPD), FSPS_UPD_SIGNATURE)) return;
for (size_t i = 0; i < ARRAY_SIZE(parmas_list); i++) { - ret = read_write_config(I2C_ADDR_EEPROM, params, + ret = read_write_config(params, parmas_list[i].eeprom_offset, parmas_list[i].fsp_upd_offset, parmas_list[i].size); diff --git a/src/mainboard/prodrive/hermes/romstage.c b/src/mainboard/prodrive/hermes/romstage.c index 9202ad8..6addac5 100644 --- a/src/mainboard/prodrive/hermes/romstage.c +++ b/src/mainboard/prodrive/hermes/romstage.c @@ -22,13 +22,12 @@ cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config());
// Overwrite memupd - if (!check_fsp_signature(I2C_ADDR_EEPROM, - offsetof(struct eeprom_layout, FSPM_UPD), + if (!check_fsp_signature(offsetof(struct eeprom_layout, FSPM_UPD), FSPM_UPD_SIGNATURE)) return;
for (size_t i = 0; i < ARRAY_SIZE(parmas_list); i++) { - ret = read_write_config(I2C_ADDR_EEPROM, memupd, + ret = read_write_config(memupd, parmas_list[i].eeprom_offset, parmas_list[i].fsp_upd_offset, parmas_list[i].size); diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index 787830e..f9e9a71 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -3,8 +3,6 @@ #include <fsp/api.h> #include <fsp/util.h>
-#define I2C_ADDR_EEPROM 0x57 - struct eeprom_board_settings { uint32_t signature; uint8_t secureboot; @@ -47,7 +45,7 @@ #define GET_VALUE_FSP_S_CONFIG(x) GET_VALUE(x, FSP_S_CONFIG, FspsConfig, FSPS_UPD) #define GET_VALUE_FSP_S_TEST_CONFIG(x) GET_VALUE(x, FSP_S_TEST_CONFIG, FspsTestConfig, FSPS_UPD)
-bool read_write_config(u8 addr, void *blob, size_t read_offset, size_t write_offset, +bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size); -int check_fsp_signature(u8 addr, size_t offset, uint64_t fsp_signature); -int check_board_settings_signature(u8 addr); +int check_fsp_signature(size_t offset, uint64_t fsp_signature); +int check_board_settings_signature(void);
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46565
to look at the new patch set (#3).
Change subject: mb/prodrive/hermes/eeprom: Move I2C address into eeprom.c ......................................................................
mb/prodrive/hermes/eeprom: Move I2C address into eeprom.c
There's no need to pass the I2C address, as there's only one eeprom that is being used to store the board settings.
Change-Id: I958304e6ed6df05af923139d44ff4fd1de204738 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/prodrive/hermes/eeprom.c M src/mainboard/prodrive/hermes/mainboard.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 5 files changed, 20 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46565/3
Angel Pons has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/46565 )
Change subject: mb/prodrive/hermes: Drop EEPROM address function parameters ......................................................................
mb/prodrive/hermes: Drop EEPROM address function parameters
Only one EEPROM is used to store the board settings, and its I2C address is constant. Thus, there's no need to pass its address as a parameter.
In addition, reduce the scope of the `I2C_ADDR_EEPROM` definition, since using it outside of eeprom.c would bypass the API's abstraction layer.
Change-Id: I958304e6ed6df05af923139d44ff4fd1de204738 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, 12 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46565/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46565 )
Change subject: mb/prodrive/hermes: Drop EEPROM address function parameters ......................................................................
Patch Set 5: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46565 )
Change subject: mb/prodrive/hermes: Drop EEPROM address function parameters ......................................................................
mb/prodrive/hermes: Drop EEPROM address function parameters
Only one EEPROM is used to store the board settings, and its I2C address is constant. Thus, there's no need to pass its address as a parameter.
In addition, reduce the scope of the `I2C_ADDR_EEPROM` definition, since using it outside of eeprom.c would bypass the API's abstraction layer.
Change-Id: I958304e6ed6df05af923139d44ff4fd1de204738 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/+/46565 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/ramstage.c M src/mainboard/prodrive/hermes/romstage.c M src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h 4 files changed, 12 insertions(+), 14 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 cece32c..d7ecdec 100644 --- a/src/mainboard/prodrive/hermes/eeprom.c +++ b/src/mainboard/prodrive/hermes/eeprom.c @@ -6,15 +6,17 @@ #include <soc/intel/common/block/smbus/smbuslib.h> #include "variants/baseboard/include/eeprom.h"
+#define I2C_ADDR_EEPROM 0x57 + /* * Check Signature in EEPROM (M24C32-FMN6TP) * If signature is there we assume that that the content is valid */ -int check_signature(u8 addr, const size_t offset, const uint64_t signature) +int check_signature(const size_t offset, const uint64_t signature) { u8 blob[8] = {0};
- if (!read_write_config(addr, blob, offset, 0, ARRAY_SIZE(blob))) { + if (!read_write_config(blob, offset, 0, ARRAY_SIZE(blob))) { /* Check signature */ if (*(uint64_t *)blob == signature) { printk(BIOS_DEBUG, "CFG EEPROM: Signature valid.\n"); @@ -27,8 +29,7 @@ }
/* Read data from offset and write it to offset in UPD */ -bool read_write_config(u8 addr, void *blob, size_t read_offset, size_t write_offset, - size_t size) +bool read_write_config(void *blob, size_t read_offset, size_t write_offset, size_t size) { int ret = 0;
@@ -42,7 +43,7 @@ for (size_t i = 0; i < size; i = i + 2) { u8 tmp[2] = {0};
- ret = do_smbus_process_call(SMBUS_IO_BASE, addr, 0, + ret = do_smbus_process_call(SMBUS_IO_BASE, I2C_ADDR_EEPROM, 0, swab16(read_offset + i), (uint16_t *)&tmp[0]); if (ret < 0) break; diff --git a/src/mainboard/prodrive/hermes/ramstage.c b/src/mainboard/prodrive/hermes/ramstage.c index 72ae014..b879be7 100644 --- a/src/mainboard/prodrive/hermes/ramstage.c +++ b/src/mainboard/prodrive/hermes/ramstage.c @@ -19,13 +19,13 @@ params->SataLedEnable = 1;
/* Overwrite params */ - if (!check_signature(I2C_ADDR_EEPROM, EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) return;
for (u8 i = 0; i <= ARRAY_SIZE(parmas_list); i++) { if (ARRAY_SIZE(parmas_list) == 0) break; - read_write_config(I2C_ADDR_EEPROM, params, EEPROM_OFFSET_FSP_CONFIG + + read_write_config(params, EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, parmas_list[i].size); diff --git a/src/mainboard/prodrive/hermes/romstage.c b/src/mainboard/prodrive/hermes/romstage.c index 6e88413..0ba9f45 100644 --- a/src/mainboard/prodrive/hermes/romstage.c +++ b/src/mainboard/prodrive/hermes/romstage.c @@ -19,11 +19,11 @@ cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config());
/* Overwrite memupd */ - if (!check_signature(I2C_ADDR_EEPROM, EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) + if (!check_signature(EEPROM_OFFSET_FSP_SIGNATURE, FSP_UPD_SIGNATURE)) return;
for (size_t i = 0; i < ARRAY_SIZE(parmas_list); i++) { - read_write_config(I2C_ADDR_EEPROM, memupd, EEPROM_OFFSET_FSP_CONFIG + + read_write_config(memupd, EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, EEPROM_OFFSET_FSP_CONFIG + parmas_list[i].offset, parmas_list[i].size); diff --git a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h index bdfce8a..95024b6 100644 --- a/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h +++ b/src/mainboard/prodrive/hermes/variants/baseboard/include/eeprom.h @@ -2,8 +2,6 @@
#include <soc/ramstage.h>
-#define I2C_ADDR_EEPROM 0x57 - #if ENV_ROMSTAGE #define FSP_UPD_SIGNATURE FSPM_UPD_SIGNATURE #define EEPROM_OFFSET_FSP_SIGNATURE 0 @@ -25,6 +23,5 @@ size_t size; } fsp_params;
-bool read_write_config(u8 addr, void *blob, size_t read_offset, size_t write_offset, - size_t size); -int check_signature(u8 addr, const size_t offset, const uint64_t signature); +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);
-
Angel Pons (Code Review) -
Hung-Te Lin (Code Review)
-
Patrick Rudolph (Code Review)