Change in coreboot[master]: src/soc/intel/common/block/cse: Add helper functions to CSE lib
Hello Rizwan Qureshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to review the following change.
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 72 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/1
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 69cb273..7697a4a 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -67,11 +67,35 @@ #define MEI_HDR_CSE_ADDR_START 0 #define MEI_HDR_CSE_ADDR (((1 << 8) - 1) << MEI_HDR_CSE_ADDR_START)
+#define HECI_OP_MODE_SEC_OVERRIDE 5
static struct cse_device { uintptr_t sec_bar; } g_cse;
+union me_hfs { + u32 data; + struct { + u32 working_state: 4; + u32 mfg_mode: 1; + u32 fpt_bad: 1; + u32 operation_state: 3; + u32 fw_init_complete: 1; + u32 ft_bup_ld_flr: 1; + u32 update_in_progress: 1; + u32 error_code: 4; + u32 operation_mode: 4; + u32 reset_count: 4; + u32 boot_options_present: 1; + u32 reserved1: 1; + u32 bist_test_state: 1; + u32 bist_reset_request: 1; + u32 current_power_source: 2; + u32 d3_support_valid: 1; + u32 d0i3_support_valid: 1; + } __packed fields; +}; + /* * Initialize the device with provided temporary BAR. If BAR is 0 use a * default. This is intended for pre-mem usage only where BARs haven't been @@ -239,6 +263,41 @@ return csr & CSR_READY; }
+ +static int cse_rec(void) +{ + union me_hfs hfs; + hfs.data = me_read_config32(PCI_ME_HFSTS1); + if(hfs.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE) + return 1; + else + return 0; +} + +void set_host_ready(void) +{ + uint32_t csr; + csr = read_host_csr(); + csr &= ~CSR_RESET; + csr |= CSR_IG; + csr |= CSR_READY; + write_host_csr(csr); +} + +int wait_cse_rec(void) +{ + struct stopwatch sw; + + stopwatch_init_msecs_expire(&sw, 15000); + while (!cse_rec()) { + udelay(100); + if (stopwatch_expired(&sw)) + return 0; + } + + return 1; +} + static int wait_heci_ready(void) { struct stopwatch sw; @@ -495,11 +554,7 @@
if (wait_heci_ready()) { /* Device is back on its imaginary feet, clear reset */ - csr = read_host_csr(); - csr &= ~CSR_RESET; - csr |= CSR_IG; - csr |= CSR_READY; - write_host_csr(csr); + set_host_ready(); return 1; }
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 9409ce5..82e07cf 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -54,11 +54,22 @@ int heci_reset(void);
/* - * Reads config value from a specified offset in the HECI Configuration + * Reads and returns config value from a specified offset in the HECI Configuration * */ uint32_t me_read_config32(int offset);
+/* + * Clears the reset state in the Host CSR + */ +void set_host_ready(void); + +/* + * Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds. + * Returns 0 on failure a 1 on success. + */ +int wait_cse_rec(void); + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 1:
(81 comments)
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 77: u32 data; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 77: u32 data; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 78: struct { code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 78: struct { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 79: u32 working_state: 4; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 79: u32 working_state: 4; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 80: u32 mfg_mode: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 80: u32 mfg_mode: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 81: u32 fpt_bad: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 81: u32 fpt_bad: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 82: u32 operation_state: 3; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 82: u32 operation_state: 3; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 83: u32 fw_init_complete: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 83: u32 fw_init_complete: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 84: u32 ft_bup_ld_flr: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 84: u32 ft_bup_ld_flr: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 85: u32 update_in_progress: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 85: u32 update_in_progress: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 86: u32 error_code: 4; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 86: u32 error_code: 4; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 87: u32 operation_mode: 4; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 87: u32 operation_mode: 4; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 88: u32 reset_count: 4; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 88: u32 reset_count: 4; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 89: u32 boot_options_present: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 89: u32 boot_options_present: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 90: u32 reserved1: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 90: u32 reserved1: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 91: u32 bist_test_state: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 91: u32 bist_test_state: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 92: u32 bist_reset_request: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 92: u32 bist_reset_request: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 93: u32 current_power_source: 2; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 93: u32 current_power_source: 2; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 94: u32 d3_support_valid: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 94: u32 d3_support_valid: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 95: u32 d0i3_support_valid: 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 95: u32 d0i3_support_valid: 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 96: } __packed fields; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 96: } __packed fields; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 269: union me_hfs hfs; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 269: union me_hfs hfs; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 270: hfs.data = me_read_config32(PCI_ME_HFSTS1); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 270: hfs.data = me_read_config32(PCI_ME_HFSTS1); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 271: if(hfs.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 271: if(hfs.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 271: if(hfs.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 272: return 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 272: return 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 273: else code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 273: else please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 274: return 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 274: return 0; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 279: uint32_t csr; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 279: uint32_t csr; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 280: csr = read_host_csr(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 280: csr = read_host_csr(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 281: csr &= ~CSR_RESET; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 281: csr &= ~CSR_RESET; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 282: csr |= CSR_IG; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 282: csr |= CSR_IG; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 283: csr |= CSR_READY; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 283: csr |= CSR_READY; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 284: write_host_csr(csr); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 284: write_host_csr(csr); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 289: struct stopwatch sw; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 289: struct stopwatch sw; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 291: stopwatch_init_msecs_expire(&sw, 15000); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 291: stopwatch_init_msecs_expire(&sw, 15000); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 292: while (!cse_rec()) { code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 292: while (!cse_rec()) { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 293: udelay(100); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 293: udelay(100); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 294: if (stopwatch_expired(&sw)) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 294: if (stopwatch_expired(&sw)) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 295: return 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 295: return 0; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 296: } code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 296: } please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 298: return 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35226/1/src/soc/intel/common/block/... PS1, Line 298: return 1; please, no spaces at the start of a line
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#2).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR. * Fix all comments on indentation
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/2//COMMIT_MSG@15 PS2, Line 15: * Fix all comments on indentation not required, the gerrit patchset history knows what you have done with the patch
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#3).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/3
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#7).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/7
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#8).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/8
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#9).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/9
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/2//COMMIT_MSG@15 PS2, Line 15: * Fix all comments on indentation
not required, the gerrit patchset history knows what you have done with the patch
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#12).
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/12
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... PS12, Line 266: remove extra line.
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... PS12, Line 272: This can go with previous line
Rizwan Qureshi has uploaded a new patch set (#13) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
src/soc/intel/common/block/cse: Add helper functions to CSE lib
Below helper function are added: * wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 69 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/13
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... PS12, Line 266:
remove extra line.
Done
https://review.coreboot.org/c/coreboot/+/35226/12/src/soc/intel/common/block... PS12, Line 272:
This can go with previous line
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 14:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 76: me_hfs which status register is this ? STS1 ? if yes then we already have the definitions in soc directory. shouldn't we remove those in this CL as well ?
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 265: one line function description will help to understand
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 282: csr |= CSR_READY; 281 and 282 can combine ?
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 289: 15000 same here ?
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 291: 100 can't use macro ?
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 75: /* : * Clears the reset state in the Host CSR : */ doesn't req multi line comments
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 76: me_hfs
which status register is this ? STS1 ? if yes then we already have the definitions in soc directory. […]
Yes, this is as same as STS1. But, the STS1 structure used different field names. If we have to generalize this part, we can move all me related stuff to common. Most of the ME functions are dumping and decoding STSx registers, and have been implemented same differently across socs.Can we do this change as part of separate task?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Add helper functions to CSE lib ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 76: me_hfs
Yes, this is as same as STS1. But, the STS1 structure used different field names. […]
SGTM
Rizwan Qureshi has uploaded a new patch set (#15) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Heci FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 78 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/15
Rizwan Qureshi has uploaded a new patch set (#16) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Heci FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 82 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/16
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 16:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 76: me_hfs
which status register is this ? STS1 ? if yes then we already have the definitions in soc directory. […]
This is HFSTS1, infact we can make this common across SoCs.
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 265:
one line function description will help to understand
Done
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 282: csr |= CSR_READY;
281 and 282 can combine ?
Done
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 289: 15000
same here ?
Done
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 291: 100
can't use macro ?
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready just to maintain naming consistency add cse in function name?
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 525: csr = read_host_csr(); : csr |= (CSR_RESET | CSR_IG); : write_host_csr(csr); can we create a static helper function ?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 282: csr |= CSR_READY;
281 and 282 can combine ?
For readability sake, the operations are separated into two statements.
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 289: 15000
same here ?
Done
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 291: 100
can't use macro ?
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready
just to maintain naming consistency add cse in function name?
There are two types of CSR register VIZ: HOST_CSR & CSE_CSR register. The function clears reset state in HOST_CSR register. It confuses if I add cse in the function name.
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 525: csr = read_host_csr(); : csr |= (CSR_RESET | CSR_IG); : write_host_csr(csr);
can we create a static helper function ?
can be done.
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 75: /* : * Clears the reset state in the Host CSR : */
doesn't req multi line comments
Done
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 282: csr |= CSR_READY;
For readability sake, the operations are separated into two statements.
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 525: csr = read_host_csr(); : csr |= (CSR_RESET | CSR_IG); : write_host_csr(csr);
can we create a static helper function ?
The function name itself indicates triggering reset. So, I think static helper function is not required. Thanks
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready
There are two types of CSR register VIZ: HOST_CSR & CSE_CSR register. […]
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 525: csr = read_host_csr(); : csr |= (CSR_RESET | CSR_IG); : write_host_csr(csr);
The function name itself indicates triggering reset. […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready i hope you have named _ready because setting CSR_READY bit but that is one part of this function.
name has to reflect what this function is doing
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready also why not static? i don't see any caller to standalone using this function?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 247: int unsigned int?
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 253: else : return 0; you can skip the else and directly return 0, if above condition is true the function would anyways return 1.
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 268: int unsigned int?
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 32: hfsts1 nit:me_hsts1? to be consistent with SOC definitions for rest of the hstsx used in SOC code.
Also add a comment for the register information. like the SOC code used to have: /* ME Host Firmware Status Register 1 */
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c... PS17, Line 285: hfs hsf1?
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c... PS17, Line 487: hfs same.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#18).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/18
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 18:
(8 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 247: int
unsigned int?
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 253: else : return 0;
you can skip the else and directly return 0, if above condition is true the function would anyways r […]
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready
i hope you have named _ready because setting CSR_READY bit but that is one part of this function. […]
No, this function makes host(BIOS/CB) ready to send messages to CSE. Hence, function named as set_host_ready().
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready
also why not static? i don't see any caller to standalone using this function?
The function will be used by subsequent patches so it can not be made static.
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 268: int
unsigned int?
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 32: hfsts1
nit:me_hsts1? to be consistent with SOC definitions for rest of the hstsx used in SOC code. […]
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c... PS17, Line 285: hfs
hsf1?
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/skylake/me.c... PS17, Line 487: hfs
same.
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#19).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_rec() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/19
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... PS19, Line 247: static uint8_t cse_rec(void) what is the reason for naming this as cse recovery? can we align the function name with definition.
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... PS19, Line 267: int wait_cse_rec(void) same as above.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#20).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/20
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... PS19, Line 247: static uint8_t cse_rec(void)
what is the reason for naming this as cse recovery? can we align the function name with definition.
Done
https://review.coreboot.org/c/coreboot/+/35226/19/src/soc/intel/common/block... PS19, Line 267: int wait_cse_rec(void)
same as above.
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 268: int
Done
I see its not done in PS#20, Please check.
https://review.coreboot.org/c/coreboot/+/35226/20/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/20/src/soc/intel/common/block... PS20, Line 247: cse_sec_override_mode nit: check_cse_sec_override_mode? would communicate the function better. what do you think?
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#21).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/21
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 21:
(1 comment)
Patch Set 20:
(2 comments)
It
https://review.coreboot.org/c/coreboot/+/35226/20/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/20/src/soc/intel/common/block... PS20, Line 247: cse_sec_override_mode
nit: check_cse_sec_override_mode? would communicate the function better. […]
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#22).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/22
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/22/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/22/src/soc/intel/common/block... PS22, Line 32: */ space is missing
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#23).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/23
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35226
to look at the new patch set (#24).
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/35226/24
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/22/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/22/src/soc/intel/common/block... PS22, Line 32: */
space is missing
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 24: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions
Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, hence move it to common. Also add below helper function,
* wait_cse_sec_override_mode() - Polls ME status for "HECI_OP_MODE_SEC_OVERRIDE". It's a special CSE mode, the mode ensures CSE does not trigger any spi cycles to CSE region.
* set_host_ready() - Clears reset state from host CSR.
TEST=Verified CSE recover mode on CML RVP & Hatch board
Change-Id: Id5c12b7abdb27c38af74ea6ee568b42ec74bcb3c Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35226 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/me.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 5 files changed, 103 insertions(+), 84 deletions(-)
Approvals: build bot (Jenkins): Verified Aamir Bohra: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index cd94be5..d6dfec9 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -32,30 +32,6 @@ ME_WSTATE_NORMAL = 0x05, };
-/* Host Firmware Status Register 1 */ -union hfsts1 { - uint32_t raw; - struct { - uint32_t working_state : 4; - uint32_t mfg_mode : 1; - uint32_t fpt_bad : 1; - uint32_t operation_state : 3; - uint32_t fw_init_complete : 1; - uint32_t ft_bup_ld_flr : 1; - uint32_t fw_upd_in_progress : 1; - uint32_t error_code : 4; - uint32_t operation_mode : 4; - uint32_t reset_count : 4; - uint32_t boot_options : 1; - uint32_t rsvd0 : 1; - uint32_t bist_state : 1; - uint32_t bist_reset_req : 1; - uint32_t power_source : 2; - uint32_t reserved1 : 1; - uint32_t d0i3_support_valid : 1; - } __packed fields; -}; - /* Host Firmware Status Register 2 */ union hfsts2 { uint32_t raw; @@ -174,7 +150,7 @@ struct version fitc; } __packed;
- union hfsts1 hfsts1; + union me_hfsts1 hfsts1; const struct mkhi_hdr fw_ver_msg = { .group_id = MKHI_GEN_GROUP_ID, .command = MKHI_GET_FW_VERSION, @@ -189,7 +165,7 @@ if (!is_cse_enabled()) return;
- hfsts1.raw = me_read_config32(PCI_ME_HFSTS1); + hfsts1.data = me_read_config32(PCI_ME_HFSTS1);
/* * Prerequisites: @@ -225,7 +201,7 @@
void dump_me_status(void *unused) { - union hfsts1 hfsts1; + union me_hfsts1 hfsts1; union hfsts2 hfsts2; union hfsts3 hfsts3; union hfsts4 hfsts4; @@ -235,7 +211,7 @@ if (!is_cse_enabled()) return;
- hfsts1.raw = me_read_config32(PCI_ME_HFSTS1); + hfsts1.data = me_read_config32(PCI_ME_HFSTS1); hfsts2.raw = me_read_config32(PCI_ME_HFSTS2); hfsts3.raw = me_read_config32(PCI_ME_HFSTS3); hfsts4.raw = me_read_config32(PCI_ME_HFSTS4); @@ -243,7 +219,7 @@ hfsts6.raw = me_read_config32(PCI_ME_HFSTS6);
printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", - hfsts1.raw); + hfsts1.data); printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", hfsts2.raw); printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", @@ -264,9 +240,9 @@ printk(BIOS_DEBUG, "ME: Firmware Init Complete : %s\n", hfsts1.fields.fw_init_complete ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Boot Options Present : %s\n", - hfsts1.fields.boot_options ? "YES" : "NO"); + hfsts1.fields.boot_options_present ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Update In Progress : %s\n", - hfsts1.fields.fw_upd_in_progress ? "YES" : "NO"); + hfsts1.fields.update_in_progress ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: D0i3 Support : %s\n", hfsts1.fields.d0i3_support_valid ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Low Power State Enabled : %s\n", diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 1671970..223eab5 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -67,6 +67,7 @@ #define MEI_HDR_CSE_ADDR_START 0 #define MEI_HDR_CSE_ADDR (((1 << 8) - 1) << MEI_HDR_CSE_ADDR_START)
+#define HECI_OP_MODE_SEC_OVERRIDE 5
static struct cse_device { uintptr_t sec_bar; @@ -239,6 +240,43 @@ return csr & CSR_READY; }
+/* + * Checks if CSE is in SEC_OVERRIDE operation mode. This is the mode where + * CSE will allow reflashing of CSE region. + */ +static uint8_t check_cse_sec_override_mode(void) +{ + union me_hfsts1 hfs1; + hfs1.data = me_read_config32(PCI_ME_HFSTS1); + if (hfs1.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE) + return 1; + return 0; +} + +/* Makes the host ready to communicate with CSE */ +void set_host_ready(void) +{ + uint32_t csr; + csr = read_host_csr(); + csr &= ~CSR_RESET; + csr |= (CSR_IG | CSR_READY); + write_host_csr(csr); +} + +/* Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds */ +uint8_t wait_cse_sec_override_mode(void) +{ + struct stopwatch sw; + stopwatch_init_msecs_expire(&sw, HECI_DELAY_READY); + while (!check_cse_sec_override_mode()) { + udelay(HECI_DELAY); + if (stopwatch_expired(&sw)) + return 0; + } + + return 1; +} + static int wait_heci_ready(void) { struct stopwatch sw; @@ -484,17 +522,12 @@
/* Send reset request */ csr = read_host_csr(); - csr |= CSR_RESET; - csr |= CSR_IG; + csr |= (CSR_RESET | CSR_IG); write_host_csr(csr);
if (wait_heci_ready()) { /* Device is back on its imaginary feet, clear reset */ - csr = read_host_csr(); - csr &= ~CSR_RESET; - csr |= CSR_IG; - csr |= CSR_READY; - write_host_csr(csr); + set_host_ready(); return 1; }
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index bce615c..30d17c8 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -29,6 +29,30 @@ PCI_ME_HFSTS6 = 0x6C, };
+/* ME Host Firmware Status register 1 */ +union me_hfsts1 { + u32 data; + struct { + u32 working_state: 4; + u32 mfg_mode: 1; + u32 fpt_bad: 1; + u32 operation_state: 3; + u32 fw_init_complete: 1; + u32 ft_bup_ld_flr: 1; + u32 update_in_progress: 1; + u32 error_code: 4; + u32 operation_mode: 4; + u32 reset_count: 4; + u32 boot_options_present: 1; + u32 reserved1: 1; + u32 bist_test_state: 1; + u32 bist_reset_request: 1; + u32 current_power_source: 2; + u32 d3_support_valid: 1; + u32 d0i3_support_valid: 1; + } __packed fields; +}; + /* set up device for use in early boot enviroument with temp bar */ void heci_init(uintptr_t bar); /* @@ -72,6 +96,16 @@ */ bool is_cse_enabled(void);
+ +/* Makes the host ready to communicate with CSE*/ +void set_host_ready(void); + +/* + * Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds. + * Returns 0 on failure a 1 on success. + */ +uint8_t wait_cse_sec_override_mode(void); + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
diff --git a/src/soc/intel/skylake/include/soc/me.h b/src/soc/intel/skylake/include/soc/me.h index fbe5033..ef84f59 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -47,30 +47,6 @@ #define ME_HFS_POWER_SOURCE_AC 1 #define ME_HFS_POWER_SOURCE_DC 2
-union me_hfs { - u32 data; - struct { - u32 working_state: 4; - u32 mfg_mode: 1; - u32 fpt_bad: 1; - u32 operation_state: 3; - u32 fw_init_complete: 1; - u32 ft_bup_ld_flr: 1; - u32 update_in_progress: 1; - u32 error_code: 4; - u32 operation_mode: 4; - u32 reset_count: 4; - u32 boot_options_present: 1; - u32 reserved1: 1; - u32 bist_test_state: 1; - u32 bist_reset_request: 1; - u32 current_power_source: 2; - u32 d3_support_valid: 1; - u32 d0i3_support_valid: 1; - } __packed fields; -}; - -#define PCI_ME_HFSTS2 0x48 /* Infrastructure Progress Values */ #define ME_HFS2_PHASE_ROM 0 #define ME_HFS2_PHASE_UKERNEL 2 diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index 9e17ef1..5fc817f 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -229,7 +229,7 @@
struct fw_ver_resp resp; size_t resp_size = sizeof(resp); - union me_hfs hfs; + union me_hfsts1 hfs1;
/* * Print ME version only if UART debugging is enabled. Else, it takes ~1 @@ -241,14 +241,14 @@ if (!is_cse_enabled()) return;
- hfs.data = me_read_config32(PCI_ME_HFSTS1); + hfs1.data = me_read_config32(PCI_ME_HFSTS1); /* * This command can be run only if: * - Working state is normal and * - Operation mode is normal. */ - if ((hfs.fields.working_state != ME_HFS_CWS_NORMAL) || - (hfs.fields.operation_mode != ME_HFS_MODE_NORMAL)) + if ((hfs1.fields.working_state != ME_HFS_CWS_NORMAL) || + (hfs1.fields.operation_mode != ME_HFS_MODE_NORMAL)) goto failed;
/* @@ -282,7 +282,7 @@
void intel_me_status(void) { - union me_hfs hfs; + union me_hfsts1 hfs1; union me_hfs2 hfs2; union me_hfs3 hfs3; union me_hfs6 hfs6; @@ -290,13 +290,13 @@ if (!is_cse_enabled()) return;
- hfs.data = me_read_config32(PCI_ME_HFSTS1); + hfs1.data = me_read_config32(PCI_ME_HFSTS1); hfs2.data = me_read_config32(PCI_ME_HFSTS2); hfs3.data = me_read_config32(PCI_ME_HFSTS3); hfs6.data = me_read_config32(PCI_ME_HFSTS6);
printk(BIOS_DEBUG, "ME: Host Firmware Status Register 1 : 0x%08X\n", - hfs.data); + hfs1.data); printk(BIOS_DEBUG, "ME: Host Firmware Status Register 2 : 0x%08X\n", hfs2.data); printk(BIOS_DEBUG, "ME: Host Firmware Status Register 3 : 0x%08X\n", @@ -309,21 +309,21 @@ hfs6.data); /* Check Current States */ printk(BIOS_DEBUG, "ME: FW Partition Table : %s\n", - hfs.fields.fpt_bad ? "BAD" : "OK"); + hfs1.fields.fpt_bad ? "BAD" : "OK"); printk(BIOS_DEBUG, "ME: Bringup Loader Failure : %s\n", - hfs.fields.ft_bup_ld_flr ? "YES" : "NO"); + hfs1.fields.ft_bup_ld_flr ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Firmware Init Complete : %s\n", - hfs.fields.fw_init_complete ? "YES" : "NO"); + hfs1.fields.fw_init_complete ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Manufacturing Mode : %s\n", - hfs.fields.mfg_mode ? "YES" : "NO"); + hfs1.fields.mfg_mode ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Boot Options Present : %s\n", - hfs.fields.boot_options_present ? "YES" : "NO"); + hfs1.fields.boot_options_present ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Update In Progress : %s\n", - hfs.fields.update_in_progress ? "YES" : "NO"); + hfs1.fields.update_in_progress ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: D3 Support : %s\n", - hfs.fields.d3_support_valid ? "YES" : "NO"); + hfs1.fields.d3_support_valid ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: D0i3 Support : %s\n", - hfs.fields.d0i3_support_valid ? "YES" : "NO"); + hfs1.fields.d0i3_support_valid ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Low Power State Enabled : %s\n", hfs2.fields.low_power_state ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: CPU Replaced : %s\n", @@ -331,13 +331,13 @@ printk(BIOS_DEBUG, "ME: CPU Replacement Valid : %s\n", hfs2.fields.cpu_replaced_valid ? "YES" : "NO"); printk(BIOS_DEBUG, "ME: Current Working State : %s\n", - me_cws_values[hfs.fields.working_state]); + me_cws_values[hfs1.fields.working_state]); printk(BIOS_DEBUG, "ME: Current Operation State : %s\n", - me_opstate_values[hfs.fields.operation_state]); + me_opstate_values[hfs1.fields.operation_state]); printk(BIOS_DEBUG, "ME: Current Operation Mode : %s\n", - me_opmode_values[hfs.fields.operation_mode]); + me_opmode_values[hfs1.fields.operation_mode]); printk(BIOS_DEBUG, "ME: Error Code : %s\n", - me_error_values[hfs.fields.error_code]); + me_error_values[hfs1.fields.error_code]); printk(BIOS_DEBUG, "ME: Progress Phase : %s\n", me_progress_values[hfs2.fields.progress_code]); printk(BIOS_DEBUG, "ME: Power Management Event : %s\n", @@ -484,14 +484,14 @@ int send_global_reset(void) { int status = -1; - union me_hfs hfs; + union me_hfsts1 hfs1;
if (!is_cse_enabled()) goto ret;
/* Check ME operating mode */ - hfs.data = me_read_config32(PCI_ME_HFSTS1); - if (hfs.fields.operation_mode) + hfs1.data = me_read_config32(PCI_ME_HFSTS1); + if (hfs1.fields.operation_mode) goto ret;
/* ME should be in Normal Mode for this command */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, Is this true even for small core? If yes, why was APL not updated to use this?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... PS25, Line 215: hfsts2 What about the other hfsts? Are they not common across SoCs?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 266: 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds How was the 15 second timeout decided?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 273: if (stopwatch_expired(&sw)) It would be good to add a print indicating that timeout occurred. Also, it might be helpful to log how much time it typically takes.
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 101: set_host_ready This is a very generic name. Since it is being exposed as an API, it should ideally be named something like cse_set_host_ready()
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
Is this true even for small core? If yes, why was APL not updated to use this?
APL is just dumping all the FWSTS registers and not utilising the decoded FWSTS1.
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... PS25, Line 215: hfsts2
What about the other hfsts? Are they not common across SoCs?
Yes, they are differences in other hfsts registers across SoCs.
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 266: 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds
How was the 15 second timeout decided?
no recommendation as such, used the same timeout as the one used for wait_heci_ready
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
APL is just dumping all the FWSTS registers and not utilising the decoded FWSTS1.
But is FWSTS1 the same for APL as well?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
But is FWSTS1 the same for APL as well?
There is one difference, for APL/GLK bit 25 and 26 are as below. Bist Test Result[26] Bist Test State[25]
Will add fix to include APL as well
#if CONFIG(SOC_INTEL_APOLLOLAKE) u32 bist_test_state: 1; u32 bist_test_result: 1; #else u32 reserved1: 1; u32 bist_test_state: 1; #endif
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
There is one difference, for APL/GLK bit 25 and 26 are as below. […]
What about the operation state, operation mode, working state and other fields? Do they all have the exact same definitions across all platforms?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
What about the operation state, operation mode, working state and other fields? Do they all have the […]
Yes, rest of the bits remain same. The actual values represented by certain bits may differ, which have to be defined in SoC code. e.g, ICL CSME FW may define one extra operation mode than a CNL CSME.
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 273: if (stopwatch_expired(&sw))
It would be good to add a print indicating that timeout occurred. […]
WIll fix in a separate patch, Done
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 101: set_host_ready
This is a very generic name. […]
Will be done as separate patch, Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
Yes, rest of the bits remain same. […]
That is exactly my concern with making these common. Now that we have HFSTS1 common, there are changes going in which are making assumptions about modes in common code: https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block...
I don't think it is right. It should be the SoC providing those values. In fact, I am really not sure, do we really gain a lot by making this structure common? Can the required state not be queried by the common code by defining a proper API for callbacks?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs,
That is exactly my concern with making these common. […]
Also it was safe to make the assumptions in the case of opmodes values because backward compatibility is maintained in this regard. The opmodes that we rely on do not change across SoCs.
Nonetheless, will check further and see how we can correctly implement this, taking into account what values will change.
-
Aamir Bohra (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)