Thanks for your suggestion. I will revise it according to your suggestions
and resend the patch.
Best Regards
-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
发送时间: 2024年9月4日 20:55
收件人: Liu Kimriver/刘金河 <kimriver.liu@xxxxxxxxxxxx>
抄送: jarkko.nikula@xxxxxxxxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx; jsd@xxxxxxxxxxxx; andi.shyti@xxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
主题: 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@xxxxxxxxxxxx>
>
> 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
