| Message ID | 20241101081243.1230797-1-loven.liu@jaguarmicro.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RESEND,TO,CC,MAILLIST] i2c: designware: fix master holding SCL low when I2C_DYNAMIC_TAR_UPDATE not set | expand |
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.
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
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
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
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
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
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>
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
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)
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(-)