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

From: Jarkko Nikula
Date: Wed Aug 30 2023 - 15:29:36 EST


Hi

On 8/22/23 12:02, Yann Sionneau wrote:
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@xxxxxxxxx>
Signed-off-by: Jonathan Borne <jborne@xxxxxxxxx>
Signed-off-by: Yann Sionneau <ysionneau@xxxxxxxxx>
---
V2 -> V3:
* do not rename timeout variable for disabling loop
in order to ease backports
* replace abort loop with regmap_read_poll_timeout()
* remove extra empty line.

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 | 17 +++++++++++++++++
drivers/i2c/busses/i2c-designware-core.h | 3 +++
2 files changed, 20 insertions(+)

This doesn't apply against current code. Looks like your base is older than v6.2? I.e. before commit 966b7d3c738a ("i2c: designware: Align defines in i2c-designware-core.h").

Jarkko