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