Change in coreboot[master]: soc/intel/common/block/cse: Move me_read_config32() to common code
Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 51 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/1
diff --git a/src/soc/intel/apollolake/cse.c b/src/soc/intel/apollolake/cse.c index 82226ec..32e9222 100644 --- a/src/soc/intel/apollolake/cse.c +++ b/src/soc/intel/apollolake/cse.c @@ -28,12 +28,6 @@ #include <device/pci_ops.h> #include <stdint.h>
-#define PCI_ME_HFSTS1 0x40 -#define PCI_ME_HFSTS2 0x48 -#define PCI_ME_HFSTS3 0x60 -#define PCI_ME_HFSTS4 0x64 -#define PCI_ME_HFSTS5 0x68 -#define PCI_ME_HFSTS6 0x6c
#define MKHI_GROUP_ID_MCA 0x0a #define READ_FILE 0x02 @@ -49,6 +43,8 @@
#define FUSE_LOCK_FILE "/fpf/intel/SocCfgLock"
+#define dump_status(index, hfs_reg) me_read_config32(hfs_reg) + /* Status values are made in such a way erase is not needed */ static enum fuse_flash_state { FUSE_FLASH_FUSED = 0xfc, @@ -186,15 +182,6 @@ printk(BIOS_CRIT, "failed to save FPF state\n"); }
-static uint32_t dump_status(int index, int reg_addr) -{ - uint32_t reg = pci_read_config32(PCH_DEV_CSE, reg_addr); - - printk(BIOS_DEBUG, "CSE FWSTS%d: 0x%08x\n", index, reg); - - return reg; -} - static void dump_cse_version(void *unused) { int res; diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index b8b4245d..dab909b 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -32,16 +32,6 @@ ME_WSTATE_NORMAL = 0x05, };
-/* HFSTS register offsets in PCI config space */ -enum { - PCI_ME_HFSTS1 = 0x40, - PCI_ME_HFSTS2 = 0x48, - PCI_ME_HFSTS3 = 0x60, - PCI_ME_HFSTS4 = 0x64, - PCI_ME_HFSTS5 = 0x68, - PCI_ME_HFSTS6 = 0x6C, -}; - /* Host Firmware Status Register 1 */ union hfsts1 { uint32_t raw; @@ -155,11 +145,6 @@ } __packed fields; };
-static uint32_t me_read_config32(int offset) -{ - return pci_read_config32(PCH_DEV_CSE, offset); -} - /* * From reading the documentation, this should work for both WHL and CML * platforms. Also, calling this function from dump_me_status() does not diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 97989f7..69cb273 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -507,6 +507,39 @@
return 0; } +uint32_t me_read_config32(int offset) +{ + uint32_t reg; + + reg = pci_read_config32(PCH_DEV_CSE, offset); + + if (!CONFIG(CONSOLE_SERIAL)) + { + uint8_t index; + + if( offset == PCI_ME_HFSTS1) + index = 1; + else if (offset == PCI_ME_HFSTS2) + index = 2; + else if (offset == PCI_ME_HFSTS3) + index = 3; + else if (offset == PCI_ME_HFSTS4) + index = 4; + else if (offset == PCI_ME_HFSTS5) + index = 5; + else if (offset == PCI_ME_HFSTS6) + index = 6; + else + index = 0; + + if (index != 0) + printk(BIOS_DEBUG, "CSE FWSTS%d: 0x%08x\n", index, reg); + else + printk(BIOS_DEBUG, "CSE [0x%x]=0x%08x\n", offset, reg); + } + return reg; +} +
#if ENV_RAMSTAGE
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 371a781..9409ce5 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -53,7 +53,23 @@ */ int heci_reset(void);
+/* + * Reads config value from a specified offset in the HECI Configuration + * + */ +uint32_t me_read_config32(int offset); + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
+/* HFSTS register offsets in PCI config space */ +enum { + PCI_ME_HFSTS1 = 0x40, + PCI_ME_HFSTS2 = 0x48, + PCI_ME_HFSTS3 = 0x60, + PCI_ME_HFSTS4 = 0x64, + PCI_ME_HFSTS5 = 0x68, + PCI_ME_HFSTS6 = 0x6C, +}; + #endif // SOC_INTEL_COMMON_MSR_H diff --git a/src/soc/intel/skylake/include/soc/me.h b/src/soc/intel/skylake/include/soc/me.h index 5a9acd5..fbe5033 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -21,7 +21,6 @@ /* * Management Engine PCI registers */ -#define PCI_ME_HFSTS1 0x40 #define ME_HFS_CWS_RESET 0 #define ME_HFS_CWS_INIT 1 #define ME_HFS_CWS_REC 2 @@ -169,7 +168,6 @@ } __packed fields; };
-#define PCI_ME_HFSTS3 0x60 #define ME_HFS3_FW_SKU_CONSUMER 0x2 #define ME_HFS3_FW_SKU_CORPORATE 0x3
@@ -186,9 +184,6 @@ } __packed fields; };
-#define PCI_ME_HFSTS4 0x64 -#define PCI_ME_HFSTS5 0x68 -#define PCI_ME_HFSTS6 0x6c #define ME_HFS6_FPF_NOT_COMMITTED 0x0 #define ME_HFS6_FPF_ERROR 0x2
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index f7aa584..e8d75e3 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -26,10 +26,6 @@ #include <stdlib.h> #include <string.h>
-static inline u32 me_read_config32(int offset) -{ - return pci_read_config32(PCH_DEV_CSE, offset); -}
/* HFSTS1[3:0] Current Working State Values */ static const char *const me_cws_values[] = {
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 516: if (!CONFIG(CONSOLE_SERIAL)) that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1) space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 70: PCI_ME_HFSTS4 = 0x64, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 70: PCI_ME_HFSTS4 = 0x64, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 71: PCI_ME_HFSTS5 = 0x68, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 71: PCI_ME_HFSTS5 = 0x68, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 72: PCI_ME_HFSTS6 = 0x6C, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 72: PCI_ME_HFSTS6 = 0x6C, please, no spaces at the start of a line
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... PS1, Line 46: use tab ?
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/+/35225
to look at the new patch set (#2).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file. * Fix all indentation comments
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 50 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35225/2//COMMIT_MSG@12 PS2, Line 12: * Fix all indentation comments not required, the gerrit patchset history knows what you have done with the patch
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1 you could create some static structure to refer offset and index to avoid such big if/else if logic
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 62: remove tab
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/+/35225
to look at the new patch set (#3).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 131 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 564: if (offset == PCI_ME_HFSTS1) trailing whitespace
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 62: <<<<<<< HEAD spaces required around that '<' (ctx:OxW)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: ======= spaces required around that '==' (ctx:ExO)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: ======= spaces required around that '==' (ctx:OxO)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: ======= spaces required around that '==' (ctx:OxO)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: ======= spaces required around that '=' (ctx:OxE)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 79: >>>>>>> b2d10c8321... src/soc/intel/common/block/cse: Add helper functions to CSE lib spaces required around that '>' (ctx:OxW)
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 79: >>>>>>> b2d10c8321... src/soc/intel/common/block/cse: Add helper functions to CSE lib spaces required around that ':' (ctx:VxW)
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/+/35225
to look at the new patch set (#4).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 57 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/4
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/+/35225
to look at the new patch set (#5).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 65 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/5
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/+/35225
to look at the new patch set (#6).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 56 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/6
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/+/35225
to look at the new patch set (#7).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 56 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/7
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 7:
(26 comments)
https://review.coreboot.org/c/coreboot/+/35225/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35225/2//COMMIT_MSG@12 PS2, Line 12: * Fix all indentation comments
not required, the gerrit patchset history knows what you have done with the patch
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... PS1, Line 46:
use tab ?
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 516: if (!CONFIG(CONSOLE_SERIAL))
that open brace { should be on the previous line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1)
space prohibited after that open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1)
space required before the open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
you could create some static structure to refer offset and index to avoid such big if/else if logic
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 511: if (offset == PCI_ME_HFSTS1) {
braces {} are not necessary for any arm of this statement
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 514: else if (offset == PCI_ME_HFSTS2) {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 517: else if (offset == PCI_ME_HFSTS3) {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 523: else if (offset == PCI_ME_HFSTS5) {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 526: else if (offset == PCI_ME_HFSTS6) {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 529: else {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 533: if (index != 0) {
braces {} are not necessary for any arm of this statement
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 546: if (!CONFIG(CONSOLE_SERIAL)) {
braces {} are not necessary for single statement blocks
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 62:
remove tab
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 70: PCI_ME_HFSTS4 = 0x64,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 71: PCI_ME_HFSTS5 = 0x68,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 71: PCI_ME_HFSTS5 = 0x68,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 72: PCI_ME_HFSTS6 = 0x6C,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 72: PCI_ME_HFSTS6 = 0x6C,
code indent should use tabs where possible
Done
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 7:
(35 comments)
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/apollolake/cs... PS1, Line 46:
use tab ?
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 516: if (!CONFIG(CONSOLE_SERIAL))
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1)
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 520: if( offset == PCI_ME_HFSTS1)
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 564: if (offset == PCI_ME_HFSTS1)
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/35225/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/4/src/soc/intel/common/block/... PS4, Line 525: if (index != 0) {
braces {} are not necessary for any arm of this statement
Done
https://review.coreboot.org/c/coreboot/+/35225/4/src/soc/intel/common/block/... PS4, Line 538: if (!CONFIG(CONSOLE_SERIAL)) {
braces {} are not necessary for single statement blocks
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 511: if (offset == PCI_ME_HFSTS1) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 514: else if (offset == PCI_ME_HFSTS2) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 517: else if (offset == PCI_ME_HFSTS3) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 523: else if (offset == PCI_ME_HFSTS5) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 526: else if (offset == PCI_ME_HFSTS6) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 529: else {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 533: if (index != 0) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 546: if (!CONFIG(CONSOLE_SERIAL)) {
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/6/src/soc/intel/common/block/... PS6, Line 540:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 62: <<<<<<< HEAD
spaces required around that '<' (ctx:OxW)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: =======
spaces required around that '==' (ctx:OxO)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: =======
spaces required around that '=' (ctx:OxE)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: =======
spaces required around that '==' (ctx:OxO)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 65: =======
spaces required around that '==' (ctx:ExO)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 79: >>>>>>> b2d10c8321... src/soc/intel/common/block/cse: Add helper functions to CSE lib
spaces required around that ':' (ctx:VxW)
Done
https://review.coreboot.org/c/coreboot/+/35225/3/src/soc/intel/common/block/... PS3, Line 79: >>>>>>> b2d10c8321... src/soc/intel/common/block/cse: Add helper functions to CSE lib
spaces required around that '>' (ctx:OxW)
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 62:
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 67: PCI_ME_HFSTS1 = 0x40,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 68: PCI_ME_HFSTS2 = 0x48,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 69: PCI_ME_HFSTS3 = 0x60,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 70: PCI_ME_HFSTS4 = 0x64,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 70: PCI_ME_HFSTS4 = 0x64,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 71: PCI_ME_HFSTS5 = 0x68,
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/1/src/soc/intel/common/block/... PS1, Line 72: PCI_ME_HFSTS6 = 0x6C,
Done
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/+/35225
to look at the new patch set (#8).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 56 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/8
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/5/src/soc/intel/common/block/... PS5, Line 520: else if (offset == PCI_ME_HFSTS4) {
else should follow close brace '}'
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/+/35225
to look at the new patch set (#9).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
TEST=Verified on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 56 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/9
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/+/35225
to look at the new patch set (#10).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 56 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/10
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... PS11, Line 46: #define dump_status(index, hfs_reg) me_read_config32(hfs_reg) you can remove dump_status from everywhere and just use me_read_config32.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/common/block... PS11, Line 510: if (offset == PCI_ME_HFSTS1) : index = 1; : else if (offset == PCI_ME_HFSTS2) : index = 2; : else if (offset == PCI_ME_HFSTS3) : index = 3; : else if (offset == PCI_ME_HFSTS4) : index = 4; : else if (offset == PCI_ME_HFSTS5) : index = 5; : else if (offset == PCI_ME_HFSTS6) : index = 6; would using a static structure beautify it? like below..
struct hfsts_index{ uint32_t hfsts; uint32_t index; } struct hfsts_index hfsts_to_index[6] = { {PCI_ME_HFSTS1, 1}, {PCI_ME_HFSTS2, 2}, {PCI_ME_HFSTS3, 3}, {PCI_ME_HFSTS4, 4}, {PCI_ME_HFSTS5, 5}, {PCI_ME_HFSTS6, 6}, };
for (i = 0, i<=6 , i++) { if (hfsts_to_index[i].hfsts == hfsts_reg) index=hfsts_to_index[i].index; }
I think just a switch statement would be more appropriate here.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
Done
its not 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/+/35225
to look at the new patch set (#12).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, move HFSTS macros specific to CSE PCI config header into common header file.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 60 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/12
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 12:
(3 comments)
Patch Set 11:
(1 comment)
It's very straight forward implementation.
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... PS11, Line 46: #define dump_status(index, hfs_reg) me_read_config32(hfs_reg)
you can remove dump_status from everywhere and just use me_read_config32.
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
its not done.
There is no benefit/optimization going with static structure. Hence, prefer to keep "else if" ladder implementation.
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/common/block... PS11, Line 510: if (offset == PCI_ME_HFSTS1) : index = 1; : else if (offset == PCI_ME_HFSTS2) : index = 2; : else if (offset == PCI_ME_HFSTS3) : index = 3; : else if (offset == PCI_ME_HFSTS4) : index = 4; : else if (offset == PCI_ME_HFSTS5) : index = 5; : else if (offset == PCI_ME_HFSTS6) : index = 6;
would using a static structure beautify it? like below.. […]
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/+/35225
to look at the new patch set (#13).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this funtion is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 60 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/13
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 524: : if (index != 0) : printk(BIOS_DEBUG, "CSE FWSTS%d: 0x%08x\n", index, reg); : else : printk(BIOS_DEBUG, "CSE [0x%x]=0x%08x\n", offset, reg); why we need to print this everytime, leave it to caller ?
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 535: PCH_DEV_CSE add DEV NULL check ?
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 537: if (!CONFIG(CONSOLE_SERIAL)) why we this guard?
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 57: * no need for multi line comments
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
There is no benefit/optimization going with static structure. […]
IMHO, you don't need to add this print then if no benefit, leave it on caller if wishes to add debug msg.
uint32_t me_read_config32(int offset) { 1. /* Do check if PCH_DEV_CSE != NULL */ 2. /* Check device is present in Bus like DID != 0xFFFF to avoid returning junk data (notify with debug msg) if HECI been disable and user doesn't realize that part and wishes to read status */ pci_read_config32(PCH_DEV_CSE, offset); }
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
IMHO, you don't need to add this print then if no benefit, leave it on caller if wishes to add debug […]
for #1 refer to https://github.com/coreboot/coreboot/blob/22d66efe652534a476e8165aa902138e19...
for #2 refer to https://github.com/coreboot/coreboot/blob/22d66efe652534a476e8165aa902138e19...
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/+/35225
to look at the new patch set (#14).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 93 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/14
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/+/35225
to look at the new patch set (#15).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 87 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/15
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/11/src/soc/intel/apollolake/c... PS11, Line 46: #define dump_status(index, hfs_reg) me_read_config32(hfs_reg)
Done
Done
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/2/src/soc/intel/common/block/... PS2, Line 518: index = 1
for #1 refer to […]
Done
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 524: : if (index != 0) : printk(BIOS_DEBUG, "CSE FWSTS%d: 0x%08x\n", index, reg); : else : printk(BIOS_DEBUG, "CSE [0x%x]=0x%08x\n", offset, reg);
why we need to print this everytime, leave it to caller ?
Done
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 535: PCH_DEV_CSE
add DEV NULL check ?
Done
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 537: if (!CONFIG(CONSOLE_SERIAL))
why we this guard?
Done
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/13/src/soc/intel/common/block... PS13, Line 57: *
no need for multi line comments
Done
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/apollolake/c... PS15, Line 189: remove extra line.
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/cannonlake/m... PS15, Line 191: remove extra line.
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... PS15, Line 523: remove extra line.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... PS15, Line 523:
remove extra line.
you mean error returns 0 and success returns -1? that strange should that be opposite ?
see what you have mentioned here https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block...
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... PS15, Line 57: * Returns 0 on failure 1 on success. you are doing something else in the function
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c... PS15, Line 282: intel_me_status are you also plan to move this into common code ? if yes then why don't you do it here in this CL so, it looks very nit.
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/+/35225
to look at the new patch set (#16).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 84 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/16
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/+/35225
to look at the new patch set (#17).
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 84 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/17
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/apollolake/c... PS15, Line 189:
remove extra line.
Done
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/cannonlake/m... PS15, Line 191:
remove extra line.
Done
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... PS15, Line 523:
you mean error returns 0 and success returns -1? that strange should that be opposite ? […]
Done
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/common/block... PS15, Line 57: * Returns 0 on failure 1 on success.
you are doing something else in the function
Done
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c... PS15, Line 282: intel_me_status
are you also plan to move this into common code ? if yes then why don't you do it here in this CL so […]
No plans to move this code to common code as the HFSTS1 register bit defination is different for each socket
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c... PS15, Line 282: intel_me_status
No plans to move this code to common code as the HFSTS1 register bit defination is different for ea […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/15/src/soc/intel/skylake/me.c... PS15, Line 282: intel_me_status
Done
i hope we could do that same using some Kconfig like version controlling, delta might be between SPT to CNP. from CNP i hope things are very much same across ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17: Code-Review+1
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17: Code-Review+2
V Sowmya has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Removed Code-Review+2 by V Sowmya v.sowmya@intel.com
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17: Code-Review+1
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17: -Code-Review
removing my vote as i have some other opinion
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... PS17, Line 189: same as CNL
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 188: if (!is_heci_enable()) return;
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 189: if (!me_read_config32(PCI_ME_HFSTS1, &hfsts1.raw)) : return; don't need any change here
hfsts1.raw = me_read_config32(PCI_ME_HFSTS1);
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 249: same as above. just check if (!is_heci_enable()) return; at line 232 and then don't need to make any change
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 505: add helper function to check HECI is enable/visible before asking for read status
bool is_heci_enable(const struct device *dev) { if (!cse_dev || !cse_dev->enabled) { printk(BIOS_WARNING, "HECI: No CSE device\n"); return false; }
if (pci_read_config16(PCH_DEV_CSE, PCI_VENDOR_ID) == 0xFFFF) { printk(BIOS_WARNING, "HECI: CSE device is hidden\n"); return false; } return true; }
let all caller check is_heci_enable() returns true before calling me_read_config32() in that case we can avoid multiple time device presence check which looks very redundant IMHO
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 506: int uint32_t me_read_config32(int offset) { return pci_read_config32(PCH_DEV_CSE, offset); }
so your caller doesn't required much changes
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c... PS17, Line 241: if (!me_read_config32(PCI_ME_HFSTS1, &hfs.data)) same as CNL
Rizwan Qureshi has uploaded a new patch set (#18) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
Also add a funtion to check if the CSE device has been enabled in the device tree and ut is visible on the bus. This API can be used by the caller to check before initiating any HECI communication.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 65 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/18
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 245: remove this extra line ?
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 491: return status; goto ret; just to make consistency if you wish ?
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 495: same here ?
Rizwan Qureshi has uploaded a new patch set (#19) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
Also add a funtion to check if the CSE device has been enabled in the devicetree and it is visible on the bus. This API can be used by the caller to check before initiating any HECI communication.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 65 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/19
Rizwan Qureshi has uploaded a new patch set (#20) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
Also add a funtion to check if the CSE device has been enabled in the devicetree and it is visible on the bus. This API can be used by the caller to check before initiating any HECI communication.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 63 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/20
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 20:
(10 comments)
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... File src/soc/intel/apollolake/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/apollolake/c... PS17, Line 189:
same as CNL
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 188:
if (!is_heci_enable()) […]
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 189: if (!me_read_config32(PCI_ME_HFSTS1, &hfsts1.raw)) : return;
don't need any change here […]
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/cannonlake/m... PS17, Line 249:
same as above. […]
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 505:
add helper function to check HECI is enable/visible before asking for read status […]
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/common/block... PS17, Line 506: int
uint32_t me_read_config32(int offset) […]
Done
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/17/src/soc/intel/skylake/me.c... PS17, Line 241: if (!me_read_config32(PCI_ME_HFSTS1, &hfs.data))
same as CNL
Done
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c File src/soc/intel/skylake/me.c:
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 245:
remove this extra line ?
Done
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 491: return status;
goto ret; just to make consistency if you wish ?
Done
https://review.coreboot.org/c/coreboot/+/35225/18/src/soc/intel/skylake/me.c... PS18, Line 495:
same here ?
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 20:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 511: No CSE device CSE device is disabled?
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 55: Configuration PCI configuration space?
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 68: /* HFSTS register offsets in PCI config space */ : enum { : PCI_ME_HFSTS1 = 0x40, : PCI_ME_HFSTS2 = 0x48, : PCI_ME_HFSTS3 = 0x60, : PCI_ME_HFSTS4 = 0x64, : PCI_ME_HFSTS5 = 0x68, : PCI_ME_HFSTS6 = 0x6C, : }; move before function declaration.
Rizwan Qureshi has uploaded a new patch set (#21) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
Also add a funtion to check if the CSE device has been enabled in the devicetree and it is visible on the bus. This API can be used by the caller to check before initiating any HECI communication.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 63 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35225/21
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 511: No CSE device
CSE device is disabled?
conveys the message
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 55: Configuration
PCI configuration space?
Done
https://review.coreboot.org/c/coreboot/+/35225/19/src/soc/intel/common/block... PS19, Line 68: /* HFSTS register offsets in PCI config space */ : enum { : PCI_ME_HFSTS1 = 0x40, : PCI_ME_HFSTS2 = 0x48, : PCI_ME_HFSTS3 = 0x60, : PCI_ME_HFSTS4 = 0x64, : PCI_ME_HFSTS5 = 0x68, : PCI_ME_HFSTS6 = 0x6C, : };
move before function declaration.
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 21: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 21: Code-Review+1
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
Patch Set 21: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35225 )
Change subject: soc/intel/common/block/cse: Move me_read_config32() to common code ......................................................................
soc/intel/common/block/cse: Move me_read_config32() to common code
me_read_config32() is defined in multiple places, move it to common location. Also, this function is usually used for reading HFSTS registers, hence move the HFSTS register definitions to common location.
Also add a funtion to check if the CSE device has been enabled in the devicetree and it is visible on the bus. This API can be used by the caller to check before initiating any HECI communication.
TEST=Verified reading HFSTS registers on CML RVP & Hatch board
Change-Id: Icdbfb6b30a007d469b5e018a313c14586addb130 Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35225 Reviewed-by: Subrata Banik subrata.banik@intel.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: V Sowmya v.sowmya@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/apollolake/cse.c 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 6 files changed, 63 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Subrata Banik: Looks good to me, approved Aamir Bohra: Looks good to me, but someone else must approve V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/cse.c b/src/soc/intel/apollolake/cse.c index 82226ec..260c6c6 100644 --- a/src/soc/intel/apollolake/cse.c +++ b/src/soc/intel/apollolake/cse.c @@ -28,12 +28,6 @@ #include <device/pci_ops.h> #include <stdint.h>
-#define PCI_ME_HFSTS1 0x40 -#define PCI_ME_HFSTS2 0x48 -#define PCI_ME_HFSTS3 0x60 -#define PCI_ME_HFSTS4 0x64 -#define PCI_ME_HFSTS5 0x68 -#define PCI_ME_HFSTS6 0x6c
#define MKHI_GROUP_ID_MCA 0x0a #define READ_FILE 0x02 @@ -188,7 +182,9 @@
static uint32_t dump_status(int index, int reg_addr) { - uint32_t reg = pci_read_config32(PCH_DEV_CSE, reg_addr); + uint32_t reg; + + reg = me_read_config32(reg_addr);
printk(BIOS_DEBUG, "CSE FWSTS%d: 0x%08x\n", index, reg);
@@ -256,6 +252,9 @@ { uint32_t fwsts1;
+ if (!is_cse_enabled()) + return; + fwsts1 = dump_status(1, PCI_ME_HFSTS1); dump_status(2, PCI_ME_HFSTS2); dump_status(3, PCI_ME_HFSTS3); diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index b8b4245d..cd94be5 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -32,16 +32,6 @@ ME_WSTATE_NORMAL = 0x05, };
-/* HFSTS register offsets in PCI config space */ -enum { - PCI_ME_HFSTS1 = 0x40, - PCI_ME_HFSTS2 = 0x48, - PCI_ME_HFSTS3 = 0x60, - PCI_ME_HFSTS4 = 0x64, - PCI_ME_HFSTS5 = 0x68, - PCI_ME_HFSTS6 = 0x6C, -}; - /* Host Firmware Status Register 1 */ union hfsts1 { uint32_t raw; @@ -155,11 +145,6 @@ } __packed fields; };
-static uint32_t me_read_config32(int offset) -{ - return pci_read_config32(PCH_DEV_CSE, offset); -} - /* * From reading the documentation, this should work for both WHL and CML * platforms. Also, calling this function from dump_me_status() does not @@ -201,6 +186,9 @@ if (!CONFIG(CONSOLE_SERIAL)) return;
+ if (!is_cse_enabled()) + return; + hfsts1.raw = me_read_config32(PCI_ME_HFSTS1);
/* @@ -244,6 +232,9 @@ union hfsts5 hfsts5; union hfsts6 hfsts6;
+ if (!is_cse_enabled()) + return; + hfsts1.raw = me_read_config32(PCI_ME_HFSTS1); hfsts2.raw = me_read_config32(PCI_ME_HFSTS2); hfsts3.raw = me_read_config32(PCI_ME_HFSTS3); diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 9520242..1671970 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -503,6 +503,28 @@ return 0; }
+bool is_cse_enabled(void) +{ + const struct device *cse_dev = pcidev_path_on_root(PCH_DEVFN_CSE); + + if (!cse_dev || !cse_dev->enabled) { + printk(BIOS_WARNING, "HECI: No CSE device\n"); + return false; + } + + if (pci_read_config16(PCH_DEV_CSE, PCI_VENDOR_ID) == 0xFFFF) { + printk(BIOS_WARNING, "HECI: CSE device is hidden\n"); + return false; + } + + return true; +} + +uint32_t me_read_config32(int offset) +{ + return pci_read_config32(PCH_DEV_CSE, offset); +} + #if ENV_RAMSTAGE
static void update_sec_bar(struct device *dev) diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 424d483..bce615c 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -19,6 +19,16 @@
#include <stdint.h>
+/* HFSTS register offsets in PCI config space */ +enum { + PCI_ME_HFSTS1 = 0x40, + PCI_ME_HFSTS2 = 0x48, + PCI_ME_HFSTS3 = 0x60, + PCI_ME_HFSTS4 = 0x64, + PCI_ME_HFSTS5 = 0x68, + PCI_ME_HFSTS6 = 0x6C, +}; + /* set up device for use in early boot enviroument with temp bar */ void heci_init(uintptr_t bar); /* @@ -52,6 +62,16 @@ */ int heci_reset(void);
+/* Reads config value from a specified offset in the CSE PCI Config space. */ +uint32_t me_read_config32(int offset); + +/* + * Check if the CSE device is enabled in device tree. Also check if the device + * is visible on the PCI bus by reading config space. + * Return true if device present and config space enabled, else return false. + */ +bool is_cse_enabled(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 5a9acd5..fbe5033 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -21,7 +21,6 @@ /* * Management Engine PCI registers */ -#define PCI_ME_HFSTS1 0x40 #define ME_HFS_CWS_RESET 0 #define ME_HFS_CWS_INIT 1 #define ME_HFS_CWS_REC 2 @@ -169,7 +168,6 @@ } __packed fields; };
-#define PCI_ME_HFSTS3 0x60 #define ME_HFS3_FW_SKU_CONSUMER 0x2 #define ME_HFS3_FW_SKU_CORPORATE 0x3
@@ -186,9 +184,6 @@ } __packed fields; };
-#define PCI_ME_HFSTS4 0x64 -#define PCI_ME_HFSTS5 0x68 -#define PCI_ME_HFSTS6 0x6c #define ME_HFS6_FPF_NOT_COMMITTED 0x0 #define ME_HFS6_FPF_ERROR 0x2
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index f7aa584..9e17ef1 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -26,10 +26,6 @@ #include <stdlib.h> #include <string.h>
-static inline u32 me_read_config32(int offset) -{ - return pci_read_config32(PCH_DEV_CSE, offset); -}
/* HFSTS1[3:0] Current Working State Values */ static const char *const me_cws_values[] = { @@ -242,6 +238,9 @@ if (!CONFIG(CONSOLE_SERIAL)) return;
+ if (!is_cse_enabled()) + return; + hfs.data = me_read_config32(PCI_ME_HFSTS1); /* * This command can be run only if: @@ -288,6 +287,9 @@ union me_hfs3 hfs3; union me_hfs6 hfs6;
+ if (!is_cse_enabled()) + return; + hfs.data = me_read_config32(PCI_ME_HFSTS1); hfs2.data = me_read_config32(PCI_ME_HFSTS2); hfs3.data = me_read_config32(PCI_ME_HFSTS3); @@ -484,6 +486,9 @@ int status = -1; union me_hfs hfs;
+ if (!is_cse_enabled()) + goto ret; + /* Check ME operating mode */ hfs.data = me_read_config32(PCI_ME_HFSTS1); if (hfs.fields.operation_mode)
-
Aamir Bohra (Code Review) -
build bot (Jenkins) (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)