| Message ID | 20220614232919.1372621-1-robert.hancock@calian.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: cadence: Change large transfer count reset logic to be unconditional | expand |
[AMD Official Use Only - General] Hi , Thanks for the patch > -----Original Message----- > From: Robert Hancock <robert.hancock@calian.com> > Sent: Wednesday, June 15, 2022 4:59 AM > To: linux-i2c@vger.kernel.org > Cc: Raviteja Narayanam <rna@xlnx.xilinx.com>; Michal Simek > <michals@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>; > wsa@kernel.org; Shubhrajyoti Datta <shubhraj@xilinx.com>; Robert > Hancock <robert.hancock@calian.com> > Subject: [PATCH] i2c: cadence: Change large transfer count reset logic to be > unconditional > > Problems were observed on the Xilinx ZynqMP platform with large I2C reads. > When a read of 277 bytes was performed, the controller NAKed the transfer > after only 252 bytes were transferred and returned an ENXIO error on the > transfer. > Can you help me reproduce the issue what is the command that you used to get the failure.
On Thu, 2022-06-16 at 05:17 +0000, Datta, Shubhrajyoti wrote: > [AMD Official Use Only - General] > > Hi , > Thanks for the patch > > -----Original Message----- > > From: Robert Hancock <robert.hancock@calian.com> > > Sent: Wednesday, June 15, 2022 4:59 AM > > To: linux-i2c@vger.kernel.org > > Cc: Raviteja Narayanam <rna@xlnx.xilinx.com>; Michal Simek > > <michals@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>; > > wsa@kernel.org; Shubhrajyoti Datta <shubhraj@xilinx.com>; Robert > > Hancock <robert.hancock@calian.com> > > Subject: [PATCH] i2c: cadence: Change large transfer count reset logic to > > be > > unconditional > > > > Problems were observed on the Xilinx ZynqMP platform with large I2C reads. > > When a read of 277 bytes was performed, the controller NAKed the transfer > > after only 252 bytes were transferred and returned an ENXIO error on the > > transfer. > > > Can you help me reproduce the issue what is the command that you used to get > the failure. The actual failure I was seeing was in the tests for the PKCS#11 library implementation for the Infineon Optiga Trust M HSM device: https://github.com/Infineon/pkcs11-optiga-trust-m Some of the tests involve reading generated RSA keys from the device and so result in such long I2C reads. However, you could probably use any I2C device which can support such a long read using a command such as: i2ctransfer 0 r300@0x61 for address 0x61. The behavior where only 252 bytes was transferred was seen using a logic analyzer on the I2C bus.
[AMD Official Use Only - General] Hi , > -----Original Message----- > From: Robert Hancock <robert.hancock@calian.com> > Sent: Wednesday, June 15, 2022 4:59 AM > To: linux-i2c@vger.kernel.org > Cc: Raviteja Narayanam <rna@xlnx.xilinx.com>; Michal Simek > <michals@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>; > wsa@kernel.org; Shubhrajyoti Datta <shubhraj@xilinx.com>; Robert > Hancock <robert.hancock@calian.com> > Subject: [PATCH] i2c: cadence: Change large transfer count reset logic to be > unconditional > > Problems were observed on the Xilinx ZynqMP platform with large I2C reads. > When a read of 277 bytes was performed, the controller NAKed the transfer > after only 252 bytes were transferred and returned an ENXIO error on the > transfer. > > There is some code in cdns_i2c_master_isr to handle this case by resetting > the transfer count in the controller before it reaches 0, to allow larger > transfers to work, but it was conditional on the > CDNS_I2C_BROKEN_HOLD_BIT quirk being set on the controller, and ZynqMP > uses the r1p14 version of the core where this quirk is not being set. The > requirement to do this to support larger reads seems like an inherently > required workaround due to the core only having an 8-bit transfer size > register, so it does not appear that this should be conditional on the broken > HOLD bit quirk which is used elsewhere in the driver. > > Remove the dependency on the CDNS_I2C_BROKEN_HOLD_BIT for this > transfer size reset logic to fix this problem. > Reviewed-by Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com> > Fixes: 63cab195bf49 ("i2c: removed work arounds in i2c driver for Zynq > Ultrascale+ MPSoC") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > ---
On 7/14/22 13:58, Datta, Shubhrajyoti wrote: > [AMD Official Use Only - General] > > Hi , > >> -----Original Message----- >> From: Robert Hancock <robert.hancock@calian.com> >> Sent: Wednesday, June 15, 2022 4:59 AM >> To: linux-i2c@vger.kernel.org >> Cc: Raviteja Narayanam <rna@xlnx.xilinx.com>; Michal Simek >> <michals@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>; >> wsa@kernel.org; Shubhrajyoti Datta <shubhraj@xilinx.com>; Robert >> Hancock <robert.hancock@calian.com> >> Subject: [PATCH] i2c: cadence: Change large transfer count reset logic to be >> unconditional >> >> Problems were observed on the Xilinx ZynqMP platform with large I2C reads. >> When a read of 277 bytes was performed, the controller NAKed the transfer >> after only 252 bytes were transferred and returned an ENXIO error on the >> transfer. >> >> There is some code in cdns_i2c_master_isr to handle this case by resetting >> the transfer count in the controller before it reaches 0, to allow larger >> transfers to work, but it was conditional on the >> CDNS_I2C_BROKEN_HOLD_BIT quirk being set on the controller, and ZynqMP >> uses the r1p14 version of the core where this quirk is not being set. The >> requirement to do this to support larger reads seems like an inherently >> required workaround due to the core only having an 8-bit transfer size >> register, so it does not appear that this should be conditional on the broken >> HOLD bit quirk which is used elsewhere in the driver. >> >> Remove the dependency on the CDNS_I2C_BROKEN_HOLD_BIT for this >> transfer size reset logic to fix this problem. >> > Reviewed-by Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com> > >> Fixes: 63cab195bf49 ("i2c: removed work arounds in i2c driver for Zynq >> Ultrascale+ MPSoC") >> Signed-off-by: Robert Hancock <robert.hancock@calian.com> >> --- Acked-by: Michal Simek <michal.simek@amd.com> Thanks, Michal
> Reviewed-by Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com>
Missing ":" here, so tools won't handle it automatically. Suggestion:
use macros to add tags, so there are no typos.
On Tue, Jun 14, 2022 at 05:29:19PM -0600, Robert Hancock wrote: > Problems were observed on the Xilinx ZynqMP platform with large I2C reads. > When a read of 277 bytes was performed, the controller NAKed the transfer > after only 252 bytes were transferred and returned an ENXIO error on the > transfer. > > There is some code in cdns_i2c_master_isr to handle this case by resetting > the transfer count in the controller before it reaches 0, to allow larger > transfers to work, but it was conditional on the CDNS_I2C_BROKEN_HOLD_BIT > quirk being set on the controller, and ZynqMP uses the r1p14 version of > the core where this quirk is not being set. The requirement to do this to > support larger reads seems like an inherently required workaround due to > the core only having an 8-bit transfer size register, so it does not > appear that this should be conditional on the broken HOLD bit quirk which > is used elsewhere in the driver. > > Remove the dependency on the CDNS_I2C_BROKEN_HOLD_BIT for this transfer > size reset logic to fix this problem. > > Fixes: 63cab195bf49 ("i2c: removed work arounds in i2c driver for Zynq Ultrascale+ MPSoC") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index cb0b6cca196f..b3a589afe9f7 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -390,9 +390,9 @@ static irqreturn_t cdns_i2c_slave_isr(void *ptr) */ static irqreturn_t cdns_i2c_master_isr(void *ptr) { - unsigned int isr_status, avail_bytes, updatetx; + unsigned int isr_status, avail_bytes; unsigned int bytes_to_send; - bool hold_quirk; + bool updatetx; struct cdns_i2c *id = ptr; /* Signal completion only after everything is updated */ int done_flag = 0; @@ -412,11 +412,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr) * Check if transfer size register needs to be updated again for a * large data receive operation. */ - updatetx = 0; - if (id->recv_count > id->curr_recv_count) - updatetx = 1; - - hold_quirk = (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT) && updatetx; + updatetx = id->recv_count > id->curr_recv_count; /* When receiving, handle data interrupt and completion interrupt */ if (id->p_recv_buf && @@ -447,7 +443,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr) break; } - if (cdns_is_holdquirk(id, hold_quirk)) + if (cdns_is_holdquirk(id, updatetx)) break; } @@ -458,7 +454,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr) * maintain transfer size non-zero while performing a large * receive operation. */ - if (cdns_is_holdquirk(id, hold_quirk)) { + if (cdns_is_holdquirk(id, updatetx)) { /* wait while fifo is full */ while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) != (id->curr_recv_count - CDNS_I2C_FIFO_DEPTH)) @@ -480,22 +476,6 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr) CDNS_I2C_XFER_SIZE_OFFSET); id->curr_recv_count = id->recv_count; } - } else if (id->recv_count && !hold_quirk && - !id->curr_recv_count) { - - /* Set the slave address in address register*/ - cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK, - CDNS_I2C_ADDR_OFFSET); - - if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) { - cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE, - CDNS_I2C_XFER_SIZE_OFFSET); - id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE; - } else { - cdns_i2c_writereg(id->recv_count, - CDNS_I2C_XFER_SIZE_OFFSET); - id->curr_recv_count = id->recv_count; - } } /* Clear hold (if not repeated start) and signal completion */
Problems were observed on the Xilinx ZynqMP platform with large I2C reads. When a read of 277 bytes was performed, the controller NAKed the transfer after only 252 bytes were transferred and returned an ENXIO error on the transfer. There is some code in cdns_i2c_master_isr to handle this case by resetting the transfer count in the controller before it reaches 0, to allow larger transfers to work, but it was conditional on the CDNS_I2C_BROKEN_HOLD_BIT quirk being set on the controller, and ZynqMP uses the r1p14 version of the core where this quirk is not being set. The requirement to do this to support larger reads seems like an inherently required workaround due to the core only having an 8-bit transfer size register, so it does not appear that this should be conditional on the broken HOLD bit quirk which is used elsewhere in the driver. Remove the dependency on the CDNS_I2C_BROKEN_HOLD_BIT for this transfer size reset logic to fix this problem. Fixes: 63cab195bf49 ("i2c: removed work arounds in i2c driver for Zynq Ultrascale+ MPSoC") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/i2c/busses/i2c-cadence.c | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-)