Re: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

Hi

On 10/7/20 8:10 AM, Michael.Wu@vatics.com wrote:
> These above scenarios appears when OS is busy or too late to handle I2C
> interrupts. Current i2c_dw_irq_handler_slave() seems that last
> IC_INTR_RX_FULL will be handled before IC_INTR_STOP_DET rising, or
> IC_INTR_STOP_DET will not be cleared before last IC_INTR_RX_FULL handled.
> I think it can't be guaranteed.
> 
Indeed i2c_dw_irq_handler_slave() handling looks doubtful when I look at 
it after your report. Especially many of those 
i2c_dw_read_clear_intrbits_slave() calls. I think there are good changes 
to miss some interrupts.

Unfortunately I don't have right now a setup to try myself but could you 
try these ideas to read and clear interrupt status in one place only and 
move I2C_SLAVE_WRITE_REQUESTED reporting after I2C_SLAVE_WRITE_RECEIVED 
like in a patch below?

diff --git a/drivers/i2c/busses/i2c-designware-slave.c 
b/drivers/i2c/busses/i2c-designware-slave.c
index 44974b53a626..97131e888e24 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,13 +167,11 @@ 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_RD_REQ) {
  		if (slave_activity) {
  			if (stat & DW_IC_INTR_RX_FULL) {
@@ -188,11 +185,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 +202,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,9 +213,11 @@ 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);
  	}

+	if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
+		i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+
  	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);