Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN

DesignWare

> parameter.
> In which case, if the TX FIFO gets empty and the last command didn't have

"In this case when the..."

> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until

"the controller will..."

> a new command is pushed into the TX FIFO or the transfer is aborted.
> 
> When the dw_apb_i2c is holding SCL low, it cannot be disabled.

"When the controller..."

> The transfer must first be aborted.
> Also, the bus recover won't work because SCL is held low by the master.
> 
> This patch checks if the master is holding SCL low in __i2c_dw_disable

Grep for "This patch" in the Submitting Patches document and fix this
accordingly.

__i2c_dw_disable()

> before trying to disable the IP.
> If SCL is held low, an abort is initiated.
> When the abort is done, the disabling can then proceed.
> 
> This whole situation can happen for instance during SMBUS read data block
> if the slave just responds with "byte count == 0".
> This puts the master in an unrecoverable state, holding SCL low and the
> current __i2c_dw_disable procedure is not working. In this situation

__i2c_dw_disable()

> only a Linux reboot can fix the i2c bus.

If reboot helps, what magic does it do that Linux OS can't repeat in software?
Please, elaborate more.

...

>  	int timeout = 100;
>  	u32 status;
> +	u32 raw_intr_stats;
> +	u32 enable;
> +	bool abort_needed;
> +	bool abort_done = false;

Perhaps reversed xmas tree order?

	bool abort_done = false;
	bool abort_needed;
	u32 raw_intr_stats;
	int timeout = 100;
	u32 status;
	u32 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 {
> +		if (abort_needed && !abort_done) {
> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
> +			continue;

This will exhaust the timeout and below can be run at most once,
is it a problem?

Also it's a tight busyloop, are you sure it's what you need?

> +		}

-- 
With Best Regards,
Andy Shevchenko