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

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

Posted by Kimriver Liu 2 months, 2 weeks ago

It was observed that 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 controller
is holding SCL low. If ENABLE bit is disabled, the software need
to enable it before trying to issue 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 can be fix the controller cannot be disabled while
SCL is held low in 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>

---
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(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..48b8d1c1080c 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));
+			/* Keep ENABLE bit is already set 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..ee870ca39c83 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 functions 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:
+ * Return 0 as controller IDLE
+ * Return a negative errno as controller ACTIVE
+ */
+static int 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 0;
+
+	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
+			1100, 20000);
+}
+
 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
-- 
2.17.1

Re: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

Posted by Andy Shevchenko 2 months, 2 weeks ago

On Wed, Sep 11, 2024 at 04:39:45PM +0800, Kimriver Liu wrote:
> It was observed that 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 controller
> is holding SCL low. If ENABLE bit is disabled, the software need
> to enable it before trying to issue 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 can be fix the controller cannot be disabled while
> SCL is held low in ENABLE bit is already disabled.

...

> +/*
> + * This functions 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:

> + * Return 0 as controller IDLE
> + * Return a negative errno as controller ACTIVE

But why non-boolean again?

> + */
> +static int 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 0;
> +
> +	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +			1100, 20000);
> +}

...

> +	/*
> +	 * 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");

No, this is rarely we skip the error code returned. Either you find the
meaningful use of the returned error code, or change to boolean.

Personally I think the latter is what we need for now.

>  	/*
>  	 * We must disable the adapter before returning and signaling the end
>  	 * of the current transfer. Otherwise the hardware might continue

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

Posted by Andy Shevchenko 2 months, 2 weeks ago

On Wed, Sep 11, 2024 at 05:45:05PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 11, 2024 at 04:39:45PM +0800, Kimriver Liu wrote:

FWIW, below are the fixes

...

> > +/*
> > + * This functions 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:
> 
> > + * Return 0 as controller IDLE
> > + * Return a negative errno as controller ACTIVE

 * False when controller is in IDLE state.
 * True when controller is in ACTIVE state.

> But why non-boolean again?
> 
> > + */
> > +static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev)

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 0;

		return false;

> > +	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> > +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> > +			1100, 20000);

			1100, 20000) != 0;

Alternatively

	int ret;
	...

	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
				       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
				       1100, 20000);
	if (ret)
		return true;

	return false;

(Also mind indentation in _read_poll_timeout() lines.)

> > +}


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

Posted by Liu Kimriver/刘金河 2 months, 2 weeks ago

Hi Andy


>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
>Sent: 2024年9月11日 22:53
>To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
>Cc: 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
>Subject: Re: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

>On Wed, Sep 11, 2024 at 05:45:05PM +0300, Andy Shevchenko wrote:
>> On Wed, Sep 11, 2024 at 04:39:45PM +0800, Kimriver Liu wrote:

>FWIW, below are the fixes
>
>...
>
>> > +/*
>> > + * This functions 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:
>> 
>> > + * Return 0 as controller IDLE
>> > + * Return a negative errno as controller ACTIVE
>
> * False when controller is in IDLE state.
> * True when controller is in ACTIVE state.
>
>> But why non-boolean again?
>> 
>> > + */
>> > +static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev)
>
>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 0;
>
>		return false;
>
>> > +	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> > +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> > +			1100, 20000);
>
>			1100, 20000) != 0;
>
>Alternatively
>
>	int ret;
>	...
>
>	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>				       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>				       1100, 20000);
>	if (ret)
>		return true;
>
>	return false;
>
> (Also mind indentation in _read_poll_timeout() lines.)
>
>> > +}

Sorry.
Thanks for your suggestions and scheme, I will update in patch v10 today.

------------------------------------------
Best Regards
Kimriver Liu

Re: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

Posted by Andi Shyti 2 months, 2 weeks ago

Andy,

On Wed, Sep 11, 2024 at 04:39:45PM GMT, Kimriver Liu wrote:
> It was observed that 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 controller
> is holding SCL low. If ENABLE bit is disabled, the software need
> to enable it before trying to issue 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 can be fix the controller cannot be disabled while
> SCL is held low in 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>

if you're happe, I would take this in.

Thanks Kimriver for following up on all the reviews, I really
appreciate your responsivness.

Andi

RE: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

Posted by Liu Kimriver/刘金河 2 months, 2 weeks ago

HI andi and andy,

>-----Original Message-----
>From: Andi Shyti <andi.shyti@kernel.org> 
>Sent: 2024年9月11日 18:16
>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
>Subject: Re: [PATCH v9] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled

>Andy,

>On Wed, Sep 11, 2024 at 04:39:45PM GMT, Kimriver Liu wrote:
>> It was observed that 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 controller
>> is holding SCL low. If ENABLE bit is disabled, the software need
>> to enable it before trying to issue 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 can be fix the controller cannot be disabled while
>> SCL is held low in 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>

>if you're happe, I would take this in.

>Thanks Kimriver for following up on all the reviews, I really
>appreciate your responsivness.

 Thanks for the review!

 I have done the testing of the V9 patch on our product , it Verified successful when controller is holding SCL low.

------------------------------------------
Best Regards
Kimriver Liu