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:
(2 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 371: By this time SCL is high
when the bus hardware is really stuck, the code would hang then
Right, I meant wait with a timeout (like the existing functions using wait_for_scl() already do). It's still possible that you send something that causes clock stretching, get an asynchronous reboot, and then get back to the point where you're trying to talk to the slave before it is done handling the previous transfer. Then you go into an unwedge path because you think the bus is wedged, and it would be nice if the recovery code could handle that. (Of course, if the slave just keeps SCL down forever there's nothing anyone can do about it.)
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 373: while (i++ < RECOVERY_CLK_CNT * 2) {
the thing that gets sent between the stop conditions isn't a start condition, since scl is low when […]
True... I don't think it would make much difference for wedged devices, though, they usually really just care about SCL toggling. Sending a real start condition would make this look less incorrect for other stuff observing the bus.
But if you want exactly that behavior, you can also write it as
wait_for_scl(bus, "before recovery"); for (int i = 0; i < RECOVERY_CLK_CNT; i++) { software_i2c[bus]->set_scl(bus, 0); wait(bus); stop_cond(bus); }