Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel Xeon Scalable Processor support ......................................................................
Patch Set 42:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/boot... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/boot... PS41, Line 33: .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), : .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE,
No it doesn't make sense to set them to zero. Let FSP TempRamInit do its job of setting MTTR. […]
I can't agree to that and here is why:
1. We don't want to have dependency on SPI flash size. With the current version of the patch if you put 64mb flash chip in, you would have to adjust both FMD file and CONFIG_CBFS_SIZE.
2. CONFIG_CBFS_SIZE is not used for anything else. In fact in this patch it is used for only that purpose and needs to be dropped. Brevity ftw.
3. if you use fast_spi_cache_bios_region() you will get MTRR set up for you for exactly same BIOS region, and that information would come from IFD, so it is fool proof.
CPX does not hang for me with CodeRegionBase/CodeRegionLength are set to 0. Did you set them to zero or left uninitialized? If it does hang with 0s then it is a FSP bug and needs to be fixed.
Now then, if you absolutely have to use CBFS_SIZE for some reason, here is what can be done to make sense of it. FMD synthax allows for config variable substitution, for example CB:23150 So what you can end up is something like this:
COREBOOT(CBFS)@0x0 ##CBFS_SIZE##
But then again, I really see no good reason to add yet another hardcoded value.
This is slightly off topic, but we should be ditching FSP-T. This value and CONFIG_CPU_MICROCODE_CBFS_LOC/CONFIG_CPU_MICROCODE_CBFS_LEN are similar and only lead to more hardcode across the codebase and only add dependencies.