[PATCH v2] i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low

[PATCH v2] i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low

[Yann Sionneau]

From: Yann Sionneau <ysionneau@kalray.eu>

The DesignWare IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
parameter.
In this case, when the TX FIFO gets empty and the last command didn't have
the STOP bit (IC_DATA_CMD[9]), the controller will hold SCL low until
a new command is pushed into the TX FIFO or the transfer is aborted.

When the controller is holding SCL low, it cannot be disabled.
The transfer must first be aborted.
Also, the bus recovery won't work because SCL is held low by the master.

Check if the master is holding SCL low in __i2c_dw_disable() before trying
to disable the controller. If SCL is held low, an abort is initiated.
When the abort is done, then proceed with disabling the controller.

This whole situation can happen for instance during SMBus read data block
if the slave just responds with "byte count == 0".
This puts the driver in an unrecoverable state, because the controller is
holding SCL low and the current __i2c_dw_disable() procedure is not
working. In this situation only a SoC reset can fix the i2c bus.

Co-developed-by: Jonathan Borne <jborne@kalray.eu>
Signed-off-by: Jonathan Borne <jborne@kalray.eu>
Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
---
V1 -> V2:
* use reverse christmas tree order for variable declarations
* use unsigned int type instead of u32 for regmap_read
* give its own loop to the abort procedure with its own timeout
* print an error message if the abort never finishes during the timeout
* rename the timeout variable for the controller disabling loop
* with the usleep_range(10, 20) it takes only 1 loop iteration.
Without it takes 75 iterations and with udelay(1) it takes 16
loop iterations.


 drivers/i2c/busses/i2c-designware-common.c | 27 ++++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-core.h   |  3 +++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 9f8574320eb2..2e46aa398f4e 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -438,9 +438,32 @@ int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
 
 void __i2c_dw_disable(struct dw_i2c_dev *dev)
 {
-	int timeout = 100;
+	unsigned int raw_intr_stats;
+	bool abort_done = false;
+	int abort_timeout = 100;
+	int dis_timeout = 100;
+	unsigned int enable;
+	bool abort_needed;
 	u32 status;
 
+	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
+	regmap_read(dev->map, DW_IC_ENABLE, &enable);
+
+	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
+
+	if (abort_needed) {
+		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
+
+		do {
+			regmap_read(dev->map, DW_IC_ENABLE, &enable);
+			abort_done = !(enable & DW_IC_ENABLE_ABORT);
+			usleep_range(10, 20);
+		} while (!abort_done && abort_timeout--);
+
+		if (!abort_done)
+			dev_err(dev->dev, "timeout while trying to abort current transfer\n");
+	}
+
 	do {
 		__i2c_dw_disable_nowait(dev);
 		/*
@@ -457,7 +480,7 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
 		 * 25us) as described in the DesignWare I2C databook.
 		 */
 		usleep_range(25, 250);
-	} while (timeout--);
+	} while (dis_timeout--);
 
 	dev_warn(dev->dev, "timeout in disabling adapter\n");
 }
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 19ae23575945..dcd9bd9ee00f 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -98,6 +98,7 @@
 #define DW_IC_INTR_START_DET	BIT(10)
 #define DW_IC_INTR_GEN_CALL	BIT(11)
 #define DW_IC_INTR_RESTART_DET	BIT(12)
+#define DW_IC_INTR_MST_ON_HOLD	BIT(13)
 
 #define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
 					 DW_IC_INTR_TX_ABRT | \
@@ -109,6 +110,8 @@
 					 DW_IC_INTR_RX_UNDER | \
 					 DW_IC_INTR_RD_REQ)
 
+#define DW_IC_ENABLE_ABORT		BIT(1)
+
 #define DW_IC_STATUS_ACTIVITY		BIT(0)
 #define DW_IC_STATUS_TFE		BIT(2)
 #define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
-- 
2.17.1

Re: [PATCH v2] i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 04:01:03PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> The DesignWare IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
> parameter.
> In this case, when the TX FIFO gets empty and the last command didn't have
> the STOP bit (IC_DATA_CMD[9]), the controller will hold SCL low until
> a new command is pushed into the TX FIFO or the transfer is aborted.
> 
> When the controller is holding SCL low, it cannot be disabled.
> The transfer must first be aborted.
> Also, the bus recovery won't work because SCL is held low by the master.
> 
> Check if the master is holding SCL low in __i2c_dw_disable() before trying
> to disable the controller. If SCL is held low, an abort is initiated.
> When the abort is done, then proceed with disabling the controller.
> 
> This whole situation can happen for instance during SMBus read data block
> if the slave just responds with "byte count == 0".
> This puts the driver in an unrecoverable state, because the controller is
> holding SCL low and the current __i2c_dw_disable() procedure is not
> working. In this situation only a SoC reset can fix the i2c bus.

Thank you for an update!
My comments below.

...

>  void __i2c_dw_disable(struct dw_i2c_dev *dev)
>  {
> -	int timeout = 100;

I would leave this untouched to make patch less invasive and
easier to backport.

> +	unsigned int raw_intr_stats;
> +	bool abort_done = false;
> +	int abort_timeout = 100;
> +	int dis_timeout = 100;
> +	unsigned int enable;
> +	bool abort_needed;
>  	u32 status;
>  
> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
> +	regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +
> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;

> +

This blank line is not needed.

> +	if (abort_needed) {
> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);

> +		do {
> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
> +			usleep_range(10, 20);
> +		} while (!abort_done && abort_timeout--);

Now as you split this loop, it may be replaced by regmap_read_poll_timeout()
call.

> +		if (!abort_done)
> +			dev_err(dev->dev, "timeout while trying to abort current transfer\n");
> +	}

...

Other than above, looks good to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low

[Yann Sionneau]

Hi,

On 8/21/23 17:32, Andy Shevchenko wrote:
> On Mon, Aug 21, 2023 at 04:01:03PM +0200, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@kalray.eu>
>>
>> The DesignWare IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
>> parameter.
>> In this case, when the TX FIFO gets empty and the last command didn't have
>> the STOP bit (IC_DATA_CMD[9]), the controller will hold SCL low until
>> a new command is pushed into the TX FIFO or the transfer is aborted.
>>
>> When the controller is holding SCL low, it cannot be disabled.
>> The transfer must first be aborted.
>> Also, the bus recovery won't work because SCL is held low by the master.
>>
>> Check if the master is holding SCL low in __i2c_dw_disable() before trying
>> to disable the controller. If SCL is held low, an abort is initiated.
>> When the abort is done, then proceed with disabling the controller.
>>
>> This whole situation can happen for instance during SMBus read data block
>> if the slave just responds with "byte count == 0".
>> This puts the driver in an unrecoverable state, because the controller is
>> holding SCL low and the current __i2c_dw_disable() procedure is not
>> working. In this situation only a SoC reset can fix the i2c bus.
> Thank you for an update!
> My comments below.
>
> ...
>
>>   void __i2c_dw_disable(struct dw_i2c_dev *dev)
>>   {
>> -	int timeout = 100;
> I would leave this untouched to make patch less invasive and
> easier to backport.
Ack
>
>> +	unsigned int raw_intr_stats;
>> +	bool abort_done = false;
>> +	int abort_timeout = 100;
>> +	int dis_timeout = 100;
>> +	unsigned int enable;
>> +	bool abort_needed;
>>   	u32 status;
>>   
>> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
>> +	regmap_read(dev->map, DW_IC_ENABLE, &enable);
>> +
>> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>> +
> This blank line is not needed.
Ack
>
>> +	if (abort_needed) {
>> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>> +		do {
>> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
>> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
>> +			usleep_range(10, 20);
>> +		} while (!abort_done && abort_timeout--);
> Now as you split this loop, it may be replaced by regmap_read_poll_timeout()
> call.
Ah yes, good idea, thanks!
>> +		if (!abort_done)
>> +			dev_err(dev->dev, "timeout while trying to abort current transfer\n");
>> +	}
> ...
>
> Other than above, looks good to me.
>
Very cool, thanks for the review!

-- 

Yann