RE: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
On 10/14/20 6:53 PM, jarkko.nik...@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
Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
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 --- 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
Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
On Wed, Oct 14, 2020 at 09:58:54AM +, michael...@vatics.com wrote: > On 10/14/20 4:53 PM, andriy.shevche...@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
RE: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
On 10/14/20 4:53 PM, andriy.shevche...@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.
>
>
>
Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
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
Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
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
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);
[PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
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
---
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, );
regmap_read(dev->map, DW_IC_ENABLE, );
regmap_read(dev->map, DW_IC_RAW_INTR_STAT, _stat);
regmap_read(dev->map, DW_IC_STATUS, );
@@ -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, );
+ if (stat & DW_IC_INTR_RX_FULL) {
+ if (dev->status != STATUS_WRITE_IN_PROGRESS) {
+ if (dev->status != STATUS_IDLE)
+
