From: Uwe Kleine-König <kleine-koenig@xxxxxxxxxxxxxx> Sent: Friday, August 15, 2014 3:27 AM
>To: Duan Fugang-B38611
>Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH] i2c: imx: double check IIF in case interrupt lost
>
>Hello,
>
>On Thu, Aug 14, 2014 at 10:09:42AM +0000, fugang.duan@xxxxxxxxxxxxx wrote:
>> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> Sent:
>> Thursday, August 14, 2014 5:42 PM
>> >On Thu, Aug 14, 2014 at 04:29:14PM +0800, Fugang Duan wrote:
>> >> diff --git a/drivers/i2c/busses/i2c-imx.c
>> >> b/drivers/i2c/busses/i2c-imx.c index aa8bc14..4b63771 100644
>> >> --- a/drivers/i2c/busses/i2c-imx.c
>> >> +++ b/drivers/i2c/busses/i2c-imx.c
>> >> @@ -285,11 +285,17 @@ static int i2c_imx_bus_busy(struct
>> >> imx_i2c_struct *i2c_imx, int for_busy)
>> >>
>> >> static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx) {
>> >> + unsigned int temp;
>> >> +
>> >> wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF,
>HZ
>> >> / 10);
>> >>
>> >> if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
>> >> - dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n",
>__func__);
>> >> - return -ETIMEDOUT;
>> >> + /* Double check IIF to avoid interrupt lost */
>> >> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>> >> + if (!(temp & I2SR_IIF)) {
>> >> + dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n",
>> >__func__);
>> >> + return -ETIMEDOUT;
>> >> + }
>> >This smells fishy. If possible the fix should be not to loose an
>interrupt.
>> >Can you
>> >
>> >If I2SR_IIF is unset in i2c_imx->i2csr this means that
>> >i2c_imx_trx_complete was already run before since the last irq
>triggered.
>> >But if then imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) returns the IIF
>> >flag set why doesn't the irq trigger? That would mean there is a
>> >hardware bug, doesn't it? Is there a related Errata? Does it apply to
>> >all SoCs using this driver? Did you check that the irq handling in the
>driver isn't racy?
>> After we debug, it seems that irq lost in the stress case.
>> Because we increase the timeout value to one "HZ" in
>> wait_event_timeout(), and it Return "0" means no interrupt. But when we
>read I2SR, IIF bit is set.
>> There have no racy in the driver code, so we judge there have interrupt
>lost.
>>
>> After apply the patch, it really solve the issue.
>I'm still not convinced. Either there is a hardware problem (then
>Freescale should emit a proper errata for it when it's completely
>understood) or there is a software problem and then your fix looks wrong.
>Can you do the following please:
>
>Make the code read:
>
> static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
> {
> wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF,
>HZ / 10);
>
> if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
> unsigned int temp = imx_i2c_read_reg(i2c_imx,
>IMX_I2C_I2SR);
> if (!(temp & I2SR_IFF)) {
> dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n",
>__func__);
> return -ETIMEDOUT;
> } else {
> dev_info(&i2c_imx->adapter.dev, "<%s> Disable
>tracing\n", __func__);
> tracing_off();
> }
> }
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n",
>__func__);
> i2c_imx->i2csr = 0;
> return 0;
> }
>
>Then compile a kernel with CONFIG_FUNCTION_TRACER=y and before repeating
>your tests do:
>
> cd /sys/kernel/debug/tracing # assuming you have debugfs mounted
>accordingly
> echo function > current_tracer
> echo 1 > tracing_on
>
>And once the "Disable tracing" message appears extract
>
> /sys/kernel/debug/tracing/trace
>
>and send it to me.
>
>Best regards
>Uwe
Uwe, thanks for much.
I am sorry to tell you I don't have the reproduced platform for the issue since the
Issue was found at one platform of customer of freescale, and I debug the issue with
Customer, at last generate the patch for customer platform, test fine. So I want to
Submit the patch. In our platforms, it is not easy to reproduce the issue.
Thanks,
Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
