Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 8:
(8 comments)
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/Kconfig... PS8, Line 483: The workbuf's base depends on the address of the reset : vector. Is this comment still valid?
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/Makefil... PS8, Line 26: bootblock-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += ../../../arch/x86/memmove.c Shouldn't we unconditionally add memmove.c to bootblock-y in arch/x86/Makefile.inc?
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/memlayo... PS8, Line 4: #include "memlayout_psp_verstage.ld" Do you think it's easier/cleaner to allow variable verstage memlayout.ld instead of the conditional?
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/memlayo... PS8, Line 58: #if !CONFIG(NO_FMAP_CACHE) We're basically saying we would never run in this config, though? Do we need to guard it? My understanding was if that was selected verstage on psp just wouldn't work.
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... PS8, Line 5: CPPFLAGS_verstage += -I$(src)/soc/amd/picasso/psp_verstage/include do the arch/ include paths get hit here first because of -I order on command line?
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/io.h:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... PS8, Line 9: outb What is pulling these deps in? Or is it everything? And we should fix the tree in general?
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/pmutil.c:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... PS8, Line 16: PSP_BOOT_MODE_S0i3_RESUME future: we'd need to reevaluate this path if things are getting pulled from spi rom or not. If so, not a great s0i3.
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/timer.c:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... PS8, Line 15: mt->microseconds = clk / 100; fwiw, experiments suggest this frequency is wrong. It's actually 25MHz. I can point you at bugs if you are interested.