Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42094 )
Change subject: amd/00730F01: Clean the Microcode updating ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@7 PS4, Line 7: amd/00730F01: Clean the Microcode updating.
Please remove the trailing dot.
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@10 PS4, Line 10: https://review.coreboot.org/c/coreboot/+/41719
Please summarize the comments in the commit message.
Done. Summary has been added.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@13 PS4, Line 13: Also, 41719 should be based on this.
This should maybe be a review comment, but it doesn’t belong in the commit message. […]
Done. Reword the comments. Made it clear that the 41719 depends on this.
https://review.coreboot.org/c/coreboot/+/42094/4//COMMIT_MSG@14 PS4, Line 14:
Tested how?
Done. The platform is not available, but all the changes were carefully checked not to change the original flow.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... File src/cpu/amd/pi/00730F01/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 104: m = (struct microcode *)c; : : if (m->processor_rev_id == equivalent_processor_rev_id) : apply_microcode_patch(m);
does cpu_microcode_blob. […]
Currently only one blob is in the CBFS. I dont change the original code flow.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 116: if (equivalent_processor_rev_id == 0) { : printk(BIOS_DEBUG, "microcode: rev id not found. Skipping microcode patch!\n"); : return; : }
since no lookup table is used any more, this check can be removed
Done. Deleted.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 124: BIOS_DEBUG
BIOS_WARNING maybe? should be at least BIOS_INFO though
Done. Changed.
https://review.coreboot.org/c/coreboot/+/42094/4/src/cpu/amd/pi/00730F01/upd... PS4, Line 131: "updates.\n");
Should fit on one line.
Done. Changed.