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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 22 Feb 2019 09:25:04 -0800
Tony Lindgren <tony@xxxxxxxxxxx> wrote:

> * Andreas Kemnade <andreas@xxxxxxxxxxxx> [190222 17:09]:
> > Tony Lindgren <tony@xxxxxxxxxxx> 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

Attachment: pgpDARJ_8pPzj.pgp
Description: OpenPGP digital signature


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux