diff mbox series

[v5,01/11] gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()

Message ID 20240102-j7200-pcie-s2r-v5-1-4b8c46711ded@bootlin.com
State Superseded
Headers show
Series Add suspend to ram support for PCIe on J7200 | expand

Commit Message

Thomas Richard April 16, 2024, 1:29 p.m. UTC 
Some IOs can be needed during suspend_noirq()/resume_noirq().
So move suspend()/resume() to noirq.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven April 23, 2024, 9:42 a.m. UTC | #1 
Hi Thomas,

On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch, which is now commit 86eb98127332748f ("gpio:
pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()")
in i2c-host/i2c/i2c-host.

This patch causes regressions on e.g. Salvator-XS.

    s2idle:

         Freezing user space processes
         Freezing user space processes completed (elapsed 0.006 seconds)
         OOM killer disabled.
         Freezing remaining freezable tasks
         Freezing remaining freezable tasks completed (elapsed 0.003 seconds)
         sd 0:0:0:0: [sda] Synchronizing SCSI cache
         ata1.00: Entering standby power mode
        +i2c-rcar e66d8000.i2c: error -16 : 10000005
        +pca953x 4-0020: Failed to sync GPIO dir registers: -16
        +pca953x 4-0020: Failed to restore register map: -16
        +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -16
        +pca953x 4-0020: PM: failed to resume async noirq: error -16

    s2ram:

         Detected VIPT I-cache on CPU7
         CPU7: Booted secondary processor 0x0000000103 [0x410fd034]
         CPU7 is up
        +i2c-rcar e66d8000.i2c: error -110 : 10000001
        +pca953x 4-0020: Failed to sync GPIO dir registers: -110
        +pca953x 4-0020: Failed to restore register map: -110
        +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -110
        +pca953x 4-0020: PM: failed to resume async noirq: error -110
         usb usb1: root hub lost power or was reset
         ...
         PM: suspend exit
         ata1: link resume succeeded after 1 retries
        -ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
        -sd 0:0:0:0: [sda] Starting disk
        -ata1.00: configured for UDMA/133
        -ata1.00: Entering active power mode
        +ata1: SATA link down (SStatus 0 SControl 300)
        +ata1: link resume succeeded after 1 retries
        +ata1: SATA link down (SStatus 0 SControl 300)
        +ata1: limiting SATA link speed to <unknown>
        +ata1: link resume succeeded after 1 retries
        +ata1: SATA link down (SStatus 0 SControl 3F0)
        +ata1.00: disable device
        +ata1.00: detaching (SCSI 0:0:0:0)
        +sd 0:0:0:0: [sda] Synchronizing SCSI cache
        +sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK

    When trying to read from /dev/sda afterwards:

        ata1: link resume succeeded after 1 retries
        ata1: SATA link down (SStatus 0 SControl 3F0)
        ata1.00: disable device
        ata1.00: detaching (SCSI 0:0:0:0)
        device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x80700 phys_seg 4 prio class 0
        device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x0 phys_seg 1 prio class 0
        Buffer I/O error on dev sda, logical block 0, async page read
        sd 0:0:0:0: [sda] Synchronizing SCSI cache
        sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK

All issues above are fixed by reverting this commit.

> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -1234,7 +1234,7 @@ static void pca953x_save_context(struct pca953x_chip *chip)
>         regcache_cache_only(chip->regmap, true);
>  }
>
> -static int pca953x_suspend(struct device *dev)
> +static int pca953x_suspend_noirq(struct device *dev)
>  {
>         struct pca953x_chip *chip = dev_get_drvdata(dev);
>
> @@ -1248,7 +1248,7 @@ static int pca953x_suspend(struct device *dev)
>         return 0;
>  }
>
> -static int pca953x_resume(struct device *dev)
> +static int pca953x_resume_noirq(struct device *dev)
>  {
>         struct pca953x_chip *chip = dev_get_drvdata(dev);
>         int ret;
> @@ -1268,7 +1268,8 @@ static int pca953x_resume(struct device *dev)
>         return ret;
>  }
>
> -static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
> +static DEFINE_NOIRQ_DEV_PM_OPS(pca953x_pm_ops,
> +                              pca953x_suspend_noirq, pca953x_resume_noirq);
>
>  /* convenience to stop overlong match-table lines */
>  #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))
>

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko April 23, 2024, 10:34 a.m. UTC | #2 
On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:

...

>         +i2c-rcar e66d8000.i2c: error -16 : 10000005

It probably means that I²C host controller is already in power off
mode and can't serve anymore.

>         +pca953x 4-0020: Failed to sync GPIO dir registers: -16
>         +pca953x 4-0020: Failed to restore register map: -16
>         +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
> returns -16
>         +pca953x 4-0020: PM: failed to resume async noirq: error -16

Yeah, with this it's kinda forcing _every_ I²C host controller PM to
be moved also to noirq() or alike.

...

> All issues above are fixed by reverting this commit.

Let's revert as we close to the end of the cycle and this is not
something that can be fixed ASAP in my opinion.

...

Thanks for the report, It's a pity that it wasn't tested before from
the mailing list...
Thomas Richard April 23, 2024, 10:53 a.m. UTC | #3 
On 4/23/24 12:34, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
>> <thomas.richard@bootlin.com> wrote:
> 
> ...
> 
>>         +i2c-rcar e66d8000.i2c: error -16 : 10000005
> 
> It probably means that I²C host controller is already in power off
> mode and can't serve anymore.

Hello,

Yes the i2c controller is already off.
In fact it's the same issue I had with the i2c-omap driver.
In suspend-noirq, the runtime pm is disabled, so you can't wakeup a
device. More details available in this thread [1].
So the trick is to wakeup the device during suspend (like I did for the
i2c-omap driver [2].

[1]
https://lore.kernel.org/all/f68c9a54-0fde-4709-9d2f-0d23a049341b@bootlin.com/
[2]
https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v5-2-4b8c46711ded@bootlin.com/

I think the patch below should fix the issue.

--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1232,7 +1232,7 @@ static void rcar_i2c_remove(struct platform_device
*pdev)
        pm_runtime_disable(dev);
 }

-static int rcar_i2c_suspend(struct device *dev)
+static int rcar_i2c_suspend_noirq(struct device *dev)
 {
        struct rcar_i2c_priv *priv = dev_get_drvdata(dev);

@@ -1240,7 +1240,7 @@ static int rcar_i2c_suspend(struct device *dev)
        return 0;
 }

-static int rcar_i2c_resume(struct device *dev)
+static int rcar_i2c_resume_noirq(struct device *dev)
 {
        struct rcar_i2c_priv *priv = dev_get_drvdata(dev);

@@ -1248,8 +1248,23 @@ static int rcar_i2c_resume(struct device *dev)
        return 0;
 }

+static int rcar_i2c_suspend(struct device *dev)
+{
+       pm_runtime_get_sync(dev);
+
+       return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+       pm_runtime_put(dev);
+
+       return 0;
+}
+
 static const struct dev_pm_ops rcar_i2c_pm_ops = {
-       NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
+       NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend_noirq,
rcar_i2c_resume_noirq)
+       SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
 };

 static struct platform_driver rcar_i2c_driver = {

> 
>>         +pca953x 4-0020: Failed to sync GPIO dir registers: -16
>>         +pca953x 4-0020: Failed to restore register map: -16
>>         +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
>> returns -16
>>         +pca953x 4-0020: PM: failed to resume async noirq: error -16
> 
> Yeah, with this it's kinda forcing _every_ I²C host controller PM to
> be moved also to noirq() or alike.

Yes indeed.
But this controller is already in noirq().
So the issue was already there.
We never saw it because we never did i2c accesses in noirq().

Best Regards,
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 00ffa168e405..6e495fc67a93 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1234,7 +1234,7 @@  static void pca953x_save_context(struct pca953x_chip *chip)
 	regcache_cache_only(chip->regmap, true);
 }
 
-static int pca953x_suspend(struct device *dev)
+static int pca953x_suspend_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 
@@ -1248,7 +1248,7 @@  static int pca953x_suspend(struct device *dev)
 	return 0;
 }
 
-static int pca953x_resume(struct device *dev)
+static int pca953x_resume_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 	int ret;
@@ -1268,7 +1268,8 @@  static int pca953x_resume(struct device *dev)
 	return ret;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
+static DEFINE_NOIRQ_DEV_PM_OPS(pca953x_pm_ops,
+			       pca953x_suspend_noirq, pca953x_resume_noirq);
 
 /* convenience to stop overlong match-table lines */
 #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))