Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 9:
(9 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?
Nope. Will remove.
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. […]
Sure. Doesn't hurt anything. When I added it, it was only needed for the vboot calls used by psp_verstage though.
I'm leaving it in this commit as a bootblock-y, but moving it to arch/x86/Makefile.inc in a follow-on patch (already done).
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. […]
I actually like this. I wrote a patch to allow verstage to have its own memlayout, but didn't like having the extra code. This situation is not very common, so I think this is cleaner and less confusing.
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 underst […]
That's true. Psp_verstage will break now if we don't have the fmap cach enabled.
Removed.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 5: CPPFLAGS_verstage += -I$(src)/soc/amd/picasso/psp_verstage/include
I haven't gotten to test this yet. Hopefully today.
Removed - this is no longer needed.
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?
Yes. CPPFLAGS_verstage gets added to the command line before verstage-generic-ccopts. We now only have the arch/io.h file in here which isn't in the arm directories, so that doesn't even matter.
Changed to verstage-generic-ccopts
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?
inb and outb are used all over, but not too much in the code that we're using for psp_verstage.
I'll start a bug and see what needs to be fixed for psp_verstage.
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. […]
I don't think they are, so this could probably be removed. It was just in the list of values returned by the get_boot_mode call, so I added it in.
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. […]
Thanks. Updated.