Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 9:
(4 comments)
Will implement the comment in a new patchset.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h File src/southbridge/intel/common/smbus.h:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 44: /* Only since ICH5 */
Did this I2C_EN bit in PCI config space appear with ICH5 as well?
Yes, Intel ICH5 datasheet contains documentation about this bit in HOSTC
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 49: int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd,
I don't see why we need SMBUS PCI dev. […]
Southbridge code is responsible of configuration of the SMBus controller. To enable the I2C mode the southbridge need to know on which PCI device the I2C_EN must be set.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 443: if (!has_i2c_read_command())
Seems unrelated. Please check when I2C_EN was implemented in hardware and add a similar test.
Since ICH5. Will use same function for i2c_block_write()
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
It's weird that you would need to write 'cmd' in two different host controller registers and without […]
I agree that the Intel datasheet is not clear about this, but we did SMBus analysis on the system also. The SMBHSTCMD register is used by the SMBus controller itself. The first data that is sent on the SMBus is the value in the SMBHSTDAT1 register. This byte is required by the I2C slave to compose a 16-byte address