[PATCH] i2c: iproc: handle invalid slave state

[PATCH] i2c: iproc: handle invalid slave state

[roman.bacik]

[-- Attachment #1: Type: text/plain, Size: 7554 bytes --]

From: Roman Bacik <roman.bacik@broadcom.com>

Add the code to handle an invalid state when both bits S_RX_EVENT
(indicating a transaction) and S_START_BUSY (indicating the end
of transaction - transition of START_BUSY from 1 to 0) are set in
the interrupt status register during a slave read.

Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
Fixes: 1ca1b4516088 ("i2c: iproc: handle Master aborted error")
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 133 ++++++++++++++++-------------
 1 file changed, 75 insertions(+), 58 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 05c80680dff4..68438d4e5d73 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -316,26 +316,44 @@ static void bcm_iproc_i2c_slave_init(
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 }
 
-static void bcm_iproc_i2c_check_slave_status(
-	struct bcm_iproc_i2c_dev *iproc_i2c)
+static bool bcm_iproc_i2c_check_slave_status
+	(struct bcm_iproc_i2c_dev *iproc_i2c, u32 status)
 {
 	u32 val;
+	bool recover = false;
 
-	val = iproc_i2c_rd_reg(iproc_i2c, S_CMD_OFFSET);
-	/* status is valid only when START_BUSY is cleared after it was set */
-	if (val & BIT(S_CMD_START_BUSY_SHIFT))
-		return;
+	/* check slave transmit status only if slave is transmitting */
+	if (!iproc_i2c->slave_rx_only) {
+		val = iproc_i2c_rd_reg(iproc_i2c, S_CMD_OFFSET);
+		/* status is valid only when START_BUSY is cleared */
+		if (!(val & BIT(S_CMD_START_BUSY_SHIFT))) {
+			val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
+			if (val == S_CMD_STATUS_TIMEOUT ||
+			    val == S_CMD_STATUS_MASTER_ABORT) {
+				dev_warn(iproc_i2c->device,
+					 (val == S_CMD_STATUS_TIMEOUT) ?
+					 "slave random stretch time timeout\n" :
+					 "Master aborted read transaction\n");
+				recover = true;
+			}
+		}
+	}
+
+	/* RX_EVENT is not valid when START_BUSY is set */
+	if ((status & BIT(IS_S_RX_EVENT_SHIFT)) &&
+	    (status & BIT(IS_S_START_BUSY_SHIFT))) {
+		dev_warn(iproc_i2c->device, "Slave aborted read transaction\n");
+		recover = true;
+	}
 
-	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
-	if (val == S_CMD_STATUS_TIMEOUT || val == S_CMD_STATUS_MASTER_ABORT) {
-		dev_err(iproc_i2c->device, (val == S_CMD_STATUS_TIMEOUT) ?
-			"slave random stretch time timeout\n" :
-			"Master aborted read transaction\n");
+	if (recover) {
 		/* re-initialize i2c for recovery */
 		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
 		bcm_iproc_i2c_slave_init(iproc_i2c, true);
 		bcm_iproc_i2c_enable_disable(iproc_i2c, true);
 	}
+
+	return recover;
 }
 
 static void bcm_iproc_i2c_slave_read(struct bcm_iproc_i2c_dev *iproc_i2c)
@@ -420,48 +438,6 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	u32 val;
 	u8 value;
 
-	/*
-	 * Slave events in case of master-write, master-write-read and,
-	 * master-read
-	 *
-	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
-	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
-	 *                    events
-	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
-	 *                    events or only IS_S_RD_EVENT_SHIFT
-	 *
-	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
-	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
-	 * full. This can happen if Master issues write requests of more than
-	 * 64 bytes.
-	 */
-	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
-	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
-	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
-		/* disable slave interrupts */
-		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-		val &= ~iproc_i2c->slave_int_mask;
-		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
-
-		if (status & BIT(IS_S_RD_EVENT_SHIFT))
-			/* Master-write-read request */
-			iproc_i2c->slave_rx_only = false;
-		else
-			/* Master-write request only */
-			iproc_i2c->slave_rx_only = true;
-
-		/* schedule tasklet to read data later */
-		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
-
-		/*
-		 * clear only IS_S_RX_EVENT_SHIFT and
-		 * IS_S_RX_FIFO_FULL_SHIFT interrupt.
-		 */
-		val = BIT(IS_S_RX_EVENT_SHIFT);
-		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT))
-			val |= BIT(IS_S_RX_FIFO_FULL_SHIFT);
-		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
-	}
 
 	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
 		iproc_i2c->tx_underrun++;
@@ -493,8 +469,9 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 		 * less than PKT_LENGTH bytes were output on the SMBUS
 		 */
 		iproc_i2c->slave_int_mask &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
-		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET,
-				 iproc_i2c->slave_int_mask);
+		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+		val &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 
 		/* End of SMBUS for Master Read */
 		val = BIT(S_TX_WR_STATUS_SHIFT);
@@ -515,9 +492,49 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 				 BIT(IS_S_START_BUSY_SHIFT));
 	}
 
-	/* check slave transmit status only if slave is transmitting */
-	if (!iproc_i2c->slave_rx_only)
-		bcm_iproc_i2c_check_slave_status(iproc_i2c);
+	/* if the controller has been reset, immediately return from the ISR */
+	if (bcm_iproc_i2c_check_slave_status(iproc_i2c, status))
+		return true;
+
+	/*
+	 * Slave events in case of master-write, master-write-read and,
+	 * master-read
+	 *
+	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
+	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events
+	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events or only IS_S_RD_EVENT_SHIFT
+	 *
+	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
+	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
+	 * full. This can happen if Master issues write requests of more than
+	 * 64 bytes.
+	 */
+	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
+		/* disable slave interrupts */
+		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+		val &= ~iproc_i2c->slave_int_mask;
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
+
+		if (status & BIT(IS_S_RD_EVENT_SHIFT))
+			/* Master-write-read request */
+			iproc_i2c->slave_rx_only = false;
+		else
+			/* Master-write request only */
+			iproc_i2c->slave_rx_only = true;
+
+		/* schedule tasklet to read data later */
+		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
+
+		/* clear IS_S_RX_FIFO_FULL_SHIFT interrupt */
+		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
+			val = BIT(IS_S_RX_FIFO_FULL_SHIFT);
+			iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
+		}
+	}
 
 	return true;
 }
-- 
2.41.0


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH] i2c: iproc: handle invalid slave state

[Ray Jui]

[-- Attachment #1: Type: text/plain, Size: 7331 bytes --]

Hi Roman,

On 8/24/2023 2:23 PM, roman.bacik@broadcom.com wrote:
> From: Roman Bacik <roman.bacik@broadcom.com>
> 
> Add the code to handle an invalid state when both bits S_RX_EVENT
> (indicating a transaction) and S_START_BUSY (indicating the end
> of transaction - transition of START_BUSY from 1 to 0) are set in
> the interrupt status register during a slave read.
> 
> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> Fixes: 1ca1b4516088 ("i2c: iproc: handle Master aborted error")
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 133 ++++++++++++++++-------------
>  1 file changed, 75 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..68438d4e5d73 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -316,26 +316,44 @@ static void bcm_iproc_i2c_slave_init(
>  	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
>  }
>  
> -static void bcm_iproc_i2c_check_slave_status(
> -	struct bcm_iproc_i2c_dev *iproc_i2c)
> +static bool bcm_iproc_i2c_check_slave_status
> +	(struct bcm_iproc_i2c_dev *iproc_i2c, u32 status)
>  {
>  	u32 val;
> +	bool recover = false;
>  
> -	val = iproc_i2c_rd_reg(iproc_i2c, S_CMD_OFFSET);
> -	/* status is valid only when START_BUSY is cleared after it was set */
> -	if (val & BIT(S_CMD_START_BUSY_SHIFT))
> -		return;
> +	/* check slave transmit status only if slave is transmitting */
> +	if (!iproc_i2c->slave_rx_only) {
> +		val = iproc_i2c_rd_reg(iproc_i2c, S_CMD_OFFSET);
> +		/* status is valid only when START_BUSY is cleared */
> +		if (!(val & BIT(S_CMD_START_BUSY_SHIFT))) {
> +			val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
> +			if (val == S_CMD_STATUS_TIMEOUT ||
> +			    val == S_CMD_STATUS_MASTER_ABORT) {
> +				dev_warn(iproc_i2c->device,
> +					 (val == S_CMD_STATUS_TIMEOUT) ?
> +					 "slave random stretch time timeout\n" :
> +					 "Master aborted read transaction\n");
> +				recover = true;
> +			}
> +		}
> +	}
> +
> +	/* RX_EVENT is not valid when START_BUSY is set */
> +	if ((status & BIT(IS_S_RX_EVENT_SHIFT)) &&
> +	    (status & BIT(IS_S_START_BUSY_SHIFT))) {
> +		dev_warn(iproc_i2c->device, "Slave aborted read transaction\n");
> +		recover = true;
> +	}
>  
> -	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
> -	if (val == S_CMD_STATUS_TIMEOUT || val == S_CMD_STATUS_MASTER_ABORT) {
> -		dev_err(iproc_i2c->device, (val == S_CMD_STATUS_TIMEOUT) ?
> -			"slave random stretch time timeout\n" :
> -			"Master aborted read transaction\n");
> +	if (recover) {
>  		/* re-initialize i2c for recovery */
>  		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
>  		bcm_iproc_i2c_slave_init(iproc_i2c, true);
>  		bcm_iproc_i2c_enable_disable(iproc_i2c, true);
>  	}
> +
> +	return recover;
>  }
>  
>  static void bcm_iproc_i2c_slave_read(struct bcm_iproc_i2c_dev *iproc_i2c)
> @@ -420,48 +438,6 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
>  	u32 val;
>  	u8 value;
>  
> -	/*
> -	 * Slave events in case of master-write, master-write-read and,
> -	 * master-read
> -	 *
> -	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
> -	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> -	 *                    events
> -	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> -	 *                    events or only IS_S_RD_EVENT_SHIFT
> -	 *
> -	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
> -	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
> -	 * full. This can happen if Master issues write requests of more than
> -	 * 64 bytes.
> -	 */
> -	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
> -	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
> -	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
> -		/* disable slave interrupts */
> -		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> -		val &= ~iproc_i2c->slave_int_mask;
> -		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
> -
> -		if (status & BIT(IS_S_RD_EVENT_SHIFT))
> -			/* Master-write-read request */
> -			iproc_i2c->slave_rx_only = false;
> -		else
> -			/* Master-write request only */
> -			iproc_i2c->slave_rx_only = true;
> -
> -		/* schedule tasklet to read data later */
> -		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
> -
> -		/*
> -		 * clear only IS_S_RX_EVENT_SHIFT and
> -		 * IS_S_RX_FIFO_FULL_SHIFT interrupt.
> -		 */
> -		val = BIT(IS_S_RX_EVENT_SHIFT);
> -		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT))
> -			val |= BIT(IS_S_RX_FIFO_FULL_SHIFT);
> -		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
> -	}
>  
>  	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
>  		iproc_i2c->tx_underrun++;
> @@ -493,8 +469,9 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
>  		 * less than PKT_LENGTH bytes were output on the SMBUS
>  		 */
>  		iproc_i2c->slave_int_mask &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
> -		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET,
> -				 iproc_i2c->slave_int_mask);
> +		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> +		val &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
> +		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
>  
>  		/* End of SMBUS for Master Read */
>  		val = BIT(S_TX_WR_STATUS_SHIFT);
> @@ -515,9 +492,49 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
>  				 BIT(IS_S_START_BUSY_SHIFT));
>  	}
>  
> -	/* check slave transmit status only if slave is transmitting */
> -	if (!iproc_i2c->slave_rx_only)
> -		bcm_iproc_i2c_check_slave_status(iproc_i2c);
> +	/* if the controller has been reset, immediately return from the ISR */
> +	if (bcm_iproc_i2c_check_slave_status(iproc_i2c, status))
> +		return true;
> +
> +	/*
> +	 * Slave events in case of master-write, master-write-read and,
> +	 * master-read
> +	 *
> +	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
> +	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> +	 *                    events
> +	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> +	 *                    events or only IS_S_RD_EVENT_SHIFT
> +	 *
> +	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
> +	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
> +	 * full. This can happen if Master issues write requests of more than
> +	 * 64 bytes.
> +	 */
> +	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
> +	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
> +	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
> +		/* disable slave interrupts */
> +		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> +		val &= ~iproc_i2c->slave_int_mask;
> +		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
> +
> +		if (status & BIT(IS_S_RD_EVENT_SHIFT))
> +			/* Master-write-read request */
> +			iproc_i2c->slave_rx_only = false;
> +		else
> +			/* Master-write request only */
> +			iproc_i2c->slave_rx_only = true;
> +
> +		/* schedule tasklet to read data later */
> +		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
> +
> +		/* clear IS_S_RX_FIFO_FULL_SHIFT interrupt */
> +		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
> +			val = BIT(IS_S_RX_FIFO_FULL_SHIFT);
> +			iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
> +		}
> +	}
>  
>  	return true;
>  }

This fix looks good to me, and we have reviewed and thoroughly tested it
internally. Thanks.

Acked-by: Ray Jui <ray.jui@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]

Re: [PATCH] i2c: iproc: handle invalid slave state

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Thu, Aug 24, 2023 at 02:23:51PM -0700, roman.bacik@broadcom.com wrote:
> From: Roman Bacik <roman.bacik@broadcom.com>
> 
> Add the code to handle an invalid state when both bits S_RX_EVENT
> (indicating a transaction) and S_START_BUSY (indicating the end
> of transaction - transition of START_BUSY from 1 to 0) are set in
> the interrupt status register during a slave read.
> 
> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> Fixes: 1ca1b4516088 ("i2c: iproc: handle Master aborted error")

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: iproc: handle invalid slave state

[Vijay Balakrishna]

On 8/28/2023 12:10 PM, Wolfram Sang wrote:
> On Thu, Aug 24, 2023 at 02:23:51PM -0700, roman.bacik@broadcom.com wrote:
>> From: Roman Bacik <roman.bacik@broadcom.com>
>>
>> Add the code to handle an invalid state when both bits S_RX_EVENT
>> (indicating a transaction) and S_START_BUSY (indicating the end
>> of transaction - transition of START_BUSY from 1 to 0) are set in
>> the interrupt status register during a slave read.
>>
>> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
>> Fixes: 1ca1b4516088 ("i2c: iproc: handle Master aborted error")
> 
> Applied to for-next, thanks!
> 

Hi Wolfram,

I don't see this patch neither in I2C for-next nor in linux-next.  May 
be got lost by accident, please update.

Thanks,
Vijay

Re: [PATCH] i2c: iproc: handle invalid slave state

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

Hi Vijay,

> I don't see this patch neither in I2C for-next nor in linux-next.  May be
> got lost by accident, please update.

Yes, I am sorry. This likely got lost somewhere because of an error on
my side. I'll apply it again. Thank you a lot for notifying me!

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]