RE: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[<Michael.Wu@vatics.com>]

Hi Jarkko,

On 10/7/20 9:08 PM, jarkko.nikula@linux.intel.com wrote:
> 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);

I would like to submit your patch in order to do my modification based on this idea.
You're the author but you didn't leave commit description.

I prepared one for this patch.

    i2c: designware: call i2c_dw_read_clear_intrbits_slave() once

    i2c_dw_read_clear_intrbits_slave() was called per each interrupt handle.
    It caused some interrupt bits which haven't been handled yet were cleared,
    the corresponding handlers would do nothing due to interrupt bits been
    discarded. For example,

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

    The root cause is that i2c_dw_read_clear_intrbits_slave() was called many
    times. Calling i2c_dw_read_clear_intrbits_slave() once in one ISR and take
    the returned stat for later handling is the solution.

I'll submit it if you accept it, or you have other suggestions....?

--
Michael Wu