[PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Janusz Uzycki]

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

Solution:
The patch adds to mxs_i2c_pio_wait_xfer_end() function
NO_SLAVE_ACK_IRQ bit polling during wait loop (until timeout).
If the bit is set the function immediately returns ENXIO error
in order to break the loop and not reset I2C block (it is in idle state then).
The function is called by mxs_i2c_pio_setup_xfer() to wait for complete xfer
after sent SELECT, READ or WRITE command.
If SELECT command is sent and selected slave address is unused by any device
on the bus I2C block sets NO_SLAVE_ACK_IRQ flag and doesn't deassert CTRL0_RUN.
Therefore we need to break the timeout loop when the flag is set,
otherwise the loop continues until long timeout (1000ms).
The change does not affect READ command because slave does not ack
any byte then (only the master does ack / or not for the last read byte).
According to i.MX28 reference manual (quoted below) it is not clear
if the patch affects WRITE command. However when no acked bytes
on WRITE command followed after address byte (SELECT command)
STAT_GOT_A_NAK flag is set rather than NO_SLAVE_ACK_IRQ (no tested).
Therefore clock stretching shouldn't be affected too.
It has confirmation in FSL BSP 2.6.35 i2c implementation which
completes xfer after NO_SLAVE_ACK_IRQ interrupt and scheduled work.
Registers on NO_SLAVE_ACK_IRQ in PIO mode:
* STAT: 0xd0000e00
	MASTER_PRESENT
	SLAVE_PRESENT
	GOT_A_NAK !
	BUS_BUSY
	CLK_GEN_BUSY
	DATA_ENGINE_BUSY
* CTRL0: 0x20230000
	RUN !
	RETAIN_CLOCK
	MASTER_MODE
	DIRECTION
* CTRL1: 0x688600a0
	RD_QUEUE_IRQ
	WR_QUEUE_IRQ
	ACK_MODE
	SLAVE_ADDRESS_BYTE=0b10000110
	BUS_FREE_IRQ
	NO_SLAVE_ACK_IRQ !

NO_SLAVE_ACK_IRQ (CTRL1):
When a start condition is transmitted in master mode, the next byte
contains an address for a targeted slave. If the targeted slave does not
acknowledge the address byte, then this interrupt is set, no further I2C
protocol is processed, and the I2C bus returns to the idle state.
This bit is set to indicate that an interrupt is requested
by the I2C controller because the slave addressed
by a master transfer did not respond with an acknowledge.

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

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 87ee72d..35ae448 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -307,6 +307,10 @@ 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) {
+		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] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Janusz Użycki]

W dniu 2014-09-10 17:18, Janusz Uzycki pisze:
> Reported problem:
> i2cdetect scanned i2c bus very slow if address was not occupied by any device.
>
> Solution:
> The patch adds to mxs_i2c_pio_wait_xfer_end() function
> NO_SLAVE_ACK_IRQ bit polling during wait loop (until timeout).
> If the bit is set the function immediately returns ENXIO error
> in order to break the loop and not reset I2C block (it is in idle state then).
> The function is called by mxs_i2c_pio_setup_xfer() to wait for complete xfer
> after sent SELECT, READ or WRITE command.
> If SELECT command is sent and selected slave address is unused by any device
> on the bus I2C block sets NO_SLAVE_ACK_IRQ flag and doesn't deassert CTRL0_RUN.
> Therefore we need to break the timeout loop when the flag is set,
> otherwise the loop continues until long timeout (1000ms).
> The change does not affect READ command because slave does not ack
> any byte then (only the master does ack / or not for the last read byte).
> According to i.MX28 reference manual (quoted below) it is not clear
> if the patch affects WRITE command. However when no acked bytes
> on WRITE command followed after address byte (SELECT command)
> STAT_GOT_A_NAK flag is set rather than NO_SLAVE_ACK_IRQ (no tested).
> Therefore clock stretching shouldn't be affected too.
> It has confirmation in FSL BSP 2.6.35 i2c implementation which
> completes xfer after NO_SLAVE_ACK_IRQ interrupt and scheduled work.
> Registers on NO_SLAVE_ACK_IRQ in PIO mode:
>
Some words of comment about clock stretching. In the patch
I meant clock stretching by master (RETAIN_CLOCK) during WRITE command.
This prevents to use the bus by other master (multimaster bus).
However there is also possible "clock low period stretching" by slave device
(I wrote about in threads before) to inform the master that clock is too 
fast
or slave is not ready. Then slave sets SCL line to low state.
The case is rather not supported by any i2c driver in Linux? The i2c 
clock freq. is fixed.
Therefore when SCL is hold by long time master treats it as NACK, show 
streching by slave
is probably handled directly by master's hardware if implemented.
Clock strechting by slave seems impossible when RETAIN_CLOCK set by master.
Clock stretching by master is not affected by the patch because the 
master decides
when ACK/NACK is received from slave during WRITE command.

best regards
Janusz

Re: [PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Marek Vasut]

On Wednesday, September 10, 2014 at 05:18:06 PM, Janusz Uzycki wrote:
> Reported problem:
> i2cdetect scanned i2c bus very slow if address was not occupied by any
> device.
> 
> Solution:
> The patch adds to mxs_i2c_pio_wait_xfer_end() function
> NO_SLAVE_ACK_IRQ bit polling during wait loop (until timeout).
> If the bit is set the function immediately returns ENXIO error
> in order to break the loop and not reset I2C block (it is in idle state
> then). The function is called by mxs_i2c_pio_setup_xfer() to wait for
> complete xfer after sent SELECT, READ or WRITE command.
> If SELECT command is sent and selected slave address is unused by any
> device on the bus I2C block sets NO_SLAVE_ACK_IRQ flag and doesn't
> deassert CTRL0_RUN. Therefore we need to break the timeout loop when the
> flag is set,
> otherwise the loop continues until long timeout (1000ms).
> The change does not affect READ command because slave does not ack
> any byte then (only the master does ack / or not for the last read byte).
> According to i.MX28 reference manual (quoted below) it is not clear
> if the patch affects WRITE command. However when no acked bytes
> on WRITE command followed after address byte (SELECT command)
> STAT_GOT_A_NAK flag is set rather than NO_SLAVE_ACK_IRQ (no tested).
> Therefore clock stretching shouldn't be affected too.
> It has confirmation in FSL BSP 2.6.35 i2c implementation which
> completes xfer after NO_SLAVE_ACK_IRQ interrupt and scheduled work.
> Registers on NO_SLAVE_ACK_IRQ in PIO mode:
> * STAT: 0xd0000e00
> 	MASTER_PRESENT
> 	SLAVE_PRESENT
> 	GOT_A_NAK !
> 	BUS_BUSY
> 	CLK_GEN_BUSY
> 	DATA_ENGINE_BUSY
> * CTRL0: 0x20230000
> 	RUN !
> 	RETAIN_CLOCK
> 	MASTER_MODE
> 	DIRECTION
> * CTRL1: 0x688600a0
> 	RD_QUEUE_IRQ
> 	WR_QUEUE_IRQ
> 	ACK_MODE
> 	SLAVE_ADDRESS_BYTE=0b10000110
> 	BUS_FREE_IRQ
> 	NO_SLAVE_ACK_IRQ !
> 
> NO_SLAVE_ACK_IRQ (CTRL1):
> When a start condition is transmitted in master mode, the next byte
> contains an address for a targeted slave. If the targeted slave does not
> acknowledge the address byte, then this interrupt is set, no further I2C
> protocol is processed, and the I2C bus returns to the idle state.
> This bit is set to indicate that an interrupt is requested
> by the I2C controller because the slave addressed
> by a master transfer did not respond with an acknowledge.
> 
> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>

OK, uh, can the commit message not be shortened to like 5-10 lines ? I think you 
really need to find your balance when it comes to documenting changes, but don't 
worry, this will happen sooner rather than later ;-)

It would be sufficient to say that you had problem with slow i2cdetect and that 
was because the i2c controller driver ignored the NO_SLAVE_ACK bit. By 
leveraging NO_SLAVE_ACK bit, the speedup happens. And this change is correct and 
doesn't break anything because <a few lines here>.

Do you know what I mean ?

Re: [PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Janusz Użycki]

W dniu 2014-09-19 04:45, Marek Vasut pisze:
> On Wednesday, September 10, 2014 at 05:18:06 PM, Janusz Uzycki wrote:
>> Reported problem:
>> i2cdetect scanned i2c bus very slow if address was not occupied by any
>> device.
>>
>> Solution:
>> The patch adds to mxs_i2c_pio_wait_xfer_end() function
>> NO_SLAVE_ACK_IRQ bit polling during wait loop (until timeout).
>> If the bit is set the function immediately returns ENXIO error
>> in order to break the loop and not reset I2C block (it is in idle state
>> then). The function is called by mxs_i2c_pio_setup_xfer() to wait for
>> complete xfer after sent SELECT, READ or WRITE command.
>> If SELECT command is sent and selected slave address is unused by any
>> device on the bus I2C block sets NO_SLAVE_ACK_IRQ flag and doesn't
>> deassert CTRL0_RUN. Therefore we need to break the timeout loop when the
>> flag is set,
>> otherwise the loop continues until long timeout (1000ms).
>> The change does not affect READ command because slave does not ack
>> any byte then (only the master does ack / or not for the last read byte).
>> According to i.MX28 reference manual (quoted below) it is not clear
>> if the patch affects WRITE command. However when no acked bytes
>> on WRITE command followed after address byte (SELECT command)
>> STAT_GOT_A_NAK flag is set rather than NO_SLAVE_ACK_IRQ (no tested).
>> Therefore clock stretching shouldn't be affected too.
>> It has confirmation in FSL BSP 2.6.35 i2c implementation which
>> completes xfer after NO_SLAVE_ACK_IRQ interrupt and scheduled work.
>> Registers on NO_SLAVE_ACK_IRQ in PIO mode:
>> * STAT: 0xd0000e00
>> 	MASTER_PRESENT
>> 	SLAVE_PRESENT
>> 	GOT_A_NAK !
>> 	BUS_BUSY
>> 	CLK_GEN_BUSY
>> 	DATA_ENGINE_BUSY
>> * CTRL0: 0x20230000
>> 	RUN !
>> 	RETAIN_CLOCK
>> 	MASTER_MODE
>> 	DIRECTION
>> * CTRL1: 0x688600a0
>> 	RD_QUEUE_IRQ
>> 	WR_QUEUE_IRQ
>> 	ACK_MODE
>> 	SLAVE_ADDRESS_BYTE=0b10000110
>> 	BUS_FREE_IRQ
>> 	NO_SLAVE_ACK_IRQ !
>>
>> NO_SLAVE_ACK_IRQ (CTRL1):
>> When a start condition is transmitted in master mode, the next byte
>> contains an address for a targeted slave. If the targeted slave does not
>> acknowledge the address byte, then this interrupt is set, no further I2C
>> protocol is processed, and the I2C bus returns to the idle state.
>> This bit is set to indicate that an interrupt is requested
>> by the I2C controller because the slave addressed
>> by a master transfer did not respond with an acknowledge.
>>
>> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
> OK, uh, can the commit message not be shortened to like 5-10 lines ? I think you
> really need to find your balance when it comes to documenting changes, but don't
> worry, this will happen sooner rather than later ;-)
>
> It would be sufficient to say that you had problem with slow i2cdetect and that
> was because the i2c controller driver ignored the NO_SLAVE_ACK bit. By
> leveraging NO_SLAVE_ACK bit, the speedup happens. And this change is correct and
> doesn't break anything because <a few lines here>.
>
> Do you know what I mean ?
>

Yes, I know. It was explanation in details rather for comments than 
final patch.
Is it ok?:
i2cdetect scanned i2c bus slow because the i2c-mxs driver ignored the 
NO_SLAVE_ACK bit
during busy-waiting loop. Thanks to the patch, the speedup happens.
The change doesn't break anything else because:
- on SELECT: NO_SLAVE_ACK bit checking is just welcome
- on READ: master (the i2c controller, no slave device) generates 
ACK/NAK bit
- on WRITE: NO_SLAVE_ACK can be treated as NAK (the same effect)
   so even the i2c controller sets NO_SLAVE_ACK on NAK (not confirmed)
   the WRITE is not effected
- on clock stretching: SCL wire is involved, it has no influence
   on the ACK bit value on SDA wire

kind regards
Janusz

Re: [PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Marek Vasut]

On Monday, September 22, 2014 at 04:33:49 PM, Janusz Użycki wrote:
> W dniu 2014-09-19 04:45, Marek Vasut pisze:
> > On Wednesday, September 10, 2014 at 05:18:06 PM, Janusz Uzycki wrote:
> >> Reported problem:
> >> i2cdetect scanned i2c bus very slow if address was not occupied by any
> >> device.
> >> 
> >> Solution:
> >> The patch adds to mxs_i2c_pio_wait_xfer_end() function
> >> NO_SLAVE_ACK_IRQ bit polling during wait loop (until timeout).
> >> If the bit is set the function immediately returns ENXIO error
> >> in order to break the loop and not reset I2C block (it is in idle state
> >> then). The function is called by mxs_i2c_pio_setup_xfer() to wait for
> >> complete xfer after sent SELECT, READ or WRITE command.
> >> If SELECT command is sent and selected slave address is unused by any
> >> device on the bus I2C block sets NO_SLAVE_ACK_IRQ flag and doesn't
> >> deassert CTRL0_RUN. Therefore we need to break the timeout loop when the
> >> flag is set,
> >> otherwise the loop continues until long timeout (1000ms).
> >> The change does not affect READ command because slave does not ack
> >> any byte then (only the master does ack / or not for the last read
> >> byte). According to i.MX28 reference manual (quoted below) it is not
> >> clear if the patch affects WRITE command. However when no acked bytes
> >> on WRITE command followed after address byte (SELECT command)
> >> STAT_GOT_A_NAK flag is set rather than NO_SLAVE_ACK_IRQ (no tested).
> >> Therefore clock stretching shouldn't be affected too.
> >> It has confirmation in FSL BSP 2.6.35 i2c implementation which
> >> completes xfer after NO_SLAVE_ACK_IRQ interrupt and scheduled work.
> >> Registers on NO_SLAVE_ACK_IRQ in PIO mode:
> >> * STAT: 0xd0000e00
> >> 
> >> 	MASTER_PRESENT
> >> 	SLAVE_PRESENT
> >> 	GOT_A_NAK !
> >> 	BUS_BUSY
> >> 	CLK_GEN_BUSY
> >> 	DATA_ENGINE_BUSY
> >> 
> >> * CTRL0: 0x20230000
> >> 
> >> 	RUN !
> >> 	RETAIN_CLOCK
> >> 	MASTER_MODE
> >> 	DIRECTION
> >> 
> >> * CTRL1: 0x688600a0
> >> 
> >> 	RD_QUEUE_IRQ
> >> 	WR_QUEUE_IRQ
> >> 	ACK_MODE
> >> 	SLAVE_ADDRESS_BYTE=0b10000110
> >> 	BUS_FREE_IRQ
> >> 	NO_SLAVE_ACK_IRQ !
> >> 
> >> NO_SLAVE_ACK_IRQ (CTRL1):
> >> When a start condition is transmitted in master mode, the next byte
> >> contains an address for a targeted slave. If the targeted slave does not
> >> acknowledge the address byte, then this interrupt is set, no further I2C
> >> protocol is processed, and the I2C bus returns to the idle state.
> >> This bit is set to indicate that an interrupt is requested
> >> by the I2C controller because the slave addressed
> >> by a master transfer did not respond with an acknowledge.
> >> 
> >> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>
> > 
> > OK, uh, can the commit message not be shortened to like 5-10 lines ? I
> > think you really need to find your balance when it comes to documenting
> > changes, but don't worry, this will happen sooner rather than later ;-)
> > 
> > It would be sufficient to say that you had problem with slow i2cdetect
> > and that was because the i2c controller driver ignored the NO_SLAVE_ACK
> > bit. By leveraging NO_SLAVE_ACK bit, the speedup happens. And this
> > change is correct and doesn't break anything because <a few lines here>.
> > 
> > Do you know what I mean ?
> 
> Yes, I know. It was explanation in details rather for comments than
> final patch.
> Is it ok?:
> i2cdetect scanned i2c bus slow because the i2c-mxs driver ignored the
> NO_SLAVE_ACK bit
> during busy-waiting loop. Thanks to the patch, the speedup happens.
> The change doesn't break anything else because:
> - on SELECT: NO_SLAVE_ACK bit checking is just welcome
> - on READ: master (the i2c controller, no slave device) generates
> ACK/NAK bit
> - on WRITE: NO_SLAVE_ACK can be treated as NAK (the same effect)
>    so even the i2c controller sets NO_SLAVE_ACK on NAK (not confirmed)
>    the WRITE is not effected
> - on clock stretching: SCL wire is involved, it has no influence
>    on the ACK bit value on SDA wire

I think Wolfram already patched the commit message, no ?

Re: [PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Tue, Sep 23, 2014 at 12:48:26PM +0200, Janusz Uzycki wrote:
> i2cdetect scanned i2c bus slow because the i2c-mxs driver ignored
> the NO_SLAVE_ACK bit during busy-waiting loop.
> Thanks to the patch, the speedup happens.
> The change doesn't break anything else because:
> - on SELECT: NO_SLAVE_ACK bit checking is just welcome
> - on READ: master (the i2c controller, no slave device) generates
>    ACK/NAK bit
> - on WRITE: NO_SLAVE_ACK can be treated as NAK (the same effect)
>    so even the i2c controller sets NO_SLAVE_ACK on NAK (not confirmed)
>    the WRITE is not effected
> - on clock stretching: SCL wire is involved, it has no influence
>    on the ACK bit value on SDA wire
> 
> Signed-off-by: Janusz Uzycki <j.uzycki-9tnw74Q4ehaHKKo6LODCOg@public.gmane.org>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Janusz Użycki]

The patch is sent.

W dniu 2014-09-23 12:48, Janusz Uzycki pisze:
> i2cdetect scanned i2c bus slow because the i2c-mxs driver ignored
> the NO_SLAVE_ACK bit during busy-waiting loop.
> Thanks to the patch, the speedup happens.
> The change doesn't break anything else because:
> - on SELECT: NO_SLAVE_ACK bit checking is just welcome
> - on READ: master (the i2c controller, no slave device) generates
>     ACK/NAK bit
> - on WRITE: NO_SLAVE_ACK can be treated as NAK (the same effect)
>     so even the i2c controller sets NO_SLAVE_ACK on NAK (not confirmed)
>     the WRITE is not effected
typo: affected

best regards
Janusz Uzycki

> - on clock stretching: SCL wire is involved, it has no influence
>     on the ACK bit value on SDA wire
>
> 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..f3c4a43 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) {
> +		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();

[PATCH] i2c-mxs: detect No Slave Ack on SELECT in PIO mode

[Janusz Uzycki]

i2cdetect scanned i2c bus slow because the i2c-mxs driver ignored
the NO_SLAVE_ACK bit during busy-waiting loop.
Thanks to the patch, the speedup happens.
The change doesn't break anything else because:
- on SELECT: NO_SLAVE_ACK bit checking is just welcome
- on READ: master (the i2c controller, no slave device) generates
   ACK/NAK bit
- on WRITE: NO_SLAVE_ACK can be treated as NAK (the same effect)
   so even the i2c controller sets NO_SLAVE_ACK on NAK (not confirmed)
   the WRITE is not effected
- on clock stretching: SCL wire is involved, it has no influence
   on the ACK bit value on SDA wire

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..f3c4a43 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) {
+		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