Issues with large reads on Cadence I2C on ZynqMP

Issues with large reads on Cadence I2C on ZynqMP

[Robert Hancock]

We have run into some problems on a Xilinx ZynqMP platform when accessing an
Infineon Optiga Trust-M HSM device over I2C using the PS I2C interface which
uses the Cadence I2C driver. The userspace interface library for this device
sometimes does fairly large reads, up to 277 bytes. When this happens, the
behavior on the I2C bus is that the controller NAKs the read after 252 bytes
are read, and an ENXIO error is returned to userspace. The library code treats
this as a transfer NAKed by the device (which is what that return code should
mean), not a premature termination by the controller, and thus keeps retrying
the transfer with the same result until a timeout is reached.

It looks like this behavior is exactly what this patch back in 2014 was
originally addressing:

commit 9fae82e1acda8d4a6881be63cc38521b84007ba9
Author: Harini Katakam <harinik@xilinx.com>
Date:   Fri Dec 12 09:48:26 2014 +0530

    i2c: cadence: Handle > 252 byte transfers
    
    The I2C controller sends a NACK to the slave when transfer size register
    reaches zero, irrespective of the hold bit. So, in order to handle
transfers
    greater than 252 bytes, the transfer size register has to be maintained at
a
    value >= 1. This patch implements the same.
    The interrupt status is cleared at the beginning of the isr instead of
    the end, to avoid missing any interrupts.
    
    Signed-off-by: Harini Katakam <harinik@xilinx.com>
    [wsa: added braces around else branch]
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

However, when ZynqMP support was introduced, it was apparently thought that
this wasn't needed:

commit 63cab195bf498676619951e81ad5791e9d47c420
Author: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Date:   Fri Jul 10 20:10:14 2015 +0530

    i2c: removed work arounds in i2c driver for Zynq Ultrascale+ MPSoC
    
    Cadence 1.0 version has bugs which have been fixed in the cadence 1.4
version.
    This patch removes the quirks present in the driver for cadence 1.4
version.
    
    Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.com>
    [wsa: fixed indentation issues in r1p10_i2c_def]
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

It seems like there may be multiple issues that this "broken hold bit" quirk
was intended to address:

-need to clear HOLD bit before transfer size reaches 0
-lack of completion interrupt after master receive if HOLD bit is set
-need to prevent transfer size reaching 0 in the middle of a long (over 252
byte transfer)

I am guessing the behavior for the first two issues may be different between
the 1.0 and 1.4 versions of the core, but the third issue may be unchanged. It
is hard for me to say for sure without knowing exactly what was changed between
these versions (as far as I know, these documents are not public). However, it
appears that changing the code in cdns_i2c_master_isr so that this behavior is
not conditional on the CDNS_I2C_BROKEN_HOLD_BIT, but leaving the checks on that
bit in other places in the driver, does fix the problem.

Looking at what the code for that quirk is doing, I am not sure the need to do
this can even be considered a bug in the core - as the transfer size register
is only 8 bits, it seems likely it wasn't really designed to work with
transfers over 256 bytes, and this method of achieving that (waiting until the
core is idle, due to the FIFO being full, before the transfer length is
exhausted, and then replenishing the transfer length before continuing the
transfer) is basically a workaround for that limitation. So I am not sure it
makes sense that this would have been somehow fixed in the 1.4 version. From
the code that executes when "broken HOLD" is not set in this function, it seems
to just expect that resetting the slave address and transfer size will be able
to continue the transfer after the controller thinks it is done, which may not
be reasonable.

Does this change, to make the transfer length reset behavior unconditional,
seem like a reasonable approach? If so I can submit a patch implementing this.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com