Designeware I2C slave confusing IC_INTR_STOP_DET handle

Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Michael.Wu]

Hi Sir,

My I2C slave sometimes gets only 2 interrupts: one is IC_INTR_RX_FULL and
the other is IC_INTR_RX_FULL with IC_INTR_STOP_DET. The 2nd interrupt
causes 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.


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.

--
BR,
Michael Wu

Re: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Jarkko Nikula]

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);

RE: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Michael.Wu]

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

RE: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Michael.Wu]

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

Re: Designeware I2C slave confusing IC_INTR_STOP_DET handle

[Jarkko Nikula]

On 10/16/20 10:26 AM, Michael.Wu@vatics.com wrote:
> 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.
...
> 
> I'll submit it if you accept it, or you have other suggestions....?
> 
Please go ahead :-)

My patch was just one quick idea part of the discussion and I can freely 
let you take the authorship to real commits since obviously you have 
done more work to discover issue, test and write actual commits with 
change logs :-)

-- 
Jarkko