diff mbox series

i2c: designware: fix slave omitted IC_INTR_STOP_DET

Message ID 20201014052532.3298-1-michael.wu@vatics.com
State New
Headers show
Series i2c: designware: fix slave omitted IC_INTR_STOP_DET | expand

Commit Message

Michael Wu Oct. 14, 2020, 5:25 a.m. UTC
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 <michael.wu@vatics.com>
---
 drivers/i2c/busses/i2c-designware-slave.c | 79 ++++++++++++-----------
 1 file changed, 40 insertions(+), 39 deletions(-)

Comments

kernel test robot Oct. 14, 2020, 7:39 a.m. UTC | #1
Hi Michael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v5.9 next-20201013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Wu/i2c-designware-fix-slave-omitted-IC_INTR_STOP_DET/20201014-132759
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: riscv-randconfig-r036-20201014 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project e7fe3c6dfede8d5781bd000741c3dea7088307a4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6bb28b0fda0a3f133918077bbfb1c6fe4809bf30
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Wu/i2c-designware-fix-slave-omitted-IC_INTR_STOP_DET/20201014-132759
        git checkout 6bb28b0fda0a3f133918077bbfb1c6fe4809bf30
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inw(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:55:76: note: expanded from macro 'inw'
   #define inw(c)          ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
   #define readw_cpu(c)            ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-designware-slave.c:13:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inl(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inl'
   #define inl(c)          ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
   #define readl_cpu(c)            ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-designware-slave.c:13:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outb(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:58:68: note: expanded from macro 'outb'
   #define outb(v,c)       ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
   #define writeb_cpu(v, c)        ((void)__raw_writeb((v), (c)))
                                                             ^
   In file included from drivers/i2c/busses/i2c-designware-slave.c:13:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outw(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outw'
   #define outw(v,c)       ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
   #define writew_cpu(v, c)        ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
                                                                                     ^
   In file included from drivers/i2c/busses/i2c-designware-slave.c:13:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outl(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outl'
   #define outl(v,c)       ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
   #define writel_cpu(v, c)        ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
                                                                                     ^
   In file included from drivers/i2c/busses/i2c-designware-slave.c:13:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:13:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:148:
   include/asm-generic/io.h:1017:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/i2c/busses/i2c-designware-slave.c:181:42: error: expected ';' after expression
                           dev->status = STATUS_WRITE_IN_PROGRESS
                                                                 ^
                                                                 ;
>> drivers/i2c/busses/i2c-designware-slave.c:195:22: error: use of undeclared identifier 'STATUS_IDEL'
                   if (dev->status != STATUS_IDEL) {
                                      ^
   7 warnings and 2 errors generated.

vim +181 drivers/i2c/busses/i2c-designware-slave.c

   151	
   152	/*
   153	 * Interrupt service routine. This gets called whenever an I2C slave interrupt
   154	 * occurs.
   155	 */
   156	
   157	static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
   158	{
   159		u32 raw_stat, stat, enabled, tmp;
   160		u8 val = 0, slave_activity;
   161	
   162		regmap_read(dev->map, DW_IC_ENABLE, &enabled);
   163		regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_stat);
   164		regmap_read(dev->map, DW_IC_STATUS, &tmp);
   165		slave_activity = ((tmp & DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
   166	
   167		if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave)
   168			return 0;
   169	
   170		stat = i2c_dw_read_clear_intrbits_slave(dev);
   171		dev_dbg(dev->dev,
   172			"%#x STATUS SLAVE_ACTIVITY=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
   173			enabled, slave_activity, raw_stat, stat);
   174	
   175		if (stat & DW_IC_INTR_RX_FULL) {
   176			if (dev->status != STATUS_WRITE_IN_PROGRESS) {
   177				if (dev->status != STATUS_IDLE)
   178					i2c_slave_event(dev->slave, I2C_SLAVE_STOP,
   179							&val);
   180	
 > 181				dev->status = STATUS_WRITE_IN_PROGRESS
   182				i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED,
   183						&val);
   184			}
   185	
   186			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
   187			val = tmp;
   188	
   189			if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
   190					     &val))
   191				dev_vdbg(dev->dev, "Byte %X acked!", val);
   192		}
   193	
   194		if (stat & DW_IC_INTR_RD_REQ) {
 > 195			if (dev->status != STATUS_IDEL) {
   196				dev->status = STATUS_IDLE;
   197				i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
   198			}
   199	
   200			if (slave_activity) {
   201				regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
   202				regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
   203	
   204				dev->status = STATUS_READ_IN_PROGRESS;
   205				i2c_slave_event(dev->slave, I2C_SLAVE_READ_REQUESTED,
   206						&val);
   207				regmap_write(dev->map, DW_IC_DATA_CMD, val);
   208			}
   209		}
   210	
   211		if (stat & DW_IC_INTR_RX_DONE) {
   212			i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED, &val);
   213			regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
   214	
   215			dev->status = STATUS_IDLE;
   216			i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
   217		}
   218	
   219		if (stat & DW_IC_INTR_STOP_DET) {
   220			if (dev->status != STATUS_IDLE) {
   221				dev->status = STATUS_IDLE;
   222	
   223				i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
   224			}
   225		}
   226	
   227		return 1;
   228	}
   229	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Andy Shevchenko Oct. 14, 2020, 8:52 a.m. UTC | #2
On Wed, Oct 14, 2020 at 03:39:51PM +0800, kernel test robot wrote:
> Hi Michael,
> 
> Thank you for the patch! Yet something to improve:

Wondering if you compile this at all...

>    include/asm-generic/io.h:1017:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
>                                                      ~~~~~~~~~~ ^
> >> drivers/i2c/busses/i2c-designware-slave.c:181:42: error: expected ';' after expression
>                            dev->status = STATUS_WRITE_IN_PROGRESS
>                                                                  ^
>                                                                  ;
> >> drivers/i2c/busses/i2c-designware-slave.c:195:22: error: use of undeclared identifier 'STATUS_IDEL'
>                    if (dev->status != STATUS_IDEL) {
>                                       ^
>    7 warnings and 2 errors generated.
Michael Wu Oct. 14, 2020, 9:58 a.m. UTC | #3
On 10/14/20 4:53 PM, andriy.shevchenko@linux.intel.com wrote:
> 

> Wondering if you compile this at all...



I'm very sorry that I did not compile it because I only have ARM SoC with my
linux 4.9.170, but I've verified the logic of this patch in my linux.

I'll correct these two syntax errors in next version. The reported
warnings occurs from drivers/i2c/busses/i2c-designware-slave.c:13 seems
not made by this modification. Please correct me if I'm wrong....

> >    include/asm-generic/io.h:1017:55: warning: performing pointer

> arithmetic on a null pointer has undefined behavior

> [-Wnull-pointer-arithmetic]

> >            return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE +

> port;

> >

> ~~~~~~~~~~ ^

> > >> drivers/i2c/busses/i2c-designware-slave.c:181:42: error: expected ';' after

> expression

> >                            dev->status =

> STATUS_WRITE_IN_PROGRESS

> >

> ^

> >

>    ;

> > >> drivers/i2c/busses/i2c-designware-slave.c:195:22: error: use of

> undeclared identifier 'STATUS_IDEL'

> >                    if (dev->status != STATUS_IDEL) {

> >                                       ^

> >    7 warnings and 2 errors generated.

> 

> 

>
Andy Shevchenko Oct. 14, 2020, 10:16 a.m. UTC | #4
On Wed, Oct 14, 2020 at 09:58:54AM +0000, Michael.Wu@vatics.com wrote:
> On 10/14/20 4:53 PM, andriy.shevchenko@linux.intel.com wrote:
> > 
> > Wondering if you compile this at all...
> 
> 
> I'm very sorry that I did not compile it because I only have ARM SoC with my
> linux 4.9.170, but I've verified the logic of this patch in my linux.
> 
> I'll correct these two syntax errors in next version. The reported
> warnings occurs from drivers/i2c/busses/i2c-designware-slave.c:13 seems
> not made by this modification. Please correct me if I'm wrong....

So, you have a chance to create a follow up patch to fix the warnings. :-)
Jarkko Nikula Oct. 14, 2020, 10:43 a.m. UTC | #5
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 <michael.wu@vatics.com>

> ---

>   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
Michael Wu Oct. 14, 2020, 11:05 a.m. UTC | #6
On 10/14/20 6:53 PM, jarkko.nikula@linux.intel.com 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
diff mbox series

Patch

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