On Fri, 22 Feb 2019 15:51:32 -0800
Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> * Andreas Kemnade <andreas@xxxxxxxxxxxx> [190222 20:08]:
> > Some more research:
> > what failed is this:
> > static int
> > omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > {
> > struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
> > int i;
> > int r;
> >
> > r = pm_runtime_get_sync(omap->dev);
> > if (r < 0) {
> > we get -EACCESS there.
>
> OK
>
> > It is indeed the case that
> > handle_twl4030_pih is called before i2c runtime resume.
> > Since we have threaded irq here, we can do some sleep in case of
> > problems. So this dirty hack helps:
> >
> > diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> > index b16c16f194fd..6c729c4ec9b0 100644
> > --- a/drivers/mfd/twl4030-irq.c
> > +++ b/drivers/mfd/twl4030-irq.c
> > @@ -34,6 +34,7 @@
> > #include <linux/of.h>
> > #include <linux/irqdomain.h>
> > #include <linux/mfd/twl.h>
> > +#include <linux/delay.h>
> >
> > #include "twl-core.h"
> >
> > @@ -298,7 +299,11 @@ static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> > REG_PIH_ISR_P1);
> > if (ret) {
> > pr_warn("twl4030: I2C error %d reading PIH ISR\n", ret);
> > - return IRQ_NONE;
> > + msleep(10);
> > + ret = twl_i2c_read_u8(TWL_MODULE_PIH, &pih_isr,
> > + REG_PIH_ISR_P1);
> > + if (ret)
> > + return IRQ_NONE;
> > }
> >
> > while (pih_isr) {
>
> How about just check for -EACCESS and return IRQ_NONE without
> warning here? And only warn for other errors.
>
well, then we have one error message less. That does not help much.
The irq is called again and again until:
[ 38.590881] twl: Read failed (mod 1, reg 0x01 count 1)
[ 38.590942] omap i2c get runtime failed: -13
[ 38.590972] twl: Read failed (mod 1, reg 0x01 count 1)
[ 38.591735] sched: RT throttling activated
[ 38.648101] omap i2c resume
[ 282.062530] OOM killer enabled.
[ 282.065826] Restarting tasks ...
so resuming takes 4 minutes. That is not acceptable.
> Then in Linux next, we now have new i2c_mark_adapter_suspended(),
> so later on if we have something like is_i2c_adapter_suspended()
> that could be added. But that's not going to be available for
> v5.0 kernels, so for the fix I'd go with just -EACCES + IRQ_NONE.
>
> > So if for some reason handle_twl4030_pih is called late enough, we will
> > not have those problems.
> > So maybe we just need to add a suspend/resume callback pair in
> > twl-core.c and disable/enable the pih there? So when the pih is run,
> > runtime_pm is ready, so you are allowed to do pm_runtime_get()
> >
> > To wake up from suspend, it is sufficient to have
> >
> > twl4030_pins: pinmux_twl4030_pins {
> > pinctrl-single,pins = <
> > OMAP3_CORE1_IOPAD(0x21e0, PIN_INPUT_PULLUP | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* sys_nirq.sys_nirq */
> > >;
> > };
>
> Well PIN_OFF_WAKEUPENABLE is nowadays managed with Linux generic
> wakeirqs with dev_pm_set_dedicated_wake_irq(). But in this case
> you should not even need to set a pad wake up as i2c1 is in
> always-on domain.
>
> Because of the RTC on twl, it's best to not add new dependencies
> pin muxing and generic wakeirqs that might be needed if we mask
> the twl interrupt for suspend.
>
well, that is the default in twl4030_omap3.dtsi
so it is nothing new.
Regards,
Andreas
Attachment:
pgpW1iokqkgmy.pgp
Description: OpenPGP digital signature
