Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35110 )
Change subject: google/kukui: Load calibration params and run DRAM calibration flow ......................................................................
Patch Set 39:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35110/39/src/mainboard/google/kukui... File src/mainboard/google/kukui/romstage.c:
https://review.coreboot.org/c/coreboot/+/35110/39/src/mainboard/google/kukui... PS39, Line 64: -1; should we enum these values?
https://review.coreboot.org/c/coreboot/+/35110/39/src/mainboard/google/kukui... PS39, Line 78: | I think you mean &
https://review.coreboot.org/c/coreboot/+/35110/39/src/mainboard/google/kukui... PS39, Line 79: return printk(BIOS_ERR, "Full calibration blob executed without saving parameters. " "Please ensure the blob is built properly.\n");
https://review.coreboot.org/c/coreboot/+/35110/39/src/mainboard/google/kukui... PS39, Line 100: if (!read_calibration_data_from_flash(&dparam, sizeof(dparam)) || : !is_valid_dramc_param(&dparam) || : dparam.header.config != config) { We probably want to give more reason telling user why we choose re-calibrating data.
bool had_valid_data = false;
if (!read...) { printk(BIOS_ERR, "Failed to read calibration data.\n"); } else if (!is_valid_dramc_param...) { // Probably dump version, magic, ... etc to make it easier for debugging. printk(BIOS_ERR, "Invalid DRAM calibration data, need to do full calibration.\n"); } else if (dparam.header.config != config) { printk(BIOS_ERR, "Incompatible config for DRAM calibration data (expected: XX, saved: XX).. } else { has_valid_data = true; }
if (has_valid_data) {...