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 commands ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 482: outb(cmd, smbus_base + SMBHSTDAT1);
This does not look right, putting cmd in to two registers.
When the controller is placed into I2C mode the output stream differs from SMBus. The I2C slave expects the next sequence: START [i2c_address] ACK [reg_address_15_8] ACK [reg_address_7_0] ACK [data7_0] ACK [data15_8] ACK [data23_16] ACK [data31_24] ACK STOP. The SMBHSTCMD byte is not send to the I2C device, but used to start SMBus with a command value. SMBHSTDAT1 is first byte send to I2C containing the reg_address_15_8
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 485: outb(bytes, smbus_base + SMBHSTDAT0);
Why? Isn't the block length determined by how long we run the loop and when last_byte bit is set.
The block length is not send to the slave address, but used for the SMBus controller only. The I2C
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 498: /* Start the command */
For i2c block command there should be a single start and single stop visible on the i2c lines. […]
For the used I2C slave device it's working. I'll check if one command is working also
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 503: if (smbus_wait_until_active(smbus_base) < 0)
This was buggy; see https://review.coreboot. […]
The new patch was created after this patch. Need to sync.