* [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
@ 2020-10-14 5:25 Michael Wu
2020-10-14 7:39 ` kernel test robot
2020-10-14 10:43 ` Jarkko Nikula
0 siblings, 2 replies; 7+ messages in thread
From: Michael Wu @ 2020-10-14 5:25 UTC (permalink / raw)
To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, linux-i2c, linux-kernel
Cc: Morgan Chang, Dean Hsiao, Paul Chen, Michael Wu
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(-)
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 5:25 [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET Michael Wu
@ 2020-10-14 7:39 ` kernel test robot
2020-10-14 8:52 ` Andy Shevchenko
2020-10-14 10:43 ` Jarkko Nikula
1 sibling, 1 reply; 7+ messages in thread
From: kernel test robot @ 2020-10-14 7:39 UTC (permalink / raw)
To: Michael Wu, Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
linux-i2c, linux-kernel
Cc: kbuild-all, clang-built-linux, Morgan Chang, Dean Hsiao,
Paul Chen, Michael Wu
[-- Attachment #1: Type: text/plain, Size: 11864 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26003 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 7:39 ` kernel test robot
@ 2020-10-14 8:52 ` Andy Shevchenko
2020-10-14 9:58 ` Michael.Wu
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2020-10-14 8:52 UTC (permalink / raw)
To: kernel test robot
Cc: Michael Wu, Jarkko Nikula, Mika Westerberg, linux-i2c,
linux-kernel, kbuild-all, clang-built-linux, Morgan Chang,
Dean Hsiao, Paul Chen
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 8:52 ` Andy Shevchenko
@ 2020-10-14 9:58 ` Michael.Wu
2020-10-14 10:16 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Michael.Wu @ 2020-10-14 9:58 UTC (permalink / raw)
To: andriy.shevchenko, lkp
Cc: jarkko.nikula, mika.westerberg, linux-i2c, linux-kernel,
kbuild-all, clang-built-linux, morgan.chang, dean.hsiao,
paul.chen
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.
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 9:58 ` Michael.Wu
@ 2020-10-14 10:16 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2020-10-14 10:16 UTC (permalink / raw)
To: Michael.Wu
Cc: lkp, jarkko.nikula, mika.westerberg, linux-i2c, linux-kernel,
kbuild-all, clang-built-linux, morgan.chang, dean.hsiao,
paul.chen
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. :-)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 5:25 [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET Michael Wu
2020-10-14 7:39 ` kernel test robot
@ 2020-10-14 10:43 ` Jarkko Nikula
2020-10-14 11:05 ` Michael.Wu
1 sibling, 1 reply; 7+ messages in thread
From: Jarkko Nikula @ 2020-10-14 10:43 UTC (permalink / raw)
To: Michael Wu, Andy Shevchenko, Mika Westerberg, linux-i2c, linux-kernel
Cc: Morgan Chang, Dean Hsiao, Paul Chen
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
2020-10-14 10:43 ` Jarkko Nikula
@ 2020-10-14 11:05 ` Michael.Wu
0 siblings, 0 replies; 7+ messages in thread
From: Michael.Wu @ 2020-10-14 11:05 UTC (permalink / raw)
To: jarkko.nikula, andriy.shevchenko, mika.westerberg, linux-i2c,
linux-kernel
Cc: morgan.chang, dean.hsiao, paul.chen
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-14 11:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 5:25 [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET Michael Wu
2020-10-14 7:39 ` kernel test robot
2020-10-14 8:52 ` Andy Shevchenko
2020-10-14 9:58 ` Michael.Wu
2020-10-14 10:16 ` Andy Shevchenko
2020-10-14 10:43 ` Jarkko Nikula
2020-10-14 11:05 ` Michael.Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).