Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: Picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 1:
(16 comments)
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Kconfig... PS1, Line 388: 3rdparty/blobs We're using 3rdparty/amd_blobs now.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 229: # type = 0x66 : PSP_UCODE_FILE1=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B1.bin : PSP_UCODE_FILE2=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B0.bin : PSP_UCODE_FILE3=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_RV2_A0.bin remove these too
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/Makefil... PS1, Line 290: OPT_PSP_UCODE_FILE1=$(call add_opt_prefix, $(PSP_UCODE_FILE1), --instance 0 --ucode) : OPT_PSP_UCODE_FILE2=$(call add_opt_prefix, $(PSP_UCODE_FILE2), --instance 1 --ucode) : OPT_PSP_UCODE_FILE3=$(call add_opt_prefix, $(PSP_UCODE_FILE3), --instance 2 --ucode) Remove these too
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c File src/soc/amd/picasso/cpu.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/cpu.c@1... PS1, Line 122: { X86_VENDOR_AMD, 0x810f80 }, : { X86_VENDOR_AMD, 0x810f81 }, : { X86_VENDOR_AMD, 0x820f81 }, It seems like we should split this into a separate patch and use #define values for the IDs.
0x810f80 is Picasso Step 0, unreleased product 0x810f81 is a duplicate of PICASSO_CPUID? 0x820f81: Not sure what this is. Do you have a board with this?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 12: BKDG for AMD Family 16h Models 30h-3Fh Processors : * 52740 Rev 3.06 - March 18, 2016 It looks like only the NDA version and not the public version AFAICS. If we need to cite an NDA doc, let's use the Picasso one. We may be able to find a public doc at https://developer.amd.com/resources/developer-guides-manuals/ that defines the info, though.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 16: F16H_MPB_MAX_SIZE 3458 : #define F16H_MPB_DATA_OFFSET 32 Even though you've said the MPB definition can be found in a Family 16h BKDG, I wouldn't use F16H in the names
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 20: FAM16h Same idea. I would remove this.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 74: msr.hi = 0; : msr.lo = (uint32_t)m; Maybe
.hi = (uint64_t)(uintptr_t)m >> 32; .lo = (uintptr_t)m & 0xffffffff;
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 77: 0xc0010020 MSR_PATCH_LOADER in cpu/amd/msr.h
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 80: m->patch_id); Nit, I would indent another tab stop here and on lines 87, 88.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 83: 0x8b We should add to cpu/amd/msr.h MSR_PATCH_LEVEL
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 99: uint32_t Maybe cast to uint8_t * to do pointer math?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 113: "Skipping microcode patch!\n"); We're not limited on line width for printk format strings, so maybe consider placing this on the same line if it helps readability.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 121: "Skipping updates.\n"); Same as above.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 15: * Family 16h */ remove
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 18: { 0x730f01, 0x7301 }, remove