[PATCH 0/2] Designware I2C slave confusing IC_INTR_STOP_DET handle

[PATCH 0/2] Designware I2C slave confusing IC_INTR_STOP_DET handle

[Michael Wu]

When an I2C slave works, sometimes both IC_INTR_RX_FULL and
IC_INTR_STOP_DET may be rising during an IRQ handle, especially when
system is busy or too late to handle interrupts.

If IC_INTR_RX_FULL is rising and the system doesn't handle immediately,
IC_INTR_STOP_DET may be rising and the system has to handle these two
events. For this there may be two problems:

1. IC_INTR_STOP_DET is rising after i2c_dw_read_clear_intrbits_slave()
   done: It seems invalidated because I2C_SLAVE_WRITE_REQUESTED is
   reported after the 1st I2C_SLAVE_WRITE_RECEIVED.

$ i2cset -f -y 2 0x42 0x00 0x41; dmesg -c
[0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
[1][irq_handler   ]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
I2C_SLAVE_WRITE_RECEIVED
[0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
[1][irq_handler   ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
I2C_SLAVE_WRITE_REQUESTED
I2C_SLAVE_WRITE_RECEIVED
[0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x710 : INTR_STAT=0x200
[1][irq_handler   ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x510 : INTR_STAT=0x0
I2C_SLAVE_STOP
[2][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x510 : INTR_STAT=0x0

  t1: ISR with the 1st IC_INTR_RX_FULL.
  t2: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave().
  t3: Enter i2c_dw_irq_handler_slave() and then report
      I2C_SLAVE_WRITE_RECEIVED due to 'if (stat & DW_IC_INTR_RX_FULL)'
      matched.
  t4: ISR with the 2nd IC_INTR_RX_FULL.
  t5: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave() while
      IC_INTR_STOP_DET has not risen yet.
  t6: IC_INTR_STOP_DET is rising after entering i2c_dw_irq_handler_slave().
      The driver reports I2C_SLAVE_WRITE_REQUESTED first due to
      'if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))'
      matched and then reports I2C_SLAVE_WRITE_RECEIVED.
  t7: Reports I2C_SLAVE_STOP due to IC_INTR_STOP_DET not be cleared yet.

2. Both IC_INTR_STOP_DET and IC_INTR_RX_FULL are rising before
   i2c_dw_read_clear_intrbits_slave(): I2C_SLAVE_STOP never be reported
   because IC_INTR_STOP_DET was cleared by
   i2c_dw_read_clear_intrbits_slave().

$ i2cset -f -y 2 0x42 0x00 0x41; dmesg -c
[0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
[1][irq_handler   ]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
I2C_SLAVE_WRITE_RECEIVED
[0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
[1][irq_handler   ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
I2C_SLAVE_WRITE_RECEIVED

  t1: ISR with the 1st IC_INTR_RX_FULL.
  t2: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave().
  t3: Enter i2c_dw_irq_handler_slave() and then report
      I2C_SLAVE_WRITE_RECEIVED due to 'if (stat & DW_IC_INTR_RX_FULL)'
      matched.
  t4: ISR with both IC_INTR_STOP_DET and the 2nd IC_INTR_RX_FULL.
  t5: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave().
      The current IC_INTR_STOP_DET was cleared by this
      i2c_dw_read_clear_intrbits_slave().
  t6: Enter i2c_dw_irq_handler_slave() and then report
      i2c_slave_event(WRITE_RECEIVED) due to
      'if (stat & DW_IC_INTR_RX_FULL)' matched.
  t7: I2C_SLAVE_STOP never be reported because IC_INTR_STOP_DET was
      cleared in t5.

In order to resolve these problems, i2c_dw_read_clear_intrbits_slave()
should be called only once in an ISR and take the returned stat to handle
those occurred events. The ISR handling has to be adjusted to conform event
reporting described in Documentation/i2c/slave-interface.rst.

Michael Wu (2):
  i2c: designware: call i2c_dw_read_clear_intrbits_slave() once
  i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

 drivers/i2c/busses/i2c-designware-slave.c | 52 +++++++++--------------
 1 file changed, 19 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/2] i2c: designware: call i2c_dw_read_clear_intrbits_slave() once

[Michael Wu]

If some bits were cleared by i2c_dw_read_clear_intrbits_slave() in
i2c_dw_isr_slave() and not handled immediately, those cleared bits would
not be shown again by later i2c_dw_read_clear_intrbits_slave(). They
therefore were forgotten to be handled.

i2c_dw_read_clear_intrbits_slave() should be called once in an ISR and take
its returned state for all later handlings.

Signed-off-by: Michael Wu <michael.wu@vatics.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-slave.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 44974b53a626..13de01a0f75f 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -159,7 +159,6 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 	u32 raw_stat, stat, enabled, tmp;
 	u8 val = 0, slave_activity;
 
-	regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
 	regmap_read(dev->map, DW_IC_ENABLE, &enabled);
 	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_stat);
 	regmap_read(dev->map, DW_IC_STATUS, &tmp);
@@ -168,6 +167,7 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 	if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave)
 		return 0;
 
+	stat = i2c_dw_read_clear_intrbits_slave(dev);
 	dev_dbg(dev->dev,
 		"%#x STATUS SLAVE_ACTIVITY=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
 		enabled, slave_activity, raw_stat, stat);
@@ -188,11 +188,9 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 						 val);
 				}
 				regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
-				stat = i2c_dw_read_clear_intrbits_slave(dev);
 			} else {
 				regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
 				regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
-				stat = i2c_dw_read_clear_intrbits_slave(dev);
 			}
 			if (!i2c_slave_event(dev->slave,
 					     I2C_SLAVE_READ_REQUESTED,
@@ -207,7 +205,6 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 			regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
 
 		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
-		stat = i2c_dw_read_clear_intrbits_slave(dev);
 		return 1;
 	}
 
@@ -219,7 +216,6 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 			dev_vdbg(dev->dev, "Byte %X acked!", val);
 	} else {
 		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
-		stat = i2c_dw_read_clear_intrbits_slave(dev);
 	}
 
 	return 1;
@@ -230,7 +226,6 @@ static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
 	struct dw_i2c_dev *dev = dev_id;
 	int ret;
 
-	i2c_dw_read_clear_intrbits_slave(dev);
 	ret = i2c_dw_irq_handler_slave(dev);
 	if (ret > 0)
 		complete(&dev->cmd_complete);
-- 
2.17.1

[PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Michael Wu]

Sometimes we would get the following flow when doing an i2cset:

0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
I2C_SLAVE_WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
I2C_SLAVE_WRITE_REQUESTED
I2C_SLAVE_WRITE_RECEIVED

Documentation/i2c/slave-interface.rst says that I2C_SLAVE_WRITE_REQUESTED,
which is mandatory, should be sent while the data did not arrive yet. It
means in a write-request I2C_SLAVE_WRITE_REQUESTED should be reported
before any I2C_SLAVE_WRITE_RECEIVED.

By the way, I2C_SLAVE_STOP didn't be reported in the above case because
DW_IC_INTR_STAT was not 0x200.

dev->status can be used to record the current state, especially Designware
I2C controller has no interrupts to identify a write-request. This patch
makes not only I2C_SLAVE_WRITE_REQUESTED been reported first when
IC_INTR_RX_FULL is rising and dev->status isn't STATUS_WRITE_IN_PROGRESS
but also I2C_SLAVE_STOP been reported when a STOP condition is received.

Signed-off-by: Michael Wu <michael.wu@vatics.com>
---
 drivers/i2c/busses/i2c-designware-slave.c | 45 +++++++++--------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 13de01a0f75f..0d15f4c1e9f7 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -172,26 +172,25 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 		"%#x STATUS SLAVE_ACTIVITY=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
 		enabled, slave_activity, raw_stat, stat);
 
-	if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
-		i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+	if (stat & DW_IC_INTR_RX_FULL) {
+		if (dev->status != STATUS_WRITE_IN_PROGRESS) {
+			dev->status = STATUS_WRITE_IN_PROGRESS;
+			i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED,
+					&val);
+		}
+
+		regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
+		val = tmp;
+		if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
+				     &val))
+			dev_vdbg(dev->dev, "Byte %X acked!", val);
+	}
 
 	if (stat & DW_IC_INTR_RD_REQ) {
 		if (slave_activity) {
-			if (stat & DW_IC_INTR_RX_FULL) {
-				regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
-				val = tmp;
-
-				if (!i2c_slave_event(dev->slave,
-						     I2C_SLAVE_WRITE_RECEIVED,
-						     &val)) {
-					dev_vdbg(dev->dev, "Byte %X acked!",
-						 val);
-				}
-				regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
-			} else {
-				regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
-				regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
-			}
+			regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
+
+			dev->status = STATUS_READ_IN_PROGRESS;
 			if (!i2c_slave_event(dev->slave,
 					     I2C_SLAVE_READ_REQUESTED,
 					     &val))
@@ -203,18 +202,10 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
 		if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
 				     &val))
 			regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
-
-		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
-		return 1;
 	}
 
-	if (stat & DW_IC_INTR_RX_FULL) {
-		regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
-		val = tmp;
-		if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
-				     &val))
-			dev_vdbg(dev->dev, "Byte %X acked!", val);
-	} else {
+	if (stat & DW_IC_INTR_STOP_DET) {
+		dev->status = STATUS_IDLE;
 		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
 	}
 
-- 
2.17.1

Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Jarkko Nikula]

On 10/30/20 10:04 AM, Michael Wu wrote:
> Sometimes we would get the following flow when doing an i2cset:
> 
> 0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
> I2C_SLAVE_WRITE_RECEIVED
> 0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
> I2C_SLAVE_WRITE_REQUESTED
> I2C_SLAVE_WRITE_RECEIVED
> 
> Documentation/i2c/slave-interface.rst says that I2C_SLAVE_WRITE_REQUESTED,
> which is mandatory, should be sent while the data did not arrive yet. It
> means in a write-request I2C_SLAVE_WRITE_REQUESTED should be reported
> before any I2C_SLAVE_WRITE_RECEIVED.
> 
> By the way, I2C_SLAVE_STOP didn't be reported in the above case because
> DW_IC_INTR_STAT was not 0x200.
> 
> dev->status can be used to record the current state, especially Designware
> I2C controller has no interrupts to identify a write-request. This patch
> makes not only I2C_SLAVE_WRITE_REQUESTED been reported first when
> IC_INTR_RX_FULL is rising and dev->status isn't STATUS_WRITE_IN_PROGRESS
> but also I2C_SLAVE_STOP been reported when a STOP condition is received.
> 
> Signed-off-by: Michael Wu <michael.wu@vatics.com>
> ---
>   drivers/i2c/busses/i2c-designware-slave.c | 45 +++++++++--------------
>   1 file changed, 18 insertions(+), 27 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Wolfram Sang]

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

Hi Michael,

> Documentation/i2c/slave-interface.rst says that I2C_SLAVE_WRITE_REQUESTED,
> which is mandatory, should be sent while the data did not arrive yet. It
> means in a write-request I2C_SLAVE_WRITE_REQUESTED should be reported
> before any I2C_SLAVE_WRITE_RECEIVED.

Correct.

> dev->status can be used to record the current state, especially Designware
> I2C controller has no interrupts to identify a write-request. This patch

Just double-checking: the designware HW does not raise an interrupt when
its own address + RW bit has been received?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Michael.Wu]

Hi Wolfram,

> > dev->status can be used to record the current state, especially Designware
> > I2C controller has no interrupts to identify a write-request. This patch
> 
> Just double-checking: the designware HW does not raise an interrupt when
> its own address + RW bit has been received?

Not exactly. There're an interrupt state name "RD_REQ" but no one named
like "WR_REQ".

For read-request, the slave will get a RD_REQ interrupt. 
For write-request, the slave won't be interrupted until data arrived to
trigger interrupt "RX_FULL".

I tried to use GPIO to simulate an I2C master. I only sent its own
address + W bit without any data and then I got only a STOP_DET interrupt.
If I sent its own address + W bit + one byte data and then I got one
RX_FULL and a STOP_DET.

It seems the controller doesn't interrupt when RW bit is W, but R does.
What do you think, Jarkko?

Sincerely,

Michael Wu

Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Wolfram Sang]

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


> Not exactly. There're an interrupt state name "RD_REQ" but no one named
> like "WR_REQ".
> 
> For read-request, the slave will get a RD_REQ interrupt. 
> For write-request, the slave won't be interrupted until data arrived to
> trigger interrupt "RX_FULL".
> 
> I tried to use GPIO to simulate an I2C master. I only sent its own
> address + W bit without any data and then I got only a STOP_DET interrupt.
> If I sent its own address + W bit + one byte data and then I got one
> RX_FULL and a STOP_DET.
> 
> It seems the controller doesn't interrupt when RW bit is W, but R does.

Thanks for the detailed explanation! Okay, then what you do looks
correct to me (from a high level perspective without really knowing the
HW): when RX is full, you first send the state WRITE_REQUESTED when
there is no other transfer on-going. Then you send WRITE_RECEIVED
immediately. I think this is the way to do it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Michael.Wu]

Hi Wolfram,

> Thanks for the detailed explanation! Okay, then what you do looks
> correct to me (from a high level perspective without really knowing the
> HW): when RX is full, you first send the state WRITE_REQUESTED when
> there is no other transfer on-going. Then you send WRITE_RECEIVED
> immediately. I think this is the way to do it.

Bingo!! Thanks for your understanding.

I think I should have a habit of writing comments... X-P

Best regards,
Michael Wu

Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED

[Jarkko Nikula]

On 11/4/20 12:17 PM, Michael.Wu@vatics.com wrote:
> Hi Wolfram,
> 
>>> dev->status can be used to record the current state, especially Designware
>>> I2C controller has no interrupts to identify a write-request. This patch
>>
>> Just double-checking: the designware HW does not raise an interrupt when
>> its own address + RW bit has been received?
> 
> Not exactly. There're an interrupt state name "RD_REQ" but no one named
> like "WR_REQ".
> 
> For read-request, the slave will get a RD_REQ interrupt.
> For write-request, the slave won't be interrupted until data arrived to
> trigger interrupt "RX_FULL".
> 
> I tried to use GPIO to simulate an I2C master. I only sent its own
> address + W bit without any data and then I got only a STOP_DET interrupt.
> If I sent its own address + W bit + one byte data and then I got one
> RX_FULL and a STOP_DET.
> 
> It seems the controller doesn't interrupt when RW bit is W, but R does.
> What do you think, Jarkko?
> 
Yes, the datasheet has a flowchart for slave mode and it shows for a 
write only RX_FULL interrupt followed by read from IC_DATA_CMD to 
retrieve received byte. Which I believe won't occur if there is no 
incoming data byte and only STOP_DET happens as you have observed. The 
flowchart however doesn't include the STOP_DET flow.

Jarkko