Received-SPF: pass (google.com: domain of linux-kernel+bounces-154926-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <c5ed5bed-9c93-47eb-8277-d78e12e96b42@bootlin.com>
Date: Tue, 23 Apr 2024 12:53:42 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 01/11] gpio: pca953x: move suspend()/resume() to
 suspend_noirq()/resume_noirq()
To: Andy Shevchenko <andy.shevchenko@gmail.com>,
 Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
 Bartosz Golaszewski <brgl@bgdev.pl>, Tony Lindgren <tony@atomide.com>,
 Aaro Koskinen <aaro.koskinen@iki.fi>,
 Janusz Krzysztofik <jmkrzyszt@gmail.com>, Vignesh R <vigneshr@ti.com>,
 Andi Shyti <andi.shyti@kernel.org>, Peter Rosin <peda@axentia.se>,
 Lorenzo Pieralisi <lpieralisi@kernel.org>,
 =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
 Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
 Siddharth Vadapalli <s-vadapalli@ti.com>, linux-gpio@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
 linux-i2c@vger.kernel.org, linux-pci@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, gregory.clement@bootlin.com,
 theo.lebrun@bootlin.com, thomas.petazzoni@bootlin.com, u-kumar1@ti.com,
 Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
 Wolfram Sang <wsa+renesas@sang-engineering.com>,
 Linux-Renesas <linux-renesas-soc@vger.kernel.org>
References: <20240102-j7200-pcie-s2r-v5-0-4b8c46711ded@bootlin.com>
 <20240102-j7200-pcie-s2r-v5-1-4b8c46711ded@bootlin.com>
 <CAMuHMdVnKX23yi7ir1LVxfXAMeeWMFzM+cdgSSTNjpn1OnC2xw@mail.gmail.com>
 <CAHp75Vf+F3ArczHQ+nSmP4uFvRdMAQWufmR6xR0xtbHfVvFm-g@mail.gmail.com>
Content-Language: en-US
From: Thomas Richard <thomas.richard@bootlin.com>
In-Reply-To: <CAHp75Vf+F3ArczHQ+nSmP4uFvRdMAQWufmR6xR0xtbHfVvFm-g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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