Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37303 )
Change subject: soc/intel/broadwell_de: Re-read SPD on CRC error ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/memory.c:
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... PS2, Line 58: = SPD_STATUS_OK
This is not strictly needed here because res will be assigned a value in line 65 and noone needs it […]
Ack
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... PS2, Line 68: "SPD CRC error, channel %u slot %u\n",
Please add the iteration.
Ack
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... PS2, Line 72: } while (tries-- && res == SPD_STATUS_CRC_ERROR);
How much time does each try add to the boot time? Could you time it using the stopwatch frame work […]
the re-read seem to take around 60ms. But since it is a relatively rare occurrence we should be ok
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... PS2, Line 74: if (res == SPD_STATUS_OK) {
braces {} are not necessary for single statement blocks
Ack
https://review.coreboot.org/c/coreboot/+/37303/2/src/soc/intel/fsp_broadwell... PS2, Line 76: }
Please print an error with error string, stating that the SMBIOS data is not added, but it should st […]
Ack