[PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Liu Peibao]

When Tx FIFO empty and last command with no STOP bit set, the master
holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
when IC_STAT_FOR_CLK_STRETCH is set.

Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>
Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 6 ++++--
 drivers/i2c/busses/i2c-designware-core.h   | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index f31d352d98b5..9d88b4fa03e4 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -524,7 +524,7 @@ int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
 void __i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	struct i2c_timings *t = &dev->timings;
-	unsigned int raw_intr_stats;
+	unsigned int raw_intr_stats, ic_stats;
 	unsigned int enable;
 	int timeout = 100;
 	bool abort_needed;
@@ -532,9 +532,11 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
 	int ret;
 
 	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats);
+	regmap_read(dev->map, DW_IC_STATUS, &ic_stats);
 	regmap_read(dev->map, DW_IC_ENABLE, &enable);
 
-	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
+	abort_needed = (raw_intr_stats & DW_IC_INTR_MST_ON_HOLD) ||
+			(ic_stats & DW_IC_STATUS_MASTER_HOLD_TX_FIFO_EMPTY);
 	if (abort_needed) {
 		if (!(enable & DW_IC_ENABLE_ENABLE)) {
 			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 8e8854ec9882..2d32896d0673 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -116,6 +116,7 @@
 #define DW_IC_STATUS_RFNE			BIT(3)
 #define DW_IC_STATUS_MASTER_ACTIVITY		BIT(5)
 #define DW_IC_STATUS_SLAVE_ACTIVITY		BIT(6)
+#define DW_IC_STATUS_MASTER_HOLD_TX_FIFO_EMPTY	BIT(7)
 
 #define DW_IC_SDA_HOLD_RX_SHIFT			16
 #define DW_IC_SDA_HOLD_RX_MASK			GENMASK(23, 16)
-- 
2.34.1

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Andy Shevchenko]

On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
> When Tx FIFO empty and last command with no STOP bit set, the master
> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
> when IC_STAT_FOR_CLK_STRETCH is set.

Who are those people? Why Angus Chen is not a committer of the change?
Please, consult with the Submitting Patches documentation to clarify on these
tags.

Also, sounds to me that Fixes tag is needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Liu Peibao]

On 2024/11/1 16:44, Andy Shevchenko wrote:
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
>> When Tx FIFO empty and last command with no STOP bit set, the master
>> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
>> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
>> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
>> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
>> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
>> when IC_STAT_FOR_CLK_STRETCH is set.
> 
> Who are those people? Why Angus Chen is not a committer of the change?
> Please, consult with the Submitting Patches documentation to clarify on these
> tags.
> 

We have discussed and analyzed this issue together. I developed this patch.
This patch was also reviewed by Angus Chen and Xiaowu Ding.

And in this case, should I replace the "SoBs" with "Reviewed-by"?

> Also, sounds to me that Fixes tag is needed.
> 

How about this tag:
Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")

BR,
Peibao

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Andi Shyti]

Hi Liu,

On Fri, Nov 01, 2024 at 06:18:36PM +0800, Liu Peibao wrote:
> On 2024/11/1 16:44, Andy Shevchenko wrote:
> > External Mail: This email originated from OUTSIDE of the organization!
> > Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
> > 
> > On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
> >> When Tx FIFO empty and last command with no STOP bit set, the master
> >> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
> >> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
> >> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
> >> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
> >> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
> >> when IC_STAT_FOR_CLK_STRETCH is set.
> > 
> > Who are those people? Why Angus Chen is not a committer of the change?
> > Please, consult with the Submitting Patches documentation to clarify on these
> > tags.
> > 
> 
> We have discussed and analyzed this issue together. I developed this patch.
> This patch was also reviewed by Angus Chen and Xiaowu Ding.

The tag list follows a specific order: tags are sorted
sequentially, with the last Signed-off-by (SoB) being the person
sending the patch, which is your email.

The other SoBs are fine, but if someone contributed to
development, consider using "Co-developed-by" instead.

If someone tested the patch, use "Tested-by"; if they reviewed
it, use "Reviewed-by"; and if they simply agreed with the
change, use "Acked-by."

Please ensure that "Reviewed-by," "Tested-by," or "Acked-by"
tags are visible in the mailing list. I do not typically accept
offline R-b, T-b, or A-b.

This is why Andy asked about those contributors. Three SoBs can
seem unusual, but it's acceptable if justified. Reviewers may
ask for clarification, and it's fine to specify contributors'
roles. You can also provide extra details after the "---"
delimiter.

> And in this case, should I replace the "SoBs" with "Reviewed-by"?
> 
> > Also, sounds to me that Fixes tag is needed.
> > 
> 
> How about this tag:
> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")

Sounds reasonable.

For accepting this patch I need an ack from either Andy, Jarkko
or Mika.

As long as the fixes are limited to the commit message there is
no need to resend the patch.

Thanks,
Andi

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Liu Peibao]

On 2024/11/6 4:30, Andi Shyti wrote:
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
> 
> 
> Hi Liu,
> 
> On Fri, Nov 01, 2024 at 06:18:36PM +0800, Liu Peibao wrote:
>> On 2024/11/1 16:44, Andy Shevchenko wrote:
>>> External Mail: This email originated from OUTSIDE of the organization!
>>> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
>>>
>>> On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
>>>> When Tx FIFO empty and last command with no STOP bit set, the master
>>>> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
>>>> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
>>>> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
>>>> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
>>>> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
>>>> when IC_STAT_FOR_CLK_STRETCH is set.
>>>
>>> Who are those people? Why Angus Chen is not a committer of the change?
>>> Please, consult with the Submitting Patches documentation to clarify on these
>>> tags.
>>>
>>
>> We have discussed and analyzed this issue together. I developed this patch.
>> This patch was also reviewed by Angus Chen and Xiaowu Ding.
> 
> The tag list follows a specific order: tags are sorted
> sequentially, with the last Signed-off-by (SoB) being the person
> sending the patch, which is your email.
> 
> The other SoBs are fine, but if someone contributed to
> development, consider using "Co-developed-by" instead.
> 
> If someone tested the patch, use "Tested-by"; if they reviewed
> it, use "Reviewed-by"; and if they simply agreed with the
> change, use "Acked-by."
> 
> Please ensure that "Reviewed-by," "Tested-by," or "Acked-by"
> tags are visible in the mailing list. I do not typically accept
> offline R-b, T-b, or A-b.
> 
> This is why Andy asked about those contributors. Three SoBs can
> seem unusual, but it's acceptable if justified. Reviewers may
> ask for clarification, and it's fine to specify contributors'
> roles. You can also provide extra details after the "---"
> delimiter.
>

I think this tag list should be much better than the original. ^-^

Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
Co-developed-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
Co-developed-by: Angus Chen <angus.chen@jaguarmicro.com>
Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>

>> And in this case, should I replace the "SoBs" with "Reviewed-by"?
>>
>>> Also, sounds to me that Fixes tag is needed.
>>>
>>
>> How about this tag:
>> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
> 
> Sounds reasonable.
> 
> For accepting this patch I need an ack from either Andy, Jarkko
> or Mika.
> 
> As long as the fixes are limited to the commit message there is
> no need to resend the patch.
> 
> Thanks,
> Andi

Got it!

BR,
Peibao

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Andi Shyti]

Hi Liu,

On Wed, Nov 06, 2024 at 03:27:42PM +0800, Liu Peibao wrote:
> On 2024/11/6 4:30, Andi Shyti wrote:
> > On Fri, Nov 01, 2024 at 06:18:36PM +0800, Liu Peibao wrote:
> >> On 2024/11/1 16:44, Andy Shevchenko wrote:
> >>> External Mail: This email originated from OUTSIDE of the organization!
> >>> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
> >>>
> >>> On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
> >>>> When Tx FIFO empty and last command with no STOP bit set, the master
> >>>> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
> >>>> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
> >>>> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
> >>>> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
> >>>> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
> >>>> when IC_STAT_FOR_CLK_STRETCH is set.
> >>>
> >>> Who are those people? Why Angus Chen is not a committer of the change?
> >>> Please, consult with the Submitting Patches documentation to clarify on these
> >>> tags.
> >>>
> >>
> >> We have discussed and analyzed this issue together. I developed this patch.
> >> This patch was also reviewed by Angus Chen and Xiaowu Ding.
> > 
> > The tag list follows a specific order: tags are sorted
> > sequentially, with the last Signed-off-by (SoB) being the person
> > sending the patch, which is your email.
> > 
> > The other SoBs are fine, but if someone contributed to
> > development, consider using "Co-developed-by" instead.
> > 
> > If someone tested the patch, use "Tested-by"; if they reviewed
> > it, use "Reviewed-by"; and if they simply agreed with the
> > change, use "Acked-by."
> > 
> > Please ensure that "Reviewed-by," "Tested-by," or "Acked-by"
> > tags are visible in the mailing list. I do not typically accept
> > offline R-b, T-b, or A-b.
> > 
> > This is why Andy asked about those contributors. Three SoBs can
> > seem unusual, but it's acceptable if justified. Reviewers may
> > ask for clarification, and it's fine to specify contributors'
> > roles. You can also provide extra details after the "---"
> > delimiter.
> >
> 
> I think this tag list should be much better than the original. ^-^
> 
> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
> Co-developed-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
> Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
> Co-developed-by: Angus Chen <angus.chen@jaguarmicro.com>
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>

Thanks, this make much more sense now.

Just one question, do we want to keep xiaowu.ding or Xiaowu Ding?
Can I change it to the second way? It looks better and that's how
it's normally signed.

Andi

> >> And in this case, should I replace the "SoBs" with "Reviewed-by"?
> >>
> >>> Also, sounds to me that Fixes tag is needed.
> >>>
> >>
> >> How about this tag:
> >> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
> > 
> > Sounds reasonable.
> > 
> > For accepting this patch I need an ack from either Andy, Jarkko
> > or Mika.
> > 
> > As long as the fixes are limited to the commit message there is
> > no need to resend the patch.
> > 
> > Thanks,
> > Andi
> 
> Got it!
> 
> BR,
> Peibao

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Liu Peibao]

On 2024/11/6 17:46, Andi Shyti wrote:
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
> 
> 
> Hi Liu,
> 
> On Wed, Nov 06, 2024 at 03:27:42PM +0800, Liu Peibao wrote:
>> On 2024/11/6 4:30, Andi Shyti wrote:
>>> On Fri, Nov 01, 2024 at 06:18:36PM +0800, Liu Peibao wrote:
>>>> On 2024/11/1 16:44, Andy Shevchenko wrote:
>>>>> External Mail: This email originated from OUTSIDE of the organization!
>>>>> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.
>>>>>
>>>>> On Fri, Nov 01, 2024 at 04:12:43PM +0800, Liu Peibao wrote:
>>>>>> When Tx FIFO empty and last command with no STOP bit set, the master
>>>>>> holds SCL low. If I2C_DYNAMIC_TAR_UPDATE is not set, BIT(13) MST_ON_HOLD
>>>>>> of IC_RAW_INTR_STAT is not Enabled, causing the __i2c_dw_disable()
>>>>>> timeout. This is quiet similar as commit 2409205acd3c ("i2c: designware:
>>>>>> fix __i2c_dw_disable() in case master is holding SCL low") mentioned.
>>>>>> Check BIT(7) MST_HOLD_TX_FIFO_EMPTY in IC_STATUS also which is available
>>>>>> when IC_STAT_FOR_CLK_STRETCH is set.
>>>>>
>>>>> Who are those people? Why Angus Chen is not a committer of the change?
>>>>> Please, consult with the Submitting Patches documentation to clarify on these
>>>>> tags.
>>>>>
>>>>
>>>> We have discussed and analyzed this issue together. I developed this patch.
>>>> This patch was also reviewed by Angus Chen and Xiaowu Ding.
>>>
>>> The tag list follows a specific order: tags are sorted
>>> sequentially, with the last Signed-off-by (SoB) being the person
>>> sending the patch, which is your email.
>>>
>>> The other SoBs are fine, but if someone contributed to
>>> development, consider using "Co-developed-by" instead.
>>>
>>> If someone tested the patch, use "Tested-by"; if they reviewed
>>> it, use "Reviewed-by"; and if they simply agreed with the
>>> change, use "Acked-by."
>>>
>>> Please ensure that "Reviewed-by," "Tested-by," or "Acked-by"
>>> tags are visible in the mailing list. I do not typically accept
>>> offline R-b, T-b, or A-b.
>>>
>>> This is why Andy asked about those contributors. Three SoBs can
>>> seem unusual, but it's acceptable if justified. Reviewers may
>>> ask for clarification, and it's fine to specify contributors'
>>> roles. You can also provide extra details after the "---"
>>> delimiter.
>>>
>>
>> I think this tag list should be much better than the original. ^-^
>>
>> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
>> Co-developed-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
>> Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
>> Co-developed-by: Angus Chen <angus.chen@jaguarmicro.com>
>> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
>> Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>
> 
> Thanks, this make much more sense now.
> 
> Just one question, do we want to keep xiaowu.ding or Xiaowu Ding?
> Can I change it to the second way? It looks better and that's how
> it's normally signed.
> 

I have confirmed with Xiaowu, and that is surely okay.

BR,
Peibao

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Jarkko Nikula]

On 11/6/24 12:09 PM, Liu Peibao wrote:
> On 2024/11/6 17:46, Andi Shyti wrote:
>>> Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
>>> Co-developed-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
>>> Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
>>> Co-developed-by: Angus Chen <angus.chen@jaguarmicro.com>
>>> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
>>> Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>
>>
>> Thanks, this make much more sense now.
>>
>> Just one question, do we want to keep xiaowu.ding or Xiaowu Ding?
>> Can I change it to the second way? It looks better and that's how
>> it's normally signed.
>>
> 
> I have confirmed with Xiaowu, and that is surely okay.
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH RESEND TO CC MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set

[Andi Shyti]

Hi,

On Wed, Nov 06, 2024 at 01:53:17PM +0200, Jarkko Nikula wrote:
> On 11/6/24 12:09 PM, Liu Peibao wrote:
> > On 2024/11/6 17:46, Andi Shyti wrote:
> > > > Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
> > > > Co-developed-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
> > > > Signed-off-by: xiaowu.ding <xiaowu.ding@jaguarmicro.com>
> > > > Co-developed-by: Angus Chen <angus.chen@jaguarmicro.com>
> > > > Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> > > > Signed-off-by: Liu Peibao <loven.liu@jaguarmicro.com>
> > > 
> > > Thanks, this make much more sense now.
> > > 
> > > Just one question, do we want to keep xiaowu.ding or Xiaowu Ding?
> > > Can I change it to the second way? It looks better and that's how
> > > it's normally signed.
> > > 
> > 
> > I have confirmed with Xiaowu, and that is surely okay.

Thanks, I fixed the commit log as agreed.

> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thanks Jarkko for acking the patch.

Merged to i2c/i2c-host-fixes.

Andi