Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35725 )
Change subject: device/software_i2c: Add function to recover bus ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c File src/device/software_i2c.c:
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 368: wait(bus); You only need these if there's a risk that our side would leave the bus in a wedged state, which shouldn't really exist for the software I2C code. If a hardware I2C controller wedges it, then the function that turns on software I2C should still reinitialize both pins to high. So I don't think you need this.
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: By this time SCL is high It's not guaranteed that SCL is high here if the device is clock-stretching. If you want to make this a little more robust, you could explicitly wait for SCL to be high. (Using start_cond() as mentioned above will automatically do that.)
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: as we need to give 9 falling-rising edges
i'm not sure what you want to say exactly with this. […]
Most simple I2C slave don't interpret a stop condition as a stop condition in the middle of a transfer. The main thing this does is toggle SCL which makes the slave finish sending out whatever bits it may still have buffered.
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 373: while (i++ < RECOVERY_CLK_CNT * 2) { I think an idiomatic way to write this on top of the existing primitives would be
for (int i = 0; i < RECOVERY_CLK_CNT; i++) { start_cond(bus); stop_cond(bus); }
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 388: wait(bus);
in the linux code there's a check if the sda pin is 1 when the scl variable is 1 and if that's the c […]
I think that would risk not unwedging in all cases. Imagine a device is trying to send back the bits 11010111, and you stop on bit 3 (the first zero). It will keep holding SDA down so the bus is wedged. Then you run this code, and the first start/stop pair will toggle SCL down and up again, so the device transitions SDA to 1. If you had an early abort check here, your code would think the bus is unwedged and return.
However, the device is actually still stuck in the middle of a transfer, it's just currently sending a 1 bit so you don't notice. When the host starts doing the next transfer and toggles SCL again, it will pull SDA back down to 0 (and mess up whatever other transfer the host was trying to make).
https://review.coreboot.org/c/coreboot/+/35725/1/src/include/device/i2c_simp... File src/include/device/i2c_simple.h:
https://review.coreboot.org/c/coreboot/+/35725/1/src/include/device/i2c_simp... PS1, Line 48: i2c_recover_bus This should be called software_i2c_recover() to keep with the existing naming convention.