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 8:
(13 comments)
https://review.coreboot.org/c/coreboot/+/41816/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41816/7//COMMIT_MSG@11 PS7, Line 11: increase
*to* increase?
Done
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/memlayo... PS7, Line 3: #if ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
Sounds good. I definitely like that option better.
Done
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/memlayo... PS7, Line 174: : /* SPDX-License-Identifier: GPL-2.0-only */
remove
Done
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'll see what I can do.
I haven't gotten to test this yet. Hopefully today.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/fch.c:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 79: printk(BIOS_DEBUG, "Mapping %s\n", bar_map[i].name);
is this debug really necessary any more?
removed.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 80: err = svc_map_fch_dev(bar_map[i].args.device, bar_map[i].args.arg0, 0, &bar);
I'm assuming the individual calls are fast, but is there a reason why we didn't make this a single c […]
Because AMD's interface lets us map one bar at a time. We can bring it up to them.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 86: printk(BIOS_DEBUG, "%s mapped to 0x%p\n", bar_map[i].name, bar);
debug still necessary?
removed.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 121: svc_debug_print("Mapping devices\n");
why svc_debug_print vs. […]
This happens earlier in the sequence before the IO is set up, so we can't use printk yet. I removed this one as unnecessary as well though.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 32: printk Can't use printk here because it can be called early if there's a failure. Switch to svc_debug_print()
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 140: /* Do not use printk() before verstage_mainboard_init() is called */
why not?
Because IO isn't initialized until that point, and the system will hang.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 156: svc_write_postcode(0x01);
you use defines elsewhere but literals here. And we switch from scv_write_postcode to post_code.
Thanks, I meant to go back and add #defines for these.
We need to use svc_write_postcode before verstage_mainboard_init so that the IO addresses get set up, then we can switch over to post_code().
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 186: svc_write_postcode(0xF1);
and switch back. […]
Yes. These were the first ones implemented and they never got updated.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 62: sha_op.DataLen = MIN(size, 1024);
why 1024 here? Any benefit to larger or smaller?
That's a VERY good question. I was hoping that the answer would be in the updated documentation, but it's not there. I'll address it with AMD in our next meeting.
In the meantime I'll turn it into a #define with a note that it should be tuned.