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

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

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
>