Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35725 )
Change subject: device/software_i2c: Add function to recover bus ......................................................................
device/software_i2c: Add function to recover bus
As the software bus might be in unknown state, add a function to reset it to known (idle) state.
Copied from linux kernel. Tested on Supermicro X11SSH-TF.
Change-Id: I264471e872cb353b28a6b71cc64a11aec59e63f2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/software_i2c.c M src/include/device/i2c_simple.h 2 files changed, 48 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35725/1
diff --git a/src/device/software_i2c.c b/src/device/software_i2c.c index 5dc9990..eecb670 100644 --- a/src/device/software_i2c.c +++ b/src/device/software_i2c.c @@ -343,3 +343,49 @@ "SDA %d, SCL %d\n", bus, bits, software_i2c[bus]->get_sda(bus), software_i2c[bus]->get_scl(bus)); } + +/* + * Try to recover the bus from unknown state. + * Useful if the power rail or reset line isn't under firmware control. + */ +#define RECOVERY_CLK_CNT 9 + +int i2c_recover_bus(unsigned bus) +{ + int i = 0, scl = 1; + + printk(BIOS_INFO, "software_i2c(%d): Trying i2c bus recovery\n", bus); + + /* + * If we can set SDA, we will always create a STOP to ensure additional + * pulses will do no harm. This is achieved by letting SDA follow SCL + * half a cycle later. Check the 'incomplete_write_byte' fault injector + * for details. + */ + software_i2c[bus]->set_scl(bus, 1); + wait(bus); + software_i2c[bus]->set_sda(bus, 1); + wait(bus); + + /* + * By this time SCL is high, as we need to give 9 falling-rising edges + */ + while (i++ < RECOVERY_CLK_CNT * 2) { + if (scl) { + /* SCL shouldn't be low here */ + if (!software_i2c[bus]->get_scl(bus)) { + printk(BIOS_ERR, "software_i2c(%d): SCL stuck low\n", bus); + return 1; + break; + } + } + + scl = !scl; + software_i2c[bus]->set_scl(bus, scl); + /* Creating STOP again, see above */ + wait(bus); + software_i2c[bus]->set_sda(bus, scl); + wait(bus); + } + return 0; +} diff --git a/src/include/device/i2c_simple.h b/src/include/device/i2c_simple.h index e3cc892..7f508b2 100644 --- a/src/include/device/i2c_simple.h +++ b/src/include/device/i2c_simple.h @@ -45,6 +45,8 @@ int i2c_write_field(unsigned int bus, uint8_t slave, uint8_t reg, uint8_t data, uint8_t mask, uint8_t shift);
+int i2c_recover_bus(unsigned int bus); + /* * software_i2c is supposed to be a debug feature. It's usually not compiled in, * but when it is it can be dynamically enabled at runtime for certain busses.
build bot (Jenkins) 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 353: int i2c_recover_bus(unsigned bus) Prefer 'unsigned int' to bare use of 'unsigned'
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 379: break; break is not useful after a goto or return
Felix Held 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:
(7 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 353: int i2c_recover_bus(unsigned bus)
Prefer 'unsigned int' to bare use of 'unsigned'
what jenkins says
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 360: If we can set SDA when using software i2c, we can always set sda. since the check from the linux code isn't here, drop this part of the comment
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 362: Check the 'incomplete_write_byte' fault injector : * for details. this probably doesn't apply to coreboot, right?
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. you send 9 additional stop conditions here, right?
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 379: break; remove break after return
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 385: /* Creating STOP again, see above */ this is only true for half of the executions of the while loop contents
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 case the rest of the 9 additional stop conditions isn't sent to save some time. would it make sense to do this here as well? also: wouldn't for example 3 additional i2c resets be sufficient here?
Paul Menzel 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:
(1 comment)
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 349: * Useful if the power rail or reset line isn't under firmware control. Add a comment here from what Linux file it was copieds
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.
Felix Held 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:
(3 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: as we need to give 9 falling-rising edges
Most simple I2C slave don't interpret a stop condition as a stop condition in the middle of a transf […]
ah, now that makes much more sense. should be added to the comment
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. […]
when the bus hardware is really stuck, the code would hang then
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 […]
the thing that gets sent between the stop conditions isn't a start condition, since scl is low when sda gets pulled low; for a start condition scl has to be low when sda gets pulled low. the two signals are basically 90 degrees out of phase and so you only get either a series of start conditions or a series of stop conditions (if i don't remember i2c completely wrong; iirc start/restart/stop conditions are encoded by toggling sda when scl is low)
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); }
Felix Held 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:
(1 comment)
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 373: while (i++ < RECOVERY_CLK_CNT * 2) {
True... […]
sending random start conditions might also make some devices unhappy; what the kernel does here seems to be the best option that is least likely to cause some weird problems. if i read the code you suggested correctly (only had a brief look at it and the functions it calls), it should do the trick
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35725
to look at the new patch set (#2).
Change subject: device/software_i2c: Add function to recover bus ......................................................................
device/software_i2c: Add function to recover bus
As the software bus might be in unknown state, add a function to reset it to known (idle) state.
Copied from linux kernel. Tested on Supermicro X11SSH-TF.
Change-Id: I264471e872cb353b28a6b71cc64a11aec59e63f2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/software_i2c.c M src/include/device/i2c_simple.h 2 files changed, 39 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35725/2
build bot (Jenkins) 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35725/2/src/device/software_i2c.c File src/device/software_i2c.c:
https://review.coreboot.org/c/coreboot/+/35725/2/src/device/software_i2c.c@3... PS2, Line 353: int software_i2c_recover_bus(unsigned bus) Prefer 'unsigned int' to bare use of 'unsigned'
Patrick Rudolph 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 2:
(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 360: If we can set SDA
when using software i2c, we can always set sda. […]
Done
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 362: Check the 'incomplete_write_byte' fault injector : * for details.
this probably doesn't apply to coreboot, right?
Done
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 sho […]
The software controller connected to this pulls both lines low (the Aspeed AST). I can add the same code in the caller, but having it here seems to best way to make sure that the bus is in a valid and known state when the functions returns.
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
ah, now that makes much more sense. […]
Done
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: By this time SCL is high
Right, I meant wait with a timeout (like the existing functions using wait_for_scl() already do). […]
what's here to do?
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 373: while (i++ < RECOVERY_CLK_CNT * 2) {
sending random start conditions might also make some devices unhappy; what the kernel does here seem […]
Done
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35725
to look at the new patch set (#3).
Change subject: device/software_i2c: Add function to recover bus ......................................................................
device/software_i2c: Add function to recover bus
As the software bus might be in unknown state, add a function to reset it to known (idle) state.
Copied from linux kernel. Tested on Supermicro X11SSH-TF.
Change-Id: I264471e872cb353b28a6b71cc64a11aec59e63f2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/software_i2c.c M src/include/device/i2c_simple.h 2 files changed, 39 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35725/3
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 3:
(3 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);
The software controller connected to this pulls both lines low (the Aspeed AST). […]
Well... you're kinda using the API wrong. The thing intializing software_i2c() (i.e. the thing setting software_i2c[n] to a non-NULL value) is supposed to initialize both lines to high. Then the rest of the software I2C code makes sure they're always both high at the end of every externally visible function. If you follow that convention, this should never be needed here.
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: By this time SCL is high
what's here to do?
Since you added a wait_for_scl() this should be handled now.
https://review.coreboot.org/c/coreboot/+/35725/3/src/include/device/i2c_simp... File src/include/device/i2c_simple.h:
https://review.coreboot.org/c/coreboot/+/35725/3/src/include/device/i2c_simp... PS3, Line 48: int software_i2c_recover_bus(unsigned int bus); nit: move up to other software_i2c functions?
Michael Niewöhner 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 3:
From X11SSM-F with i2c + vga patch applied:
PCI: 06:00.0 init ... ASpeed AST2050: initializing video device ast_detect_chip: AST 2400 detected ast_detect_chip: VGA not enabled on entry, requesting chip POST ast_detect_chip: Analog VGA only ast_driver_load: dram 1632000000 1 16 01000000 software_i2c(0): Trying i2c bus recovery software_i2c(0): ERROR: Clock stretching timeout before recovery! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! software_i2c(0): ERROR: Clock stretching timeout before stop condition! WARNING: EDID block does NOT fully conform to EDID 1.3. Missing name descriptor Display has 1920px x 1080px Using framebuffer 1680px x 1050px pitch 6720 ast_get_vbios_mode_info: FB BPP = 32 ast_get_vbios_mode_info: crtc_hdisplay = 1680 ASpeed high resolution framebuffer initialized PCI: 06:00.0 init finished in 1374298 usecs
Patrick Rudolph has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35725 )
Change subject: device/software_i2c: Add function to recover bus ......................................................................
Abandoned
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Julius Werner (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)