When an I2C slave works, sometimes both IC_INTR_RX_FULL and
IC_INTR_STOP_DET are 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 WRITE_REQUESTED is done after the
1st 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
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
WRITE_REQUESTED
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
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 do
i2c_slave_event(WRITE_RECEIVED) because
if (stat & DW_IC_INTR_RX_FULL).
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: Enter i2c_dw_irq_handler_slave() and then IC_INTR_STOP_DET is
rising. i2c_slave_event(WRITE_REQUESTED) will be done first because
if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET)) and
then doing i2c_slave_event(WRITE_RECEIVED).
t7: do i2c_slave_event(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(): STOP cannot wait because
IC_INTR_STOP_DET is 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
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
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 do
i2c_slave_event(WRITE_RECEIVED) because
if (stat & DW_IC_INTR_RX_FULL).
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 is cleared by this
i2c_dw_read_clear_intrbits_slave().
t6: Enter i2c_dw_irq_handler_slave() and then do
i2c_slave_event(WRITE_RECEIVED) because
if (stat & DW_IC_INTR_RX_FULL).
t7: i2c_slave_event(STOP) never be done 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 one time in ISR and take the returned stat to handle
those occurred events.
Signed-off-by: Michael Wu <[email protected]>
---
drivers/i2c/busses/i2c-designware-slave.c | 79 ++++++++++++-----------
1 file changed, 40 insertions(+), 39 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 44974b53a626..8b3047fb2eae 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,58 +167,61 @@ 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);
- 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) {
+ if (dev->status != STATUS_IDLE)
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP,
+ &val);
+
+ 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 (dev->status != STATUS_IDEL) {
+ dev->status = STATUS_IDLE;
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ }
+
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);
- 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,
- &val))
- regmap_write(dev->map, DW_IC_DATA_CMD, val);
+ regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
+ regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
+
+ dev->status = STATUS_READ_IN_PROGRESS;
+ i2c_slave_event(dev->slave, I2C_SLAVE_READ_REQUESTED,
+ &val);
+ regmap_write(dev->map, DW_IC_DATA_CMD, val);
}
}
if (stat & DW_IC_INTR_RX_DONE) {
- 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_READ_PROCESSED, &val);
+ regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
+ dev->status = STATUS_IDLE;
i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
- stat = i2c_dw_read_clear_intrbits_slave(dev);
- 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 {
- i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
- stat = i2c_dw_read_clear_intrbits_slave(dev);
+ if (stat & DW_IC_INTR_STOP_DET) {
+ if (dev->status != STATUS_IDLE) {
+ dev->status = STATUS_IDLE;
+
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ }
}
return 1;
@@ -230,7 +232,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
On 10/14/20 6:53 PM, [email protected] wrote:
> Thanks for the patch. I was thinking this too after your report but
> haven't found actually time to look at implementing it.
>
> But what I was thinking it is probably good to have two patches. First
> patch that changes only i2c_dw_read_clear_intrbits_slave() semantics so
> that it's called only once like here and second patch that does other
> logic changes. Makes easier to catch possible regressions I think.
>
> Jarkko
I can't agree with you more.
I'll separate them into two patches in next version.
By the way, I found a way to compile my patch by myself via
"make multi_v7_defconfig"
and
"make drivers/i2c/busses/i2c-designware-slave.o".
I've done and there were no warnings about
drivers/i2c/busses/i2c-designware-slave.c:13.
--
Michael Wu
Hi
On 10/14/20 8:25 AM, Michael Wu wrote:
> When an I2C slave works, sometimes both IC_INTR_RX_FULL and
> IC_INTR_STOP_DET are 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:
> e
> 1. IC_INTR_STOP_DET is rising after i2c_dw_read_clear_intrbits_slave()
> done: It seems invalidated because WRITE_REQUESTED is done after the
> 1st 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
> 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
> WRITE_REQUESTED
> 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
> 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 do
> i2c_slave_event(WRITE_RECEIVED) because
> if (stat & DW_IC_INTR_RX_FULL).
> 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: Enter i2c_dw_irq_handler_slave() and then IC_INTR_STOP_DET is
> rising. i2c_slave_event(WRITE_REQUESTED) will be done first because
> if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET)) and
> then doing i2c_slave_event(WRITE_RECEIVED).
> t7: do i2c_slave_event(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(): STOP cannot wait because
> IC_INTR_STOP_DET is 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
> 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
> 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 do
> i2c_slave_event(WRITE_RECEIVED) because
> if (stat & DW_IC_INTR_RX_FULL).
> 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 is cleared by this
> i2c_dw_read_clear_intrbits_slave().
> t6: Enter i2c_dw_irq_handler_slave() and then do
> i2c_slave_event(WRITE_RECEIVED) because
> if (stat & DW_IC_INTR_RX_FULL).
> t7: i2c_slave_event(STOP) never be done 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 one time in ISR and take the returned stat to handle
> those occurred events.
>
> Signed-off-by: Michael Wu <[email protected]>
> ---
> drivers/i2c/busses/i2c-designware-slave.c | 79 ++++++++++++-----------
> 1 file changed, 40 insertions(+), 39 deletions(-)
>
Thanks for the patch. I was thinking this too after your report but
haven't found actually time to look at implementing it.
But what I was thinking it is probably good to have two patches. First
patch that changes only i2c_dw_read_clear_intrbits_slave() semantics so
that it's called only once like here and second patch that does other
logic changes. Makes easier to catch possible regressions I think.
Jarkko