Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 7:
(8 comments)
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?
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 call with all the bar's that we want mapped? Just thinking about performance later.
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?
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. printk here?
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 140: /* Do not use printk() before verstage_mainboard_init() is called */ why not?
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.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 186: svc_write_postcode(0xF1); and switch back. Consolidate?
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?