Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40351 )
Change subject: mb/dell/optiplex_9010: Add Dell OptiPlex 9010 SFF support ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40351/9/src/mainboard/dell/optiplex... File src/mainboard/dell/optiplex_9010/romstage.c:
https://review.coreboot.org/c/coreboot/+/40351/9/src/mainboard/dell/optiplex... PS9, Line 17: uint16_t ec_fw_version; : : sch5545_emi_init(0x2e); : if (sch5545_emi_get_int_mask_high()) : printk(BIOS_SPEW, "EC interrupt mask MSB is not 0\n"); : : sch5545_emi_disable_interrupts(); : sch5545_ec_early_init(); : sch5545_ec_hwm_early_init(); : : if (!s3resume) { : ec_fw_version = sch5545_get_ec_fw_version(); : printk(BIOS_DEBUG, "SCH5545 EC firmware version %04x\n", ec_fw_version); : sch5545_update_ec_firmware(ec_fw_version); : } : printk(BIOS_DEBUG, "EC early init complete.\n");
IMO bootblock should do only basic initialization of CAR and Super I/O. Putting more to the bootblock makes no sense. Also the initialization and firmware update must be done quite early, because EC awaits for communication from the host. If it doesn't receive the required initialization sequence after few seconds from system reset, it enters kind of error path. The error path means the fans will be spinned up to high speed and EC does not respond to any messages sent by host anymore. So it is not good to do it later (i.e. ramstage) if EC already entered error path.
Can you add this explanation as a comment here?
https://review.coreboot.org/c/coreboot/+/40351/9/src/mainboard/dell/optiplex... PS9, Line 39: : if (CONFIG(CONSOLE_SERIAL)) : sch5545_enable_uart(0x2e, 0);
Yes, I am replicating it here because of an issue described above. […]
Ack