diff mbox series

[v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

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

Commit Message

Liu Kimriver/刘金河 Sept. 13, 2024, 3:31 a.m. UTC
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>
---
V10->V11:
	1. update commit messages and comments
	2. add Reviewed-by: Andy Shevchenko <andy@kernel.org>
V9->V10:
	1. return a boolean value for i2c_dw_is_controller_active()
	2. update comments and indentation alignment
V8->V9:
	1. update commit messages and comments
	2. add Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
	3. add fixes tag
	4. change function i2c_dw_is_master_idling to i2c_dw_is_controller_active
	5. update subject as: fix controller is holding SCL low while ENABLE bit is disabled
V7->V8:
	1. calculate this delay based on the actual speed in use
	  fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz))
	2. add Reviewed-by: Mika Westerberg<mika.westerberg@linux.intel.com>
V6->V7:
	1. add Subject versioning [PATCH v7]
	2. change fsleep(25) to usleep_range(25, 250)
	3. Add macro definition DW_iC_ENABLE_ENABLE to fix compile errors
	  | Reported-by: kernel test robot <lkp@intel.com>
	  | Closes:https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/
	4. base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=master
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@siengine.com/
---
 drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++
 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 35 ++++++++++++++++++++++
 3 files changed, 50 insertions(+)

Comments

Andy Shevchenko Sept. 13, 2024, 7:40 a.m. UTC | #1
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.
Liu Kimriver/刘金河 Sept. 13, 2024, 8:31 a.m. UTC | #2
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
Andi Shyti Sept. 13, 2024, 8:52 a.m. UTC | #3
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
Liu Kimriver/刘金河 Sept. 13, 2024, 9:32 a.m. UTC | #4
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
Andi Shyti Sept. 20, 2024, 8:52 a.m. UTC | #5
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
Liu Kimriver/刘金河 Sept. 20, 2024, 9:41 a.m. UTC | #6
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 mbox series

Patch

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