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

[Andreas Kemnade]

On Fri, 22 Feb 2019 09:25:04 -0800
Tony Lindgren <tony at atomide.com> wrote:

> * Andreas Kemnade <andreas at kemnade.info> [190222 17:09]:
> > Tony Lindgren <tony at atomide.com> wrote:  
> > > It's probaby some twl related code is (wrongly) trying to read/write
> > > registers at noirq suspend level. Sounds like some driver tagged with
> > > noirq suspend ops while it should not.
> > >   
> > static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> > in
> > drivers/mfd/twl4030-irq.c
> > is doing the failed i2c access.  
> 
> I wonder if tagging drivers/mfd/twl4030-irq.c request_threaded_irq
> with IRQF_NO_SUSPEND in addition to IRQF_ONESHOT might help if this

IRQF_NO_SUSPEND does not help.

> triggers right after resume_device_irqs()? Or do we already have
> IRQF_NO_SUSPEND set and some interrupt like USB battery charging
> is triggering during suspend?

well, if we are doing rtcwake, we could expect to have a twl4030 rtc
interrupt sitting behind the pih.
I would consider usb battery charging irq here a bug because
twl4030_charger and phy-twl4030_usb are modules and not loaded.

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.

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) {


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 */
                >;
        };

I can even do
@ -752,7 +757,9 @@ int twl4030_init_irq(struct device *dev, int irq_num)
                dev_err(dev, "could not claim irq%d: %d\n", irq_num, status);
                goto fail_rqirq;
        }
+#if 0
        enable_irq_wake(irq_num);
+#endif
 
        return irq_base;
 fail_rqirq:

at least here. So disabling that irq on suspend might be an option.

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/20190222/1f42e382/attachment.asc>