"Strashko, Grygorii" <grygorii.strashko@xxxxxx> writes:
> Hi Kevin,
>
> yep, [1] is the same fix - thanks.
> Hi Kalle,
>
> i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig)
> Could you check it with your use case, pls? (just to be sure that idea is right)
I think the ideas is right, but [1] introduces more potential problems
since it disables the IRQ at the INTC well before being disabled at the
device. With runtime PM autosuspend timeouts, that means any IRQs
firing during the autosuspend delay will be lost, which basically
nullifies the use of autosuspend.
Since Shubhrajyoti didn't respond to my runtime PM comments on the
original patch, I'll respin this patch by moving the INTC disable/enable
into the runtime PM callbacks and make the changelog a bit more clear.
Grygorii, that pm_runtime_resume() change is needed to. Can you spin a
patch with just that change with a nice descriptive changelog about why
it is needed? Thanks.
Kevin
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a0e49f6..cb09e20 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (IS_ERR_VALUE(r))
> goto out;
>
> + /* We have the bus, enable IRQ */
> + enable_irq(dev->irq);
> +
> r = omap_i2c_wait_for_bb(dev);
> if (r < 0)
> goto out;
> @@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> r = num;
>
> omap_i2c_wait_for_bb(dev);
> + disable_irq(dev->irq);
> out:
> pm_runtime_put(dev->dev);
> return r;
> @@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev)
> omap_i2c_isr;
> r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev);
>
> + /* We enable IRQ only when request for I2C from master */
> + disable_irq(dev->irq);
> +
> if (r) {
> dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
> goto err_unuse_clocks;
> @@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
> }
> #endif /* CONFIG_PM_RUNTIME */
>
> +static int omap_i2c_suspend(struct device *dev)
> +{
> + int ret;
> +
> + /*
> + * Enable I2C device, so it will be accessible during
> + * later stages of suspending when device Runtime PM is disabled.
> + * I2C device will be turned off at "noirq" suspend stage.
> + */
> + ret = pm_runtime_resume(dev);
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
> +
> static struct dev_pm_ops omap_i2c_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
> SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> omap_i2c_runtime_resume, NULL)
> };
>
> - Grygorii
> ________________________________________
> From: Kevin Hilman [khilman@xxxxxxxxxxxxxxxxxxx]
> Sent: Friday, October 12, 2012 5:34 PM
> To: Strashko, Grygorii
> Cc: Kalle Jokiniemi; linux-i2c@xxxxxxxxxxxxxxx; w.sang@xxxxxxxxxxxxxx; ben-linux@xxxxxxxxx; tony@xxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; Datta, Shubhrajyoti; Kankroliwala, Huzefa
> Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
>
> "Strashko, Grygorii" <grygorii.strashko@xxxxxx> writes:
>
>> Hi All,
>>
>> Sorry, for the late reply.
>> + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
>
> Hi Grygorii, thanks for reviewing. I was hoping you would have some
> ideas here as this was sounding familiar to something you had
> mentioned elsewhere.
>
>> Regarding this patch - from my point of view, it fixes corner case and not an issue in general.
>> Let take a look on resume sequence:
>> - platform resume
>> - syscore resume
>> - resume_noirq
>> - enable IRQs - resume_device_irqs()
>> |- at this point IRQ handler will be invoked if IRQ state is IRQS_PENDING.
>> |- so, the I2C device IRQ handler may be called at time when I2C adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. (I2C device and I2C adapter may use different physical IRQ lines)
>> - resume_late
>> |- enable I2C bus IRQ
>>
>> Possibly, the better way is to enable/disable I2C bus IRQ when needed - in our case in omap_i2c_xfer().
>> We use such approach in Android kernel 3.4
>> (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b61760aaaa07365e)
>
> I agree, that should work and cover the cases where I2C is used by other
> processors also. Shubhrajyoti already posted something similar[1] but
> it needed some rework (comments from Russell and myself.)
>
> Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or
> follow up with the earlier patch[1]?
>
> Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can
> come up with a broader solution. Thanks.
>
> Kevin
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
