<p>Julius Werner has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/25963">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">software_i2c: Improve error behavior<br><br>This patch fixes a bug in the software_i2c (bitbang) framework where it<br>would previously not return an error when receiving a NACK on a write<br>transaction (deviating behavior from our hardware I2C drivers). It also<br>adds explicit error codes to be returned for the different kinds of<br>failure conditions so they are more useful for debugging when dumped.<br><br>Change-Id: Ie63bf35123d89dcd99a1f9c079d4cae6a33b0b09<br>Signed-off-by: Julius Werner <jwerner@chromium.org><br>---<br>M src/device/software_i2c.c<br>1 file changed, 29 insertions(+), 25 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/25963/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/device/software_i2c.c b/src/device/software_i2c.c</span><br><span>index ec6722b..402f1d4 100644</span><br><span>--- a/src/device/software_i2c.c</span><br><span>+++ b/src/device/software_i2c.c</span><br><span>@@ -29,6 +29,11 @@</span><br><span> </span><br><span> #define spew(...) do { if (SPEW) printk(BIOS_SPEW, ##__VA_ARGS__); } while (0)</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#define ERR_NACK -2</span><br><span style="color: hsl(120, 100%, 40%);">+#define ERR_TIMEOUT -3</span><br><span style="color: hsl(120, 100%, 40%);">+#define ERR_ARB -4</span><br><span style="color: hsl(120, 100%, 40%);">+#define ERR_WEDGE -5</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> struct software_i2c_ops *software_i2c[SOFTWARE_I2C_MAX_BUS];</span><br><span> </span><br><span> /*</span><br><span>@@ -71,7 +76,7 @@</span><br><span> if (!__wait(bus, TIMEOUT_US, 1)) {</span><br><span> printk(BIOS_ERR, "software_i2c(%d): ERROR: Clock stretching "</span><br><span> "timeout %s!\n", bus, error_context);</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_TIMEOUT;</span><br><span> }</span><br><span> </span><br><span> return 0;</span><br><span>@@ -88,13 +93,13 @@</span><br><span> /* Might need to wait for clock stretching if repeated start. */</span><br><span> software_i2c[bus]->set_scl(bus, 1);</span><br><span> if (wait_for_scl(bus, "before start condition"))</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_TIMEOUT;</span><br><span> wait(bus); /* Repeated start setup time, minimum 4.7us */</span><br><span> </span><br><span> if (!software_i2c[bus]->get_sda(bus)) {</span><br><span> printk(BIOS_ERR, "software_i2c(%d): Arbitration lost trying "</span><br><span> "to send start condition!\n", bus);</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_ARB;</span><br><span> }</span><br><span> </span><br><span> /* SCL is high, transition SDA low as first part of start condition. */</span><br><span>@@ -121,7 +126,7 @@</span><br><span> assert(!software_i2c[bus]->get_scl(bus));</span><br><span> software_i2c[bus]->set_scl(bus, 1);</span><br><span> if (wait_for_scl(bus, "before stop condition"))</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_TIMEOUT;</span><br><span> wait(bus); /* Stop bit setup time, minimum 4us */</span><br><span> </span><br><span> /* SCL is high, transition SDA high to signal stop condition. */</span><br><span>@@ -148,20 +153,20 @@</span><br><span> if (bit && !software_i2c[bus]->get_sda(bus)) {</span><br><span> printk(BIOS_ERR, "software_i2c(%d): ERROR: SDA wedged low "</span><br><span> "by slave before clock pulse on transmit!\n", bus);</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_WEDGE;</span><br><span> }</span><br><span> </span><br><span> /* Clock stretching */</span><br><span> assert(!software_i2c[bus]->get_scl(bus));</span><br><span> software_i2c[bus]->set_scl(bus, 1);</span><br><span> if (wait_for_scl(bus, "on transmit"))</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_TIMEOUT;</span><br><span> wait(bus);</span><br><span> </span><br><span> if (bit && !software_i2c[bus]->get_sda(bus)) {</span><br><span> printk(BIOS_ERR, "software_i2c(%d): ERROR: SDA wedged low "</span><br><span> "by slave after clock pulse on transmit!\n", bus);</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_WEDGE;</span><br><span> }</span><br><span> </span><br><span> assert(software_i2c[bus]->get_scl(bus));</span><br><span>@@ -185,7 +190,7 @@</span><br><span> assert(!software_i2c[bus]->get_scl(bus));</span><br><span> software_i2c[bus]->set_scl(bus, 1);</span><br><span> if (wait_for_scl(bus, "on receive"))</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ERR_TIMEOUT;</span><br><span> </span><br><span> /* SCL is high, now data is valid */</span><br><span> bit = software_i2c[bus]->get_sda(bus);</span><br><span>@@ -202,11 +207,11 @@</span><br><span> static int out_byte(unsigned bus, u8 byte)</span><br><span> {</span><br><span> unsigned bit;</span><br><span style="color: hsl(0, 100%, 40%);">- int nack;</span><br><span style="color: hsl(120, 100%, 40%);">+ int nack, ret;</span><br><span> </span><br><span> for (bit = 0; bit < 8; bit++)</span><br><span style="color: hsl(0, 100%, 40%);">- if (out_bit(bus, (byte >> (7 - bit)) & 0x1) < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((ret = out_bit(bus, (byte >> (7 - bit)) & 0x1)) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> </span><br><span> nack = in_bit(bus);</span><br><span> </span><br><span>@@ -214,22 +219,22 @@</span><br><span> printk(BIOS_DEBUG, "software_i2c(%d): wrote byte 0x%02x, "</span><br><span> "received %s\n", bus, byte, nack ? "NAK" : "ACK");</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- return nack;</span><br><span style="color: hsl(120, 100%, 40%);">+ return nack > 0 ? ERR_NACK : nack;</span><br><span> }</span><br><span> </span><br><span> static int in_byte(unsigned bus, int ack)</span><br><span> {</span><br><span> u8 byte = 0;</span><br><span style="color: hsl(0, 100%, 40%);">- int i;</span><br><span style="color: hsl(120, 100%, 40%);">+ int i, ret;</span><br><span> for (i = 0; i < 8; ++i) {</span><br><span> int bit = in_bit(bus);</span><br><span> if (bit < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return bit;</span><br><span> byte = (byte << 1) | bit;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (out_bit(bus, !ack) < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((ret = out_bit(bus, !ack)) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> </span><br><span> if (DEBUG)</span><br><span> printk(BIOS_DEBUG, "software_i2c(%d): read byte 0x%02x, "</span><br><span>@@ -240,17 +245,16 @@</span><br><span> </span><br><span> int software_i2c_transfer(unsigned bus, struct i2c_msg *segments, int count)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- int i;</span><br><span style="color: hsl(120, 100%, 40%);">+ int i, ret;</span><br><span> struct i2c_msg *seg;</span><br><span> </span><br><span> for (seg = segments; seg - segments < count; seg++) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (start_cond(bus) < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((ret = start_cond(bus)) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> const u8 addr_dir = seg->slave << 1 | !!(seg->flags & I2C_M_RD);</span><br><span style="color: hsl(0, 100%, 40%);">- if (out_byte(bus, addr_dir) < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((ret = out_byte(bus, addr_dir)) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> for (i = 0; i < seg->len; i++) {</span><br><span style="color: hsl(0, 100%, 40%);">- int ret;</span><br><span> if (seg->flags & I2C_M_RD) {</span><br><span> ret = in_byte(bus, i < seg->len - 1);</span><br><span> seg->buf[i] = (u8)ret;</span><br><span>@@ -258,11 +262,11 @@</span><br><span> ret = out_byte(bus, seg->buf[i]);</span><br><span> }</span><br><span> if (ret < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> }</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- if (stop_cond(bus) < 0)</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((ret = stop_cond(bus)) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return ret;</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/25963">change 25963</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/25963"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ie63bf35123d89dcd99a1f9c079d4cae6a33b0b09 </div>
<div style="display:none"> Gerrit-Change-Number: 25963 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Julius Werner <jwerner@chromium.org> </div>