Re: [PATCH] i2c: designware: Release slave SCL hold in __i2c_dw_disable()

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Wed, Nov 04, 2026 at 01:37:45PM +0000, Atif Ashhar wrote:
> When an external I2C master initiates a read to the Synopsys DW_apb_i2c
> adapter in slave mode during kexec teardown, runtime suspend, or driver
> unbind, the slave hardware stretches SCL low (0V) waiting for TX data,
> setting DW_IC_STATUS_SLAVE_HOLD_TX_FIFO_EMPTY.
> 
> In master mode, __i2c_dw_disable() issues DW_IC_ENABLE_ABORT to release
> the bus. However, according to the Synopsys DW_apb_i2c Databook, the
> ABORT bit applies only to master transfers and has no effect in slave
> mode. Because the slave cannot disable while stretching SCL,
> __i2c_dw_disable() times out, leaving DW_IC_ENABLE_STATUS[0] = 1 across
> kexec or rebind. Subsequent writes to DW_IC_SAR are write-protected and
> silently dropped by hardware, wedging the slave and blocking any other
> devices sharing the physical bus.
> 
> Fix this by:
> 
> 1. In __i2c_dw_disable(), if DW_IC_STATUS_SLAVE_HOLD_TX_FIFO_EMPTY is
>    set while disabling, temporarily re-enable the controller so the APB
>    interface accepts data commands without racing with the slave ISR,
>    clear pending RD_REQ/TX_ABRT interrupts, write a dummy 0xFF byte to
>    DW_IC_DATA_CMD to release the SCL stretch, and immediately request
>    disable via __i2c_dw_disable_nowait(). Also drain the RX FIFO if RFNE
>    is set during SLAVE_ACTIVITY.
> 
> 2. In i2c_dw_reg_slave(), replace __i2c_dw_disable_nowait() with
>    __i2c_dw_disable() to ensure the controller is fully disabled before
>    calling i2c_dw_set_mode(), which programs DW_IC_SAR and DW_IC_CON.
> 
> Fixes: 9f3e065c54b0 ("i2c: designware: add SLAVE mode functions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Atif Ashhar <atifashhar@google.com>
> ---
>  drivers/i2c/busses/i2c-designware-common.c | 46 ++++++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-slave.c  |  2 +-
>  include/linux/designware_i2c.h             |  1 +
>  3 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index a1eca6cd4b75..ae737928820c 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -619,6 +619,50 @@ static int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
>  	return ret;
>  }
>  
> +/*
> + * DW_IC_ENABLE_ABORT only aborts master transfers. If the adapter is acting
> + * as a slave and holding SCL low waiting for TX data (SLAVE_HOLD_TX_FIFO_EMPTY),
> + * hardware refuses to disable (IC_ENABLE_STATUS[0] stays 1) until a byte is
> + * written to DW_IC_DATA_CMD.
> + *
> + * Mask interrupts and temporarily set DW_IC_ENABLE_ENABLE so the APB interface
> + * accepts writes to the TX/RX FIFOs without racing with the slave ISR, clear
> + * pending RD_REQ/TX_ABRT interrupts, feed a dummy 0xFF byte to release SCL
> + * (or drain any pending RX byte), and immediately request disable again via
> + * __i2c_dw_disable_nowait() so the controller initiates disable upon
> + * completing the current byte.
> + */
> +static void i2c_dw_release_slave_hold(struct dw_i2c_dev *dev)
> +{
> +	unsigned int status, dummy;
> +	bool tx_hold, rx_pending;
> +
> +	if (!IS_ENABLED(CONFIG_I2C_DESIGNWARE_SLAVE) || dev->mode != DW_IC_SLAVE)
> +		return;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +
> +	tx_hold = status & DW_IC_STATUS_SLAVE_HOLD_TX_FIFO_EMPTY;
> +	rx_pending = (status & DW_IC_STATUS_SLAVE_ACTIVITY) &&
> +		     (status & DW_IC_STATUS_RFNE);
> +
> +	if (!tx_hold && !rx_pending)
> +		return;
> +
> +	regmap_write(dev->map, DW_IC_INTR_MASK, 0);
> +	regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +
> +	if (tx_hold) {
> +		regmap_read(dev->map, DW_IC_CLR_RD_REQ, &dummy);
> +		regmap_read(dev->map, DW_IC_CLR_TX_ABRT, &dummy);
> +		regmap_write(dev->map, DW_IC_DATA_CMD, 0xFF);

I'm not super familiar with the slave mode so adding Heikki just in case.
Can this cause some unintended traffic on the bus towards the master?

Also use 0xff instead.

> +	}
> +	if (rx_pending)
> +		regmap_read(dev->map, DW_IC_DATA_CMD, &dummy);
> +
> +	__i2c_dw_disable_nowait(dev);
> +}
> +
>  void __i2c_dw_disable(struct dw_i2c_dev *dev)
>  {
>  	struct i2c_timings *t = &dev->timings;
> @@ -676,6 +720,8 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
>  		if (!(status & 1))
>  			return;
>  
> +		i2c_dw_release_slave_hold(dev);
> +
>  		/*
>  		 * Wait 10 times the signaling period of the highest I2C
>  		 * transfer supported by the driver (for 400kHz this is
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
> index 0abcc7757b23..e51a6b60292a 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -40,7 +40,7 @@ int i2c_dw_reg_slave(struct i2c_client *slave)
>  		return ret;
>  
>  	pm_runtime_get_sync(dev->dev);
> -	__i2c_dw_disable_nowait(dev);
> +	__i2c_dw_disable(dev);
>  	dev->slave = slave;
>  	i2c_dw_set_mode(dev, DW_IC_SLAVE);
>  
> diff --git a/include/linux/designware_i2c.h b/include/linux/designware_i2c.h
> index 53f37f18a722..113a9d84e34d 100644
> --- a/include/linux/designware_i2c.h
> +++ b/include/linux/designware_i2c.h
> @@ -100,6 +100,7 @@
>  #define DW_IC_STATUS_MASTER_ACTIVITY		BIT(5)
>  #define DW_IC_STATUS_SLAVE_ACTIVITY		BIT(6)
>  #define DW_IC_STATUS_MASTER_HOLD_TX_FIFO_EMPTY	BIT(7)
> +#define DW_IC_STATUS_SLAVE_HOLD_TX_FIFO_EMPTY	BIT(9)
>  
>  /* DW_IC_SMBUS_INTR_* bits */
>  #define DW_IC_SMBUS_INTR_ALERT			BIT(10)
> -- 
> 2.56.0.317.g454f8302e1-goog