Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc@... PS3, Line 42: ramstage-y += smbus.c I don't see how this could work in ramstage. The PCI device is visible to the allocator, which will likely reasign a different i/o address. Then the static addresses in `smbus.c` wouldn't be valid any longer.
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 15: nit: single empty line, please
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 24: #define PCH_DEV_SMBUS dev_find_slot(0, PCI_DEVFN(SMBUS_DEV, SMBUS_FUNC)) I don't think we actually need this. You don't seem to be using the SMBus in ramstage and I doubt it's working (see comment in `Makefile.inc`).
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@18 PS3, Line 18: #include <device/pci_def.h> is this needed?
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@22 PS3, Line 22: #include <soc/intel/common/block/smbus/smbuslib.h> it's really confusing
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@32 PS3, Line 32: /* Disable interrupts */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTCTL, 0), : /* Clear errors */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTSTAT, 0xff), The sb/common driver should handle these.