Message ID | 4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1726197817.git.kimriver.liu@siengine.com |
---|---|
State | Superseded |
Headers | show |
Series | [v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled | expand |
Hi Kimriver, On Fri, Sep 13, 2024 at 11:31:46AM GMT, Kimriver Liu wrote: > It was observed that issuing the ABORT bit (IC_ENABLE[1]) will not > work when IC_ENABLE is already disabled. > > Check if the ENABLE bit (IC_ENABLE[0]) is disabled when the controller > is holding SCL low. If the ENABLE bit is disabled, the software needs > to enable it before trying to issue the ABORT bit. otherwise, > 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 fixes the issue where the controller cannot be disabled > while SCL is held low if the ENABLE bit is already disabled. > > Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low") > Signed-off-by: Kimriver Liu <kimriver.liu@siengine.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Reviewed-by: Andy Shevchenko <andy@kernel.org> I'm sorry for the delay, but I needed to wait for the previous batch of fixes to be merged. [...] > +/* > + * This function waits controller idling before disabling I2C > + * 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. > + */ I took the liberty of making some small changes to the comment: +/* + * This function waits for the controller to be idle before disabling I2C + * When the controller is not in the IDLE state, the MST_ACTIVITY bit + * (IC_STATUS[5]) is set. + * + * Values: + * 0x1 (ACTIVE): Controller not idle + * 0x0 (IDLE): Controller is idle + * + * The function is called after completing the current transfer. + * + * Returns: + * False when the controller is in the IDLE state. + * True when the controller is in the ACTIVE state. + */ for an improved clarity and address a few grammatical issues. Please verify that it's correct. I merged your patch to i2c/i2c-host-fixes along with the latest changes proposed by Andy. Thanks for your work, Andi
HI, Andi >-----Original Message----- >From: Andi Shyti <andi.shyti@kernel.org> >Sent: 2024年9月20日 16:52 >To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> >Cc: jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Andy Shevchenko ><andy@kernel.org> >Subject: Re: [PATCH v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled >Hi Kimriver, >On Fri, Sep 13, 2024 at 11:31:46AM GMT, Kimriver Liu wrote: >> It was observed that issuing the ABORT bit (IC_ENABLE[1]) will not >> work when IC_ENABLE is already disabled. >> >> Check if the ENABLE bit (IC_ENABLE[0]) is disabled when the controller >> is holding SCL low. If the ENABLE bit is disabled, the software needs >> to enable it before trying to issue the ABORT bit. otherwise, >> 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 fixes the issue where the controller cannot be disabled >> while SCL is held low if the ENABLE bit is already disabled. >> >> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low") >> Signed-off-by: Kimriver Liu <kimriver.liu@siengine.com> >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> Reviewed-by: Andy Shevchenko <andy@kernel.org> >I'm sorry for the delay, but I needed to wait for the previous >batch of fixes to be merged. > > [...] > >> +/* >> + * This function waits controller idling before disabling I2C >> + * 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. >> + */ >I took the liberty of making some small changes to the comment: >+/* >+ * This function waits for the controller to be idle before disabling I2C >+ * When the controller is not in the IDLE state, the MST_ACTIVITY bit >+ * (IC_STATUS[5]) is set. >+ * >+ * Values: >+ * 0x1 (ACTIVE): Controller not idle >+ * 0x0 (IDLE): Controller is idle >+ * >+ * The function is called after completing the current transfer. >+ * >+ * Returns: >+ * False when the controller is in the IDLE state. >+ * True when the controller is in the ACTIVE state. >+ */ >for an improved clarity and address a few grammatical issues. >Please verify that it's correct. >I merged your patch to i2c/i2c-host-fixes along with the latest >changes proposed by Andy. >Thanks for your work, >Andi Your small changes make the comments clearer and more perfect. Thanks. ------------------------------------------ Best Regards Kimriver Liu
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index e8a688d04aee..2d71489ab213 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -441,6 +441,7 @@ int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev) void __i2c_dw_disable(struct dw_i2c_dev *dev) { + struct i2c_timings *t = &dev->timings; unsigned int raw_intr_stats; unsigned int enable; int timeout = 100; @@ -453,6 +454,19 @@ 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); + /* + * 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)); + /*Set ENABLE bit before setting ABORT*/ + enable |= DW_IC_ENABLE_ENABLE; + } + 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-core.h b/drivers/i2c/busses/i2c-designware-core.h index e9606c00b8d1..e45daedad967 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -109,6 +109,7 @@ DW_IC_INTR_RX_UNDER | \ DW_IC_INTR_RD_REQ) +#define DW_IC_ENABLE_ENABLE BIT(0) #define DW_IC_ENABLE_ABORT BIT(1) #define DW_IC_STATUS_ACTIVITY BIT(0) diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index c7e56002809a..4fe69a0996f3 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -253,6 +253,31 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) __i2c_dw_write_intr_mask(dev, DW_IC_INTR_MASTER_MASK); } +/* + * This function waits controller idling before disabling I2C + * 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; +} + static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev) { u32 val; @@ -788,6 +813,16 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) goto done; } + /* + * 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"); + /* * We must disable the adapter before returning and signaling the end * of the current transfer. Otherwise the hardware might continue