Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
Patch Set 4: Code-Review+2
(5 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
Since I will use it for SPD retrieval only, I may remove it from ramstage
Ack
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
Done
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 […]
Done
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>
Yes, for the PCI_BASE_ADDRESS_4, PCI_COMMAND and PCI_COMMAND_IO values
Oh, I'm stupid :-/
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),
Yes, it clears the errors and interrupts on command setup. I will test without these.
Ack