On Thu, Sep 12, 2024 at 06:32:48PM GMT, Andy Shevchenko wrote: > Thu, Sep 12, 2024 at 02:11:12PM +0800, Kimriver Liu kirjoitti: > > It was observed that issuing ABORT bit (IC_ENABLE[1]) will not > > work when IC_ENABLE is already disabled. > > > > Check if ENABLE bit (IC_ENABLE[0]) is disabled when the controller > > is holding SCL low. If ENABLE bit is disabled, the software need /need/needs/ > > to enable it before trying to issue ABORT bit. otherwise, /ABORT/the ABORT/ > > the controller ignores any write to ABORT bit. > > > > These kernel logs show up whenever an I2C transaction is > > attempted after this failure. > > i2c_designware e95e0000.i2c: timeout waiting for bus ready > > i2c_designware e95e0000.i2c: timeout in disabling adapter > > > The patch can be fix the controller cannot be disabled while > > SCL is held low in ENABLE bit is already disabled. > > There are English words but a complete nonsense together. We could reword this to: The patch fixes the issue where the controller cannot be disabled while SCL is held low if the ENABLE bit is already disabled. > Fix the condition when controller cannot be disabled while SCL /when/where/ > is held low and ENABLE bit is already disabled. /ENABLE/the ENABLE/ If you agree with the changes, I can apply them before merging. > > Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx> Thanks a lot, Andy! I really appreciate your reviews! > ... > > > + if (!(enable & DW_IC_ENABLE_ENABLE)) { > > + regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE); > > + /* > > + * Wait 10 times the signaling period of the highest I2C > > + * transfer supported by the driver (for 400KHz this is > > + * 25us) to ensure the I2C ENABLE bit is already set > > + * as described in the DesignWare I2C databook. > > + */ > > + fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz)); > > > + /* Keep ENABLE bit is already set before Setting ABORT.*/ > > /* Set ENABLE bit before setting ABORT */ > > > > + enable |= DW_IC_ENABLE_ENABLE; > > + } > > ... > > > +/* > > + * This functions waits controller idling before disabling I2C > > s/functions/function/ I can fix it before merging. > > + * When the controller is not in the IDLE state, > > + * MST_ACTIVITY bit (IC_STATUS[5]) is set. > > + * Values: > > + * 0x1 (ACTIVE): Controller not idle > > + * 0x0 (IDLE): Controller is idle > > + * The function is called after returning the end of the current transfer > > + * Returns: > > + * False when controller is in IDLE state. > > + * True when controller is in ACTIVE state. > > + */ > > +static bool i2c_dw_is_controller_active(struct dw_i2c_dev *dev) > > +{ > > + u32 status; > > + > > + regmap_read(dev->map, DW_IC_STATUS, &status); > > + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY)) > > + return false; > > + > > + return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > > + !(status & DW_IC_STATUS_MASTER_ACTIVITY), > > + 1100, 20000) != 0; > > return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > !(status & DW_IC_STATUS_MASTER_ACTIVITY), > 1100, 20000) != 0; > > (in the second line replaced 7 spaces by a single TAB to fix the indentation) I can fix it before merging. Kimriver, if you agree, i can merge this and queue it for the next week's fixes merge request. Andi > > +} > > ... > > > + /* > > + * This happens rarely (~1:500) and is hard to reproduce. Debug trace > > + * showed that IC_STATUS had value of 0x23 when STOP_DET occurred, > > + * if disable IC_ENABLE.ENABLE immediately that can result in > > + * IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low. Check if > > + * controller is still ACTIVE before disabling I2C. > > + */ > > + if (i2c_dw_is_controller_active(dev)) > > + dev_err(dev->dev, "controller active\n"); > > -- > With Best Regards, > Andy Shevchenko > >