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

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

[kimriver liu]

From: "kimriver.liu" <kimriver.liu@siengine.com>

Failure in normal Stop operational path

This failure happens rarely and is hard to reproduce. Debug
trace showed that IC_STATUS had value of 0x23 when STOP_DET
occurred, immediately disable ENABLE bit that can result in
IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.

Failure in ENABLE bit is disabled path

It was observed that master is holding SCL low and the
IC_ENABLE is already disabled, Enable ABORT bit and
ENABLE bit simultaneously cannot take effect.

Check if the master is holding SCL low after ENABLE bit is
already disabled. If SCL is held low, The software can set
this ABORT bit only when ENABLE is already set，otherwise,
the controller ignores any write to ABORT bit. When the
abort is done, then proceed with disabling the controller.

These kernel logs show up whenever an I2C transaction is
attempted after this failure.
i2c_designware e95e0000.i2c: timeout in disabling adapter
i2c_designware e95e0000.i2c: timeout waiting for bus ready

The patch can be fix the controller cannot be disabled while
SCL is held low in ENABLE bit is already disabled.

Signed-off-by: kimriver.liu <kimriver.liu@siengine.com>
---
 drivers/i2c/busses/i2c-designware-common.c |  5 +++++
 drivers/i2c/busses/i2c-designware-master.c | 21 ++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..e1596b67e92f 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -453,6 +453,11 @@ 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) {
+			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
+			enable |= DW_IC_ENABLE_ENABLE;
+			usleep_range(25, 100);
+		}
 		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..f86d03b0472a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -253,6 +253,23 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	__i2c_dw_write_intr_mask(dev, DW_IC_INTR_MASTER_MASK);
 }
 
+static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev)
+{
+	u32 status = 0;
+	int ret = 0;
+
+	regmap_read(dev->map, DW_IC_STATUS, &status);
+	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
+		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+				!(status & DW_IC_STATUS_MASTER_ACTIVITY),
+				1100, 20000);
+		if (ret)
+			dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
 {
 	u32 val;
@@ -796,7 +813,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	 * additional interrupts are a hardware bug or this driver doesn't
 	 * handle them correctly yet.
 	 */
-	__i2c_dw_disable_nowait(dev);
+	ret = i2c_dw_check_mst_activity(dev);
+	if (!ret)
+		__i2c_dw_disable_nowait(dev);
 
 	if (dev->msg_err) {
 		ret = dev->msg_err;
-- 
2.17.1

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

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 02:42:24PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>
> 
> Failure in normal Stop operational path
> 
> This failure happens rarely and is hard to reproduce. Debug
> trace showed that IC_STATUS had value of 0x23 when STOP_DET
> occurred, immediately disable ENABLE bit that can result in
> IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
> 
> Failure in ENABLE bit is disabled path
> 
> It was observed that master is holding SCL low and the
> IC_ENABLE is already disabled, Enable ABORT bit and
> ENABLE bit simultaneously cannot take effect.
> 
> Check if the master is holding SCL low after ENABLE bit is
> already disabled. If SCL is held low, The software can set
> this ABORT bit only when ENABLE is already set，otherwise,
> the controller ignores any write to ABORT bit. When the
> abort is done, then proceed with disabling the controller.
> 
> These kernel logs show up whenever an I2C transaction is
> attempted after this failure.
> i2c_designware e95e0000.i2c: timeout in disabling adapter
> i2c_designware e95e0000.i2c: timeout waiting for bus ready
> 
> The patch can be fix the controller cannot be disabled while
> SCL is held low in ENABLE bit is already disabled.

...

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!enable) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;

> +			usleep_range(25, 100);

fsleep()

And add a short comment to explain the chosen value.

> +		}

...

> +static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev)
> +{
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
> +		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +				!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +				1100, 20000);
> +		if (ret)
> +			dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);
> +	}
> +
> +	return ret;

This can be rewritten as

	u32 status = 0;
	int ret;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!status & DW_IC_STATUS_MASTER_ACTIVITY))
		return 0;

	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
			1100, 20000);
	if (ret)
		dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);

	return ret;

> +}

...

> +	ret = i2c_dw_check_mst_activity(dev);
> +	if (!ret)
> +		__i2c_dw_disable_nowait(dev);

...but looking at the usage, I think the proper is to have the above to return
boolean. And also update the name to follow the usual pattern for boolean helpers.

static bool i2c_dw_is_mst_idling(struct dw_i2c_dev *dev)
...
	if (i2c_dw_is_mst_idling(dev))
		__i2c_dw_disable_nowait(dev);

...

Also what does the heck "mst" stand for? Please, use decrypted words in
function names and error messages..

-- 
With Best Regards,
Andy Shevchenko

回复: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Liu Kimriver/刘金河]

	Thanks for your suggestion. I will revise it according to your suggestions 
and resend the patch.

Best Regards

-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
发送时间: 2024年9月4日 20:55
收件人: 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 Wed, Sep 04, 2024 at 02:42:24PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>
> 
> Failure in normal Stop operational path
> 
> This failure happens rarely and is hard to reproduce. Debug trace 
> showed that IC_STATUS had value of 0x23 when STOP_DET occurred, 
> immediately disable ENABLE bit that can result in 
> IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
> 
> Failure in ENABLE bit is disabled path
> 
> It was observed that master is holding SCL low and the IC_ENABLE is 
> already disabled, Enable ABORT bit and ENABLE bit simultaneously 
> cannot take effect.
> 
> Check if the master is holding SCL low after ENABLE bit is already 
> disabled. If SCL is held low, The software can set this ABORT bit only 
> when ENABLE is already set，otherwise,
> the controller ignores any write to ABORT bit. When the abort is done, 
> then proceed with disabling the controller.
> 
> These kernel logs show up whenever an I2C transaction is attempted 
> after this failure.
> i2c_designware e95e0000.i2c: timeout in disabling adapter 
> i2c_designware e95e0000.i2c: timeout waiting for bus ready
> 
> The patch can be fix the controller cannot be disabled while SCL is 
> held low in ENABLE bit is already disabled.

...

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!enable) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;

> +			usleep_range(25, 100);

fsleep()

And add a short comment to explain the chosen value.

> +		}

...

> +static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev) {
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
> +		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +				!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +				1100, 20000);
> +		if (ret)
> +			dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);
> +	}
> +
> +	return ret;

This can be rewritten as

	u32 status = 0;
	int ret;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!status & DW_IC_STATUS_MASTER_ACTIVITY))
		return 0;

	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
			1100, 20000);
	if (ret)
		dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);

	return ret;

> +}

...

> +	ret = i2c_dw_check_mst_activity(dev);
> +	if (!ret)
> +		__i2c_dw_disable_nowait(dev);

...but looking at the usage, I think the proper is to have the above to return boolean. And also update the name to follow the usual pattern for boolean helpers.

static bool i2c_dw_is_mst_idling(struct dw_i2c_dev *dev) ...
	if (i2c_dw_is_mst_idling(dev))
		__i2c_dw_disable_nowait(dev);

...

Also what does the heck "mst" stand for? Please, use decrypted words in function names and error messages..

--
With Best Regards,
Andy Shevchenko

回复: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Liu Kimriver/刘金河]

Andy Shevchenko: 
  Today, I followed your suggestion to resend the patch, but I found that 
there is an inaccurate issue with determining whether i2c is enabled,
I will update the patch again this afternoon and send it to you. Thank you.

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
-		if (!enable) {
+		if (!(enable & DW_IC_ENABLE_ENABLE)) {
 			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
 			enable |= DW_IC_ENABLE_ENABLE;

Best Regards
Kimriver.liu

-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
发送时间: 2024年9月4日 20:55
收件人: 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 Wed, Sep 04, 2024 at 02:42:24PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>
> 
> Failure in normal Stop operational path
> 
> This failure happens rarely and is hard to reproduce. Debug trace 
> showed that IC_STATUS had value of 0x23 when STOP_DET occurred, 
> immediately disable ENABLE bit that can result in 
> IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
> 
> Failure in ENABLE bit is disabled path
> 
> It was observed that master is holding SCL low and the IC_ENABLE is 
> already disabled, Enable ABORT bit and ENABLE bit simultaneously 
> cannot take effect.
> 
> Check if the master is holding SCL low after ENABLE bit is already 
> disabled. If SCL is held low, The software can set this ABORT bit only 
> when ENABLE is already set，otherwise,
> the controller ignores any write to ABORT bit. When the abort is done, 
> then proceed with disabling the controller.
> 
> These kernel logs show up whenever an I2C transaction is attempted 
> after this failure.
> i2c_designware e95e0000.i2c: timeout in disabling adapter 
> i2c_designware e95e0000.i2c: timeout waiting for bus ready
> 
> The patch can be fix the controller cannot be disabled while SCL is 
> held low in ENABLE bit is already disabled.

...

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!enable) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;

> +			usleep_range(25, 100);

fsleep()

And add a short comment to explain the chosen value.

> +		}

...

> +static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev) {
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
> +		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +				!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +				1100, 20000);
> +		if (ret)
> +			dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);
> +	}
> +
> +	return ret;

This can be rewritten as

	u32 status = 0;
	int ret;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!status & DW_IC_STATUS_MASTER_ACTIVITY))
		return 0;

	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
			1100, 20000);
	if (ret)
		dev_err(dev->dev, "i2c mst activity not idle %d\n", ret);

	return ret;

> +}

...

> +	ret = i2c_dw_check_mst_activity(dev);
> +	if (!ret)
> +		__i2c_dw_disable_nowait(dev);

...but looking at the usage, I think the proper is to have the above to return boolean. And also update the name to follow the usual pattern for boolean helpers.

static bool i2c_dw_is_mst_idling(struct dw_i2c_dev *dev) ...
	if (i2c_dw_is_mst_idling(dev))
		__i2c_dw_disable_nowait(dev);

...

Also what does the heck "mst" stand for? Please, use decrypted words in function names and error messages..

--
With Best Regards,
Andy Shevchenko

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

[Kimriver Liu]

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>

---
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 | 11 +++++++++++
 drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..2b3398cd4382 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -453,6 +453,17 @@ 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..132b7237c004 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -253,6 +253,19 @@ 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;
+
+	regmap_read(dev->map, DW_IC_STATUS, &status);
+	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
+		return true;
+
+	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 +801,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 not idling\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] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Mika Westerberg]

Hi,

On Fri, Sep 06, 2024 at 03:47:31PM +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>
> 
> ---
> 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 | 11 +++++++++++
>  drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..2b3398cd4382 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -453,6 +453,17 @@ 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..132b7237c004 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -253,6 +253,19 @@ 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;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> +		return true;
> +
> +	return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +			1100, 20000);
> +}

Yeah, I now realize that i2c_dw_wait_bus_not_busy() checks for
DW_IC_STATUS_ACTIVITY not for DW_IC_STATUS_MASTER_ACTIVITY as I thought
so consolidating them makes not that much sense.

This looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

回复: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Liu Kimriver/刘金河]

Hi 
  I am sorry for not replying to questions in time on Friday.


-----邮件原件-----
发件人: Mika Westerberg <mika.westerberg@linux.intel.com> 
发送时间: 2024年9月6日 16:08
收件人: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
抄送: jarkko.nikula@linux.intel.com; andriy.shevchenko@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

Hi,

On Fri, Sep 06, 2024 at 03:47:31PM +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>
> 
> ---
> 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 | 11 +++++++++++
>  drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..2b3398cd4382 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -453,6 +453,17 @@ 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..132b7237c004 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -253,6 +253,19 @@ 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;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> +		return true;
> +
> +	return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +			1100, 20000);
> +}

>Yeah, I now realize that i2c_dw_wait_bus_not_busy() checks for
>DW_IC_STATUS_ACTIVITY not for DW_IC_STATUS_MASTER_ACTIVITY as I thought
>so consolidating them makes not that much sense.

>This looks good to me,

Thanks.
 This case happens rarely and is hard to reproduce. We reproduce this issue 
 in RTL simulation. It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY
 idling before disabling I2C when I2C transfer completed.  as described in the
 DesignWare I2C databook(Flowchart for DW_apb_i2c Controller)


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