RE: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[<Michael.Wu@vatics.com>]

Hi Sir,

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


After adding your suggestion, it seems work fine with drivers/i2c/i2c-slave-eeprom.c.

The log was usually:

# i2cset -f -y 2 0x42 0x00 0x41;dmesg -c
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x710 : INTR_STAT=0x200
STOP

Sometimes it wound be:

# i2cset -f -y 2 0x42 0x08 0x45;dmesg -c
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
WRITE_RECEIVED
WRITE_REQUESTED

drivers/i2c/i2c-slave-eeprom.c does the same action when triggered by either I2C_SLAVE_STOP or I2C_SLAVE_WRITE_REQUESTED, so one of them being the last event are fine.

But Documentation/i2c/slave-interface.rst said that I2C_SLAVE_WRITE_REQUESTED comes without any data arrived. It means I2C_SLAVE_WRITE_REQUESTED should be front of any WRITE_RECEIVED in a write-request. The second log should be an illegal result.

I2C_SLAVE_WRITE_REQUESTED is made when RX_FULL and STOP_DET are both rising. What situation meet this condition? I guess this STOP_DET was left by previous transmission and then the system is finally handling it while data of the next request arrived. If yes, put "if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))" in front of other handling is correct but it violates I2C_SLAVE_WRITE_REQUESTED dentition when these two interrupts of one request have to handle in one ISR. I think there have to be a solution to distinguish these two cases.

Note that my Linux is 4.9.170 and back-ported drivers/i2c/i2c-designware-slave.c from 4.13 (commit 9f3e065c54b05b03bd39dbbcc5a44f2f1807994d).

--
BR,
Michael Wu