[PATCH 1/2] i2c-mxs: debug error message: second SELECT -> READ

[PATCH 1/2] i2c-mxs: debug error message: second SELECT -> READ

[Janusz Użycki]

Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
---
  linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c 
b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
index 0cde4e6..87ee72d 100644
--- a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
+++ b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
@@ -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

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

[Janusz Użycki]

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 <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
---
  linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c 
b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
index 87ee72d..b498708 100644
--- a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
+++ b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
@@ -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

Re: [PATCH 1/2] i2c-mxs: debug error message: second SELECT -> READ

[Marek Vasut]

On Monday, September 08, 2014 at 08:04:22 PM, Janusz Użycki wrote:

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 ?

> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
> ---
>   linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 2 +-

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

>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> index 0cde4e6..87ee72d 100644
> --- a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> +++ b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> @@ -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

Best regards,
Marek Vasut

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

[Marek Vasut]

On Monday, September 08, 2014 at 08:06:12 PM, Janusz Użycki wrote:
> 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 ?

> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
> ---
>   linux-3.14.17/drivers/i2c/busses/i2c-mxs.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> index 87ee72d..b498708 100644
> --- a/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> +++ b/linux-3.14.17/drivers/i2c/busses/i2c-mxs.c
> @@ -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

Best regards,
Marek Vasut

Re: [PATCH 1/2] i2c-mxs: fixed error message in pio transfer

[Janusz Użycki]

W dniu 2014-09-08 20:14, Marek Vasut pisze:
> On Monday, September 08, 2014 at 08:04:22 PM, Janusz Użycki wrote:
>
> 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.

>> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
>> ---
>>    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.

best regards
Janusz

Subject: [PATCH] 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.

Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
---
  drivers/i2c/busses/i2c-mxs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 0cde4e6..87ee72d 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -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

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

[Janusz Użycki]

W dniu 2014-09-08 20:33, Marek Vasut pisze:
> On Monday, September 08, 2014 at 08:06:12 PM, Janusz Użycki wrote:
>> 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).

best regards
Janusz

Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
---
   drivers/i2c/busses/i2c-mxs.c | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 87ee72d..b498708 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -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

Re: [PATCH 1/2] i2c-mxs: fixed error message in pio transfer

[Marek Vasut]

On Tuesday, September 09, 2014 at 10:16:34 AM, Janusz Użycki wrote:
> W dniu 2014-09-08 20:14, Marek Vasut pisze:
> > On Monday, September 08, 2014 at 08:04:22 PM, Janusz Użycki wrote:
> > 
> > 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.
> 
> >> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
> >> ---
> >> 
> >>    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.

Can you please repost the patches using git send-email ?

Thanks!

Best regards,
Marek Vasut

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

[Marek Vasut]

On Tuesday, September 09, 2014 at 11:27:36 AM, Janusz Użycki wrote:
> W dniu 2014-09-08 20:33, Marek Vasut pisze:
> > On Monday, September 08, 2014 at 08:06:12 PM, Janusz Użycki wrote:
> >> 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?

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

> |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.

Best regards,
Marek Vasut

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

[Janusz Użycki]

W dniu 2014-09-09 14:48, Marek Vasut pisze:
>>> 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)

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.

>
>> |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.
Yes but error service could be more common probably.

best regards
Janusz

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

[Marek Vasut]

On Tuesday, September 09, 2014 at 03:10:25 PM, Janusz Użycki wrote:
> W dniu 2014-09-09 14:48, Marek Vasut pisze:
> >>> 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)
> 
> 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.

> 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

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

Do you mean clock stretching ?

> However I haven't seen any driver which supports the method.
> 
> >> |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.
> 
> Yes but error service could be more common probably.

Feel free to prepare a patch please!

Best regards,
Marek Vasut

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

[Janusz Użycki]

W dniu 2014-09-09 15:59, Marek Vasut pisze:
> On Tuesday, September 09, 2014 at 03:10:25 PM, Janusz Użycki wrote:
>> W dniu 2014-09-09 14:48, Marek Vasut pisze:
>>>>> 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)
>> 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.
>
>> 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
>
>> 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.
>> However I haven't seen any driver which supports the method.
>>
>>>> |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.
>> 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?

best regards
Janusz

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

[Marek Vasut]

On Tuesday, September 09, 2014 at 05:05:24 PM, Janusz Użycki wrote:

[...]

> >> 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.

OK

> >> However I haven't seen any driver which supports the method.
> >> 
> >>>> |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.
> >> 
> >> 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.

You should be really careful here. Please note that the I2C is not a PnP bus and 
probing the I2C might have adverse effect on various devices attached to the 
bus.

> 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?

Yes, esp. in short transfers.

Best regards,
Marek Vasut