mbox series

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

Message ID 20210626102806.15402-1-raviteja.narayanam@xilinx.com
Headers show
Series i2c: xiic: Add features, bug fixes. | expand

Message

Raviteja Narayanam June 26, 2021, 10:27 a.m. UTC
-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(-)

Comments

Michal Simek June 28, 2021, 7:23 a.m. UTC | #1
On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
> -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>

Thanks,
Michal
Marek Vasut July 16, 2021, 4:01 p.m. UTC | #2
On 6/28/21 9:23 AM, Michal Simek wrote:
> 
> 
> On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
>> -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:

xiic-i2c a0010000.i2c: mmio a0010000 irq 36
xiic-i2c a0120000.i2c: mmio a0120000 irq 38
atmel_mxt_ts 3-004a: supply vdda not found, using dummy regulator
atmel_mxt_ts 3-004a: supply vdd not found, using dummy regulator

xiic-i2c a0120000.i2c: Timeout waiting at Tx empty

atmel_mxt_ts 3-004a: __mxt_read_reg: i2c transfer failed (-5)
atmel_mxt_ts 3-004a: mxt_bootloader_read: i2c recv failed (-5)
atmel_mxt_ts 3-004a: Trying alternate bootloader address
atmel_mxt_ts 3-004a: mxt_bootloader_read: i2c recv failed (-5)
atmel_mxt_ts: probe of 3-004a failed with error -5
Raviteja Narayanam July 19, 2021, 10:09 a.m. UTC | #3
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Friday, July 16, 2021 9:31 PM
> To: Michal Simek <michals@xilinx.com>; Raviteja Narayanam
> <rna@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 6/28/21 9:23 AM, Michal Simek wrote:
> >
> >
> > On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
> >> -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. 
If it failed without adding anything, then please check whether the vivado design constraints
are correctly applied or not. 
Also check if the other devices on the bus are detected and i2ctransfer command is successful on them.
It would be helpful to know if the device ' atmel_mxt_ts' is successfully probed with next-20210716
without applying this patchset. 

I have tested this again on our boards with eeprom and other sensors, this is working fine for us.

Regards,
Raviteja N

> 
> xiic-i2c a0010000.i2c: mmio a0010000 irq 36 xiic-i2c a0120000.i2c: mmio
> a0120000 irq 38 atmel_mxt_ts 3-004a: supply vdda not found, using dummy
> regulator atmel_mxt_ts 3-004a: supply vdd not found, using dummy
> regulator
> 
> xiic-i2c a0120000.i2c: Timeout waiting at Tx empty
> 
> atmel_mxt_ts 3-004a: __mxt_read_reg: i2c transfer failed (-5) atmel_mxt_ts
> 3-004a: mxt_bootloader_read: i2c recv failed (-5) atmel_mxt_ts 3-004a:
> Trying alternate bootloader address atmel_mxt_ts 3-004a:
> mxt_bootloader_read: i2c recv failed (-5)
> atmel_mxt_ts: probe of 3-004a failed with error -5
Marek Vasut July 19, 2021, 6 p.m. UTC | #4
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.

> 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 ?
Raviteja Narayanam July 20, 2021, 2:19 p.m. UTC | #5
> -----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
Marek Vasut July 20, 2021, 9:43 p.m. UTC | #6
On 7/20/21 4:19 PM, Raviteja Narayanam wrote:

Hi,

[...]

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

Could it be that you never see the problem because you always talk to 
one single device ?

Do you also test writes which are not 1 byte long ?

>          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.

Could it be the i2c-dev serializes each of those transfers , so no race 
can be triggered ?

> 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.

Thank you
Raviteja Narayanam July 26, 2021, 5:26 a.m. UTC | #7
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, July 21, 2021 3:14 AM
> 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/20/21 4:19 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>> 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
> 
> Could it be that you never see the problem because you always talk to one
> single device ?

There are transfers to other devices as well. 
Our board has multiple power monitors, eeprom and other misc devices that
are accessed through the same driver and are working fine.

> 
> Do you also test writes which are not 1 byte long ?
>

Yes, like for eeprom 1 page (16 bytes)  is written.
 
> >          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.
> 
> Could it be the i2c-dev serializes each of those transfers , so no race can be
> triggered ?
> 

Yes, that is true because all our tests are going through the i2c-core only
and there is a lock at adapter level in the core.
It has to be reproducible through the i2c standard interface, which is not
happening at our setup.

I can take your patches that are targeted for this issue, rebase, test
and send them.

Regards,
Raviteja N
Marek Vasut July 26, 2021, 1:12 p.m. UTC | #8
On 7/26/21 7:26 AM, Raviteja Narayanam wrote:

Hi,

[...]

>>>>> 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
>>
>> Could it be that you never see the problem because you always talk to one
>> single device ?
> 
> There are transfers to other devices as well.

The above test only accesses device at address 0x54, right ?

> Our board has multiple power monitors, eeprom and other misc devices that
> are accessed through the same driver and are working fine.

That does not seem to be what the test above does .

>> Do you also test writes which are not 1 byte long ?
>>
> 
> Yes, like for eeprom 1 page (16 bytes)  is written.

I suspect the atmel mxt does much longer writes, try 255 bytes or so.

>>>           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.
>>
>> Could it be the i2c-dev serializes each of those transfers , so no race can be
>> triggered ?
>>
> 
> Yes, that is true because all our tests are going through the i2c-core only
> and there is a lock at adapter level in the core.
> It has to be reproducible through the i2c standard interface, which is not
> happening at our setup.
> 
> I can take your patches that are targeted for this issue, rebase, test
> and send them.

I think you and Michal talked about getting the atmel mxt touchscreen, 
so you can test that yourself as well.
Raviteja Narayanam July 28, 2021, 10:11 a.m. UTC | #9
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, July 26, 2021 6:43 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/26/21 7:26 AM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>>> 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
> >>
> >> Could it be that you never see the problem because you always talk to
> >> one single device ?
> >
> > There are transfers to other devices as well.
> 
> The above test only accesses device at address 0x54, right ?

Above code is just one part.
We are doing read/writes to all devices present on this board https://www.xilinx.com/support/documentation/boards_and_kits/zcu102/ug1182-zcu102-eval-bd.pdf 

> 
> > Our board has multiple power monitors, eeprom and other misc devices
> > that are accessed through the same driver and are working fine.
> 
> That does not seem to be what the test above does .
> 
> >> Do you also test writes which are not 1 byte long ?
> >>
> >
> > Yes, like for eeprom 1 page (16 bytes)  is written.
> 
> I suspect the atmel mxt does much longer writes, try 255 bytes or so.

Ok, I will do longer writes (in the range of 255) on supported slave devices.

> 
> >>>           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.
> >>
> >> Could it be the i2c-dev serializes each of those transfers , so no
> >> race can be triggered ?
> >>
> >
> > Yes, that is true because all our tests are going through the i2c-core
> > only and there is a lock at adapter level in the core.
> > It has to be reproducible through the i2c standard interface, which is
> > not happening at our setup.
> >
> > I can take your patches that are targeted for this issue, rebase, test
> > and send them.
> 
> I think you and Michal talked about getting the atmel mxt touchscreen, so
> you can test that yourself as well.

Yes, that is the plan, we are trying to get the part. Only problem is it is subject to
availability and may take more time to get it delivered to our place.

Regards,
Raviteja N
Marek Vasut July 28, 2021, 6:47 p.m. UTC | #10
On 7/28/21 12:11 PM, Raviteja Narayanam wrote:
[...]

>>>>>>> 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
>>>>
>>>> Could it be that you never see the problem because you always talk to
>>>> one single device ?
>>>
>>> There are transfers to other devices as well.
>>
>> The above test only accesses device at address 0x54, right ?
> 
> Above code is just one part.
> We are doing read/writes to all devices present on this board https://www.xilinx.com/support/documentation/boards_and_kits/zcu102/ug1182-zcu102-eval-bd.pdf

Can you share details of how those tests were performed ?

>>> Our board has multiple power monitors, eeprom and other misc devices
>>> that are accessed through the same driver and are working fine.
>>
>> That does not seem to be what the test above does .
>>
>>>> Do you also test writes which are not 1 byte long ?
>>>>
>>>
>>> Yes, like for eeprom 1 page (16 bytes)  is written.
>>
>> I suspect the atmel mxt does much longer writes, try 255 bytes or so.
> 
> Ok, I will do longer writes (in the range of 255) on supported slave devices.

Thank you
Manikanta Guntupalli June 28, 2022, 7:50 a.m. UTC | #11
Hi Marek,

-----Original Message-----
From: Marek Vasut <marex@denx.de> 
Sent: Thursday, July 29, 2021 12:17 AM
To: Raviteja Narayanam <rna@xilinx.com>; Simek, Michal <michal.simek@amd.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/28/21 12:11 PM, Raviteja Narayanam wrote:
[...]

>>
>> I suspect the atmel mxt does much longer writes, try 255 bytes or so.
> 

I was able to probe the atmel successfully after applying the below series.
https://lore.kernel.org/linux-arm-kernel/1656072327-13628-4-git-send-email-manikanta.guntupalli@xilinx.com/T/

Could you please confirm, if it works for you.


Thanks,
Manikanta.