diff mbox series

i2c: cadence: Change large transfer count reset logic to be unconditional

Message ID 20220614232919.1372621-1-robert.hancock@calian.com
State Accepted
Commit 4ca8ca873d454635c20d508261bfc0081af75cf8
Headers show
Series i2c: cadence: Change large transfer count reset logic to be unconditional | expand

Commit Message

Robert Hancock June 14, 2022, 11:29 p.m. UTC
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(-)

Comments

Shubhrajyoti Datta June 16, 2022, 5:17 a.m. UTC | #1
[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.
Robert Hancock June 16, 2022, 5:03 p.m. UTC | #2
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.
Shubhrajyoti Datta July 14, 2022, 11:58 a.m. UTC | #3
[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>
> ---
Michal Simek July 14, 2022, 12:35 p.m. UTC | #4
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
Wolfram Sang July 16, 2022, 12:46 p.m. UTC | #5
> 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.
Wolfram Sang July 16, 2022, 12:46 p.m. UTC | #6
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 mbox series

Patch

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