| Message ID | 1383413009-4912-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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;
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(-)