Nico Huber has posted comments on this change. ( https://review.coreboot.org/18925 )
Change subject: chipset_enable: Add support for Intel Skylake ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/18925/2/chipset_enable.c File chipset_enable.c:
Line 852:
Here, I would do : Here, I would do : pci_get_dev(pci_acc, dev->domain, dev->bus, dev->dev, 5);
Hmmm, 1f.5 (or rather D31F5) is what one would read in a datasheet and I don't want to make it less recognizable.
Might even make that '5' into a #define to avoid having the magic number here. #define PCH_100_SPI_INTERFACE_PCI_FUNCTION
I would agree if that number would be used somewhere else to. But as long as that's not the case, I'd choose readability.
Line 858:
Any reason this doesn't call enable_flash_ich_fwh ? I'm having a hard time
Firmware Hub (FWH) is basically a parallel flash chip on the LPC bus. While booting from LPC is still supported, IIRC, the system needs a SPI chip anyway (Firmware Descriptor, ME etc) and I haven't seen any Intel system with an LPC flash in years. So it doesn't make sense to put any effort there (couldn't test it anyway), but I should probably mention SPI-only in the commit message.
-
Nico Huber (Code Review)