drivers/i2c/busses/i2c-designware-common.c | 13 +++++++++++++ drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++ 3 files changed, 36 insertions(+)
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>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
V7->V8:
1.calculate this delay based on the actual speed in use
fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz))
2. add 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
| Reported-by: kernel test robot <lkp@intel.com>
| Closes:https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/
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 | 13 +++++++++++++
drivers/i2c/busses/i2c-designware-core.h | 1 +
drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++
3 files changed, 36 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..3ff50ec942e9 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -441,6 +441,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 enable;
int timeout = 100;
@@ -453,6 +454,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 10 times the signaling period of the highest I2C
+ * transfer supported by the driver (for 400KHz this is
+ * 25us) to ensure the I2C ENABLE bit is already set
+ * as described in the DesignWare I2C databook.
+ */
+ fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
+ }
+
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.1On 9/10/24 9:13 AM, 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> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > V7->V8: > 1.calculate this delay based on the actual speed in use > fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz)) > 2. add 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 > | Reported-by: kernel test robot <lkp@intel.com> > | Closes:https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/ > 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 | 13 +++++++++++++ > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++ > 3 files changed, 36 insertions(+) > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Hi Jarkko >-----Original Message----- >From: Jarkko Nikula <jarkko.nikula@linux.intel.com> >Sent: 2024年9月10日 17:03 >To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> >Cc: 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 >Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled >On 9/10/24 9:13 AM, 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> >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> >> --- >> V7->V8: >> 1.calculate this delay based on the actual speed in use >> fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz)) >> 2. add 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 >> | Reported-by: kernel test robot <lkp@intel.com> >> | Closes:https://lore.kernel.org/oe-kbuild-all/202409082011.9JF6aYsk-lkp@intel.com/ >> 4. base: >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commi >> t/?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 >> V1->comments >> >> link to V1: >> https://lore.kernel.org/lkml/20240904064224.2394-1-kimriver.liu@siengi >> ne.com/ >> --- >> drivers/i2c/busses/i2c-designware-common.c | 13 +++++++++++++ >> drivers/i2c/busses/i2c-designware-core.h | 1 + >> drivers/i2c/busses/i2c-designware-master.c | 22 ++++++++++++++++++++++ >> 3 files changed, 36 insertions(+) >> >Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Thanks for the Acked! I will update a new version V9 based on Andy's suggestions. ------------------------------------------ Best Regards Kimriver Liu
On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote:
> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when
"...observed that issuing..."
...bit (..."
> IC_ENABLE is already disabled.
>
> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is
"...bit (..."
master --> controller
> 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.
Fixes tag?
...
> 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);
This call might also need a one line comment.
> + enable |= DW_IC_ENABLE_ENABLE;
More natural is to put this after the fsleep() call. The rationale is that it
will be easier to see what exactly is going to be written back to the
register.
> + /*
> + * Wait 10 times the signaling period of the highest I2C
> + * transfer supported by the driver (for 400KHz this is
> + * 25us) to ensure the I2C ENABLE bit is already set
> + * as described in the DesignWare I2C databook.
> + */
> + fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
...somewhere here...
> + }
> +
> regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
...
> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
Sorry if I made a mistake, but again, looking at the usage you have again
negation here and there...
i2c_dw_is_controller_active
(note new terminology, dunno if it makes sense start using it in function
names, as we have more of them following old style)
> +{
> + u32 status;
> +
> + regmap_read(dev->map, DW_IC_STATUS, &status);
> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> + return true;
return false;
.,,
> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> + 1100, 20000);
...and drop !.
> +}
...
> + /*
> + * This happens rarely and is hard to reproduce. Debug trace
Rarely how? Perhaps put a ration in the parentheses, like
"...rarely (~1:100)..."
> + * 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))
...and here
if (i2c_dw_is_controller_active(dev))
But please double check that I haven't made any mistakes in all this logic.
> + dev_err(dev->dev, "I2C master not idling\n");
--
With Best Regards,
Andy Shevchenko
Hi Andy,
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>Sent: 2024年9月10日 17:03
>To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
>Cc: 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
>Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote:
>> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when
>
>"...observed that issuing..."
>...bit (..."
>> IC_ENABLE is already disabled.
>>
>> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is
>"...bit (..."
>master --> controller
Update it in V9
>> 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.
>Fixes tag?
Patch rebase: on Linux v6.11.0-rc6 (89f5e14d05b)
>...
>> 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);
>
>This call might also need a one line comment.
Last Wednesday
>> + enable |= DW_IC_ENABLE_ENABLE;
>More natural is to put this after the fsleep() call. The rationale is that it
>will be easier to see what exactly is going to be written back to the
>register.
Ok
>> + /*
>> + * Wait 10 times the signaling period of the highest I2C
> >+ * transfer supported by the driver (for 400KHz this is
> >+ * 25us) to ensure the I2C ENABLE bit is already set
>> + * as described in the DesignWare I2C databook.
>> + */
> >+ fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
>
>...somewhere here...
>
Ok
>> + }
> +
>> regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
...
>> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
>Sorry if I made a mistake, but again, looking at the usage you have again
>negation here and there...
> i2c_dw_is_controller_active
> (note new terminology, dunno if it makes sense start using it in function
> names, as we have more of them following old style)
Last week , You suggested that I used this i2c_dw_is_master_idling(dev)
>> +{
>> + u32 status;
>> +
>> + regmap_read(dev->map, DW_IC_STATUS, &status);
>> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> + return true;
return false;
.,,
>> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> + 1100, 20000);
>...and drop !.
We reproduce this issue in RTL simulation(About(~1:500) in our soc). It is necessary
to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling before disabling I2C when
I2C transfer completed. as described in the DesignWare
I2C databook(Flowchart for DW_apb_i2c Controller)
>> +}
...
>> + /*
>> + * This happens rarely and is hard to reproduce. Debug trace
>Rarely how? Perhaps put a ration in the parentheses, like
>"...rarely (~1:100)..."
About(~1:500) in our soc
>> + * 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))
>...and here
> if (i2c_dw_is_controller_active(dev))
>But please double check that I haven't made any mistakes in all this logic.
Last week , You suggested that I used this i2c_dw_is_master_idling(dev)
keep using i2c_dw_is_master_idling(dev) , Ok?
>> + dev_err(dev->dev, "I2C master not idling\n");
------------------------------------------
Best Regards
Kimriver Liu
On 9/10/24 12:38 PM, Liu Kimriver/刘金河 wrote: > Hi Andy, > >> -----Original Message----- >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Sent: 2024年9月10日 17:03 >> To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> >> Cc: 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 >> Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled > >> On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote: >>> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when >> >> "...observed that issuing..." >> ...bit (..." > > >>> IC_ENABLE is already disabled. >>> >>> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is > >> "...bit (..." >> master --> controller > > Update it in V9 > Please add back also kernel errors that are shown when the issue occurs. I saw those mentioned in the commit log in some earlier version of the patch. Those may help googling the solution (i.e. this patch) if somebody sees similar error on their HW.
Hi, Jarkko >-----Original Message----- >From: Jarkko Nikula <jarkko.nikula@linux.intel.com> >Sent: 2024年9月10日 19:20 >To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com> >Cc: mika.westerberg@linux.intel.com; jsd@semihalf.com; andi.shyti@kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled >On 9/10/24 12:38 PM, Liu Kimriver/刘金河 wrote: >> Hi Andy, >> >>> -----Original Message----- >>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> Sent: 2024年9月10日 17:03 >>> To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> >>> Cc: 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 >>> Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL >>> low while ENABLE bit is disabled >> >>> On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote: >>>> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when >>> >>> "...observed that issuing..." >>> ...bit (..." >> >> >>>> IC_ENABLE is already disabled. >>>> >>>> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is >> >>> "...bit (..." >>> master --> controller >> >> Update it in V9 >> >Please add back also kernel errors that are shown when the issue occurs. >I saw those mentioned in the commit log in some earlier version of the patch. Those may help googling the solution (i.e. this patch) if somebody sees similar error on their HW. Ok Add back kernel errors logs in V9: 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. Thanks! I will be off work, If there are still emails that I have not been replied to, I will reply to your email immediately after going to work tomorrow. ------------------------------------------ Best Regards Kimriver Liu
On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >Sent: 2024年9月10日 17:03
> >To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
> >On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote:
...
> >master --> controller
>
> Update it in V9
Also in the Subject.
...
> >> 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.
>
> >Fixes tag?
>
> Patch rebase: on Linux v6.11.0-rc6 (89f5e14d05b)
No, this one is done by understanding where the problem appear first.
What you mentioned above may be achieved by using --base option when
format the patch.
...
> >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
>
> >Sorry if I made a mistake, but again, looking at the usage you have again
> >negation here and there...
>
> > i2c_dw_is_controller_active
>
> > (note new terminology, dunno if it makes sense start using it in function
> > names, as we have more of them following old style)
>
> Last week , You suggested that I used this i2c_dw_is_master_idling(dev)
Yes, sorry about that. I did maybe not clearly get how it is going to
look like.
> >> +{
> >> + u32 status;
> >> +
> >> + regmap_read(dev->map, DW_IC_STATUS, &status);
> >> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> + return true;
>
> return false;
>
> >> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> + 1100, 20000);
>
> >...and drop !.
>
> We reproduce this issue in RTL simulation(About(~1:500) in our soc). It is necessary
> to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling before disabling I2C when
> I2C transfer completed. as described in the DesignWare
> I2C databook(Flowchart for DW_apb_i2c Controller)
Cool, but here I'm talking purely about inverting the logic (with renaming),
nothing more.
> >> +}
...
> >> + /*
> >> + * This happens rarely and is hard to reproduce. Debug trace
>
> >Rarely how? Perhaps put a ration in the parentheses, like
>
> >"...rarely (~1:100)..."
> About(~1:500) in our soc
Yes, what I showed was just an example, put the real numbers into the comment.
> >> + * 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))
>
> >...and here
>
> > if (i2c_dw_is_controller_active(dev))
>
> >But please double check that I haven't made any mistakes in all this logic.
>
> Last week , You suggested that I used this i2c_dw_is_master_idling(dev)
> keep using i2c_dw_is_master_idling(dev) , Ok?
See above.
> >> + dev_err(dev->dev, "I2C master not idling\n");
--
With Best Regards,
Andy Shevchenko
HI, Andy
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>Sent: 2024年9月10日 18:45
>To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
>Cc: 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
>Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
>> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> >Sent: 2024年9月10日 17:03
>> >To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com> On Tue, Sep 10, 2024
>> >at 02:13:09PM +0800, Kimriver Liu wrote:
>...
>> >master --> controller
>>
>> Update it in V9
>Also in the Subject.
OK, update it in [PATCH v9]
...
> >> 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.
>
> >Fixes tag?
>>
>> Patch rebase: on Linux v6.11.0-rc6 (89f5e14d05b)
>No, this one is done by understanding where the problem appear first.
>What you mentioned above may be achieved by using --base option when format the patch.
Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")
>...
> >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
>>
>> >Sorry if I made a mistake, but again, looking at the usage you have
>> >again negation here and there...
>
>> > i2c_dw_is_controller_active
>>
>> > (note new terminology, dunno if it makes sense start using it in
>> > function names, as we have more of them following old style)
>>
>> Last week , You suggested that I used this
>> i2c_dw_is_master_idling(dev)
>Yes, sorry about that. I did maybe not clearly get how it is going to look like.
>> >> +{
>> >> + u32 status;
>> >> +
>> >> + regmap_read(dev->map, DW_IC_STATUS, &status);
>> >> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> >> + return true;
>>
>> return false;
>>
>> >> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> >> + 1100, 20000);
>>
>> >...and drop !.
>>
>> We reproduce this issue in RTL simulation(About(~1:500) in our soc).
>> It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling
>> before disabling I2C when I2C transfer completed. as described in
>> the DesignWare I2C databook(Flowchart for DW_apb_i2c Controller)
>Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
as described in the DesignWare I2C databook:
DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
Controller FSM Activity Status. When the Controller Finite
State Machine (FSM) is not in the IDLE state, this bit is set.
Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of
SLV_ACTIVITY and MST_ACTIVITY bits.
Values:
■ 0x1 (ACTIVE): Controller not idle
■ 0x0 (IDLE): Controller is idle
We need waiting DW_IC_STATUS.MST_ACTIVITY idling,
If Controller not idle, Wait for a while.
Return value:
false(0): Controller is idle
timeout(-110): Controller activity
Ok, change the function name i2c_dw_is_master_idling(dev) to i2c_dw_is_controller_active(dev)
it seems more reasonable
static int i2c_dw_is_controller_ active(struct dw_i2c_dev *dev)
{
u32 status;
regmap_read(dev->map, DW_IC_STATUS, &status);
if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
return false;
return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
!(status & DW_IC_STATUS_MASTER_ACTIVITY),
1100, 20000);
}
>> >> +}
...
>> >> + /*
>> >> + * This happens rarely and is hard to reproduce. Debug trace
>>
>> >Rarely how? Perhaps put a ration in the parentheses, like
>>
>> >"...rarely (~1:100)..."
>> About(~1:500) in our soc
>Yes, what I showed was just an example, put the real numbers into the comment.
* This happens rarely (~1:500) 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))
>>
>> >...and here
>>
>> > if (i2c_dw_is_controller_active(dev))
>>
>> >But please double check that I haven't made any mistakes in all this logic.
>>
>> Last week , You suggested that I used this
>> i2c_dw_is_master_idling(dev) keep using i2c_dw_is_master_idling(dev) , Ok?
See above.
> >> + dev_err(dev->dev, "I2C master not idling\n");
I will be off work, If there are still emails that I have not been replied to,
I will reply to your email immediately after going to work tomorrow.
Thanks you for your suggestion!
------------------------------------------
Best Regards
Kimriver Liu
On Tue, Sep 10, 2024 at 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
> >-----Original Message-----
> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >Sent: 2024年9月10日 18:45
> >On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
> >> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> >Sent: 2024年9月10日 17:03
> >> >at 02:13:09PM +0800, Kimriver Liu wrote:
...
> > >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
> >>
> >> >Sorry if I made a mistake, but again, looking at the usage you have
> >> >again negation here and there...
> >
> >> > i2c_dw_is_controller_active
> >>
> >> > (note new terminology, dunno if it makes sense start using it in
> >> > function names, as we have more of them following old style)
> >>
> >> Last week , You suggested that I used this
> >> i2c_dw_is_master_idling(dev)
>
> >Yes, sorry about that. I did maybe not clearly get how it is going to look like.
>
> >> >> +{
> >> >> + u32 status;
> >> >> +
> >> >> + regmap_read(dev->map, DW_IC_STATUS, &status);
> >> >> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> >> + return true;
> >>
> >> return false;
> >>
> >> >> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> >> + 1100, 20000);
> >>
> >> >...and drop !.
> >>
> >> We reproduce this issue in RTL simulation(About(~1:500) in our soc).
> >> It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling
> >> before disabling I2C when I2C transfer completed. as described in
> >> the DesignWare I2C databook(Flowchart for DW_apb_i2c Controller)
>
> >Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
>
> as described in the DesignWare I2C databook:
> DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
> Controller FSM Activity Status. When the Controller Finite
> State Machine (FSM) is not in the IDLE state, this bit is set.
> Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of
> SLV_ACTIVITY and MST_ACTIVITY bits.
> Values:
> ■ 0x1 (ACTIVE): Controller not idle
> ■ 0x0 (IDLE): Controller is idle
>
> We need waiting DW_IC_STATUS.MST_ACTIVITY idling,
> If Controller not idle, Wait for a while.
> Return value:
> false(0): Controller is idle
> timeout(-110): Controller activity
>
> Ok, change the function name i2c_dw_is_master_idling(dev) to i2c_dw_is_controller_active(dev)
> it seems more reasonable
>
> static int i2c_dw_is_controller_ active(struct dw_i2c_dev *dev)
> {
> u32 status;
>
> regmap_read(dev->map, DW_IC_STATUS, &status);
> if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> return false;
>
> return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> 1100, 20000);
> }
Yes, thank you. This is pure readability wise, you may actually leave the above
text as a comment on top of that helper. It will add a value of understanding
what's behind the scenes.
> >> >> +}
...
> I will be off work, If there are still emails that I have not been replied
> to, I will reply to your email immediately after going to work tomorrow.
No problem. Just keep your time, proof-read and test the v9 before sending and
I believe it will be the last iteration. Thank you for your patience and energy
to push this change forward!
...
> Thanks you for your suggestion!
You are welcome!
--
With Best Regards,
Andy Shevchenko
Hi Andy
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>Sent: 2024年9月10日 19:59
>To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
>Cc: 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
>Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>On Tue, Sep 10, 2024 at 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
>> >-----Original Message-----
>> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> >Sent: 2024年9月10日 18:45
>> >On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
>> >> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> >> >Sent: 2024年9月10日 17:03
>> >> >at 02:13:09PM +0800, Kimriver Liu wrote:
>
>...
>
>> > >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
>> >>
>> >> >Sorry if I made a mistake, but again, looking at the usage you
>> >> >have again negation here and there...
>> >
>> >> > i2c_dw_is_controller_active
>> >>
>> >> > (note new terminology, dunno if it makes sense start using it in
>> >> > function names, as we have more of them following old style)
>> >>
>> >> Last week , You suggested that I used this
>> >> i2c_dw_is_master_idling(dev)
>>
>> >Yes, sorry about that. I did maybe not clearly get how it is going to look like.
>>
>> >> >> +{
>> >> >> + u32 status;
>> >> >> +
>> >> >> + regmap_read(dev->map, DW_IC_STATUS, &status);
>> >> >> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> >> >> + return true;
>> >>
>> >> return false;
>> >>
>> >> >> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> >> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> >> >> + 1100, 20000);
>> >>
>> >> >...and drop !.
>> >>
>> >> We reproduce this issue in RTL simulation(About(~1:500) in our soc).
>> >> It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling
>> >> before disabling I2C when I2C transfer completed. as described in
>> >> the DesignWare I2C databook(Flowchart for DW_apb_i2c Controller)
>>
>> >Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
>>
>> as described in the DesignWare I2C databook:
>> DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
>> Controller FSM Activity Status. When the Controller Finite State
>> Machine (FSM) is not in the IDLE state, this bit is set.
>> Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of SLV_ACTIVITY
>> and MST_ACTIVITY bits.
>> Values:
>> ■ 0x1 (ACTIVE): Controller not idle
>> ■ 0x0 (IDLE): Controller is idle
>>
>> We need waiting DW_IC_STATUS.MST_ACTIVITY idling, If Controller not
>> idle, Wait for a while.
>> Return value:
>> false(0): Controller is idle
>> timeout(-110): Controller activity
>>
>> Ok, change the function name i2c_dw_is_master_idling(dev) to
>> i2c_dw_is_controller_active(dev) it seems more reasonable
>>
Change above text as a comment:
/*
* This functions waits controller idling before disabling I2C
* When the controller is not in the IDLE state,
* MST_ACTIVITY bit (IC_STATUS[5]) is set:
* 0x1 (ACTIVE): Controller not idle
* 0x0 (IDLE): Controller is idle
* The function is called after returning the end of the current transfer
* Returns:
* Return 0 as controller IDLE,
* Return a negative errno as controller ACTIVE
*/
>> static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev) {
>> u32 status;
>>
>> regmap_read(dev->map, DW_IC_STATUS, &status);
>> if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> return 0;
>>
>> return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> 1100, 20000);
>> }
>Yes, thank you. This is pure readability wise, you may actually leave the above text as a comment on top of that helper. It will add a value of understanding what's behind the scenes.
> >> >> +}
...
>> I will be off work, If there are still emails that I have not been
>> replied to, I will reply to your email immediately after going to work tomorrow.
>No problem. Just keep your time, proof-read and test the v9 before sending and I believe it will be the last iteration. Thank you for your patience and energy to push this change forward!
After the testing and validation are completed, I will resend v9 version.
Thank you!
>
>...
>
>> Thanks you for your suggestion!
>You are welcome!
------------------------------------------
Best Regards
Kimriver Liu
On Wed, Sep 11, 2024 at 01:37:43AM +0000, Liu Kimriver/刘金河 wrote:
> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >Sent: 2024年9月10日 19:59
> >On Tue, Sep 10, 2024 at 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
> >> >-----Original Message-----
> >> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> >Sent: 2024年9月10日 18:45
> >> >On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
> >> >> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> >> >Sent: 2024年9月10日 17:03
> >> >> >at 02:13:09PM +0800, Kimriver Liu wrote:
...
> >> > >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
> >> >>
> >> >> >Sorry if I made a mistake, but again, looking at the usage you
> >> >> >have again negation here and there...
> >> >
> >> >> > i2c_dw_is_controller_active
> >> >>
> >> >> > (note new terminology, dunno if it makes sense start using it in
> >> >> > function names, as we have more of them following old style)
> >> >>
> >> >> Last week , You suggested that I used this
> >> >> i2c_dw_is_master_idling(dev)
> >>
> >> >Yes, sorry about that. I did maybe not clearly get how it is going to look like.
> >>
> >> >> >> +{
> >> >> >> + u32 status;
> >> >> >> +
> >> >> >> + regmap_read(dev->map, DW_IC_STATUS, &status);
> >> >> >> + if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> >> >> + return true;
> >> >>
> >> >> return false;
> >> >>
> >> >> >> + return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> >> >> + !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> >> >> + 1100, 20000);
> >> >>
> >> >> >...and drop !.
> >> >>
> >> >> We reproduce this issue in RTL simulation(About(~1:500) in our soc).
> >> >> It is necessary to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling
> >> >> before disabling I2C when I2C transfer completed. as described in
> >> >> the DesignWare I2C databook(Flowchart for DW_apb_i2c Controller)
> >>
> >> >Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
> >>
> >> as described in the DesignWare I2C databook:
> >> DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
> >> Controller FSM Activity Status. When the Controller Finite State
> >> Machine (FSM) is not in the IDLE state, this bit is set.
> >> Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of SLV_ACTIVITY
> >> and MST_ACTIVITY bits.
> >> Values:
> >> ■ 0x1 (ACTIVE): Controller not idle
> >> ■ 0x0 (IDLE): Controller is idle
> >>
> >> We need waiting DW_IC_STATUS.MST_ACTIVITY idling, If Controller not
> >> idle, Wait for a while.
> >> Return value:
> >> false(0): Controller is idle
> >> timeout(-110): Controller activity
> >>
> >> Ok, change the function name i2c_dw_is_master_idling(dev) to
> >> i2c_dw_is_controller_active(dev) it seems more reasonable
>
> Change above text as a comment:
>
> /*
> * This functions waits controller idling before disabling I2C
> * When the controller is not in the IDLE state,
> * MST_ACTIVITY bit (IC_STATUS[5]) is set:
> * 0x1 (ACTIVE): Controller not idle
> * 0x0 (IDLE): Controller is idle
> * The function is called after returning the end of the current transfer
> * Returns:
> * Return 0 as controller IDLE,
> * Return a negative errno as controller ACTIVE
Why does it return a non-boolean value again?
> */
>
> >> static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev) {
> >> u32 status;
> >>
> >> regmap_read(dev->map, DW_IC_STATUS, &status);
> >> if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> return 0;
> >>
> >> return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> 1100, 20000);
> >> }
>
> >Yes, thank you. This is pure readability wise, you may actually leave the
> >above text as a comment on top of that helper. It will add a value of
> >understanding what's behind the scenes.
>
> > >> >> +}
--
With Best Regards,
Andy Shevchenko
© 2016 - 2024 Red Hat, Inc.