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

["Liu Kimriver/刘金河" <kimriver.liu@siengine.com>]

	Thanks for your suggestion. I will revise it according to your suggestions 
and resend the patch.

Best Regards

-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
发送时间: 2024年9月4日 20:55
收件人: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
抄送: jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; andi.shyti@kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

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