[PATCH v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[PATCH v2 1/2] i2c: ocores: increase poll timeout to 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.

The behavior in the regular case is unchanged, spinning for up to 1ms,
but the open-coded poll loop is replaced with read_poll_timeout_atomic()
as suggested by Andrew Lunn. In cases where 1ms is not sufficient,
read_poll_timeout() is used, allowing a total transfer time up to the
timeout set in struct i2c_adapter (defaulting to 1s, configurable through
the I2C_TIMEOUT ioctl).

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

v2: avoid spinning for a whole second, switch to
read_poll_timeout[_atomic]()

 drivers/i2c/busses/i2c-ocores.c | 44 ++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 482b37c8a1297..c4587194d46be 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/wait.h>
 #include <linux/platform_data/i2c-ocores.h>
 #include <linux/slab.h>
@@ -258,7 +259,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,30 +270,32 @@ 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);
+	unsigned long max_wait;
+	u8 status;
+	int ret;
 
-		if ((status & mask) == val)
-			break;
+	/* In most cases the wait is short, so we spin for up to 1ms. */
+	ret = read_poll_timeout_atomic(oc_getreg, status,
+				       (status & mask) == val,
+					0, 1000, false, i2c, reg);
+	if (ret != -ETIMEDOUT)
+		return ret;
 
-		if (time_after(jiffies, j))
-			return -ETIMEDOUT;
-	}
-	return 0;
+	max_wait = jiffies_to_usecs(max(0L, timeout - jiffies)) + 1;
+	return read_poll_timeout(oc_getreg, status, (status & mask) == val,
+				 10000, max_wait, false, i2c, reg);
 }
 
 /**
  * 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 +313,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 +335,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 v2 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>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
---

v2: collect acked-by

 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 c4587194d46be..4a8ce167a3d9f 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -383,7 +383,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 v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Andrew Lunn]

On Thu, Oct 09, 2025 at 11:19:49AM +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.
> 
> The behavior in the regular case is unchanged, spinning for up to 1ms,
> but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> as suggested by Andrew Lunn. In cases where 1ms is not sufficient,
> read_poll_timeout() is used, allowing a total transfer time up to the
> timeout set in struct i2c_adapter (defaulting to 1s, configurable through
> the I2C_TIMEOUT ioctl).

Thanks

Did you test with CONFIG_DEBUG_ATOMIC_SLEEP enabled? I don't think it
is an issue, but the old code could be used in atomic context because
it never slept.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

On Thu, Oct 09, 2025 at 11:19:50AM +0200, Matthias Schiffer wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Matthias Schiffer]

On Thu, 2025-10-09 at 15:20 +0200, Andrew Lunn wrote:
> On Thu, Oct 09, 2025 at 11:19:49AM +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.
> > 
> > The behavior in the regular case is unchanged, spinning for up to 1ms,
> > but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> > as suggested by Andrew Lunn. In cases where 1ms is not sufficient,
> > read_poll_timeout() is used, allowing a total transfer time up to the
> > timeout set in struct i2c_adapter (defaulting to 1s, configurable through
> > the I2C_TIMEOUT ioctl).
> 
> Thanks
> 
> Did you test with CONFIG_DEBUG_ATOMIC_SLEEP enabled? I don't think it
> is an issue, but the old code could be used in atomic context because
> it never slept.

I did not, but there is only one call chain ocores_xfer_core ->
ocores_process_polling -> ocores_poll_wait -> ocores_wait, which is definitely
not used in atomic context (in IRQ mode, ocores_xfer_core calls
wait_event_timeout, which might_sleep()).

Best,
Matthias



> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew

-- 
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 v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Matthias Schiffer]

On Thu, 2025-10-09 at 15:26 +0200, Matthias Schiffer wrote:
> On Thu, 2025-10-09 at 15:20 +0200, Andrew Lunn wrote:
> > On Thu, Oct 09, 2025 at 11:19:49AM +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.
> > > 
> > > The behavior in the regular case is unchanged, spinning for up to 1ms,
> > > but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> > > as suggested by Andrew Lunn. In cases where 1ms is not sufficient,
> > > read_poll_timeout() is used, allowing a total transfer time up to the
> > > timeout set in struct i2c_adapter (defaulting to 1s, configurable through
> > > the I2C_TIMEOUT ioctl).
> > 
> > Thanks
> > 
> > Did you test with CONFIG_DEBUG_ATOMIC_SLEEP enabled? I don't think it
> > is an issue, but the old code could be used in atomic context because
> > it never slept.
> 
> I did not, but there is only one call chain ocores_xfer_core ->
> ocores_process_polling -> ocores_poll_wait -> ocores_wait, which is definitely
> not used in atomic context (in IRQ mode, ocores_xfer_core calls
> wait_event_timeout, which might_sleep()).
> 
> Best,
> Matthias
> 
> 
> 
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> >     Andrew
> 

Hi everyone,

are these patches still missing anything?

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 v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]


> The behavior in the regular case is unchanged, spinning for up to 1ms,
> but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> as suggested by Andrew Lunn.

Hmm, spinning 1ms is still a lot. Can't we just use read_poll_timeout()
for the whole timeout? I can't see that it will cause a regression. But
please correct me if I am wrong.

The series looks good for me apart from the above.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Andrew Lunn]

On Tue, Jan 13, 2026 at 02:02:07PM +0100, Wolfram Sang wrote:
> 
> > The behavior in the regular case is unchanged, spinning for up to 1ms,
> > but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> > as suggested by Andrew Lunn.
> 
> Hmm, spinning 1ms is still a lot. Can't we just use read_poll_timeout()
> for the whole timeout? I can't see that it will cause a regression. But
> please correct me if I am wrong.

I've forgotten the context, but

/**
 * ocores_poll_wait() - Wait until is possible to process some data
 * @i2c: ocores I2C device instance
 *
 * Used when the device is in polling mode (interrupts disabled).

If interrupts are disabled, you cannot use read_poll_timeout().  You
have to use read_poll_timeout_atomic(). And that spins anyway.

     Andrew

Re: [PATCH v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Matthias Schiffer]

On Tue, 2026-01-13 at 15:21 +0100, Andrew Lunn wrote:
> On Tue, Jan 13, 2026 at 02:02:07PM +0100, Wolfram Sang wrote:
> > 
> > > The behavior in the regular case is unchanged, spinning for up to 1ms,
> > > but the open-coded poll loop is replaced with read_poll_timeout_atomic()
> > > as suggested by Andrew Lunn.
> > 
> > Hmm, spinning 1ms is still a lot. Can't we just use read_poll_timeout()
> > for the whole timeout? I can't see that it will cause a regression. But
> > please correct me if I am wrong.
> 
> I've forgotten the context, but
> 
> /**
>  * ocores_poll_wait() - Wait until is possible to process some data
>  * @i2c: ocores I2C device instance
>  *
>  * Used when the device is in polling mode (interrupts disabled).
> 
> If interrupts are disabled, you cannot use read_poll_timeout().  You
> have to use read_poll_timeout_atomic(). And that spins anyway.
> 
>      Andrew


This code does not have interrupts disabled, we could not fall back from
read_poll_timeout_atomic() to read_poll_timeout() otherwise. My understanding is
that a sleeping wait would make it more likely for a switch to a different task
to happen after every byte, negatively impacting I2C performance; this is not
something I have verified however.

It is spinning for 1ms because that's what the old code did (which only spun
without fallback to sleeping). Reducing this to the time needed to transfer 1
byte in the absence of clock stretching should not cause issues (200us for
50kHz; could also be made to depend on the clock rate, so it would be even less
spinning at higher frequencies).

Best,
Matthias

Re: [PATCH v2 1/2] i2c: ocores: increase poll timeout to total transfer timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

Hi Matthias,

> This code does not have interrupts disabled, we could not fall back from
> read_poll_timeout_atomic() to read_poll_timeout() otherwise.

Yes, that's what I thought as well.

> that a sleeping wait would make it more likely for a switch to a different task
> to happen after every byte, negatively impacting I2C performance; this is not
> something I have verified however.

Quite some prominent controller drivers use readX_poll_timeout(), so it
really seems there is no huge penalty in practice. Especially not with
cores so fast these days that tasks can be served even in 100us.

> It is spinning for 1ms because that's what the old code did (which only spun
> without fallback to sleeping).

I understand. Yet, I think the spinning is legacy behaviour and I would
prefer plain readX_poll_timeout(). Unless there is something I
overlooked. Makes the code simpler and easier to maintain.

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]