| Message ID | 20210626102806.15402-1-raviteja.narayanam@xilinx.com |
|---|---|
| Headers | show |
| Series | i2c: xiic: Add features, bug fixes. | expand |
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
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
> -----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
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 ?
> -----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
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
> -----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
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.
> -----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
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
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.
W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze: > Xilinx I2C IP has two modes of operation, both of which implement > I2C transactions. The only difference from sw perspective is the > programming sequence for these modes. > Dynamic mode -> Simple to program, less number of steps in sequence. > Standard mode -> Gives flexibility, more number of steps in sequence. > > In dynamic mode, during the i2c-read transactions, if there is a > delay(> 200us) between the register writes (address & byte count), > read transaction fails. On a system with load, this scenario is > occurring frequently. > To avoid this, switch to standard mode if there is a read request. > > Added a quirk to identify the IP version effected by this and follow > the standard mode. > > Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> [...] If those two modes only differ in software complexity but we are not able to support only the simpler one and we have support for the more complicated (standard mode) anyways, we know that standard mode can handle or the cases while dynamic mode cannot, we also know that dynamic mode is broken on some versions of the core, why do we actually keep support for dynamic mode? Krzysztof
On 6/29/22 14:47, Krzysztof Adamski wrote: > W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze: >> Xilinx I2C IP has two modes of operation, both of which implement >> I2C transactions. The only difference from sw perspective is the >> programming sequence for these modes. >> Dynamic mode -> Simple to program, less number of steps in sequence. >> Standard mode -> Gives flexibility, more number of steps in sequence. >> >> In dynamic mode, during the i2c-read transactions, if there is a >> delay(> 200us) between the register writes (address & byte count), >> read transaction fails. On a system with load, this scenario is >> occurring frequently. >> To avoid this, switch to standard mode if there is a read request. >> >> Added a quirk to identify the IP version effected by this and follow >> the standard mode. >> >> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> > > [...] > > If those two modes only differ in software complexity but we are not > able to support only the simpler one and we have support for the more > complicated (standard mode) anyways, we know that standard mode > can handle or the cases while dynamic mode cannot, we also know that > dynamic mode is broken on some versions of the core, why do we actually > keep support for dynamic mode? If I recall it right, the dynamic mode was supposed to handle transfers longer than 255 Bytes, which the core cannot do in Standard mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot of time debugging the race conditions in the XIIC, which I ultimately fixed (the patches are upstream), but the long transfers I rather fixed in the MXT driver instead. I also recall there was supposed to be some update for the XIIC core coming with newer vivado, but I might be wrong about that.
Hi Marek, W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >> [...] >> >> If those two modes only differ in software complexity but we are not >> able to support only the simpler one and we have support for the more >> complicated (standard mode) anyways, we know that standard mode >> can handle or the cases while dynamic mode cannot, we also know that >> dynamic mode is broken on some versions of the core, why do we actually >> keep support for dynamic mode? > > If I recall it right, the dynamic mode was supposed to handle > transfers longer than 255 Bytes, which the core cannot do in Standard > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot > of time debugging the race conditions in the XIIC, which I ultimately > fixed (the patches are upstream), but the long transfers I rather > fixed in the MXT driver instead. > > I also recall there was supposed to be some update for the XIIC core > coming with newer vivado, but I might be wrong about that. It seems to be the other way around - dynamic mode is limited to 255 bytes - when you trigger dynamic mode you first write the address of the slave to the FIFO, then you write the length as one byte so you can't request more than 255 bytes. So *standard* mode is used for those messages. In other words - dynamic mode is the one that is more limited - everything that you can do in dynamic mode you can also do in standard mode. So why don't we use standard mode always for everything? Krzysztof
On 6/29/22 16:09, Krzysztof Adamski wrote: > Hi Marek, > > W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >>> [...] >>> >>> If those two modes only differ in software complexity but we are not >>> able to support only the simpler one and we have support for the more >>> complicated (standard mode) anyways, we know that standard mode >>> can handle or the cases while dynamic mode cannot, we also know that >>> dynamic mode is broken on some versions of the core, why do we actually >>> keep support for dynamic mode? >> >> If I recall it right, the dynamic mode was supposed to handle >> transfers longer than 255 Bytes, which the core cannot do in Standard >> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot >> of time debugging the race conditions in the XIIC, which I ultimately >> fixed (the patches are upstream), but the long transfers I rather >> fixed in the MXT driver instead. >> >> I also recall there was supposed to be some update for the XIIC core >> coming with newer vivado, but I might be wrong about that. > > It seems to be the other way around - dynamic mode is limited to 255 > bytes - when you trigger dynamic mode you first write the address of the > slave to the FIFO, then you write the length as one byte so you can't > request more than 255 bytes. So *standard* mode is used for those > messages. In other words - dynamic mode is the one that is more limited > - everything that you can do in dynamic mode you can also do in standard > mode. So why don't we use standard mode always for everything? Sigh, it's been a year since I looked into this, sorry. One of the modes is maybe not supported on all the XIIC core instances ?
[AMD Official Use Only - General] Hi Krzysztof, > -----Original Message----- > From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Sent: Wednesday, June 29, 2022 7:40 PM > To: Marek Vasut <marex@denx.de>; Raviteja Narayanam > <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; > michal.simek@xilinx.com > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > git@xilinx.com; joe@perches.com > Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- > read > > [CAUTION: External Email] > > CAUTION: This message has originated from an External Source. Please use > proper judgment and caution when opening attachments, clicking links, or > responding to this email. > > > Hi Marek, > > W dniu 29.06.2022 o 16:05, Marek Vasut pisze: > >> [...] > >> > >> If those two modes only differ in software complexity but we are not > >> able to support only the simpler one and we have support for the more > >> complicated (standard mode) anyways, we know that standard mode can > >> handle or the cases while dynamic mode cannot, we also know that > >> dynamic mode is broken on some versions of the core, why do we > >> actually keep support for dynamic mode? > > > > If I recall it right, the dynamic mode was supposed to handle > > transfers longer than 255 Bytes, which the core cannot do in Standard > > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot > > of time debugging the race conditions in the XIIC, which I ultimately > > fixed (the patches are upstream), but the long transfers I rather > > fixed in the MXT driver instead. > > > > I also recall there was supposed to be some update for the XIIC core > > coming with newer vivado, but I might be wrong about that. > > It seems to be the other way around - dynamic mode is limited to 255 bytes - > when you trigger dynamic mode you first write the address of the slave to > the FIFO, then you write the length as one byte so you can't request more > than 255 bytes. So *standard* mode is used for those messages. In other > words - dynamic mode is the one that is more limited > - everything that you can do in dynamic mode you can also do in standard > mode. So why don't we use standard mode always for everything? However the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change) Also the dynamic mode is nicer on the processor resources. We set the bytes and the controller takes care of transferring. However do not have any strong views open to suggestions. > > Krzysztof
W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze: > [AMD Official Use Only - General] > > Hi Krzysztof, > >> -----Original Message----- >> From: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> Sent: Wednesday, June 29, 2022 7:40 PM >> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam >> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; >> michal.simek@xilinx.com >> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; >> git@xilinx.com; joe@perches.com >> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- >> read >> >> [CAUTION: External Email] >> >> CAUTION: This message has originated from an External Source. Please use >> proper judgment and caution when opening attachments, clicking links, or >> responding to this email. >> >> >> Hi Marek, >> >> W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >>>> [...] >>>> >>>> If those two modes only differ in software complexity but we are not >>>> able to support only the simpler one and we have support for the more >>>> complicated (standard mode) anyways, we know that standard mode can >>>> handle or the cases while dynamic mode cannot, we also know that >>>> dynamic mode is broken on some versions of the core, why do we >>>> actually keep support for dynamic mode? >>> If I recall it right, the dynamic mode was supposed to handle >>> transfers longer than 255 Bytes, which the core cannot do in Standard >>> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot >>> of time debugging the race conditions in the XIIC, which I ultimately >>> fixed (the patches are upstream), but the long transfers I rather >>> fixed in the MXT driver instead. >>> >>> I also recall there was supposed to be some update for the XIIC core >>> coming with newer vivado, but I might be wrong about that. >> It seems to be the other way around - dynamic mode is limited to 255 bytes - >> when you trigger dynamic mode you first write the address of the slave to >> the FIFO, then you write the length as one byte so you can't request more >> than 255 bytes. So *standard* mode is used for those messages. In other >> words - dynamic mode is the one that is more limited >> - everything that you can do in dynamic mode you can also do in standard >> mode. So why don't we use standard mode always for everything? > However the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change) > Also the dynamic mode is nicer on the processor resources. We set the bytes and the controller takes care of > transferring. > > However do not have any strong views open to suggestions. All I'm saying is that before this patchset, the dynamic mode was used in all cases and it made sense - it is easier to work with. But it turned out it has its limitations and support for standard mode was added with several cases that switch to that mode. The commit message suggests that the only difference is in how complicated the code for handling them is, does not say why dynamic mode might still be preferred. And supporting both of them complicates the code noticeably. My understanding now is that the code struggles to use the dynamic mode in all cases that it can because that produces less interrupts and so it is slightly lighter on resources. So it is a code complication vs effectiveness tradeoff. Since this is I2C - a slow bus, I'm not sure it is worth it but also don't have strong opinion on that. If nothing else, I think it would make sense to update the commit message a little bit to better explain why it is worth keeping both modes. Krzysztof
[AMD Official Use Only - General] > -----Original Message----- > From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Sent: Friday, July 1, 2022 12:32 PM > To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; Marek Vasut > <marex@denx.de>; Raviteja Narayanam <raviteja.narayanam@xilinx.com>; > linux-i2c@vger.kernel.org; michal.simek@xilinx.com > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > git@xilinx.com; joe@perches.com > Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- > read > > [CAUTION: External Email] > > W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze: > > [AMD Official Use Only - General] > > > > Hi Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > >> Sent: Wednesday, June 29, 2022 7:40 PM > >> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam > >> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; > >> michal.simek@xilinx.com > >> Cc: linux-arm-kernel@lists.infradead.org; > >> linux-kernel@vger.kernel.org; git@xilinx.com; joe@perches.com > >> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode > >> for i2c- read > >> > >> [CAUTION: External Email] > >> > >> CAUTION: This message has originated from an External Source. Please > >> use proper judgment and caution when opening attachments, clicking > >> links, or responding to this email. > >> > >> > >> Hi Marek, > >> > >> W dniu 29.06.2022 o 16:05, Marek Vasut pisze: > >>>> [...] > >>>> > >>>> If those two modes only differ in software complexity but we are > >>>> not able to support only the simpler one and we have support for > >>>> the more complicated (standard mode) anyways, we know that > standard > >>>> mode can handle or the cases while dynamic mode cannot, we also > >>>> know that dynamic mode is broken on some versions of the core, why > >>>> do we actually keep support for dynamic mode? > >>> If I recall it right, the dynamic mode was supposed to handle > >>> transfers longer than 255 Bytes, which the core cannot do in > >>> Standard mode. It is needed e.g. by Atmel MXT touch controller. I > >>> spent a lot of time debugging the race conditions in the XIIC, which > >>> I ultimately fixed (the patches are upstream), but the long > >>> transfers I rather fixed in the MXT driver instead. > >>> > >>> I also recall there was supposed to be some update for the XIIC core > >>> coming with newer vivado, but I might be wrong about that. > >> It seems to be the other way around - dynamic mode is limited to 255 > >> bytes - when you trigger dynamic mode you first write the address of > >> the slave to the FIFO, then you write the length as one byte so you > >> can't request more than 255 bytes. So *standard* mode is used for > >> those messages. In other words - dynamic mode is the one that is more > >> limited > >> - everything that you can do in dynamic mode you can also do in > >> standard mode. So why don't we use standard mode always for > everything? > > However the current mode is dynamic mode so for less than 255 we can > > use dynamic mode.(the current behavior will not change) Also the > > dynamic mode is nicer on the processor resources. We set the bytes and > the controller takes care of transferring. > > > > However do not have any strong views open to suggestions. > > All I'm saying is that before this patchset, the dynamic mode was used in all > cases and it made sense - it is easier to work with. But it turned out it has its > limitations and support for standard mode was added with several cases that > switch to that mode. The commit message suggests that the only difference is > in how complicated the code for handling them is, does not say why dynamic > mode might still be preferred. And supporting both of them complicates the > code noticeably. > My understanding now is that the code struggles to use the dynamic mode in > all cases that it can because that produces less interrupts and so it is slightly > lighter on resources. So it is a code complication vs effectiveness tradeoff. > Since this is I2C - a slow bus, I'm not sure it is worth it but also don't have > strong opinion on that. If nothing else, I think it would make sense to update > the commit message a little bit to better explain why it is worth keeping both > modes. Will update the commit message in the next version. > > Krzysztof