Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
I will modify the arguments removing offset as input parameter.
You really should not touch any of the SMBUS IO-mapped registers before checking that the host is not busy (setup_command() in common/smbus.c). Same goes for the PCI configuration registers. Admittedly, we are not that likely to multi-task here but still... Datasheets specificly say to not touch IO register bank while transaction is in progress. FYI: Back in 2005 or so, bad SMBUS state machines bricked a lot of Thinkpads, although the error was on the EEPROM device back then.
I think it's better to fork do_smbus_block_write() to do_i2c_block_write(), and replace SMBHSTCMD with SMBHSTDAT1 there __if__ that is really needed. Use of SMBHSTDAT1 differs from any Intel document/datasheet I have looked at (most suck equally for SMBus host) so let's try to document this carefully.
https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/sm...
That is the best doc what I have found so far about block formats wih I2C_EN but dates to 2009. I am looking at Figure 14 page 11.