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
