[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

[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 | 12 +++++++++++
 drivers/i2c/busses/i2c-designware-master.c | 23 +++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..74cefbb62bcd 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -453,6 +453,18 @@ 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;
+
+			/*
+			 * 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..aba0b8fdfe9a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -253,6 +253,26 @@ 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;
+	int ret;
+
+	regmap_read(dev->map, DW_IC_STATUS, &status);
+	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
+		return true;
+
+	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 master controller not idle %d\n", ret);
+		return false;
+	}
+
+	return true;
+}
+
 static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
 {
 	u32 val;
@@ -796,7 +816,8 @@ 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);
+	if (i2c_dw_is_master_idling(dev))
+		__i2c_dw_disable_nowait(dev);
 
 	if (dev->msg_err) {
 		ret = dev->msg_err;
-- 
2.17.1

[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 | 12 +++++++++++
 drivers/i2c/busses/i2c-designware-master.c | 23 +++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..54acf8554582 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -453,6 +453,18 @@ 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..aba0b8fdfe9a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -253,6 +253,26 @@ 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;
+	int ret;
+
+	regmap_read(dev->map, DW_IC_STATUS, &status);
+	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
+		return true;
+
+	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 master controller not idle %d\n", ret);
+		return false;
+	}
+
+	return true;
+}
+
 static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
 {
 	u32 val;
@@ -796,7 +816,8 @@ 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);
+	if (i2c_dw_is_master_idling(dev))
+		__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 Thu, Sep 05, 2024 at 01:57:53PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>

You forgot bumping patch version and adding a changelog.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 03:42:11PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@siengine.com>

You forgot bumping patch version in the Subject and now it's quite confusing.

> Failure in normal Stop operational path

Is this a subsection?
Make it more clear, by using additional formatting, like

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

Ditto.

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

Here is the place for comments and changelog.
Since it's not the first version of the patch, changelog is a must.

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

---

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

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..54acf8554582 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -453,6 +453,18 @@ 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..6a053f3b5501 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -253,6 +253,24 @@ 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;
+	int ret;
+
+	regmap_read(dev->map, DW_IC_STATUS, &status);
+	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
+		return true;
+
+	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
+			1100, 20000);
+	if (ret)
+		return false;
+
+	return true;
+}
+
 static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
 {
 	u32 val;
@@ -788,6 +806,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 controller not idle\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 01:42:50PM +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>
> 
> ---
> 
> 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 | 12 ++++++++++
>  drivers/i2c/busses/i2c-designware-master.c | 27 ++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..54acf8554582 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -453,6 +453,18 @@ 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;
> +

Drop the empty line here.

> +			/*
> +			 * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit

i2c -> I2C

> +			 * 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..6a053f3b5501 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -253,6 +253,24 @@ 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;
> +	int ret;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> +		return true;
> +
> +	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +			1100, 20000);
> +	if (ret)
> +		return false;
> +
> +	return true;

You can also do:

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


But this is already done in i2c_dw_wait_bus_not_busy() (reverse of this)
so perhaps consolidate them?

> +}
> +
>  static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
>  {
>  	u32 val;
> @@ -788,6 +806,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 controller not idle\n");

i2c -> I2C

(the driver log messages are not too consistent though, something to
improve).

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

[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>

---
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 +++++++++++
 1 file changed, 11 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,
-- 
2.17.1

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

[Mika Westerberg]

On Fri, Sep 06, 2024 at 02:54:49PM +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>
> 
> ---
> V4->V5: delete master idling checking

Hm, why did you do that? I suggested to consolidate the two into one
function (if possible) not drop it.

[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>

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

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 01:42:50PM +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>
> 
> ---

> V3->V4:

Nice, but the Subject (which is most important part) still has no versioning :-(

>       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

...

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

And if we use 100kHz?
Please, calculate this delay based on the actual speed in use
(or about to be in use).

> +		}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 02:54:49PM +0800, Kimriver Liu wrote:

> ---
> V4->V5: delete master idling checking

Please, slow down, read Submitting Patches and Submitting Patches Checklist
documentation, make sure you understand versioning of the patches
(this still is v1 formally), address as many comments as you can
(the rest should be replied to the reviewers and explaining why
 you can't / won't satisfy them). Only *after* that send a new version.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

This is *still* version 1!

-- 
With Best Regards,
Andy Shevchenko

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

[Andi Shyti]

Hi Kimriver,

On Fri, Sep 06, 2024 at 03:47:31PM GMT, 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/

You've ignored all of Andy's reviews, and this is not acceptable.

You've submitted this patch three times today in a short period,
and Andy has responded to each one with the same comment, which
you continue to disregard.

You must follow up on the reviews. I will accept this patch for
now, but if you keep ignoring feedback, I will reject future
patches until you address all reviews from those taking their
time to review your changes.

Andi

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

[Andi Shyti]

Hi Kimriver,

...

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

BTW, your patch doesn't compile. Please make sure that you have
everything in place and please resend.

This time I expect you to follow Andy's suggestion.

Thanks,
Andi

Re: [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, when I left the office early on Friday.
 I sincerely apologize to you again.

-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
发送时间: 2024年9月6日 19:48
收件人: 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 Fri, Sep 06, 2024 at 01:42:50PM +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>
> 
> ---

> V3->V4:

>Nice, but the Subject (which is most important part) still has no versioning :-(

 Thanks or your guidance.
 ON Monday , I will update the Subject versioning and resend V7 patch:  
 [PATCH V7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

>       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 
> V1->comments

...

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

>And if we use 100kHz?
>Please, calculate this delay based on the actual speed in use (or about to be in use).

Yes, I will change fsleep(25) to usleep_range(25, 250), for next V7 version.
Wait 10 times the signaling period of the highest I2C transfer supported
by the driver (for 400KHz this is* 25us)

> +		}

---------------
Kimriver Liu

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

[Liu Kimriver/刘金河]

Hi Andi
 I am sorry for not replying to questions in time, when I left the office early on Friday.
 I sincerely apologize to you again.

I will improve my response progress and follow Andy's suggestion. Thanks.
And will resend new patch V7 on Moday:
[PATCH V7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

-----邮件原件-----
发件人: Andi Shyti <andi.shyti@kernel.org> 
发送时间: 2024年9月7日 5:35
收件人: Liu Kimriver/刘金河 <kimriver.liu@siengine.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
主题: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

Hi Kimriver,

...

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

>BTW, your patch doesn't compile. Please make sure that you have everything in place and please resend.

>This time I expect you to follow Andy's suggestion.

Sorry, I forget to merge DW_IC_ENABLE_ENABLE patch to here. I will update V7 patch on Monday,
[PATCH V7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
I will update V7 patch on Monday which includes the following issues:
-------
V6->V7:
	1. add Subject versioning
	2. change fsleep(25) to usleep_range(25, 250)
	3. Add macro definition DW_iC_ENABLE_ENABLE for compiling error
------------------
Best Regards
Kimriver Liu

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

[kernel test robot]

Hi kimriver,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/kimriver-liu/i2c-designware-fix-master-is-holding-SCL-low-while-ENABLE-bit-is-disabled/20240905-154711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240905074211.2278-1-kimriver.liu%40siengine.com
patch subject: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-designware-common.c: In function '__i2c_dw_disable':
>> drivers/i2c/busses/i2c-designware-common.c:538:32: error: 'DW_IC_ENABLE_ENABLE' undeclared (first use in this function); did you mean 'DW_IC_ENABLE_STATUS'?
     538 |                 if (!(enable & DW_IC_ENABLE_ENABLE)) {
         |                                ^~~~~~~~~~~~~~~~~~~
         |                                DW_IC_ENABLE_STATUS
   drivers/i2c/busses/i2c-designware-common.c:538:32: note: each undeclared identifier is reported only once for each function it appears in


vim +538 drivers/i2c/busses/i2c-designware-common.c

   523	
   524	void __i2c_dw_disable(struct dw_i2c_dev *dev)
   525	{
   526		unsigned int raw_intr_stats;
   527		unsigned int enable;
   528		int timeout = 100;
   529		bool abort_needed;
   530		unsigned int status;
   531		int ret;
   532	
   533		regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
   534		regmap_read(dev->map, DW_IC_ENABLE, &enable);
   535	
   536		abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
   537		if (abort_needed) {
 > 538			if (!(enable & DW_IC_ENABLE_ENABLE)) {
   539				regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
   540				enable |= DW_IC_ENABLE_ENABLE;
   541	
   542				/*
   543				 * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit
   544				 * is already set by the driver (for 400KHz this is 25us)
   545				 * as described in the DesignWare I2C databook.
   546				 */
   547				fsleep(25);
   548			}
   549	
   550			regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
   551			ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
   552						       !(enable & DW_IC_ENABLE_ABORT), 10,
   553						       100);
   554			if (ret)
   555				dev_err(dev->dev, "timeout while trying to abort current transfer\n");
   556		}
   557	
   558		do {
   559			__i2c_dw_disable_nowait(dev);
   560			/*
   561			 * The enable status register may be unimplemented, but
   562			 * in that case this test reads zero and exits the loop.
   563			 */
   564			regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
   565			if ((status & 1) == 0)
   566				return;
   567	
   568			/*
   569			 * Wait 10 times the signaling period of the highest I2C
   570			 * transfer supported by the driver (for 400KHz this is
   571			 * 25us) as described in the DesignWare I2C databook.
   572			 */
   573			usleep_range(25, 250);
   574		} while (timeout--);
   575	
   576		dev_warn(dev->dev, "timeout in disabling adapter\n");
   577	}
   578	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Liu Kimriver/刘金河]

HI，robot,
 Today I will resend a version v7 patch([PATCH V7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled)
 based on the latest Linux master branch to resolve compile errors.

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

-----Original Message-----
From: kernel test robot <lkp@intel.com> 
Sent: 2024年9月8日 21:32
To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>; jarkko.nikula@linux.intel.com
Cc: oe-kbuild-all@lists.linux.dev; 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; Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
Subject: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

Hi kimriver,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/kimriver-liu/i2c-designware-fix-master-is-holding-SCL-low-while-ENABLE-bit-is-disabled/20240905-154711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240905074211.2278-1-kimriver.liu%40siengine.com
patch subject: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-designware-common.c: In function '__i2c_dw_disable':
>> drivers/i2c/busses/i2c-designware-common.c:538:32: error: 'DW_IC_ENABLE_ENABLE' undeclared (first use in this function); did you mean 'DW_IC_ENABLE_STATUS'?
     538 |                 if (!(enable & DW_IC_ENABLE_ENABLE)) {
         |                                ^~~~~~~~~~~~~~~~~~~
         |                                DW_IC_ENABLE_STATUS
   drivers/i2c/busses/i2c-designware-common.c:538:32: note: each undeclared identifier is reported only once for each function it appears in


vim +538 drivers/i2c/busses/i2c-designware-common.c

   523	
   524	void __i2c_dw_disable(struct dw_i2c_dev *dev)
   525	{
   526		unsigned int raw_intr_stats;
   527		unsigned int enable;
   528		int timeout = 100;
   529		bool abort_needed;
   530		unsigned int status;
   531		int ret;
   532	
   533		regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
   534		regmap_read(dev->map, DW_IC_ENABLE, &enable);
   535	
   536		abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
   537		if (abort_needed) {
 > 538			if (!(enable & DW_IC_ENABLE_ENABLE)) {
   539				regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
   540				enable |= DW_IC_ENABLE_ENABLE;
   541	
   542				/*
   543				 * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit
   544				 * is already set by the driver (for 400KHz this is 25us)
   545				 * as described in the DesignWare I2C databook.
   546				 */
   547				fsleep(25);
   548			}
   549	
   550			regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
   551			ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
   552						       !(enable & DW_IC_ENABLE_ABORT), 10,
   553						       100);
   554			if (ret)
   555				dev_err(dev->dev, "timeout while trying to abort current transfer\n");
   556		}
   557	
   558		do {
   559			__i2c_dw_disable_nowait(dev);
   560			/*
   561			 * The enable status register may be unimplemented, but
   562			 * in that case this test reads zero and exits the loop.
   563			 */
   564			regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
   565			if ((status & 1) == 0)
   566				return;
   567	
   568			/*
   569			 * Wait 10 times the signaling period of the highest I2C
   570			 * transfer supported by the driver (for 400KHz this is
   571			 * 25us) as described in the DesignWare I2C databook.
   572			 */
   573			usleep_range(25, 250);
   574		} while (timeout--);
   575	
   576		dev_warn(dev->dev, "timeout in disabling adapter\n");
   577	}
   578	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Liu Kimriver/刘金河]

Hi jarkko 

-----Original Message-----
From: kernel test robot <lkp@intel.com> 
Sent: 2024年9月8日 21:32
To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>; jarkko.nikula@linux.intel.com
Cc: oe-kbuild-all@lists.linux.dev; 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; Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
Subject: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

>>Hi kimriver,

>>kernel test robot noticed the following build errors:

>> [auto build test ERROR on andi-shyti/i2c/i2c-host]
>> [also build test ERROR on linus/master v6.11-rc6 next-20240906]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>And when submitting patch, we suggest to use '--base' as documented in
>>https://git-scm.com/docs/git-format-patch#_base_tree_information]


 I applied patch to the wrong git tree last week , I had resent V7 patch To resolve build test ERROR.
 patch Subject: [PATCH v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
 New patch link: 
  https://lore.kernel.org/all/20240909015646.2285-1-kimriver.liu@siengine.com/
The patch rebase on the latest Linux v6.11.0-rc6 (89f5e14d05b) branch (the latest linux master branch)


>>url:    https://github.com/intel-lab-lkp/linux/commits/kimriver-liu/i2c-designware-fix-master-is-holding-SCL-low-while-ENABLE-bit-is-disabled/20240905-154711
>>base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
>>patch link:    https://lore.kernel.org/r/20240905074211.2278-1-kimriver.liu%40siengine.com
>>patch subject: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>>config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/config)
>>compiler: sh4-linux-gcc (GCC) 14.1.0
>>reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409082011.9JF6aYsk-lkp@intel.com/reproduce)

>>If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>the same patch/commit), kindly add following tags
>>| Reported-by: kernel test robot <lkp@intel.com>
>>| Closes: https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/

>>All errors (new ones prefixed by >>):

>>   drivers/i2c/busses/i2c-designware-common.c: In function '__i2c_dw_disable':
>>>> drivers/i2c/busses/i2c-designware-common.c:538:32: error: 'DW_IC_ENABLE_ENABLE' undeclared (first use in this function); did you mean 'DW_IC_ENABLE_STATUS'?
>>     538 |                 if (!(enable & DW_IC_ENABLE_ENABLE)) {
>>         |                                ^~~~~~~~~~~~~~~~~~~
>>         |                                DW_IC_ENABLE_STATUS
>>   drivers/i2c/busses/i2c-designware-common.c:538:32: note: each undeclared identifier is reported only once for each function it appears in


I fixexd the issue at the same commit as In V7 version patch link:  
   https://lore.kernel.org/all/20240909015646.2285-1-kimriver.liu@siengine.com/ 
patch Subject: [PATCH v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
Declaration DW_IC_ENABLE_ENABLE as follow:
 
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)


>>vim +538 drivers/i2c/busses/i2c-designware-common.c

>>   523	
>>   524	void __i2c_dw_disable(struct dw_i2c_dev *dev)
>>   525	{
>>   526		unsigned int raw_intr_stats;
>>   527		unsigned int enable;
>>   528		int timeout = 100;
>>   529		bool abort_needed;
>>   530		unsigned int status;
>>   531		int ret;
>>   532	
>>   533		regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
>>   534		regmap_read(dev->map, DW_IC_ENABLE, &enable);
>>   535	
>>   536		abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>>   537		if (abort_needed) {
>> > 538			if (!(enable & DW_IC_ENABLE_ENABLE)) {
>>   539				regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
>>   540				enable |= DW_IC_ENABLE_ENABLE;
>>   541	
>>   542				/*
>>   543				 * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit
>>   544				 * is already set by the driver (for 400KHz this is 25us)
>>   545				 * as described in the DesignWare I2C databook.
>>   546				 */
>>   547				fsleep(25);
>>   548			}
>>   549	
>>   550			regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>>   551			ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
>>   552						       !(enable & DW_IC_ENABLE_ABORT), 10,
>>   553						       100);
>>   554			if (ret)
>>   555				dev_err(dev->dev, "timeout while trying to abort current transfer\n");
>>   556		}
>>   557	
>>   558		do {
>>   559			__i2c_dw_disable_nowait(dev);
>>   560			/*
>>   561			 * The enable status register may be unimplemented, but
>>   562			 * in that case this test reads zero and exits the loop.
>>   563			 */
>>   564			regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
>>   565			if ((status & 1) == 0)
>>   566				return;
>>   567	
>>   568			/*
>>   569			 * Wait 10 times the signaling period of the highest I2C
>>   570			 * transfer supported by the driver (for 400KHz this is
>>   571			 * 25us) as described in the DesignWare I2C databook.
>>   572			 */
>>   573			usleep_range(25, 250);
>>   574		} while (timeout--);
>>   575	
>>   576		dev_warn(dev->dev, "timeout in disabling adapter\n");
>>   577	}
>>   578	
------------------------------------------
Best Regards
Kimriver Liu