Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 9:
(14 comments)
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Kconfig... PS9, Line 13: VBOOT && Is this necessary? Don't think VERSTAGE will do anything without VBOOT.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Makefil... PS9, Line 26: arch/x86/memmove.c Why not add bootblock-y += memmove.c to arch/x86/Makefile.inc? I don't think child dirs should be reaching into parent dirs to add code.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/chip.h@... PS9, Line 15: #include <arch/x86/include/arch/smp/mpspec.h> /* point from top level */ hrmm, just delete the include. It's not used. irq_override only uses uint8_t.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 35: . = VERSTAGE_START; : _sram = .; SRAM_START macro
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 52: VERSTAGE_SIZE Is this a hard limit or a self imposed one?
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 53: . = VBOOT_WORK_START; : _vboot2_work = .; : _evboot2_work = VBOOT_WORK_START + VBOOT_WORK_SIZE; : . = _evboot2_work; Maybe use the REGION macro?
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 59: _fmap_cache = .; : _efmap_cache = _fmap_cache + FMAP_CACHE_SIZE; FMAP_CACHE(., FMAP_SIZE)
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 65: _stack = PSP_VERSTAGE_STACK_START; : PSP_VERSTAGE_STACK_BASE = _stack; : _estack = PSP_VERSTAGE_STACK_START + PSP_VERSTAGE_STACK_SIZE; REGION
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 69: _esram = SRAM_START + SRAM_SIZE; SRAM_END
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 3: ifeq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),y) You are already guarding the subdir in the parent dir. I would remove this.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/fch.c:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 34: void * bar clang-format
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.h:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 8: nit: Add tabs
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 161: svc_debug_print No message?
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 210: stage_entry What's this for?