Message ID | 4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1726197817.git.kimriver.liu@siengine.com |
---|---|
State | Accepted |
Delegated to: | Andi Shyti |
Headers | show |
Series | [v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled | expand |
On Fri, Sep 13, 2024 at 6:35 AM Kimriver Liu <kimriver.liu@siengine.com> 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. ... > + /*Set ENABLE bit before setting ABORT*/ Missing spaces ... > +/* > + * This function waits controller idling before disabling I2C waits for controller > + * 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. Yeah, I know that this is a copy of what I suggested, but if we going to amend, these should be with definite article * False when the controller is in the IDLE state. * True when the controller is in the ACTIVE state. > + */ ... > + return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > + !(status & DW_IC_STATUS_MASTER_ACTIVITY), > + 1100, 20000) != 0; You broke the indentation again.
Hi Andy Subject: [PATCH v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled I will change the subject to: Subject: [PATCH v11] i2c: designware: fix controller is holding SCL low while the ENABLE bit is disabled >-----Original Message----- >From: Andy Shevchenko <andy.shevchenko@gmail.com> >Sent: 2024年9月13日 15:41 >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; andi.shyti@kernel.org; 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 >On Fri, Sep 13, 2024 at 6:35 AM Kimriver Liu <kimriver.liu@siengine.com> 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. > >... > >> + /*Set ENABLE bit before setting ABORT*/ /* Set ENABLE bit before setting ABORT */ >Missing spaces > >... >> +/* >> + * This function waits controller idling before disabling I2C >waits for controller + * This function waits for 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. >Yeah, I know that this is a copy of what I suggested, but if we going to amend, these should be with definite article > * False when the controller is in the IDLE state. > * True when the controller is in the ACTIVE state. >> + */ >... >> + return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY), >> + 1100, 20000) != 0; >You broke the indentation again. it has been indented and aligned from the web: https://lore.kernel.org/all/4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1726197817.git.kimriver.liu@siengine.com/ Thanks! >-- ------------------------------------------ Best Regards Kimriver Liu
Hi Kimriver, please, don't send v12 anymore, I will take care of these little notes from Andy. You did a great job at following up on all the reviews, thanks! Andi On Fri, Sep 13, 2024 at 08:31:18AM GMT, Liu Kimriver/刘金河 wrote: > Hi Andy > > Subject: [PATCH v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled > I will change the subject to: > Subject: [PATCH v11] i2c: designware: fix controller is holding SCL low while the ENABLE bit is disabled > > >-----Original Message----- > >From: Andy Shevchenko <andy.shevchenko@gmail.com> > >Sent: 2024年9月13日 15:41 > >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; andi.shyti@kernel.org; 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 > > >On Fri, Sep 13, 2024 at 6:35 AM Kimriver Liu <kimriver.liu@siengine.com> 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. > > > >... > > > >> + /*Set ENABLE bit before setting ABORT*/ > > /* Set ENABLE bit before setting ABORT */ > > >Missing spaces > > > >... > > >> +/* > >> + * This function waits controller idling before disabling I2C > > >waits for controller > > + * This function waits for 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. > > >Yeah, I know that this is a copy of what I suggested, but if we going to amend, these should be with definite article > > > * False when the controller is in the IDLE state. > > * True when the controller is in the ACTIVE state. > > > > >> + */ > > >... > > >> + return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, > >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY), > >> + 1100, 20000) != 0; > > >You broke the indentation again. > > it has been indented and aligned from the web: > https://lore.kernel.org/all/4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1726197817.git.kimriver.liu@siengine.com/ > > Thanks! > > >-- > ------------------------------------------ > Best Regards > Kimriver Liu
HI, Thank you for taking the time to review the patch and considering it. Thanks a bunch! ------------------------------------------ Best Regards Kimriver Liu > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: 2024年9月13日 16:53 > To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; 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, > please, don't send v12 anymore, I will take care of these little notes from Andy. > You did a great job at following up on all the reviews, thanks! > Andi > On Fri, Sep 13, 2024 at 08:31:18AM GMT, Liu Kimriver/刘金河 wrote: > > Hi Andy >> >> Subject: [PATCH v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled >> I will change the subject to: >> Subject: [PATCH v11] i2c: designware: fix controller is holding SCL >> low while the ENABLE bit is disabled >> > > >-----Original Message----- >> >From: Andy Shevchenko <andy.shevchenko@gmail.com> > > >Sent: 2024年9月13日 15:41 > > >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; > > >andi.shyti@kernel.org; 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 >> >> >On Fri, Sep 13, 2024 at 6:35 AM Kimriver Liu <kimriver.liu@siengine.com> 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. > >> > >>... > >> > >>> + /*Set ENABLE bit before setting ABORT*/ > > > > /* Set ENABLE bit before setting ABORT */ > > > >>Missing spaces > >> > >>... >> >> >> +/* >> >> + * This function waits controller idling before disabling I2C >> >> >waits for controller >> >> + * This function waits for 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. >> > >>Yeah, I know that this is a copy of what I suggested, but if we going > >>to amend, these should be with definite article >> >> > * False when the controller is in the IDLE state. >> > * True when the controller is in the ACTIVE state. >> >> >> >> >> + */ >> >> >... >> >> >> + return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status, >> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY), >> >> + 1100, 20000) != 0; >> >> >You broke the indentation again. >> >> it has been indented and aligned from the web: >> https://lore.kernel.org/all/4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1 >> 726197817.git.kimriver.liu@siengine.com/ >> >> Thanks! >> >> >-- ------------------------------------------ Best Regards Kimriver Liu
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