Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

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

On Wed, Sep 04, 2024 at 02:42:24PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>
> 
> Failure in normal Stop operational path
> 
> This failure happens rarely and is hard to reproduce. Debug
> trace showed that IC_STATUS had value of 0x23 when STOP_DET
> occurred, immediately disable ENABLE bit that can result in
> IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
> 
> Failure in ENABLE bit is disabled path
> 
> It was observed that master is holding SCL low and the
> IC_ENABLE is already disabled, Enable ABORT bit and
> ENABLE bit simultaneously cannot take effect.
> 
> Check if the master is holding SCL low after ENABLE bit is
> already disabled. If SCL is held low, The software can set
> this ABORT bit only when ENABLE is already set，otherwise,
> the controller ignores any write to ABORT bit. When the
> abort is done, then proceed with disabling the controller.
> 
> These kernel logs show up whenever an I2C transaction is
> attempted after this failure.
> i2c_designware e95e0000.i2c: timeout in disabling adapter
> i2c_designware e95e0000.i2c: timeout waiting for bus ready
> 
> The patch can be fix the controller cannot be disabled while
> SCL is held low in ENABLE bit is already disabled.

...

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!enable) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;

> +			usleep_range(25, 100);

fsleep()

And add a short comment to explain the chosen value.

> +		}

...

> +static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev)
> +{
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
> +		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +				!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +				1100, 20000);
> +		if (ret)
> +			dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);
> +	}
> +
> +	return ret;

This can be rewritten as

	u32 status = 0;
	int ret;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!status & DW_IC_STATUS_MASTER_ACTIVITY))
		return 0;

	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
			1100, 20000);
	if (ret)
		dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);

	return ret;

> +}

...

> +	ret = i2c_dw_check_mst_activity(dev);
> +	if (!ret)
> +		__i2c_dw_disable_nowait(dev);

...but looking at the usage, I think the proper is to have the above to return
boolean. And also update the name to follow the usual pattern for boolean helpers.

static bool i2c_dw_is_mst_idling(struct dw_i2c_dev *dev)
...
	if (i2c_dw_is_mst_idling(dev))
		__i2c_dw_disable_nowait(dev);

...

Also what does the heck "mst" stand for? Please, use decrypted words in
function names and error messages..

-- 
With Best Regards,
Andy Shevchenko