[PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Matthias Schiffer]

When a target makes use of clock stretching, a timeout of 1ms may not be
enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
takes ~320ms to send its ACK after a flash command has been
submitted.

Replace the per-iteration timeout of 1ms with limiting the total
transfer time to the timeout set in struct i2c_adapter (defaulting to
1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
add a cpu_relax() to the busy poll loop.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/i2c/busses/i2c-ocores.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0f67e57cdeff6..1746c8821a149 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -258,7 +258,7 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
  * @reg: register to query
  * @mask: bitmask to apply on register value
  * @val: expected result
- * @timeout: timeout in jiffies
+ * @timeout: absolute timeout in jiffies
  *
  * Timeout is necessary to avoid to stay here forever when the chip
  * does not answer correctly.
@@ -269,17 +269,16 @@ static int ocores_wait(struct ocores_i2c *i2c,
 		       int reg, u8 mask, u8 val,
 		       const unsigned long timeout)
 {
-	unsigned long j;
-
-	j = jiffies + timeout;
 	while (1) {
 		u8 status = oc_getreg(i2c, reg);
 
 		if ((status & mask) == val)
 			break;
 
-		if (time_after(jiffies, j))
+		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
+
+		cpu_relax();
 	}
 	return 0;
 }
@@ -287,12 +286,13 @@ static int ocores_wait(struct ocores_i2c *i2c,
 /**
  * ocores_poll_wait() - Wait until is possible to process some data
  * @i2c: ocores I2C device instance
+ * @timeout: absolute timeout in jiffies
  *
  * Used when the device is in polling mode (interrupts disabled).
  *
  * Return: 0 on success, -ETIMEDOUT on timeout
  */
-static int ocores_poll_wait(struct ocores_i2c *i2c)
+static int ocores_poll_wait(struct ocores_i2c *i2c, unsigned long timeout)
 {
 	u8 mask;
 	int err;
@@ -310,15 +310,11 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
 		udelay((8 * 1000) / i2c->bus_clock_khz);
 	}
 
-	/*
-	 * once we are here we expect to get the expected result immediately
-	 * so if after 1ms we timeout then something is broken.
-	 */
-	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
+	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, timeout);
 	if (err)
-		dev_warn(i2c->adap.dev.parent,
-			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
-			 __func__, mask);
+		dev_dbg(i2c->adap.dev.parent,
+			"%s: STATUS timeout, bit 0x%x did not clear\n",
+			__func__, mask);
 	return err;
 }
 
@@ -336,11 +332,12 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
  */
 static int ocores_process_polling(struct ocores_i2c *i2c)
 {
+	unsigned long timeout = jiffies + i2c->adap.timeout;
 	irqreturn_t ret;
 	int err = 0;
 
 	while (1) {
-		err = ocores_poll_wait(i2c);
+		err = ocores_poll_wait(i2c, timeout);
 		if (err)
 			break; /* timeout */
 
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 2/2] i2c: ocores: respect adapter timeout in IRQ mode

[Matthias Schiffer]

While the timeout field of the i2c_adapter defaults to 1s, it can be
changed, for example using the I2C_TIMEOUT ioctl. Change the ocores
driver to use this timeout instead of hardcoding 1s, also making it
consistent with polling mode.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/i2c/busses/i2c-ocores.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 1746c8821a149..518e4cf821a7a 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -380,7 +380,8 @@ static int ocores_xfer_core(struct ocores_i2c *i2c,
 	} else {
 		if (wait_event_timeout(i2c->wait,
 				       (i2c->state == STATE_ERROR) ||
-				       (i2c->state == STATE_DONE), HZ) == 0)
+				       (i2c->state == STATE_DONE),
+				       i2c->adap.timeout) == 0)
 			ret = -ETIMEDOUT;
 	}
 	if (ret) {
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 2/2] i2c: ocores: respect adapter timeout in IRQ mode

[Peter Korsgaard]

>>>>> "Matthias" == Matthias Schiffer <matthias.schiffer@ew.tq-group.com> writes:

 > While the timeout field of the i2c_adapter defaults to 1s, it can be
 > changed, for example using the I2C_TIMEOUT ioctl. Change the ocores
 > driver to use this timeout instead of hardcoding 1s, also making it
 > consistent with polling mode.

 > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Acked-by: Peter Korsgaard <peter@korsgaard.com>

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Andrew Lunn]

On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> When a target makes use of clock stretching, a timeout of 1ms may not be
> enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> takes ~320ms to send its ACK after a flash command has been
> submitted.
> 
> Replace the per-iteration timeout of 1ms with limiting the total
> transfer time to the timeout set in struct i2c_adapter (defaulting to
> 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> add a cpu_relax() to the busy poll loop.

1s is a long time to spin. Maybe it would be better to keep with the
current spin for 1ms, and then use one of the helpers from iopoll.h to
do a sleeping wait? Say with 10ms sleeps, up to the 1s maximum?

	Andrew

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Matthias Schiffer]

On Tue, 2025-10-07 at 14:34 +0200, Andrew Lunn wrote:
> On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> > When a target makes use of clock stretching, a timeout of 1ms may not be
> > enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> > takes ~320ms to send its ACK after a flash command has been
> > submitted.
> > 
> > Replace the per-iteration timeout of 1ms with limiting the total
> > transfer time to the timeout set in struct i2c_adapter (defaulting to
> > 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> > add a cpu_relax() to the busy poll loop.
> 
> 1s is a long time to spin. Maybe it would be better to keep with the
> current spin for 1ms, and then use one of the helpers from iopoll.h to
> do a sleeping wait? Say with 10ms sleeps, up to the 1s maximum?
> 
> 	Andrew

Makes sense. I don't think I can use something from iopoll.h directly, as i2c-
ocores has its own ioreadX abstraction to deal with different register widths
and endianesses, but a combination of spin + sleep is probably the way to go.

Best,
Matthias



-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Andrew Lunn]

On Tue, Oct 07, 2025 at 04:06:36PM +0200, Matthias Schiffer wrote:
> On Tue, 2025-10-07 at 14:34 +0200, Andrew Lunn wrote:
> > On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> > > When a target makes use of clock stretching, a timeout of 1ms may not be
> > > enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> > > takes ~320ms to send its ACK after a flash command has been
> > > submitted.
> > > 
> > > Replace the per-iteration timeout of 1ms with limiting the total
> > > transfer time to the timeout set in struct i2c_adapter (defaulting to
> > > 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> > > add a cpu_relax() to the busy poll loop.
> > 
> > 1s is a long time to spin. Maybe it would be better to keep with the
> > current spin for 1ms, and then use one of the helpers from iopoll.h to
> > do a sleeping wait? Say with 10ms sleeps, up to the 1s maximum?
> > 
> > 	Andrew
> 
> Makes sense. I don't think I can use something from iopoll.h directly, as i2c-
> ocores has its own ioreadX abstraction to deal with different register widths
> and endianesses, but a combination of spin + sleep is probably the way to go.

I think iopoll.h should work.


		u8 status = oc_getreg(i2c, reg);

		if ((status & mask) == val)
			break;

This maps to

u8 status;
     
ret = read_poll_timeout(oc_getreg, status, (status & mask) == val,
                       10000, 1000000, false, i2c, reg);

Andrew

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Matthias Schiffer]

On Tue, 2025-10-07 at 16:20 +0200, Andrew Lunn wrote:
> On Tue, Oct 07, 2025 at 04:06:36PM +0200, Matthias Schiffer wrote:
> > On Tue, 2025-10-07 at 14:34 +0200, Andrew Lunn wrote:
> > > On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> > > > When a target makes use of clock stretching, a timeout of 1ms may not be
> > > > enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> > > > takes ~320ms to send its ACK after a flash command has been
> > > > submitted.
> > > > 
> > > > Replace the per-iteration timeout of 1ms with limiting the total
> > > > transfer time to the timeout set in struct i2c_adapter (defaulting to
> > > > 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> > > > add a cpu_relax() to the busy poll loop.
> > > 
> > > 1s is a long time to spin. Maybe it would be better to keep with the
> > > current spin for 1ms, and then use one of the helpers from iopoll.h to
> > > do a sleeping wait? Say with 10ms sleeps, up to the 1s maximum?
> > > 
> > > 	Andrew
> > 
> > Makes sense. I don't think I can use something from iopoll.h directly, as i2c-
> > ocores has its own ioreadX abstraction to deal with different register widths
> > and endianesses, but a combination of spin + sleep is probably the way to go.
> 
> I think iopoll.h should work.
> 
> 
> 		u8 status = oc_getreg(i2c, reg);
> 
> 		if ((status & mask) == val)
> 			break;
> 
> This maps to
> 
> u8 status;
>      
> ret = read_poll_timeout(oc_getreg, status, (status & mask) == val,
>                        10000, 1000000, false, i2c, reg);
> 
> Andrew

Ah, you are right, that should work.

If we want to keep the spin case for short waits, not duplicating the read and
mask check seems preferable to me though - maybe something like the following
(which could also be extended to exponentially increasing sleeps or similar if
we want to start with something smaller than 10ms):

	unsigned long spin_timeout = jiffies + msecs_to_jiffies(1);
	...

		if (time_before(jiffies, spin_timeout))
			cpu_relax();
		else
			msleep(10);

Best,
Matthias


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Andrew Lunn]

On Tue, Oct 07, 2025 at 04:41:00PM +0200, Matthias Schiffer wrote:
> On Tue, 2025-10-07 at 16:20 +0200, Andrew Lunn wrote:
> > On Tue, Oct 07, 2025 at 04:06:36PM +0200, Matthias Schiffer wrote:
> > > On Tue, 2025-10-07 at 14:34 +0200, Andrew Lunn wrote:
> > > > On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> > > > > When a target makes use of clock stretching, a timeout of 1ms may not be
> > > > > enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> > > > > takes ~320ms to send its ACK after a flash command has been
> > > > > submitted.
> > > > > 
> > > > > Replace the per-iteration timeout of 1ms with limiting the total
> > > > > transfer time to the timeout set in struct i2c_adapter (defaulting to
> > > > > 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> > > > > add a cpu_relax() to the busy poll loop.
> > > > 
> > > > 1s is a long time to spin. Maybe it would be better to keep with the
> > > > current spin for 1ms, and then use one of the helpers from iopoll.h to
> > > > do a sleeping wait? Say with 10ms sleeps, up to the 1s maximum?
> > > > 
> > > > 	Andrew
> > > 
> > > Makes sense. I don't think I can use something from iopoll.h directly, as i2c-
> > > ocores has its own ioreadX abstraction to deal with different register widths
> > > and endianesses, but a combination of spin + sleep is probably the way to go.
> > 
> > I think iopoll.h should work.
> > 
> > 
> > 		u8 status = oc_getreg(i2c, reg);
> > 
> > 		if ((status & mask) == val)
> > 			break;
> > 
> > This maps to
> > 
> > u8 status;
> >      
> > ret = read_poll_timeout(oc_getreg, status, (status & mask) == val,
> >                        10000, 1000000, false, i2c, reg);
> > 
> > Andrew
> 
> Ah, you are right, that should work.
> 
> If we want to keep the spin case for short waits, not duplicating the read and
> mask check seems preferable to me though - maybe something like the following
> (which could also be extended to exponentially increasing sleeps or similar if
> we want to start with something smaller than 10ms):

I think something like this will work:

u8 status;

ret = read_poll_timeout_atomic(oc_getreg, status, (status & mask) == val,
                              100, 1000, false, i2c, reg);

if (ret != ETIMEDOUT)
    return ret;

return read_poll_timeout(oc_getreg, status, (status & mask) == val,
                         10000, 1000000, false, i2c, reg);

which probably ends up being the same length, but simpler than the
current ocores_wait().

	Andrew

Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

[Andi Shyti]

Hi Matthias,

On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> When a target makes use of clock stretching, a timeout of 1ms may not be
> enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> takes ~320ms to send its ACK after a flash command has been
> submitted.

besides, the specification doesn't impose any maximum time.

> Replace the per-iteration timeout of 1ms with limiting the total
> transfer time to the timeout set in struct i2c_adapter (defaulting to
> 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> add a cpu_relax() to the busy poll loop.
> 

...

> @@ -269,17 +269,16 @@ static int ocores_wait(struct ocores_i2c *i2c,
>  		       int reg, u8 mask, u8 val,
>  		       const unsigned long timeout)
>  {
> -	unsigned long j;
> -
> -	j = jiffies + timeout;

Any reason we don't take "jiffies + i2c->adap.timeout" and avoud
all the changes below? It also simplifies the parameters list.

>  	while (1) {
>  		u8 status = oc_getreg(i2c, reg);
>  
>  		if ((status & mask) == val)
>  			break;
>  
> -		if (time_after(jiffies, j))
> +		if (time_after(jiffies, timeout))
>  			return -ETIMEDOUT;
> +
> +		cpu_relax();

Good.

>  	}
>  	return 0;
>  }

...

> -	/*
> -	 * once we are here we expect to get the expected result immediately
> -	 * so if after 1ms we timeout then something is broken.
> -	 */

Why have you deleted this comment completely?

> -	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> +	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, timeout);
>  	if (err)
> -		dev_warn(i2c->adap.dev.parent,
> -			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> -			 __func__, mask);
> +		dev_dbg(i2c->adap.dev.parent,
> +			"%s: STATUS timeout, bit 0x%x did not clear\n",
> +			__func__, mask);

Why are you changing from warn to dbg? This change is not
mentioned in the commit log.

Andi

>  	return err;
>  }