Hi, On Fri, Sep 06, 2024 at 01:42:50PM +0800, Kimriver Liu wrote: > It was observed 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 master is > holding SCL low. If ENABLE bit is disabled, the software need > enable it before trying to issue ABORT bit. otherwise, > the controller ignores any write to ABORT bit > > Signed-off-by: Kimriver Liu <kimriver.liu@xxxxxxxxxxxx> > > --- > > V3->V4: > 1. update commit messages and add patch version and changelog > 2. move print the error message in i2c_dw_xfer > V2->V3: change (!enable) to (!(enable & DW_IC_ENABLE_ENABLE)) > V1->V2: used standard words in function names and addressed review comments > > link to V1: > https://lore.kernel.org/lkml/20240904064224.2394-1-kimriver.liu@xxxxxxxxxxxx/ > --- > drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++ > drivers/i2c/busses/i2c-designware-master.c | 27 ++++++++++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c > index e8a688d04aee..54acf8554582 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -453,6 +453,18 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev) > > abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD; > if (abort_needed) { > + if (!(enable & DW_IC_ENABLE_ENABLE)) { > + regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE); > + enable |= DW_IC_ENABLE_ENABLE; > + Drop the empty line here. > + /* > + * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit i2c -> I2C > + * is already set by the driver (for 400KHz this is 25us) > + * as described in the DesignWare I2C databook. > + */ > + fsleep(25); > + } > + > regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT); > ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable, > !(enable & DW_IC_ENABLE_ABORT), 10, > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > index c7e56002809a..6a053f3b5501 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -253,6 +253,24 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) > __i2c_dw_write_intr_mask(dev, DW_IC_INTR_MASTER_MASK); > } > > +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev) > +{ > + u32 status; > + int ret; > + > + regmap_read(dev->map, DW_IC_STATUS, &status); > + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY)) > + return true; > + > + ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > + !(status & DW_IC_STATUS_MASTER_ACTIVITY), > + 1100, 20000); > + if (ret) > + return false; > + > + return true; You can also do: return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, !(status & DW_IC_STATUS_MASTER_ACTIVITY), 1100, 20000); But this is already done in i2c_dw_wait_bus_not_busy() (reverse of this) so perhaps consolidate them? > +} > + > static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev) > { > u32 val; > @@ -788,6 +806,15 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > goto done; > } > > + /* > + * This happens rarely 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. > + */ > + if (!i2c_dw_is_master_idling(dev)) > + dev_err(dev->dev, "i2c master controller not idle\n"); i2c -> I2C (the driver log messages are not too consistent though, something to improve). > + > /* > * We must disable the adapter before returning and signaling the end > * of the current transfer. Otherwise the hardware might continue > -- > 2.17.1
