Attention is currently required from: Andrey Petrov, Arthur Heymans, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Paul Menzel, Ronak Kanabar, Shuo Liu, Subrata Banik, Tim Chu, Wonkyu Kim.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80277?usp=email )
Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ......................................................................
Patch Set 4:
(16 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80277/comment/62a62a86_bb8be310 : PS2, Line 39: In addition, this commit sets`PLATFORM_USES_FSP2_X86_32' to `n' by : default if FSP 2.4 is enabled as 64-bits FSP should be norm moving : forward.
Maybe make that a separate commit, so it also gets more exposure.
Done
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/80277/comment/2422e2f8_a78ceff5 : PS2, Line 62: Starting with FSP specification 2.4 they can be 64-bits.
i thought u said elsewhere that FSP 2.4 also supports 32-bit? […]
`they can be 64-bits` does not mean they cannot be 32-bits
Would not the following work in a soc Kconfig ? ``` config PLATFORM_USES_FSP2_X86_32 default n ```
File src/drivers/intel/fsp2_0/debug.c:
https://review.coreboot.org/c/coreboot/+/80277/comment/3367aecb_78ee0292 : PS2, Line 94: efi_return_status_t
need to include `#include <efi/efi_datatype. […]
This is already include throught the `fsp/util.h`. Also if I included in the right alphabetical order then I will hit a EFIAPI is already defined issue as EDK headers are defining it if not already and soc_binding.h will try to define it appropriately after that.
https://review.coreboot.org/c/coreboot/+/80277/comment/b1a9fede_b9724649 : PS2, Line 97: status
why not use `size_t status` directly ?
I tried to use size_t but then it breaks due to some EFI_STATUS usage in some EDK2 prototype. Anyway, the use of `size_t` was nice but more like a hack. I moved to a helper function approach instead.
File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/80277/comment/ffad1b94_701e8cc7 : PS2, Line 11: #define __efiapi EFIAPI
why not inside `efi_datatype. […]
Done
File src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h:
https://review.coreboot.org/c/coreboot/+/80277/comment/630427a2_881c6c1f : PS2, Line 13: #include <efi/efi_datatype.h>
order ?
Unfortunately, the order matter as soc_binding is in charge of defining EFIAPI appropriately. I added a comment. Let me know if you have a better suggestion.
File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/80277/comment/745c9673_becabd8c : PS2, Line 39: #if CONFIG(PLATFORM_USES_FSP2_4)
look at line #36, i don;t think u need a guard here as well. […]
Done
File src/drivers/intel/fsp2_0/include/fsp/soc_binding.h:
https://review.coreboot.org/c/coreboot/+/80277/comment/5e0a8959_6a1beecf : PS2, Line 8: #if CONFIG(VENDOR_INTEL)
why Intel? this is UEFI calling convention and nothing SoC vendor specific […]
I "temporarily" added this to see if it would help with coreboot jenkins error on AMD platform.. I removed it.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80277/comment/5eec4725_6ea9ffa3 : PS2, Line 282: efi_return_status_t
size_t?
Done
File src/include/efi/efi_datatype.h:
https://review.coreboot.org/c/coreboot/+/80277/comment/9a369a98_be788dfc : PS2, Line 81: statuses
???
Done
https://review.coreboot.org/c/coreboot/+/80277/comment/6e2563fe_89f26c8b : PS2, Line 88: _Static_assert(sizeof(size_t) == sizeof(efi_return_status_t), : "Unexpected EFI_STATUS size");
if u use `size_t` over `efi_return_status_t` then I don't believe u need this logic
Done
File src/soc/intel/xeon_sp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/80277/comment/c9eee100_cf6f7c82 : PS2, Line 5: #include <fsp/util.h>
please submit this cl separately
Done
https://review.coreboot.org/c/coreboot/+/80277/comment/4bf8e945_9fe02f04 : PS2, Line 9: #include <soc/iomap.h>
same
Done
https://review.coreboot.org/c/coreboot/+/80277/comment/5ec24a9e_65567eef : PS2, Line 10: #include <console/console.h> : #include <cpu/x86/mtrr.h>
this is also out of order
Done
https://review.coreboot.org/c/coreboot/+/80277/comment/a0c62701_4b454012 : PS2, Line 12: #include <intelblocks/lpc_lib.h>
same
Done
https://review.coreboot.org/c/coreboot/+/80277/comment/e8ed2170_5a725d08 : PS2, Line 15: #include <soc/bootblock.h>
same
Done