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

[PATCH v7] 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>

---
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
	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 | 12 ++++++++++++
 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..a105fd1e6b29 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;
+			/*
+			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
+			 * is already set. Wait 10 times the signaling period of the highest
+			 * I2C transfer supported by the driver(for 400KHz this is 25us)
+			 * as described in the DesignWare I2C databook.
+			 */
+			usleep_range(25, 250);
+		}
+
 		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..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 v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Andi Shyti]

Hi Kimriver,

On Mon, Sep 09, 2024 at 09:56:46AM 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>

You forgot:

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
> 	4. base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=master

Thanks a lot for following up! :-)

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

...

> --- 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;
> +			/*
> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
> +			 * is already set. Wait 10 times the signaling period of the highest
> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
> +			 * as described in the DesignWare I2C databook.
> +			 */
> +			usleep_range(25, 250);

I think there is a misunderstanding here. Andy asked you to use
flseep and improve the calculation: "Please, calculate this delay
based on the actual speed in use (or about to be in use)."[*]

Andy can you please clarify with Kimriver here?

Thanks,
Andi

[*] Message-ID: <6392ecd3f9934e9d8641b5f608ee6d60@siengine.com>

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

[Liu Kimriver/刘金河]

HI andi

Due to a 12 time difference，I had been off work.
I am very sorry that I can't reply email in time. I will reply to your email  immediately after going to work tomorrow.

发自我的 iPhone

>From: Andi Shyti <andi.shyti@kernel.org>
>To: Kimriver Liu <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 v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>Date: Mon, 9 Sep 2024 15:08:25 +0200	[thread overview]
>Message-ID: <traj5uhqbny5yro3hf72k2qpga7ez7cuqv2mesvl73fku2b5xq@uw5dqjdc6mms> (raw)
>In-Reply-To: <20240909015646.2285-1-kimriver.liu@siengine.com>

>Hi Kimriver,

>On Mon, Sep 09, 2024 at 09:56:46AM 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>

>You forgot:

>Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
  
  Thank you for telling me
  
>> ---
>> 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
>> 	4. base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=master

>Thanks a lot for following up! :-)

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

...

>> --- 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;
>> +			/*
>> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
>> +			 * is already set. Wait 10 times the signaling period of the highest
>> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
>> +			 * as described in the DesignWare I2C databook.
>> +			 */
>> +			usleep_range(25, 250);

>I think there is a misunderstanding here. Andy asked you to use
>flseep and improve the calculation: "Please, calculate this delay
>based on the actual speed in use (or about to be in use)."[*]

>Andy can you please clarify with Kimriver here?

 if we use 400kHz ,need setting flseep(25);
 if we use 100kHz ,need setting flseep(100);
 Overall, take the maximum value:flseep(100);

> --
> 2.17.1

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

[Andi Shyti]

Hi Kimriver,

On Mon, Sep 09, 2024 at 02:26:30PM GMT, Liu Kimriver/刘金河 wrote:
> 
> HI andi
> 
> Due to a 12 time difference，I had been off work.
> I am very sorry that I can't reply email in time. I will reply to your email  immediately after going to work tomorrow.

It's not a problem, take your time! :-)

> >> --- 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;
> >> +			/*
> >> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
> >> +			 * is already set. Wait 10 times the signaling period of the highest
> >> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
> >> +			 * as described in the DesignWare I2C databook.
> >> +			 */
> >> +			usleep_range(25, 250);
> 
> >I think there is a misunderstanding here. Andy asked you to use
> >flseep and improve the calculation: "Please, calculate this delay
> >based on the actual speed in use (or about to be in use)."[*]
> 
> >Andy can you please clarify with Kimriver here?
> 
>  if we use 400kHz ,need setting flseep(25);
>  if we use 100kHz ,need setting flseep(100);
>  Overall, take the maximum value:flseep(100);

Thanks for clarifying, then this is what Andy asked to do,
instead of waiting an average random amount from 25 to 250us.
Does it make sense to you?

Thanks,
Andi

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

[Liu Kimriver/刘金河]

Hi Andi


> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org> 
> Sent: 2024年9月10日 1:37
> To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
> Cc: andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jarkko.nikula@linux.intel.com
> Subject: Re: [PATCH v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

> Hi Kimriver,

> On Mon, Sep 09, 2024 at 02:26:30PM GMT, Liu Kimriver/刘金河 wrote:
> > 
> > HI andi
> > 
> > Due to a 12 time difference，I had been off work.
> > I am very sorry that I can't reply email in time. I will reply to your email  immediately after going to work tomorrow.

> It's not a problem, take your time! :-)

> > >> --- 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;
> > >> +			/*
> > >> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
> > >> +			 * is already set. Wait 10 times the signaling period of the highest
> > >> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
> > >> +			 * as described in the DesignWare I2C databook.
> > >> +			 */
> > >> +			usleep_range(25, 250);
> > 
> > >I think there is a misunderstanding here. Andy asked you to use 
> > >flseep and improve the calculation: "Please, calculate this delay 
> > >based on the actual speed in use (or about to be in use)."[*]
> > 
> > >Andy can you please clarify with Kimriver here?
> > 
>>   if we use 400kHz ,need setting flseep(25);  if we use 100kHz ,need 
> > setting flseep(100);  Overall, take the maximum value:flseep(100);

> Thanks for clarifying, then this is what Andy asked to do, instead of waiting an average random amount from 25 to 250us.
> Does it make sense to you?

 as described in the DesignWare I2C databook:
 Define a timer interval (ti2c_poll) equal to the 10 times the signaling period for the highest I2C transfer
 speed used in the system and supported by DW_apb_i2c. For example, if the highest I2C transfer mode
 is 400 kb/s, then this ti2c_poll is 25μs.

 Now I understand how to calculate this delay based on actual usage speed:
 fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
 I will update code and re-send it as V8.

Thanks.


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