Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30822 )
Change subject: sb/intel/i82801gx: Detect if the southbridge supports AHCI ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/9/src/southbridge/intel/i82801gx/sata.... PS9, Line 54: struct southbridge_intel_i82801gx_config *config = dev->chip_info;
The whole part to check for AHCI support needs only be run […]
Done
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... File src/southbridge/intel/i82801gx/sata.c:
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 64: return; This still bails out too early. It should skip the `ahci_supported` part but still to the SATA_MAP programming below.
https://review.coreboot.org/#/c/30822/11/src/southbridge/intel/i82801gx/sata... PS11, Line 87: /* At this point, the new pci id will appear on the bus */ I think code flow could benefit here by further separating the support-check from the hardware configuration. e.g.
if (config->sata_mode == SATA_MODE_AHCI) { ahci_supported = lpc_dev && !(pci_read_config32(...) & AHCI_UNSUPPORTED); if (!ahci_supported) config->sata_mode = SATA_MODE_IDE_PLAIN; }
switch (config->sata_mode) { /* hw config */ }