Kyösti Mälkki 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:
(5 comments)
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?
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.fn passed here?
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 426: smb_ctlr_reg = pci_read_config32(smbus_dev, 0x40); Invent a name for register 0x40.
Now it would be cleaner to explicitly clear or set this bit inside setup_command(), but I quess we can leave that as followup work, as this particular commit is blocking some platform/board port, apparently.
Also I would have liked to see the symmetric i2c_block_read() implemented and tested here already.
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.
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 comments this looks like an error to me.
Intel datasheets for block commands are a mess, I imagine you really only need to write one of them.