[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZuAjMmr7q4f8VJpA@smile.fi.intel.com>
Date: Tue, 10 Sep 2024 13:45:06 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
Cc: "jarkko.nikula@...ux.intel.com" <jarkko.nikula@...ux.intel.com>,
"mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
"jsd@...ihalf.com" <jsd@...ihalf.com>,
"andi.shyti@...nel.org" <andi.shyti@...nel.org>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.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@...ux.intel.com>
> >Sent: 2024年9月10日 17:03
> >To: Liu Kimriver/刘金河 <kimriver.liu@...ngine.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
Powered by blists - more mailing lists