RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam <rna@xilinx.com>]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, July 19, 2021 11:30 PM
> To: Raviteja Narayanam <rna@xilinx.com>; Michal Simek
> <michals@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; joe@perches.com
> Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.
> 
> On 7/19/21 12:09 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>> -Add 'standard mode' feature for reads > 255 bytes.
> >>>> -Add 'smbus block read' functionality.
> >>>> -Add 'xlnx,axi-iic-2.1' new IP version support.
> >>>> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
> >>>> -Remove 'local_irq_save/restore' calls as discussed here:
> >> https://www.spinics.net/lists/linux-i2c/msg46483.html.
> >>>> -Some trivial fixes.
> >>>>
> >>>> Changes in v2:
> >>>> -Grouped the commits as fixes first and then features.
> >>>> -The first 4 commits fix the dynamic mode broken feature.
> >>>> -Corrected the indentation in coding style issues.
> >>>>
> >>>> Michal Simek (1):
> >>>>     i2c: xiic: Fix coding style issues
> >>>>
> >>>> Raviteja Narayanam (7):
> >>>>     i2c: xiic: Fix Tx Interrupt path for grouped messages
> >>>>     i2c: xiic: Add standard mode support for > 255 byte read transfers
> >>>>     i2c: xiic: Switch to Xiic standard mode for i2c-read
> >>>>     i2c: xiic: Remove interrupt enable/disable in Rx path
> >>>>     dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
> >>>>     i2c: xiic: Update compatible with new IP version
> >>>>     i2c: xiic: Add smbus_block_read functionality
> >>>>
> >>>> Shubhrajyoti Datta (2):
> >>>>     i2c: xiic: Return value of xiic_reinit
> >>>>     i2c: xiic: Fix the type check for xiic_wakeup
> >>>>
> >>>>    .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
> >>>>    drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
> >>>>    2 files changed, 487 insertions(+), 110 deletions(-)
> >>>>
> >>>
> >>> Acked-by: Michal Simek <michal.simek@xilinx.com>
> >>
> >> I just tested this patchset on next-20210716 and the XIIC failures
> >> are still present, see:
> >
> > The probe of ' atmel_mxt_ts' failed as per the error. May I know the
> > details of your test case if you tweaked any i2ctransfers/added delays.
> 
> It is still the same test case from a year ago -- Atmel MXT touchscreen
> controller connected to XIIC I2C IP in ZynqMP FPGA, both drivers are
> compiled into the kernel. Also, it is not the "new" XIIC IP revision, but older
> one from Vivado 2019 or so.
> 
> > If it failed without adding anything, then please check whether the
> > vivado design constraints are correctly applied or not.
> 
> They are, we already checked multiple times and the FPGA part is OK.
> 
> > Also check if the other devices on the bus are detected and i2ctransfer
> command is successful on them.
> 
> Note that this problem is very likely a race condition in the XIIC driver, so a
> trivial test like i2ctransfer on idle system from userspace is unlikely to trigger
> it. When the system is under heavy load e.g.
> during the kernel boot, that is when these corner cases start showing up.

Thanks for all the details, Marek. 

> 
> > It would be helpful to know if the device ' atmel_mxt_ts' is
> > successfully probed with next-20210716 without applying this patchset.
> 
> Sometimes, the XIIC driver in current mainline Linux suffers from race
> conditions on SMP, so it depends.
> 
> The MXT driver also has to be patched to avoid longer than 255 byte
> transfers, because that is currently broken with XIIC.
> 
> > I have tested this again on our boards with eeprom and other sensors, this
> is working fine for us.
> 
> Can you share details of how those tests were performed ?

Stress test - 1:
Heavy ethernet traffic running in the background. 
I2c commands script (like below) running. We can see visible stutter in the output as expected, but nothing failed.

i=0
while [ 1 ]
do
		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
                             i2ctransfer -y -f 2 w1@0X54 0X00 r1@0X54
        i=$(expr $i + 1)
        echo "$i"
done

Stress test - 2:
Two i2c scripts running in parallel with commands as shown above with different bus numbers (as a result of mux), but going into same XIIC adapter.
This is also working fine.

Stress test - 3:
Two i2c scripts running in parallel with same commands in separate terminals. This is also working fine.

From your log, the race condition is occurring at boot time during i2c clients registration. I am starting a similar test at my setup
to reproduce this issue at boot time.

Regards,
Raviteja N