[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop
The endless waiting for a bit to be set can cause a hang, add a timeout so we prevent such situation. A testcase for such a hang is below. The testcase assumes a device to be present at address 0x50 and a device to NOT be present at address 0x42 . Also note that the "sleep 1" induced delays are imperative for this bug to manifest .
i2c read 0x42 0x0.2 0x10 0x42000000 ; sleep 1 ; \ i2c read 0x50 0x0.2 0x10 0x42000000 ; sleep 1 ; \ i2c read 0x42 0x0.2 0x10 0x42000000
The expected result of the above command is:
Error reading the chip. Error reading the chip.
While without this patch, we observe a hang in the last read from 0x42 precisely when waiting for this bit to be set.
Signed-off-by: Marek Vasut marex@denx.de Cc: Fabio Estevam fabio.estevam@freescale.com Cc: Heiko Schocher hs@denx.de Cc: Stefano Babic sbabic@denx.de --- drivers/i2c/mxs_i2c.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c index 46106b7..49300d4 100644 --- a/drivers/i2c/mxs_i2c.c +++ b/drivers/i2c/mxs_i2c.c @@ -150,6 +150,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; uint32_t tmp = 0; + int timeout = MXS_I2C_MAX_TIMEOUT; int ret; int i;
@@ -169,9 +170,17 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
for (i = 0; i < len; i++) { if (!(i & 3)) { - while (readl(&i2c_regs->hw_i2c_queuestat) & - I2C_QUEUESTAT_RD_QUEUE_EMPTY) - ; + while (--timeout) { + tmp = readl(&i2c_regs->hw_i2c_queuestat); + if (!(tmp & I2C_QUEUESTAT_RD_QUEUE_EMPTY)) + break; + } + + if (!timeout) { + debug("MXS I2C: Failed receiving data!\n"); + return -EINVAL; + } + tmp = readl(&i2c_regs->hw_i2c_queuedata); } buffer[i] = tmp & 0xff;
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!\n");return -EINVAL;}
This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Best regards, Stefano Babic
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!\n");return -EINVAL;}This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Best regards, Marek Vasut
Hi Marek,
On 04/11/2013 12:50, Marek Vasut wrote:
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!\n");return -EINVAL;}This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We should print the error code in the i2c stack (do_i2c_read) instead of checking only if the return value is not null, as we do now.
Best regards, Stefano Babic
Hello Stefano, Marek,
Am 04.11.2013 13:03, schrieb Stefano Babic:
Hi Marek,
On 04/11/2013 12:50, Marek Vasut wrote:
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!\n");return -EINVAL;}This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
Yes, that should be -ETIMEDOUT
should print the error code in the i2c stack (do_i2c_read) instead of checking only if the return value is not null, as we do now.
Yep, printing in do_i2c_read() the error code would be nice. Patches are welcome :-)
bye, Heiko
Hi Heiko,
Hello Stefano, Marek,
Am 04.11.2013 13:03, schrieb Stefano Babic:
Hi Marek,
On 04/11/2013 12:50, Marek Vasut wrote:
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!
\n");
return -EINVAL;}This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
Yes, that should be -ETIMEDOUT
Full ACK.
should print the error code in the i2c stack (do_i2c_read) instead of checking only if the return value is not null, as we do now.
Yep, printing in do_i2c_read() the error code would be nice. Patches are welcome :-)
OK, shall I also print out an error message then?
Best regards, Marek Vasut
Hello Marek,
Am 04.11.2013 14:13, schrieb Marek Vasut:
Hi Heiko,
Hello Stefano, Marek,
Am 04.11.2013 13:03, schrieb Stefano Babic:
Hi Marek,
On 04/11/2013 12:50, Marek Vasut wrote:
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote:
if (!timeout) {debug("MXS I2C: Failed receiving data!\n");
return -EINVAL;}This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
Yes, that should be -ETIMEDOUT
Full ACK.
should print the error code in the i2c stack (do_i2c_read) instead of checking only if the return value is not null, as we do now.
Yep, printing in do_i2c_read() the error code would be nice. Patches are welcome :-)
OK, shall I also print out an error message then?
You mean in the mxs i2c driver? I think, this is not needed, as Stefano suggested.
bye, Heiko
Dear Heiko Schocher,
Hello Marek,
Am 04.11.2013 14:13, schrieb Marek Vasut:
Hi Heiko,
Hello Stefano, Marek,
Am 04.11.2013 13:03, schrieb Stefano Babic:
Hi Marek,
On 04/11/2013 12:50, Marek Vasut wrote:
Hi Stefano,
Hi Marek,
On 02/11/2013 18:23, Marek Vasut wrote: > + if (!timeout) { > + debug("MXS I2C: Failed receiving data!
\n");
> + return -EINVAL; > + } > +
This is a real error and not a debug information. IMHO it should be better to print the error unconditionally with puts/printf, reporting that the timer elapsed.
Returning -EINVAL will make the i2c stack trigger an output, so having it duplicated here is pointless I believe.
Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
Yes, that should be -ETIMEDOUT
Full ACK.
should print the error code in the i2c stack (do_i2c_read) instead of checking only if the return value is not null, as we do now.
Yep, printing in do_i2c_read() the error code would be nice. Patches are welcome :-)
OK, shall I also print out an error message then?
You mean in the mxs i2c driver? I think, this is not needed, as Stefano suggested.
OK, thanks! V2 out.
Best regards, Marek Vasut
participants (3)
-
Heiko Schocher -
Marek Vasut -
Stefano Babic