Hi I am sorry for not replying to questions in time on Friday. -----邮件原件----- 发件人: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> 发送时间: 2024年9月6日 16:08 收件人: Liu Kimriver/刘金河 <kimriver.liu@xxxxxxxxxxxx> 抄送: jarkko.nikula@xxxxxxxxxxxxxxx; andriy.shevchenko@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 Hi, On Fri, Sep 06, 2024 at 03:47:31PM +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> > > --- > V5->V6: restore i2c_dw_is_master_idling() function checking > V4->V5: delete master idling checking > 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 | 11 +++++++++++ > drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c > index e8a688d04aee..2b3398cd4382 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -453,6 +453,17 @@ 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; > + /* > + * Wait two ic_clk delay when enabling the I2C to ensure ENABLE bit > + * 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..132b7237c004 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -253,6 +253,19 @@ 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; > + > + regmap_read(dev->map, DW_IC_STATUS, &status); > + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY)) > + return true; > + > + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > + !(status & DW_IC_STATUS_MASTER_ACTIVITY), > + 1100, 20000); > +} >Yeah, I now realize that i2c_dw_wait_bus_not_busy() checks for >DW_IC_STATUS_ACTIVITY not for DW_IC_STATUS_MASTER_ACTIVITY as I thought >so consolidating them makes not that much sense. >This looks good to me, Thanks. This case happens rarely and is hard to reproduce. We reproduce this issue in RTL simulation. It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling before disabling I2C when I2C transfer completed. as described in the DesignWare I2C databook(Flowchart for DW_apb_i2c Controller) ----------------- Best Regards Kimriver Liu
