Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18925 )
Change subject: chipset_enable: Add support for Intel Skylake / Kabylake ......................................................................
Patch Set 5: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/18925/2/chipset_enable.c File chipset_enable.c:
Line 852:
Here, I would do :
It just confused me at first, rather than be recognizable, I guess I'd still keep the dev->domain, and dev->bus, but you're right about the dev->dev not being used and using 0x1f instead. I'd at least put a comment to say "SPI interface is on D31:F5 according to spec". But maybe it's not clear to me just because I'm not used to this ?
Line 858:
Firmware Hub (FWH) is basically a parallel flash chip on the LPC bus.
Humm.. could it be by any chance the EC chip ? The librems have a separate flash chip for the EC, but it's SPI too (afaik), and I've seen that chromebooks supports flashing the EC by using the 'internal:bus=lpc' option (which doesn't work with upstream flashrom). I don't know if it would apply to the librems (since librems use a second SPI flash), but it would be interesting for me to check that later. Right now I'm still wondering how I can read/write the EC firmware if I wanted to. Good idea on the mention of SPI-only in the commit message.
-
Youness Alaoui (Code Review)