Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37490 )
Change subject: soc/amd/picasso: Add bootblock ......................................................................
Patch Set 4:
(2 comments)
Because of the separation with util/cbfstool/amdcompress, there is a bunch of redundant parameters passed on commandline, while the information could be derived from ELF headers. IMHO, once we have created the final ELF it should be forbidden to reference the Kconfigs that were used inside the linker scripts.
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... PS4, Line 213: PSP_BIOSBIN_DEST=$(shell printf "%x" $(call int-subtract, $(call int-add, $(CONFIG_X86_RESET_VECTOR) 0x10) $(PSP_BIOSBIN_SIZE))) I believe we would want to avoid new references to C_ENV_BOOTBLOCK_SIZE and take the actual uncompressed progsz from ELF headers.
As for the reference to X86_RESET_VECTOR, I had a look at some (leaked to me via virtual directory listing) datasheets and it is more powerful in expression than it needs to be since hardware is wired at IP=fff0. You would avoid the reference and calculation here if you had made amdfwtool parse the ELF (and optionally compress too).
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... PS4, Line 301: $(PSP_BIOSBIN_FILE): $(PSP_ELF_FILE) $(AMDCOMPRESS) I am looking at CB:33401 where util/cbfstool/amdcompress.c was introduced and I am not so happy about it. AFAICS it has nothing to do with CBFS and the feature it implements should have been incorporated inside util/amdfwtool instead.