Change in coreboot[master]: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platf...
Hello Anjaneya "Reddy" Chagam,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43335
to review the following change.
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs
Platform HOBs (in particular IIO_UDS and MemoryMap HOBs) are of HOB type HOB_TYPE_GUID_EXTENSION, therefore it does not have resource structure.
Remove the errorneous code related to resource structure.
Remove unnecessary function prototypes from heade files, and define them as static.
Since we have the HOB pointer, there is not need to search HOB by GUID. Remove unnecessary calling of fsp_find_extension_hob_by_guid().
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ib99bce39e6eb2aeb95242dfba36774653bbe91fd --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 3 files changed, 25 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/43335/1
diff --git a/src/soc/intel/xeon_sp/cpx/hob_display.c b/src/soc/intel/xeon_sp/cpx/hob_display.c index d5910c3..0263472 100644 --- a/src/soc/intel/xeon_sp/cpx/hob_display.c +++ b/src/soc/intel/xeon_sp/cpx/hob_display.c @@ -33,37 +33,14 @@ return NULL; }
-void soc_display_hob(const struct hob_header *hob) +static void soc_display_memmap_hob(const struct SystemMemoryMapHob *hob_addr) { - const struct hob_resource *res; + struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)hob_addr;
- res = fsp_hob_header_to_resource(hob); - assert(res != NULL); - printk(BIOS_DEBUG, "\tResource type: 0x%x, attribute: 0x%x, addr: 0x%08llx, len: 0x%08llx\n", - res->type, res->attribute_type, res->addr, res->length); - printk(BIOS_DEBUG, "\tOwner GUID: "); - fsp_print_guid(res->owner_guid); - printk(BIOS_DEBUG, " (%s)\n", fsp_get_guid_name(res->owner_guid)); + printk(BIOS_DEBUG, "================== MEMORY MAP HOB DATA ==================\n"); + printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", + hob, sizeof(*hob));
- if (fsp_guid_compare(res->owner_guid, fsp_hob_iio_uds_guid)) - soc_display_iio_universal_data_hob(); - else if (fsp_guid_compare(res->owner_guid, fsp_hob_memmap_guid)) - soc_display_memmap_hob(); -} - -void soc_display_memmap_hob(void) -{ - size_t hob_size; - const struct SystemMemoryMapHob *hob; - const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID; - - hob = (const struct SystemMemoryMapHob *)fsp_find_extension_hob_by_guid( - mem_hob_guid, &hob_size); - assert(hob != NULL); - - printk(BIOS_DEBUG, "===================== MEMORY MAP HOB DATA =====================\n"); - printk(BIOS_DEBUG, "hob: %p, size: 0x%lx, structure size: 0x%lx\n", - hob, hob_size, sizeof(*hob)); printk(BIOS_DEBUG, "\tlowMemBase: 0x%x, lowMemSize: 0x%x, highMemBase: 0x%x, " "highMemSize: 0x%x\n", hob->lowMemBase, hob->lowMemSize, hob->highMemBase, hob->highMemSize); @@ -82,17 +59,11 @@ hexdump(hob, sizeof(*hob)); }
-void soc_display_iio_universal_data_hob(void) +static void soc_display_iio_universal_data_hob(const IIO_UDS *hob) { - size_t hob_size; - const IIO_UDS *hob; - const uint8_t fsp_hob_iio_universal_data_guid[16] = FSP_HOB_IIO_UNIVERSAL_DATA_GUID; - - hob = (const IIO_UDS *)fsp_find_extension_hob_by_guid( - fsp_hob_iio_universal_data_guid, &hob_size); - assert(hob != NULL); - printk(BIOS_DEBUG, "===================== IIO_UDS HOB DATA =====================\n"); + printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", + hob, sizeof(*hob));
printk(BIOS_DEBUG, "\t===================== SYSTEM STATUS =====================\n"); printk(BIOS_DEBUG, "\tnumCpus: 0x%x\n", hob->SystemStatus.numCpus); @@ -194,4 +165,21 @@ iio_resource.PcieInfo.PortInfo[p].Function); } } + + hexdump(hob, sizeof(*hob)); +} + +void soc_display_hob(const struct hob_header *hob) +{ + uint8_t *guid; + + if (hob->type != HOB_TYPE_GUID_EXTENSION) + return; + + guid = (uint8_t *) fsp_hob_header_to_resource(hob); + + if (fsp_guid_compare(guid, fsp_hob_iio_uds_guid)) + soc_display_iio_universal_data_hob((const IIO_UDS *)(guid + 16)); + else if (fsp_guid_compare(guid, fsp_hob_memmap_guid)) + soc_display_memmap_hob((const struct SystemMemoryMapHob *)(guid + 16)); } diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index d9ef24d..412730b 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -42,6 +42,5 @@ const struct SystemMemoryMapHob *get_system_memory_map(void);
void set_bios_init_completion(void); -void soc_display_iio_universal_data_hob(void);
#endif /* _SOC_UTIL_H_ */ diff --git a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h index ee86a6d..df7787a 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h +++ b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h @@ -109,6 +109,4 @@
#pragma pack()
-void soc_display_memmap_hob(void); - #endif
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@10 PS1, Line 10: it does not have they do not have
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@12 PS1, Line 12: errorneous no second `r`: erroneous
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@14 PS1, Line 14: heade header
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@10 PS1, Line 10: it does not have
they do not have
Done
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@12 PS1, Line 12: errorneous
no second `r`: erroneous
Done
https://review.coreboot.org/c/coreboot/+/43335/1//COMMIT_MSG@14 PS1, Line 14: heade
header
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Christian Walter, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43335
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs
Platform HOBs (in particular IIO_UDS and MemoryMap HOBs) are of HOB type HOB_TYPE_GUID_EXTENSION, therefore they do not have resource structure.
Remove the erroneous code related to resource structure.
Remove unnecessary function prototypes from header files, and define them as static in hob_display.c.
Since we have the HOB pointer, there is not need to search HOB by GUID. Remove unnecessary calling of fsp_find_extension_hob_by_guid().
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ib99bce39e6eb2aeb95242dfba36774653bbe91fd --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 3 files changed, 25 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/43335/2
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43335 )
Change subject: soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs ......................................................................
soc/intel/xeon_sp/cpx: use HOB_TYPE_GUID_EXTENSION to interpret platform HOBs
Platform HOBs (in particular IIO_UDS and MemoryMap HOBs) are of HOB type HOB_TYPE_GUID_EXTENSION, therefore they do not have resource structure.
Remove the erroneous code related to resource structure.
Remove unnecessary function prototypes from header files, and define them as static in hob_display.c.
Since we have the HOB pointer, there is not need to search HOB by GUID. Remove unnecessary calling of fsp_find_extension_hob_by_guid().
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ib99bce39e6eb2aeb95242dfba36774653bbe91fd Reviewed-on: https://review.coreboot.org/c/coreboot/+/43335 Reviewed-by: Christian Walter christian.walter@9elements.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 3 files changed, 25 insertions(+), 40 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Christian Walter: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/cpx/hob_display.c b/src/soc/intel/xeon_sp/cpx/hob_display.c index d5910c3..0263472 100644 --- a/src/soc/intel/xeon_sp/cpx/hob_display.c +++ b/src/soc/intel/xeon_sp/cpx/hob_display.c @@ -33,37 +33,14 @@ return NULL; }
-void soc_display_hob(const struct hob_header *hob) +static void soc_display_memmap_hob(const struct SystemMemoryMapHob *hob_addr) { - const struct hob_resource *res; + struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)hob_addr;
- res = fsp_hob_header_to_resource(hob); - assert(res != NULL); - printk(BIOS_DEBUG, "\tResource type: 0x%x, attribute: 0x%x, addr: 0x%08llx, len: 0x%08llx\n", - res->type, res->attribute_type, res->addr, res->length); - printk(BIOS_DEBUG, "\tOwner GUID: "); - fsp_print_guid(res->owner_guid); - printk(BIOS_DEBUG, " (%s)\n", fsp_get_guid_name(res->owner_guid)); + printk(BIOS_DEBUG, "================== MEMORY MAP HOB DATA ==================\n"); + printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", + hob, sizeof(*hob));
- if (fsp_guid_compare(res->owner_guid, fsp_hob_iio_uds_guid)) - soc_display_iio_universal_data_hob(); - else if (fsp_guid_compare(res->owner_guid, fsp_hob_memmap_guid)) - soc_display_memmap_hob(); -} - -void soc_display_memmap_hob(void) -{ - size_t hob_size; - const struct SystemMemoryMapHob *hob; - const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID; - - hob = (const struct SystemMemoryMapHob *)fsp_find_extension_hob_by_guid( - mem_hob_guid, &hob_size); - assert(hob != NULL); - - printk(BIOS_DEBUG, "===================== MEMORY MAP HOB DATA =====================\n"); - printk(BIOS_DEBUG, "hob: %p, size: 0x%lx, structure size: 0x%lx\n", - hob, hob_size, sizeof(*hob)); printk(BIOS_DEBUG, "\tlowMemBase: 0x%x, lowMemSize: 0x%x, highMemBase: 0x%x, " "highMemSize: 0x%x\n", hob->lowMemBase, hob->lowMemSize, hob->highMemBase, hob->highMemSize); @@ -82,17 +59,11 @@ hexdump(hob, sizeof(*hob)); }
-void soc_display_iio_universal_data_hob(void) +static void soc_display_iio_universal_data_hob(const IIO_UDS *hob) { - size_t hob_size; - const IIO_UDS *hob; - const uint8_t fsp_hob_iio_universal_data_guid[16] = FSP_HOB_IIO_UNIVERSAL_DATA_GUID; - - hob = (const IIO_UDS *)fsp_find_extension_hob_by_guid( - fsp_hob_iio_universal_data_guid, &hob_size); - assert(hob != NULL); - printk(BIOS_DEBUG, "===================== IIO_UDS HOB DATA =====================\n"); + printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", + hob, sizeof(*hob));
printk(BIOS_DEBUG, "\t===================== SYSTEM STATUS =====================\n"); printk(BIOS_DEBUG, "\tnumCpus: 0x%x\n", hob->SystemStatus.numCpus); @@ -194,4 +165,21 @@ iio_resource.PcieInfo.PortInfo[p].Function); } } + + hexdump(hob, sizeof(*hob)); +} + +void soc_display_hob(const struct hob_header *hob) +{ + uint8_t *guid; + + if (hob->type != HOB_TYPE_GUID_EXTENSION) + return; + + guid = (uint8_t *) fsp_hob_header_to_resource(hob); + + if (fsp_guid_compare(guid, fsp_hob_iio_uds_guid)) + soc_display_iio_universal_data_hob((const IIO_UDS *)(guid + 16)); + else if (fsp_guid_compare(guid, fsp_hob_memmap_guid)) + soc_display_memmap_hob((const struct SystemMemoryMapHob *)(guid + 16)); } diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index d9ef24d..412730b 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -42,6 +42,5 @@ const struct SystemMemoryMapHob *get_system_memory_map(void);
void set_bios_init_completion(void); -void soc_display_iio_universal_data_hob(void);
#endif /* _SOC_UTIL_H_ */ diff --git a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h index ee86a6d..df7787a 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h +++ b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h @@ -109,6 +109,4 @@
#pragma pack()
-void soc_display_memmap_hob(void); - #endif
-
Angel Pons (Code Review) -
Christian Walter (Code Review)
-
Jonathan Zhang (Code Review)
-
Patrick Georgi (Code Review)