Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41719 )
Change subject: amd/picasso: Load x86 microcode from CBFS modules ......................................................................
Patch Set 7:
(26 comments)
Resolve comments in a batch.
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@7 PS1, Line 7: Picasso
amd/picasso
Done. Changed.
https://review.coreboot.org/c/coreboot/+/41719/1//COMMIT_MSG@11 PS1, Line 11: seleceted
selected
Done. Typo has been fixed.
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41719/2//COMMIT_MSG@6 PS2, Line 6: : AMD/Picasso
Please make it lowercase.
Done. Changed in latest patchset.
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.
Done. This config has been removed. Instead cpu_microcode_bins is defined.
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
Done. removed.
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
Done
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. […]
Done. Has rebased on latest change with new revision ID.
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/include... PS5, Line 20: #define RAVEN1_B0_CPUID 0x00810f10
Done
The duplicated definitions have been removed.
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 2: /* This file is part of the coreboot project. */
Please remove the second line.
Done
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. […]
Done
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 […]
Done. Now it has been changed to #define F17H_MPB_MAX_SIZE 3200 #define F17H_MPB_DATA_OFFSET 32
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 20: FAM16h
Same idea. I would remove this.
Done
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 […]
Done.
changed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 77: 0xc0010020
MSR_PATCH_LOADER in cpu/amd/msr. […]
Done
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.
Done. Fixed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 83: 0x8b
We should add to cpu/amd/msr. […]
Done. Changed. The definition is already there.
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?
Done. Changed.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 102: 3200
Can there be a macro for this somewhere?
Done. Changed to macro.
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 sam […]
Done. connected into one line.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 121: "Skipping updates.\n");
Same as above.
Done.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 125: #if 0 : if (ucode_len > F16H_MPB_MAX_SIZE || : ucode_len < F16H_MPB_DATA_OFFSET) { : printk(BIOS_DEBUG, "microcode file invalid. Skipping " : "updates.\n"); : return; : } : #endif
remove?
Done. Removed.
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... File src/soc/amd/picasso/microcode_picasso.c:
https://review.coreboot.org/c/coreboot/+/41719/2/src/soc/amd/picasso/microco... PS2, Line 2: /* This file is part of the coreboot project. */
This line should be removed.
Done. This line has been removed. the one in update_microcode.c has also be removed.
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 23: /* Array terminator */ : { 0xffffff, 0x0000 },
any reason to not use ARRAY_SIZE in the for loop?
No reason. Does it has to?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 27: u32 new_id;
initialize here?
Done. changed in patchset 8.
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 43: : u32 equivalent_processor_rev_id = : get_equivalent_processor_rev_id(cpu_deviceid); : amd_update_microcode_from_cbfs(equivalent_processor_rev_id);
amd_update_microcode_from_cbfs takes a u32 as argument, but get_equivalent_processor_rev_id returns […]
Done. changed to u16. And a separated patch has been uploaded and merged.
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... File src/soc/amd/picasso/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/41719/5/src/soc/amd/picasso/update_... PS5, Line 27: int
unsigned
Change to u8 in Patchset 8.