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

[Thomas Richard <thomas.richard@bootlin.com>]

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,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel