[PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Tommaso Merciai]

Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
suspend support for the Renesas I2C driver and following this change
on RZ/G3E the following WARNING is seen on entering suspend ...

[  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
[  134.285536] ------------[ cut here ]------------
[  134.290298] i2c i2c-2: Transfer while suspended
[  134.295174] WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
[  134.365507] Tainted: [W]=WARN
[  134.368485] Hardware name: Renesas SMARC EVK version 2 based on r9a09g047e57 (DT)
[  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214
[  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
[  134.391717] sp : ffff800083f23860
[  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27: ffff800082ed5d60
[  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
[  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936
[  134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006
[  134.423771] x17: 6f63657320313030 x16: 2e30206465737061 x15: ffff800083f23280
[  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09
[  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
[  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000
[  134.452495] x5 : 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000
[  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
[  134.466851] Call trace:
[  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P)
[  134.473715]  i2c_smbus_xfer+0xbc/0x120
[  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
[  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208]
[  134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208]
[  134.493226]  __rtc_read_time+0x44/0x88
[  134.497012]  rtc_read_time+0x3c/0x68
[  134.500622]  rtc_suspend+0x9c/0x170

The warning is triggered because I2C transfers can still be attempted
while the controller is already suspended, due to inappropriate ordering
of the system sleep callbacks.

Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ
phase, ensuring the adapter is fully quiesced after late suspend and
properly resumed before the early resume phase.

To support NOIRQ resume, the hardware initialization path must not invoke
runtime PM APIs. Split the init code so that the low-level hardware setup
can be executed without pm_runtime_get/put(). This avoids violating the
constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions
between runtime PM and system sleep (v2)"), which forbids runtime PM
calls during early resume.

Cc: stable@vger.kernel.org
Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
---
 drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 3e8f126cb7f7..9acc8936cdf7 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
 	.functionality = riic_func,
 };
 
-static int riic_init_hw(struct riic_dev *riic)
+static int __riic_init_hw(struct riic_dev *riic)
 {
-	int ret;
 	unsigned long rate;
 	unsigned long ns_per_tick;
 	int total_ticks, cks, brl, brh;
@@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
 		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
 		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
 
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret)
-		return ret;
-
 	/* Changing the order of accessing IICRST and ICE may break things! */
 	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
 	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
@@ -451,10 +446,25 @@ static int riic_init_hw(struct riic_dev *riic)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-	pm_runtime_put_autosuspend(dev);
 	return 0;
 }
 
+static int riic_init_hw(struct riic_dev *riic)
+{
+	struct device *dev = riic->adapter.dev.parent;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = __riic_init_hw(riic);
+
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
 static int riic_get_scl(struct i2c_adapter *adap)
 {
 	struct riic_dev *riic = i2c_get_adapdata(adap);
@@ -572,6 +582,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
 
+	platform_set_drvdata(pdev, riic);
+
 	/* Default 0 to save power. Can be overridden via sysfs for lower latency. */
 	pm_runtime_set_autosuspend_delay(dev, 0);
 	pm_runtime_use_autosuspend(dev);
@@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		goto out;
 
-	platform_set_drvdata(pdev, riic);
-
 	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
 	return 0;
 
@@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
 	.num_irqs = ARRAY_SIZE(riic_rzt2h_irqs),
 };
 
-static int riic_i2c_suspend(struct device *dev)
+static int riic_i2c_runtime_suspend(struct device *dev)
 {
 	struct riic_dev *riic = dev_get_drvdata(dev);
-	int ret;
-
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret)
-		return ret;
-
-	i2c_mark_adapter_suspended(&riic->adapter);
 
 	/* Disable output on SDA, SCL pins. */
 	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
 
-	pm_runtime_mark_last_busy(dev);
-	pm_runtime_put_sync(dev);
-
 	return reset_control_assert(riic->rstc);
 }
 
-static int riic_i2c_resume(struct device *dev)
+static int riic_i2c_runtime_resume(struct device *dev)
 {
 	struct riic_dev *riic = dev_get_drvdata(dev);
 	int ret;
@@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = riic_init_hw(riic);
+	ret = __riic_init_hw(riic);
 	if (ret) {
 		/*
 		 * In case this happens there is no way to recover from this
@@ -708,13 +708,30 @@ static int riic_i2c_resume(struct device *dev)
 		return ret;
 	}
 
+	return 0;
+}
+
+static int riic_i2c_suspend(struct device *dev)
+{
+	struct riic_dev *riic = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&riic->adapter);
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int riic_i2c_resume(struct device *dev)
+{
+	struct riic_dev *riic = dev_get_drvdata(dev);
+
 	i2c_mark_adapter_resumed(&riic->adapter);
 
-	return 0;
+	return pm_runtime_force_resume(dev);
 }
 
 static const struct dev_pm_ops riic_i2c_pm_ops = {
-	SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend, riic_i2c_resume)
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend, riic_i2c_resume)
+	RUNTIME_PM_OPS(riic_i2c_runtime_suspend, riic_i2c_runtime_resume, NULL)
 };
 
 static const struct of_device_id riic_i2c_dt_ids[] = {
-- 
2.43.0

Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Andi Shyti]

Hi Tommaso,

On Fri, Dec 12, 2025 at 12:58:57PM +0100, Tommaso Merciai wrote:
> Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
> suspend support for the Renesas I2C driver and following this change
> on RZ/G3E the following WARNING is seen on entering suspend ...
> 
> [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> [  134.285536] ------------[ cut here ]------------
> [  134.290298] i2c i2c-2: Transfer while suspended
> [  134.295174] WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> [  134.365507] Tainted: [W]=WARN
> [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on r9a09g047e57 (DT)
> [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214
> [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> [  134.391717] sp : ffff800083f23860
> [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27: ffff800082ed5d60
> [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936
> [  134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006
> [  134.423771] x17: 6f63657320313030 x16: 2e30206465737061 x15: ffff800083f23280
> [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09
> [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000
> [  134.452495] x5 : 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000
> [  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
> [  134.466851] Call trace:
> [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P)
> [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208]
> [  134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208]
> [  134.493226]  __rtc_read_time+0x44/0x88
> [  134.497012]  rtc_read_time+0x3c/0x68
> [  134.500622]  rtc_suspend+0x9c/0x170
> 
> The warning is triggered because I2C transfers can still be attempted
> while the controller is already suspended, due to inappropriate ordering
> of the system sleep callbacks.
> 
> Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ
> phase, ensuring the adapter is fully quiesced after late suspend and
> properly resumed before the early resume phase.
> 
> To support NOIRQ resume, the hardware initialization path must not invoke
> runtime PM APIs. Split the init code so that the low-level hardware setup
> can be executed without pm_runtime_get/put(). This avoids violating the
> constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions
> between runtime PM and system sleep (v2)"), which forbids runtime PM
> calls during early resume.
> 
> Cc: stable@vger.kernel.org
> Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

can I have an ack from Chris here?

...

> +static int riic_i2c_suspend(struct device *dev)
> +{
> +	struct riic_dev *riic = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&riic->adapter);
> +
> +	return pm_runtime_force_suspend(dev);

We should perhaps swap i2c_mark_adapter_suspended() and
pm_runtime_force_suspend()?

Andi

> +}

Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Claudiu Beznea]

Hi, Tommaso,

On 12/12/25 13:58, Tommaso Merciai wrote:
> Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
> suspend support for the Renesas I2C driver and following this change
> on RZ/G3E the following WARNING is seen on entering suspend ...
> 
> [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> [  134.285536] ------------[ cut here ]------------
> [  134.290298] i2c i2c-2: Transfer while suspended
> [  134.295174] WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> [  134.365507] Tainted: [W]=WARN
> [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on r9a09g047e57 (DT)
> [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214
> [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> [  134.391717] sp : ffff800083f23860
> [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27: ffff800082ed5d60
> [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936
> [  134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006
> [  134.423771] x17: 6f63657320313030 x16: 2e30206465737061 x15: ffff800083f23280
> [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09
> [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000
> [  134.452495] x5 : 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000
> [  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
> [  134.466851] Call trace:
> [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P)
> [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208]
> [  134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208]
> [  134.493226]  __rtc_read_time+0x44/0x88
> [  134.497012]  rtc_read_time+0x3c/0x68
> [  134.500622]  rtc_suspend+0x9c/0x170
> 
> The warning is triggered because I2C transfers can still be attempted
> while the controller is already suspended, due to inappropriate ordering
> of the system sleep callbacks.
> 
> Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ
> phase, ensuring the adapter is fully quiesced after late suspend and
> properly resumed before the early resume phase.
> 
> To support NOIRQ resume, the hardware initialization path must not invoke
> runtime PM APIs. Split the init code so that the low-level hardware setup
> can be executed without pm_runtime_get/put(). This avoids violating the
> constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions
> between runtime PM and system sleep (v2)"), which forbids runtime PM
> calls during early resume.
> 
> Cc: stable@vger.kernel.org
> Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> ---
>  drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index 3e8f126cb7f7..9acc8936cdf7 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
>  	.functionality = riic_func,
>  };
>  
> -static int riic_init_hw(struct riic_dev *riic)
> +static int __riic_init_hw(struct riic_dev *riic)
>  {
> -	int ret;
>  	unsigned long rate;
>  	unsigned long ns_per_tick;
>  	int total_ticks, cks, brl, brh;
> @@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
>  		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
>  		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
>  
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> -
>  	/* Changing the order of accessing IICRST and ICE may break things! */
>  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
>  	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> @@ -451,10 +446,25 @@ static int riic_init_hw(struct riic_dev *riic)
>  
>  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
>  
> -	pm_runtime_put_autosuspend(dev);
>  	return 0;
>  }
>  
> +static int riic_init_hw(struct riic_dev *riic)
> +{
> +	struct device *dev = riic->adapter.dev.parent;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = __riic_init_hw(riic);
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +
>  static int riic_get_scl(struct i2c_adapter *adap)
>  {
>  	struct riic_dev *riic = i2c_get_adapdata(adap);
> @@ -572,6 +582,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  
>  	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
>  
> +	platform_set_drvdata(pdev, riic);
> +
>  	/* Default 0 to save power. Can be overridden via sysfs for lower latency. */
>  	pm_runtime_set_autosuspend_delay(dev, 0);
>  	pm_runtime_use_autosuspend(dev);
> @@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto out;
>  
> -	platform_set_drvdata(pdev, riic);
> -
>  	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
>  	return 0;
>  
> @@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
>  	.num_irqs = ARRAY_SIZE(riic_rzt2h_irqs),
>  };
>  
> -static int riic_i2c_suspend(struct device *dev)
> +static int riic_i2c_runtime_suspend(struct device *dev)
>  {
>  	struct riic_dev *riic = dev_get_drvdata(dev);
> -	int ret;
> -
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> -
> -	i2c_mark_adapter_suspended(&riic->adapter);
>  
>  	/* Disable output on SDA, SCL pins. */
>  	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
>  
> -	pm_runtime_mark_last_busy(dev);
> -	pm_runtime_put_sync(dev);
> -
>  	return reset_control_assert(riic->rstc);

Reset assert/de-assert was moved to the RPM callbacks. Is this intended?
You may want to at least mention it in the commit description.

>  }
>  
> -static int riic_i2c_resume(struct device *dev)
> +static int riic_i2c_runtime_resume(struct device *dev)
>  {
>  	struct riic_dev *riic = dev_get_drvdata(dev);
>  	int ret;
> @@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = riic_init_hw(riic);
> +	ret = __riic_init_hw(riic);

Moving reset assert/de-assert and controller deinit/re-initialization on
runtime suspend/resume may increase the runtime suspend/resume latency. In
case of consecutive requests this may translate into controller performance
downgrade. If you keep it like this, you may want to increase the default
autosuspend delay, at least, to avoid controller reconfiguration after each
individual struct i2c_algorithm::xfer() call.

>  	if (ret) {
>  		/*
>  		 * In case this happens there is no way to recover from this
> @@ -708,13 +708,30 @@ static int riic_i2c_resume(struct device *dev)
>  		return ret;

This one could be dropped

>  	}
>  
> +	return 0;

And use here directly, like:

	return ret;

Thank you,
Claudiu

Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Tommaso Merciai]

Hi Andi,

Thanks for your review!

On Wed, Dec 17, 2025 at 01:10:27AM +0100, Andi Shyti wrote:
> Hi Tommaso,
> 
> On Fri, Dec 12, 2025 at 12:58:57PM +0100, Tommaso Merciai wrote:
> > Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
> > suspend support for the Renesas I2C driver and following this change
> > on RZ/G3E the following WARNING is seen on entering suspend ...
> > 
> > [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> > [  134.285536] ------------[ cut here ]------------
> > [  134.290298] i2c i2c-2: Transfer while suspended
> > [  134.295174] WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> > [  134.365507] Tainted: [W]=WARN
> > [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on r9a09g047e57 (DT)
> > [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214
> > [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> > [  134.391717] sp : ffff800083f23860
> > [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27: ffff800082ed5d60
> > [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> > [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936
> > [  134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006
> > [  134.423771] x17: 6f63657320313030 x16: 2e30206465737061 x15: ffff800083f23280
> > [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09
> > [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> > [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000
> > [  134.452495] x5 : 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000
> > [  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
> > [  134.466851] Call trace:
> > [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P)
> > [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> > [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> > [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208]
> > [  134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208]
> > [  134.493226]  __rtc_read_time+0x44/0x88
> > [  134.497012]  rtc_read_time+0x3c/0x68
> > [  134.500622]  rtc_suspend+0x9c/0x170
> > 
> > The warning is triggered because I2C transfers can still be attempted
> > while the controller is already suspended, due to inappropriate ordering
> > of the system sleep callbacks.
> > 
> > Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ
> > phase, ensuring the adapter is fully quiesced after late suspend and
> > properly resumed before the early resume phase.
> > 
> > To support NOIRQ resume, the hardware initialization path must not invoke
> > runtime PM APIs. Split the init code so that the low-level hardware setup
> > can be executed without pm_runtime_get/put(). This avoids violating the
> > constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions
> > between runtime PM and system sleep (v2)"), which forbids runtime PM
> > calls during early resume.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> 
> can I have an ack from Chris here?
> 
> ...
> 
> > +static int riic_i2c_suspend(struct device *dev)
> > +{
> > +	struct riic_dev *riic = dev_get_drvdata(dev);
> > +
> > +	i2c_mark_adapter_suspended(&riic->adapter);
> > +
> > +	return pm_runtime_force_suspend(dev);
> 
> We should perhaps swap i2c_mark_adapter_suspended() and
> pm_runtime_force_suspend()?

Please correct me if I'm wrong but I'm seeing similar pattern into:

 - drivers/i2c/busses/i2c-sprd.c -> sprd_i2c_suspend_noirq()

What do you think?

Thanks & Regards,
Tommaso

> 
> Andi
> 
> > +}

Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Tommaso Merciai]

Hi Claudiu,
Thanks for your review!


On Wed, Dec 17, 2025 at 11:41:43AM +0200, Claudiu Beznea wrote:
> Hi, Tommaso,
> 
> On 12/12/25 13:58, Tommaso Merciai wrote:
> > Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
> > suspend support for the Renesas I2C driver and following this change
> > on RZ/G3E the following WARNING is seen on entering suspend ...
> > 
> > [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> > [  134.285536] ------------[ cut here ]------------
> > [  134.290298] i2c i2c-2: Transfer while suspended
> > [  134.295174] WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> > [  134.365507] Tainted: [W]=WARN
> > [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on r9a09g047e57 (DT)
> > [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214
> > [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> > [  134.391717] sp : ffff800083f23860
> > [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27: ffff800082ed5d60
> > [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> > [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936
> > [  134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006
> > [  134.423771] x17: 6f63657320313030 x16: 2e30206465737061 x15: ffff800083f23280
> > [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09
> > [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> > [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000
> > [  134.452495] x5 : 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000
> > [  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
> > [  134.466851] Call trace:
> > [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P)
> > [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> > [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> > [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208]
> > [  134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208]
> > [  134.493226]  __rtc_read_time+0x44/0x88
> > [  134.497012]  rtc_read_time+0x3c/0x68
> > [  134.500622]  rtc_suspend+0x9c/0x170
> > 
> > The warning is triggered because I2C transfers can still be attempted
> > while the controller is already suspended, due to inappropriate ordering
> > of the system sleep callbacks.
> > 
> > Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ
> > phase, ensuring the adapter is fully quiesced after late suspend and
> > properly resumed before the early resume phase.
> > 
> > To support NOIRQ resume, the hardware initialization path must not invoke
> > runtime PM APIs. Split the init code so that the low-level hardware setup
> > can be executed without pm_runtime_get/put(). This avoids violating the
> > constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions
> > between runtime PM and system sleep (v2)"), which forbids runtime PM
> > calls during early resume.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > ---
> >  drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
> >  1 file changed, 41 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > index 3e8f126cb7f7..9acc8936cdf7 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
> >  	.functionality = riic_func,
> >  };
> >  
> > -static int riic_init_hw(struct riic_dev *riic)
> > +static int __riic_init_hw(struct riic_dev *riic)
> >  {
> > -	int ret;
> >  	unsigned long rate;
> >  	unsigned long ns_per_tick;
> >  	int total_ticks, cks, brl, brh;
> > @@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
> >  		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
> >  		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
> >  
> > -	ret = pm_runtime_resume_and_get(dev);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Changing the order of accessing IICRST and ICE may break things! */
> >  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
> >  	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> > @@ -451,10 +446,25 @@ static int riic_init_hw(struct riic_dev *riic)
> >  
> >  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> >  
> > -	pm_runtime_put_autosuspend(dev);
> >  	return 0;
> >  }
> >  
> > +static int riic_init_hw(struct riic_dev *riic)
> > +{
> > +	struct device *dev = riic->adapter.dev.parent;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __riic_init_hw(riic);
> > +
> > +	pm_runtime_put_autosuspend(dev);
> > +
> > +	return ret;
> > +}
> > +
> >  static int riic_get_scl(struct i2c_adapter *adap)
> >  {
> >  	struct riic_dev *riic = i2c_get_adapdata(adap);
> > @@ -572,6 +582,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
> >  
> >  	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
> >  
> > +	platform_set_drvdata(pdev, riic);
> > +
> >  	/* Default 0 to save power. Can be overridden via sysfs for lower latency. */
> >  	pm_runtime_set_autosuspend_delay(dev, 0);
> >  	pm_runtime_use_autosuspend(dev);
> > @@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto out;
> >  
> > -	platform_set_drvdata(pdev, riic);
> > -
> >  	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
> >  	return 0;
> >  
> > @@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
> >  	.num_irqs = ARRAY_SIZE(riic_rzt2h_irqs),
> >  };
> >  
> > -static int riic_i2c_suspend(struct device *dev)
> > +static int riic_i2c_runtime_suspend(struct device *dev)
> >  {
> >  	struct riic_dev *riic = dev_get_drvdata(dev);
> > -	int ret;
> > -
> > -	ret = pm_runtime_resume_and_get(dev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	i2c_mark_adapter_suspended(&riic->adapter);
> >  
> >  	/* Disable output on SDA, SCL pins. */
> >  	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
> >  
> > -	pm_runtime_mark_last_busy(dev);
> > -	pm_runtime_put_sync(dev);
> > -
> >  	return reset_control_assert(riic->rstc);
> 
> Reset assert/de-assert was moved to the RPM callbacks. Is this intended?
> You may want to at least mention it in the commit description.

Ack, thanks.
I will add that into the commit description.

> 
> >  }
> >  
> > -static int riic_i2c_resume(struct device *dev)
> > +static int riic_i2c_runtime_resume(struct device *dev)
> >  {
> >  	struct riic_dev *riic = dev_get_drvdata(dev);
> >  	int ret;
> > @@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = riic_init_hw(riic);
> > +	ret = __riic_init_hw(riic);
> 
> Moving reset assert/de-assert and controller deinit/re-initialization on
> runtime suspend/resume may increase the runtime suspend/resume latency. In
> case of consecutive requests this may translate into controller performance
> downgrade. If you keep it like this, you may want to increase the default
> autosuspend delay, at least, to avoid controller reconfiguration after each
> individual struct i2c_algorithm::xfer() call.


We can set autosuspend delay to 250 ms.
What do you think?

        pm_runtime_set_autosuspend_delay(dev, 250);

> 
> >  	if (ret) {
> >  		/*
> >  		 * In case this happens there is no way to recover from this
> > @@ -708,13 +708,30 @@ static int riic_i2c_resume(struct device *dev)
> >  		return ret;
> 
> This one could be dropped
> 
> >  	}
> >  
> > +	return 0;
> 
> And use here directly, like:
> 
> 	return ret;

Ack. Thanks.
Will fix that into the next version.


Kind Regards,
Tommaso


> 
> Thank you,
> Claudiu

RE: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Biju Das]

Hi Tommaso,

> -----Original Message-----
> From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> Sent: 18 December 2025 08:09
> Subject: Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
> 
> Hi Claudiu,
> Thanks for your review!
> 
> 
> On Wed, Dec 17, 2025 at 11:41:43AM +0200, Claudiu Beznea wrote:
> > Hi, Tommaso,
> >
> > On 12/12/25 13:58, Tommaso Merciai wrote:
> > > Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added
> > > suspend support for the Renesas I2C driver and following this change
> > > on RZ/G3E the following WARNING is seen on entering suspend ...
> > >
> > > [  134.275704] Freezing remaining freezable tasks completed (elapsed
> > > 0.001 seconds) [  134.285536] ------------[ cut here ]------------ [
> > > 134.290298] i2c i2c-2: Transfer while suspended [  134.295174]
> > > WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214,
> > > CPU#0: systemd-sleep/388 [  134.365507] Tainted: [W]=WARN [
> > > 134.368485] Hardware name: Renesas SMARC EVK version 2 based on
> > > r9a09g047e57 (DT) [  134.375961] pstate: 60400005 (nZCv daif +PAN
> > > -UAO -TCO -DIT -SSBS BTYPE=--) [  134.382935] pc :
> > > __i2c_smbus_xfer+0x1e4/0x214 [  134.387329] lr :
> > > __i2c_smbus_xfer+0x1e4/0x214 [  134.391717] sp : ffff800083f23860 [
> > > 134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27:
> > > ffff800082ed5d60 [  134.402226] x26: 0000001f4395fd74 x25:
> > > 0000000000000007 x24: 0000000000000001 [  134.409408] x23:
> > > 0000000000000000 x22: 000000000000006f x21: ffff800083f23936 [
> > > 134.416589] x20: ffff0000c090e140 x19: ffff0000c090e0d0 x18:
> > > 0000000000000006 [  134.423771] x17: 6f63657320313030 x16:
> > > 2e30206465737061 x15: ffff800083f23280 [  134.430953] x14:
> > > 0000000000000000 x13: ffff800082b16ce8 x12: 0000000000000f09 [
> > > 134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 :
> > > ffff800082b16ce8 [  134.445315] x8 : 00000000ffffefff x7 :
> > > ffff800082b6ece8 x6 : 80000000fffff000 [  134.452495] x5 : 0000000000000504 x4 : 0000000000000000
> x3 : 0000000000000000 [  134.459672] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c9ee9e80
> [  134.466851] Call trace:
> > > [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P) [  134.473715]
> > > i2c_smbus_xfer+0xbc/0x120 [  134.477507]
> > > i2c_smbus_read_byte_data+0x4c/0x84
> > > [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208] [
> > > 134.487703]  isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208] [
> > > 134.493226]  __rtc_read_time+0x44/0x88 [  134.497012]
> > > rtc_read_time+0x3c/0x68 [  134.500622]  rtc_suspend+0x9c/0x170
> > >
> > > The warning is triggered because I2C transfers can still be
> > > attempted while the controller is already suspended, due to
> > > inappropriate ordering of the system sleep callbacks.
> > >
> > > Fix this by moving the system sleep suspend/resume callbacks to the
> > > NOIRQ phase, ensuring the adapter is fully quiesced after late
> > > suspend and properly resumed before the early resume phase.
> > >
> > > To support NOIRQ resume, the hardware initialization path must not
> > > invoke runtime PM APIs. Split the init code so that the low-level
> > > hardware setup can be executed without pm_runtime_get/put(). This
> > > avoids violating the constraint introduced by commit 1e2ef05bb8cf
> > > ("PM: Limit race conditions between runtime PM and system sleep
> > > (v2)"), which forbids runtime PM calls during early resume.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > > ---
> > >  drivers/i2c/busses/i2c-riic.c | 65
> > > ++++++++++++++++++++++-------------
> > >  1 file changed, 41 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-riic.c
> > > b/drivers/i2c/busses/i2c-riic.c index 3e8f126cb7f7..9acc8936cdf7
> > > 100644
> > > --- a/drivers/i2c/busses/i2c-riic.c
> > > +++ b/drivers/i2c/busses/i2c-riic.c
> > > @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
> > >  	.functionality = riic_func,
> > >  };
> > >
> >
> > Moving reset assert/de-assert and controller deinit/re-initialization
> > on runtime suspend/resume may increase the runtime suspend/resume
> > latency. In case of consecutive requests this may translate into
> > controller performance downgrade. If you keep it like this, you may
> > want to increase the default autosuspend delay, at least, to avoid
> > controller reconfiguration after each individual struct i2c_algorithm::xfer() call.
> 
> 
> We can set autosuspend delay to 250 ms.

Just a question, What is the delay in current driver? 

Cheers,
Biju

RE: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Biju Das]

Hi Tommaso,

> -----Original Message-----
> From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> Sent: 12 December 2025 11:59
> Subject: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
> 
> Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added suspend support for the Renesas
> I2C driver and following this change on RZ/G3E the following WARNING is seen on entering suspend ...
> 
> [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds) [  134.285536] ---
> ---------[ cut here ]------------ [  134.290298] i2c i2c-2: Transfer while suspended [  134.295174]
> WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> [  134.365507] Tainted: [W]=WARN [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on
> r9a09g047e57 (DT) [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214 [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> [  134.391717] sp : ffff800083f23860 [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27:
> ffff800082ed5d60 [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936 [  134.416589] x20:
> ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006 [  134.423771] x17: 6f63657320313030 x16:
> 2e30206465737061 x15: ffff800083f23280 [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12:
> 0000000000000f09 [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000 [  134.452495] x5 :
> 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000 [  134.459672] x2 : 0000000000000000 x1 :
> 0000000000000000 x0 : ffff0000c9ee9e80 [  134.466851] Call trace:
> [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P) [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208] [  134.487703]
> isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208] [  134.493226]  __rtc_read_time+0x44/0x88 [  134.497012]
> rtc_read_time+0x3c/0x68 [  134.500622]  rtc_suspend+0x9c/0x170
> 
> The warning is triggered because I2C transfers can still be attempted while the controller is already
> suspended, due to inappropriate ordering of the system sleep callbacks.
> 
> Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ phase, ensuring the adapter
> is fully quiesced after late suspend and properly resumed before the early resume phase.
> 
> To support NOIRQ resume, the hardware initialization path must not invoke runtime PM APIs. Split the
> init code so that the low-level hardware setup can be executed without pm_runtime_get/put(). This
> avoids violating the constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions between
> runtime PM and system sleep (v2)"), which forbids runtime PM calls during early resume.
> 
> Cc: stable@vger.kernel.org
> Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> ---
>  drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> 3e8f126cb7f7..9acc8936cdf7 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
>  	.functionality = riic_func,
>  };
> 
> -static int riic_init_hw(struct riic_dev *riic)
> +static int __riic_init_hw(struct riic_dev *riic)
>  {
> -	int ret;
>  	unsigned long rate;
>  	unsigned long ns_per_tick;
>  	int total_ticks, cks, brl, brh;
> @@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
>  		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
>  		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
> 
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> -
>  	/* Changing the order of accessing IICRST and ICE may break things! */
>  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
>  	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1); @@ -451,10 +446,25 @@ static int
> riic_init_hw(struct riic_dev *riic)
> 
>  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> 
> -	pm_runtime_put_autosuspend(dev);
>  	return 0;
>  }
> 
> +static int riic_init_hw(struct riic_dev *riic) {
> +	struct device *dev = riic->adapter.dev.parent;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = __riic_init_hw(riic);
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +
>  static int riic_get_scl(struct i2c_adapter *adap)  {
>  	struct riic_dev *riic = i2c_get_adapdata(adap); @@ -572,6 +582,8 @@ static int
> riic_i2c_probe(struct platform_device *pdev)
> 
>  	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
> 
> +	platform_set_drvdata(pdev, riic);
> +
>  	/* Default 0 to save power. Can be overridden via sysfs for lower latency. */
>  	pm_runtime_set_autosuspend_delay(dev, 0);
>  	pm_runtime_use_autosuspend(dev);
> @@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto out;
> 
> -	platform_set_drvdata(pdev, riic);
> -
>  	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
>  	return 0;
> 
> @@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
>  	.num_irqs = ARRAY_SIZE(riic_rzt2h_irqs),  };
> 
> -static int riic_i2c_suspend(struct device *dev)
> +static int riic_i2c_runtime_suspend(struct device *dev)
>  {
>  	struct riic_dev *riic = dev_get_drvdata(dev);
> -	int ret;
> -
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> -
> -	i2c_mark_adapter_suspended(&riic->adapter);
> 
>  	/* Disable output on SDA, SCL pins. */
>  	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
> 
> -	pm_runtime_mark_last_busy(dev);
> -	pm_runtime_put_sync(dev);
> -
>  	return reset_control_assert(riic->rstc);  }
> 
> -static int riic_i2c_resume(struct device *dev)
> +static int riic_i2c_runtime_resume(struct device *dev)
>  {
>  	struct riic_dev *riic = dev_get_drvdata(dev);
>  	int ret;
> @@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
>  	if (ret)
>  		return ret;
> 
> -	ret = riic_init_hw(riic);
> +	ret = __riic_init_hw(riic);
>  	if (ret) {
>  		/*
>  		 * In case this happens there is no way to recover from this @@ -708,13 +708,30 @@ static
> int riic_i2c_resume(struct device *dev)
>  		return ret;
>  	}
> 
> +	return 0;
> +}
> +
> +static int riic_i2c_suspend(struct device *dev) {
> +	struct riic_dev *riic = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&riic->adapter);
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int riic_i2c_resume(struct device *dev) {
> +	struct riic_dev *riic = dev_get_drvdata(dev);
> +
>  	i2c_mark_adapter_resumed(&riic->adapter);


Is it not possible reconfigure i2c controller here like [1]

So that we can avoid reinitializing the i2ccontroller after every transfer (ie, rpm)

[1]
https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/i2c/busses/i2c-tegra.c#L1965

Cheers,
Biju

Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase

[Tommaso Merciai]

Hi Biju,
Thanks for your review!

On Thu, Dec 18, 2025 at 09:23:19AM +0000, Biju Das wrote:
> Hi Tommaso,
> 
> > -----Original Message-----
> > From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > Sent: 12 December 2025 11:59
> > Subject: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
> > 
> > Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added suspend support for the Renesas
> > I2C driver and following this change on RZ/G3E the following WARNING is seen on entering suspend ...
> > 
> > [  134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds) [  134.285536] ---
> > ---------[ cut here ]------------ [  134.290298] i2c i2c-2: Transfer while suspended [  134.295174]
> > WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> > [  134.365507] Tainted: [W]=WARN [  134.368485] Hardware name: Renesas SMARC EVK version 2 based on
> > r9a09g047e57 (DT) [  134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [  134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214 [  134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> > [  134.391717] sp : ffff800083f23860 [  134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27:
> > ffff800082ed5d60 [  134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> > [  134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936 [  134.416589] x20:
> > ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006 [  134.423771] x17: 6f63657320313030 x16:
> > 2e30206465737061 x15: ffff800083f23280 [  134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12:
> > 0000000000000f09 [  134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> > [  134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000 [  134.452495] x5 :
> > 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000 [  134.459672] x2 : 0000000000000000 x1 :
> > 0000000000000000 x0 : ffff0000c9ee9e80 [  134.466851] Call trace:
> > [  134.469311]  __i2c_smbus_xfer+0x1e4/0x214 (P) [  134.473715]  i2c_smbus_xfer+0xbc/0x120
> > [  134.477507]  i2c_smbus_read_byte_data+0x4c/0x84
> > [  134.482077]  isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208] [  134.487703]
> > isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208] [  134.493226]  __rtc_read_time+0x44/0x88 [  134.497012]
> > rtc_read_time+0x3c/0x68 [  134.500622]  rtc_suspend+0x9c/0x170
> > 
> > The warning is triggered because I2C transfers can still be attempted while the controller is already
> > suspended, due to inappropriate ordering of the system sleep callbacks.
> > 
> > Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ phase, ensuring the adapter
> > is fully quiesced after late suspend and properly resumed before the early resume phase.
> > 
> > To support NOIRQ resume, the hardware initialization path must not invoke runtime PM APIs. Split the
> > init code so that the low-level hardware setup can be executed without pm_runtime_get/put(). This
> > avoids violating the constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions between
> > runtime PM and system sleep (v2)"), which forbids runtime PM calls during early resume.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > ---
> >  drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
> >  1 file changed, 41 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> > 3e8f126cb7f7..9acc8936cdf7 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
> >  	.functionality = riic_func,
> >  };
> > 
> > -static int riic_init_hw(struct riic_dev *riic)
> > +static int __riic_init_hw(struct riic_dev *riic)
> >  {
> > -	int ret;
> >  	unsigned long rate;
> >  	unsigned long ns_per_tick;
> >  	int total_ticks, cks, brl, brh;
> > @@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
> >  		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
> >  		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
> > 
> > -	ret = pm_runtime_resume_and_get(dev);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Changing the order of accessing IICRST and ICE may break things! */
> >  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
> >  	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1); @@ -451,10 +446,25 @@ static int
> > riic_init_hw(struct riic_dev *riic)
> > 
> >  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> > 
> > -	pm_runtime_put_autosuspend(dev);
> >  	return 0;
> >  }
> > 
> > +static int riic_init_hw(struct riic_dev *riic) {
> > +	struct device *dev = riic->adapter.dev.parent;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __riic_init_hw(riic);
> > +
> > +	pm_runtime_put_autosuspend(dev);
> > +
> > +	return ret;
> > +}
> > +
> >  static int riic_get_scl(struct i2c_adapter *adap)  {
> >  	struct riic_dev *riic = i2c_get_adapdata(adap); @@ -572,6 +582,8 @@ static int
> > riic_i2c_probe(struct platform_device *pdev)
> > 
> >  	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
> > 
> > +	platform_set_drvdata(pdev, riic);
> > +
> >  	/* Default 0 to save power. Can be overridden via sysfs for lower latency. */
> >  	pm_runtime_set_autosuspend_delay(dev, 0);
> >  	pm_runtime_use_autosuspend(dev);
> > @@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto out;
> > 
> > -	platform_set_drvdata(pdev, riic);
> > -
> >  	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
> >  	return 0;
> > 
> > @@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
> >  	.num_irqs = ARRAY_SIZE(riic_rzt2h_irqs),  };
> > 
> > -static int riic_i2c_suspend(struct device *dev)
> > +static int riic_i2c_runtime_suspend(struct device *dev)
> >  {
> >  	struct riic_dev *riic = dev_get_drvdata(dev);
> > -	int ret;
> > -
> > -	ret = pm_runtime_resume_and_get(dev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	i2c_mark_adapter_suspended(&riic->adapter);
> > 
> >  	/* Disable output on SDA, SCL pins. */
> >  	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
> > 
> > -	pm_runtime_mark_last_busy(dev);
> > -	pm_runtime_put_sync(dev);
> > -
> >  	return reset_control_assert(riic->rstc);  }
> > 
> > -static int riic_i2c_resume(struct device *dev)
> > +static int riic_i2c_runtime_resume(struct device *dev)
> >  {
> >  	struct riic_dev *riic = dev_get_drvdata(dev);
> >  	int ret;
> > @@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> > 
> > -	ret = riic_init_hw(riic);
> > +	ret = __riic_init_hw(riic);
> >  	if (ret) {
> >  		/*
> >  		 * In case this happens there is no way to recover from this @@ -708,13 +708,30 @@ static
> > int riic_i2c_resume(struct device *dev)
> >  		return ret;
> >  	}
> > 
> > +	return 0;
> > +}
> > +
> > +static int riic_i2c_suspend(struct device *dev) {
> > +	struct riic_dev *riic = dev_get_drvdata(dev);
> > +
> > +	i2c_mark_adapter_suspended(&riic->adapter);
> > +
> > +	return pm_runtime_force_suspend(dev);
> > +}
> > +
> > +static int riic_i2c_resume(struct device *dev) {
> > +	struct riic_dev *riic = dev_get_drvdata(dev);
> > +
> >  	i2c_mark_adapter_resumed(&riic->adapter);
> 
> 
> Is it not possible reconfigure i2c controller here like [1]
> 
> So that we can avoid reinitializing the i2ccontroller after every transfer (ie, rpm)
> 
> [1]
> https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/i2c/busses/i2c-tegra.c#L1965

Thanks for sharing. On my side I found [2] that can be used as reference
also for riic_i2c using:

	NOIRQ_SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend_noirq, riic_i2c_resume_noirq)
	SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend, riic_i2c_resume)

I'm preparing v2 based on that commit:

   4262df2a69c3 ("i2c: imx-lpi2c: make controller available until the system
   enters suspend_noirq() and from resume_noirq().")

Kind Regards,
Tommaso

[2] https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/i2c/busses/i2c-imx-lpi2c.c#L1537

> 
> Cheers,
> Biju
>