[Letux-kernel] [PATCHv2] i2c: omap: Use noirq system sleep pm ops to idle device for suspend

[Andreas Kemnade]

On Fri, 22 Feb 2019 15:51:32 -0800
Tony Lindgren <tony at atomide.com> wrote:

> * Andreas Kemnade <andreas at kemnade.info> [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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.goldelico.com/pipermail/letux-kernel/attachments/20190223/d199f80a/attachment-0001.asc>