Nico Huber 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 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
I suggest using CB:33225. The function smbus_i2c_block_write() will input parameters address, offset, bytes and *buf. I'll upload the mainboard functions where this support is used later today.
https://review.coreboot.org/c/coreboot/+/33433 contains the calling function of the smbus_i2c_block_write().
I prefer to focus and merge SoC Braswell patchset CB:33225. Then we might consider to abondon this patchset (at the moment?) Or have both patchset?
Ack, if it's working without this, we should abandon it.
Btw. this also shows that SMBHSTDAT1 is not involved but SMBHSTCMD is used instead to transfer the first byte.