Arthur Heymans 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:
(6 comments)
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 102: 3200 Can there be a macro for this somewhere?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/microco... PS1, Line 106: uint32_t equivalent_processor_rev_id It might be good to explain this equivalent_processor_rev_id in the commit message?
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?
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?
https://review.coreboot.org/c/coreboot/+/41719/1/src/soc/amd/picasso/update_... PS1, Line 27: u32 new_id; initialize here?
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 a u16?