| Message ID | 20240906054250.2745-1-kimriver.liu@siengine.com |
|---|---|
| State | Superseded |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: designware: fix master is holding SCL low while ENABLE bit is disabled | expand |
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@siengine.com> > > --- > > 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@siengine.com/ > --- > 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
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@siengine.com> > > --- > V3->V4: Nice, but the Subject (which is most important part) still has no versioning :-( > 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 ... > + /* > + * 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); And if we use 100kHz? Please, calculate this delay based on the actual speed in use (or about to be in use). > + }
Hi, I am sorry for not replying to questions in time, when I left the office early on Friday. I sincerely apologize to you again. -----邮件原件----- 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 发送时间: 2024年9月6日 19:48 收件人: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> 抄送: jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; andi.shyti@kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org 主题: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled 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@siengine.com> > > --- > V3->V4: >Nice, but the Subject (which is most important part) still has no versioning :-( Thanks or your guidance. ON Monday , I will update the Subject versioning and resend V7 patch: [PATCH V7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled > 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 > V1->comments ... > + /* > + * 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); >And if we use 100kHz? >Please, calculate this delay based on the actual speed in use (or about to be in use). Yes, I will change fsleep(25) to usleep_range(25, 250), for next V7 version. Wait 10 times the signaling period of the highest I2C transfer supported by the driver (for 400KHz this is* 25us) > + } --------------- Kimriver Liu
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; + + /* + * 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..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; +} + 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"); + /* * We must disable the adapter before returning and signaling the end * of the current transfer. Otherwise the hardware might continue
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@siengine.com> --- 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@siengine.com/ --- drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++ drivers/i2c/busses/i2c-designware-master.c | 27 ++++++++++++++++++++++ 2 files changed, 39 insertions(+)