i2c-mxs: debug error message: second SELECT -> READ

Subject: [PATCH 2/2] i2c-mxs: fixed PIO NACK error instead of timeout
(1000ms)

i2cdetect scanned i2c bus very slow if address was not occupied by any device.

Signed-off-by Janusz Uzycki <[email protected]>

---
  linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 3 +++
  1 file changed, 3 insertions(+)

linux-3.14.17/drivers/i2c/busses/i2c-mxs.c +3 -0

index 87ee72d..b498708 100644
@@ -307,6 +307,9 @@ static int mxs_i2c_pio_wait_xfer_end(struct
 mxs_i2c_dev *i2c)
         unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
         while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) {
+               /*int ret = mxs_i2c_pio_check_error_state(i2c);*/
+               if (readl(i2c->regs + MXS_I2C_CTRL1) &
 MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ)
+                       return -ENXIO;
                 if (time_after(jiffies, timeout))

                         return -ETIMEDOUT;
                 cond_resched();

--
1.7.11.3

Quoted 5 lines

Subject: [PATCH 2/2] i2c-mxs: fixed PIO NACK error instead of timeout
(1000ms)

i2cdetect scanned i2c bus very slow if address was not occupied by any device.

Shouldn't this check be used only after the 'SELECT' command ?

W dniu 2014-09-08 20:33, Marek Vasut pisze:

Quoted 9 lines · Janusz Użycki

Quoted 6 lines

Subject: [PATCH 2/2] i2c-mxs: fixed PIO NACK error instead of timeout

   (1000ms)

i2cdetect scanned i2c bus very slow if address was not occupied by any device.

Shouldn't this check be used only after the 'SELECT' command ?

It looks |||mxs_i2c_isr()| for DMA transfer does not differentiate commands also
and does not mask irqs for each command.

|STAT_GOT_A_NAK| is a separate bit.|CTRL1_NO_SLAVE_ACK_IRQ can be set

both after SELECT and WRITE command. Should we differentiate?

|Checking CTRL1_NO_SLAVE_ACK_IRQ |bit for SELECT command will increase

code size only
without special profit. Current PIO implementation also gathers all errors together and reads them on the end by mxs_i2c_pio_check_error_state(). Probably mxs_i2c_pio_check_error_state() call or
enabling interrupt masks for PIO could be better than
direct |CTRL1_NO_SLAVE_ACK_IRQ |bit checking for clear code. It also could support multimaster for PIO (MASTER_LOSS).

Quoted 8 lines

Quoted 1 line

Shouldn't this check be used only after the 'SELECT' command ?

It looks |||mxs_i2c_isr()| for DMA transfer does not differentiate commands also
and does not mask irqs for each command.

|STAT_GOT_A_NAK| is a separate bit.|CTRL1_NO_SLAVE_ACK_IRQ can be set

both after SELECT and WRITE command. Should we differentiate?

What about writes that take long time, will checking this bit not break them ? (like programming a slow eeprom or such)

Quoted 10 lines

|Checking CTRL1_NO_SLAVE_ACK_IRQ |bit for SELECT command will increase

code size only
without special profit. Current PIO implementation also gathers all errors together and reads them on the end by mxs_i2c_pio_check_error_state(). Probably mxs_i2c_pio_check_error_state() call or
enabling interrupt masks for PIO could be better than
direct |CTRL1_NO_SLAVE_ACK_IRQ |bit checking for clear code. It also could support multimaster for PIO (MASTER_LOSS).

Actually, the PIO is explicitly IRQ-less and is used only for transferring very short amounts of data.

W dniu 2014-09-09 14:48, Marek Vasut pisze:

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shouldn't this check be used only after the 'SELECT' command ?

It looks |||mxs_i2c_isr()| for DMA transfer does not differentiate commands also
and does not mask irqs for each command.

|STAT_GOT_A_NAK| is a separate bit.|CTRL1_NO_SLAVE_ACK_IRQ can be set

Quoted 4 lines

Quoted 1 line

both after SELECT and WRITE command. Should we differentiate?

What about writes that take long time, will checking this bit not break them ? (like programming a slow eeprom or such)

No, master clock speed (SCL) decides here.
If slave does not confirm I2C address (SELECT) using ACK any timeout doesn't help.

RUN bit is not deasserted.
The same thing is for WRITE cmd. Each byte has to be acked otherwise STAT_GOT_A_NAK is set.

If it is too fast a slave has possibility to inform by setting SCL low.

However I haven't seen any driver which supports the method.

Quoted 11 lines

Quoted 10 lines

|Checking CTRL1_NO_SLAVE_ACK_IRQ |bit for SELECT command will increase

code size only
without special profit. Current PIO implementation also gathers all errors together and reads them on the end by mxs_i2c_pio_check_error_state(). Probably mxs_i2c_pio_check_error_state() call or
enabling interrupt masks for PIO could be better than direct |CTRL1_NO_SLAVE_ACK_IRQ |bit checking for clear code. It also could support multimaster for PIO (MASTER_LOSS).

Quoted 2 lines

Actually, the PIO is explicitly IRQ-less and is used only for transferring very short amounts of data.

Yes but error service could be more common probably.

Quoted 6 lines

W dniu 2014-09-09 14:48, Marek Vasut pisze:

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Shouldn't this check be used only after the 'SELECT' command ?

It looks |||mxs_i2c_isr()| for DMA transfer does not differentiate commands also
and does not mask irqs for each command.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

|STAT_GOT_A_NAK| is a separate bit.|CTRL1_NO_SLAVE_ACK_IRQ can be set

both after SELECT and WRITE command. Should we differentiate?

What about writes that take long time, will checking this bit not break them ? (like programming a slow eeprom or such)

No, master clock speed (SCL) decides here.
If slave does not confirm I2C address (SELECT) using ACK any timeout doesn't help.

The SELECT case is OK in that aspect. But I was talking about WRITEs.

Quoted 3 lines

RUN bit is not deasserted.
The same thing is for WRITE cmd. Each byte has to be acked otherwise STAT_GOT_A_NAK is set.

OK

Quoted 1 line

If it is too fast a slave has possibility to inform by setting SCL low.

Do you mean clock stretching ?

Quoted 12 lines

However I haven't seen any driver which supports the method.

Quoted 10 lines

Quoted 10 lines

|Checking CTRL1_NO_SLAVE_ACK_IRQ |bit for SELECT command will increase

code size only
without special profit. Current PIO implementation also gathers all errors together and reads them on the end by mxs_i2c_pio_check_error_state(). Probably mxs_i2c_pio_check_error_state() call or
enabling interrupt masks for PIO could be better than direct |CTRL1_NO_SLAVE_ACK_IRQ |bit checking for clear code. It also could support multimaster for PIO (MASTER_LOSS).

Quoted 3 lines

Quoted 2 lines

Actually, the PIO is explicitly IRQ-less and is used only for transferring very short amounts of data.

Yes but error service could be more common probably.

Feel free to prepare a patch please!

Quoted 3 lines

Quoted 1 line

If it is too fast a slave has possibility to inform by setting SCL low.

Do you mean clock stretching ?

Yes, I didn't notice it is the same.

Quoted 13 lines

Quoted 13 lines

However I haven't seen any driver which supports the method.

Quoted 11 lines

Quoted 10 lines

|Checking CTRL1_NO_SLAVE_ACK_IRQ |bit for SELECT command will increase

code size only
without special profit. Current PIO implementation also gathers all errors together and reads them on the end by mxs_i2c_pio_check_error_state(). Probably mxs_i2c_pio_check_error_state() call or
enabling interrupt masks for PIO could be better than direct |CTRL1_NO_SLAVE_ACK_IRQ |bit checking for clear code. It also could support multimaster for PIO (MASTER_LOSS).

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Actually, the PIO is explicitly IRQ-less and is used only for transferring very short amounts of data.

Yes but error service could be more common probably.

Feel free to prepare a patch please!

On the moment the fix is enough for me. I did need i2cdetect to work.

I looked on 2.6.35 FSL BSP. There both DMA and PIO mode use status interrupt and cmd_complete. It was changed to speed up?

Signed-off-by Janusz Uzycki <[email protected]>

---
  linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)

linux-3.14.17/drivers/i2c/busses/i2c-mxs.c +1 -2

index 0cde4e6..87ee72d 100644
@@ -429,7 +429,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter
 *adap,
                 ret = mxs_i2c_pio_wait_xfer_end(i2c);
                 if (ret) {
                         dev_err(i2c->dev,
-                               "PIO: Failed to send SELECT command!\n");
+                               "PIO: Failed to send READ command!\n");
                         goto cleanup;
                 }
 
--

1.7.11.3

Thanks, but the Subject of the email doesn't make much sense. Also, a short commit message exlaining what you did and why you did it would be nice.

While the patch is obvious and obviously correct, can you please fix those two minor things ?

Quoted 3 lines

Signed-off-by Janusz Uzycki <[email protected]>

---
   linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 2 +-

btw. how did this 'linux-3.14.17' get in the path ?

W dniu 2014-09-08 20:14, Marek Vasut pisze:

Quoted 8 lines · Janusz Użycki

Thanks, but the Subject of the email doesn't make much sense. Also, a short commit message exlaining what you did and why you did it would be nice.

While the patch is obvious and obviously correct, can you please fix those two minor things ?

i2c-mxs: fixed error message in pio transfer

If I2C_M_RD flag is set SELECT command is sent
and afterward READ command.
The patch fixes READ command to return READ failure error message instead of SELECT failure error message.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Janusz Uzycki <[email protected]>

---

   linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 2 +-

btw. how did this 'linux-3.14.17' get in the path ?

I have some scripts in my repo and I forgot about longer patch.