HI, Andy
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>Sent: 2024年9月10日 18:45
>To: Liu Kimriver/刘金河 <kimriver.liu@xxxxxxxxxxxx>
>Cc: jarkko.nikula@xxxxxxxxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx; jsd@xxxxxxxxxxxx; andi.shyti@xxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>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@xxxxxxxxxxxxxxx>
>> >Sent: 2024年9月10日 17:03
>> >To: Liu Kimriver/刘金河 <kimriver.liu@xxxxxxxxxxxx> 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
