diff mbox series

[RFC] i2c: imx: avoid rescheduling when waiting for bus not busy

Message ID 20240531142437.74831-1-eichest@gmail.com (mailing list archive)
State New
Headers show
Series [RFC] i2c: imx: avoid rescheduling when waiting for bus not busy | expand

Commit Message

Stefan Eichenberger May 31, 2024, 2:24 p.m. UTC
From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
bus is idle. The imx i2c driver will call schedule when waiting for the
bus to become idle after switching to master mode. When the i2c
controller switches to master mode it pulls SCL and SDA low, if the
ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
clocking, it will timeout and ignore all signals until the next start
condition occurs (SCL and SDA low). This can occur when the system load
is high and schedule returns after more than 25 ms.

This rfc tries to solve the problem by using a udelay for the first 10
ms before calling schedule. This reduces the chance that we will
reschedule. However, it is still theoretically possible for the problem
to occur. To properly solve the problem, we would also need to disable
interrupts during the transfer.

After some internal discussion, we see three possible solutions:
1. Use udelay as shown in this rfc and also disable the interrupts
   during the transfer. This would solve the problem but disable the
   interrupts. Also, we would have to re-enable the interrupts if the
   timeout is longer than 1ms (TBD).
2. We use a retry mechanism in the ti-ads1015 driver. When we see a
   timeout, we try again.
3. We use the suggested solution and accept that there is an edge case
   where the timeout can happen.

There may be a better way to do this, which is why this is an RFC.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/i2c/busses/i2c-imx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko May 31, 2024, 2:45 p.m. UTC | #1
On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> bus is idle. The imx i2c driver will call schedule when waiting for the
> bus to become idle after switching to master mode. When the i2c
> controller switches to master mode it pulls SCL and SDA low, if the
> ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> clocking, it will timeout and ignore all signals until the next start
> condition occurs (SCL and SDA low). This can occur when the system load
> is high and schedule returns after more than 25 ms.
> 
> This rfc tries to solve the problem by using a udelay for the first 10
> ms before calling schedule. This reduces the chance that we will
> reschedule. However, it is still theoretically possible for the problem
> to occur. To properly solve the problem, we would also need to disable
> interrupts during the transfer.
> 
> After some internal discussion, we see three possible solutions:
> 1. Use udelay as shown in this rfc and also disable the interrupts
>    during the transfer. This would solve the problem but disable the
>    interrupts. Also, we would have to re-enable the interrupts if the
>    timeout is longer than 1ms (TBD).
> 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
>    timeout, we try again.
> 3. We use the suggested solution and accept that there is an edge case
>    where the timeout can happen.
> 
> There may be a better way to do this, which is why this is an RFC.

...

> +			/*
> +			 * Avoid rescheduling in the first 10 ms to avoid
> +			 * timeouts for SMBus like devices
> +			 */
> +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> +				udelay(10);
> +			else
> +				schedule();

Isn't there cond_resched() or so for such things?
More info here: 494e46d08d35 ("airo: Replace in_atomic() usage.")
Andrew Lunn June 2, 2024, 2:31 p.m. UTC | #2
On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> bus is idle. The imx i2c driver will call schedule when waiting for the
> bus to become idle after switching to master mode. When the i2c
> controller switches to master mode it pulls SCL and SDA low, if the
> ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> clocking, it will timeout and ignore all signals until the next start
> condition occurs (SCL and SDA low).

Does the I2C specification say anything about this behaviour, or is it
specific to this device?

> This rfc tries to solve the problem by using a udelay for the first 10
> ms before calling schedule. This reduces the chance that we will
> reschedule. However, it is still theoretically possible for the problem
> to occur. To properly solve the problem, we would also need to disable
> interrupts during the transfer.
> 
> After some internal discussion, we see three possible solutions:
> 1. Use udelay as shown in this rfc and also disable the interrupts
>    during the transfer. This would solve the problem but disable the
>    interrupts. Also, we would have to re-enable the interrupts if the
>    timeout is longer than 1ms (TBD).
> 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
>    timeout, we try again.
> 3. We use the suggested solution and accept that there is an edge case
>    where the timeout can happen.

2. has the advantage you fix it for any system with this device, not
just those using an IMX. Once question would be, is such a retry safe
in all conditions. Does the timeout happen before any non idempotent
operation is performed?

If the I2C specification allows this behaviour, maybe a more generic
solution is needed, since it could affect more devices?

	Andrew
Stefan Eichenberger June 3, 2024, 7:34 a.m. UTC | #3
On Sun, Jun 02, 2024 at 04:31:27PM +0200, Andrew Lunn wrote:
> On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > bus is idle. The imx i2c driver will call schedule when waiting for the
> > bus to become idle after switching to master mode. When the i2c
> > controller switches to master mode it pulls SCL and SDA low, if the
> > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > clocking, it will timeout and ignore all signals until the next start
> > condition occurs (SCL and SDA low).
> 
> Does the I2C specification say anything about this behaviour, or is it
> specific to this device?
> 

The timeouting mechanism is normally used in SMBus mode. However, for
this specific device they still call it I2C which is a bit confusing.
The difference between I2C and SMBus is that SMBus has a timeout while
the I2C uses a recovery mechanism. Besides that the two protocols are
identical.

> > This rfc tries to solve the problem by using a udelay for the first 10
> > ms before calling schedule. This reduces the chance that we will
> > reschedule. However, it is still theoretically possible for the problem
> > to occur. To properly solve the problem, we would also need to disable
> > interrupts during the transfer.
> > 
> > After some internal discussion, we see three possible solutions:
> > 1. Use udelay as shown in this rfc and also disable the interrupts
> >    during the transfer. This would solve the problem but disable the
> >    interrupts. Also, we would have to re-enable the interrupts if the
> >    timeout is longer than 1ms (TBD).
> > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> >    timeout, we try again.
> > 3. We use the suggested solution and accept that there is an edge case
> >    where the timeout can happen.
> 
> 2. has the advantage you fix it for any system with this device, not
> just those using an IMX. Once question would be, is such a retry safe
> in all conditions. Does the timeout happen before any non idempotent
> operation is performed?
> 
> If the I2C specification allows this behaviour, maybe a more generic
> solution is needed, since it could affect more devices?

Maybe I could add a smbus_xfer function to the i2c driver and then
change the ti-ads1015 driver to use the smbus_xfer function instead of
i2c. However, I would still have to disable preemption while the SMBus
transfer is happening which concerns me a bit.

Regards,
Stefan
Stefan Eichenberger June 3, 2024, 7:43 a.m. UTC | #4
On Fri, May 31, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > bus is idle. The imx i2c driver will call schedule when waiting for the
> > bus to become idle after switching to master mode. When the i2c
> > controller switches to master mode it pulls SCL and SDA low, if the
> > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > clocking, it will timeout and ignore all signals until the next start
> > condition occurs (SCL and SDA low). This can occur when the system load
> > is high and schedule returns after more than 25 ms.
> > 
> > This rfc tries to solve the problem by using a udelay for the first 10
> > ms before calling schedule. This reduces the chance that we will
> > reschedule. However, it is still theoretically possible for the problem
> > to occur. To properly solve the problem, we would also need to disable
> > interrupts during the transfer.
> > 
> > After some internal discussion, we see three possible solutions:
> > 1. Use udelay as shown in this rfc and also disable the interrupts
> >    during the transfer. This would solve the problem but disable the
> >    interrupts. Also, we would have to re-enable the interrupts if the
> >    timeout is longer than 1ms (TBD).
> > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> >    timeout, we try again.
> > 3. We use the suggested solution and accept that there is an edge case
> >    where the timeout can happen.
> > 
> > There may be a better way to do this, which is why this is an RFC.
> 
> ...
> 
> > +			/*
> > +			 * Avoid rescheduling in the first 10 ms to avoid
> > +			 * timeouts for SMBus like devices
> > +			 */
> > +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> > +				udelay(10);
> > +			else
> > +				schedule();
> 
> Isn't there cond_resched() or so for such things?
> More info here: 494e46d08d35 ("airo: Replace in_atomic() usage.")

The problem would be that I have to disable preemption during the
transfer, then cond_resched would do nothing if I understand it
correctly. However, an I2C transfer @100kHz for 3 bytes takes at least
240us + overhead (e.g. waiting for the bus idle) which might end in a
close to ms ranage. This is what concerns me.

Regards,
Stefan
Stefan Eichenberger June 21, 2024, 3:22 p.m. UTC | #5
Hi Andi, Andrew, Wolfram, Oleksij,

After some internal discussion we still have some questions which are
blocking us from solving the issue.

On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> bus is idle. The imx i2c driver will call schedule when waiting for the
> bus to become idle after switching to master mode. When the i2c
> controller switches to master mode it pulls SCL and SDA low, if the
> ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> clocking, it will timeout and ignore all signals until the next start
> condition occurs (SCL and SDA low). This can occur when the system load
> is high and schedule returns after more than 25 ms.
> 
> This rfc tries to solve the problem by using a udelay for the first 10
> ms before calling schedule. This reduces the chance that we will
> reschedule. However, it is still theoretically possible for the problem
> to occur. To properly solve the problem, we would also need to disable
> interrupts during the transfer.
> 
> After some internal discussion, we see three possible solutions:
> 1. Use udelay as shown in this rfc and also disable the interrupts
>    during the transfer. This would solve the problem but disable the
>    interrupts. Also, we would have to re-enable the interrupts if the
>    timeout is longer than 1ms (TBD).
> 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
>    timeout, we try again.
> 3. We use the suggested solution and accept that there is an edge case
>    where the timeout can happen.
> 
> There may be a better way to do this, which is why this is an RFC.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 3842e527116b7..179f8367490a5 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -503,10 +503,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
>  				"<%s> I2C bus is busy\n", __func__);
>  			return -ETIMEDOUT;
>  		}
> -		if (atomic)
> +		if (atomic) {
>  			udelay(100);
> -		else
> -			schedule();
> +		} else {
> +			/*
> +			 * Avoid rescheduling in the first 10 ms to avoid
> +			 * timeouts for SMBus like devices
> +			 */
> +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> +				udelay(10);
> +			else
> +				schedule();
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.40.1

If we want to be sure that the ADS1015 I2C ADC will never timeout, we
would have to add a patch to disable preemption during transmission.
This would look like this:

@@ -1244,6 +1248,12 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 	bool is_lastmsg = false;
 	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
 
+	/* If we are in SMBus mode we need to do the transfer atomically */
+	if (i2c_imx->smbus_mode) {
+		preempt_disable();
+		atomic = true;
+	}
+
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx, atomic);
 	if (result) {
@@ -1320,6 +1330,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 	if (i2c_imx->slave)
 		i2c_imx_slave_init(i2c_imx);
 
+	if (i2c_imx->smbus_mode)
+		preempt_enable();
+
 	return (result < 0) ? result : num;
 }

However, we are aware that disabling preemption is not a good idea. So
we were discussing how this is normally handled with SMBus devices? Is
it just expected that SMBus devices will timeout in rare cases?

For our use case, the problem would be solved if we could get rid of the
schedule call and replace it with a udelay. It seems that the i.MX8M
Mini I2C controller needs a few ms to clear the IBB flag. In the
reference manual, they write:
> I2C bus busy bit. Indicates the status of the bus. NOTE: When I2C is
> enabled (I2C_I2CR[IEN] = 1), it continuously polls the bus data (SDA)
> and clock (SCL) signals to determine a Start or Stop condition. Bus is
> idle. If a Stop signal is detected, IBB is cleared. Bus is busy. When
> Start is detected, IBB is set.
Unfortunately, it is not clear how often they poll. In our tests the
issue disappeard when we used udelay instead of usleep or schedule for
the first 10 ms.

Since we know we don't have a multi-master configuration, another way
would be to not test for IBB and just start the transfer. We saw that
other drivers use the multi-master device tree property to determine if
multi-master is supported. Here another quote from the reference manual:
> On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be
> tested to determine whether the serial bus is free.
We assume this means it is not necessary to test for IBB if we know we
are in a single-master configuration.

Would either of these workarounds be acceptable, and which would be
preferred?

Thanks,
Stefan
Francesco Dolcini June 21, 2024, 5:24 p.m. UTC | #6
Hello Stefan,

On Fri, Jun 21, 2024 at 05:22:46PM +0200, Stefan Eichenberger wrote:
> Hi Andi, Andrew, Wolfram, Oleksij,
> 
> After some internal discussion we still have some questions which are
> blocking us from solving the issue.
> 
> On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > bus is idle. The imx i2c driver will call schedule when waiting for the
> > bus to become idle after switching to master mode. When the i2c
> > controller switches to master mode it pulls SCL and SDA low, if the
> > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > clocking, it will timeout and ignore all signals until the next start
> > condition occurs (SCL and SDA low). This can occur when the system load
> > is high and schedule returns after more than 25 ms.
> > 
> > This rfc tries to solve the problem by using a udelay for the first 10
> > ms before calling schedule. This reduces the chance that we will
> > reschedule. However, it is still theoretically possible for the problem
> > to occur. To properly solve the problem, we would also need to disable
> > interrupts during the transfer.
> > 
> > After some internal discussion, we see three possible solutions:
> > 1. Use udelay as shown in this rfc and also disable the interrupts
> >    during the transfer. This would solve the problem but disable the
> >    interrupts. Also, we would have to re-enable the interrupts if the
> >    timeout is longer than 1ms (TBD).
> > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> >    timeout, we try again.
> > 3. We use the suggested solution and accept that there is an edge case
> >    where the timeout can happen.
> > 
> > There may be a better way to do this, which is why this is an RFC.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---

...

> > On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be
> > tested to determine whether the serial bus is free.
> We assume this means it is not necessary to test for IBB if we know we
> are in a single-master configuration.

To me this is a very interesting option. If we can confirm that this
busy-wait-loop is not required when we have a single master
configuration we can just solve the issue in a clean way.

And once the driver knows if it is multi-master mode or not we can also
get rid of the EAGAIN that does not make any sense with single-master.
There was an old discussion with some great contribution from Oleksij on
this topic.

Francesco
Oleksij Rempel June 22, 2024, 5:02 a.m. UTC | #7
Hi Stefan,

On Fri, Jun 21, 2024 at 05:22:46PM +0200, Stefan Eichenberger wrote:
> Hi Andi, Andrew, Wolfram, Oleksij,
> 
> After some internal discussion we still have some questions which are
> blocking us from solving the issue.
> 
> On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > bus is idle. The imx i2c driver will call schedule when waiting for the
> > bus to become idle after switching to master mode. When the i2c
> > controller switches to master mode it pulls SCL and SDA low, if the
> > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > clocking, it will timeout and ignore all signals until the next start
> > condition occurs (SCL and SDA low). This can occur when the system load
> > is high and schedule returns after more than 25 ms.
> > 
> > This rfc tries to solve the problem by using a udelay for the first 10
> > ms before calling schedule. This reduces the chance that we will
> > reschedule. However, it is still theoretically possible for the problem
> > to occur. To properly solve the problem, we would also need to disable
> > interrupts during the transfer.
> > 
> > After some internal discussion, we see three possible solutions:
> > 1. Use udelay as shown in this rfc and also disable the interrupts
> >    during the transfer. This would solve the problem but disable the
> >    interrupts. Also, we would have to re-enable the interrupts if the
> >    timeout is longer than 1ms (TBD).
> > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> >    timeout, we try again.
> > 3. We use the suggested solution and accept that there is an edge case
> >    where the timeout can happen.
> > 
> > There may be a better way to do this, which is why this is an RFC.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 3842e527116b7..179f8367490a5 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -503,10 +503,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
> >  				"<%s> I2C bus is busy\n", __func__);
> >  			return -ETIMEDOUT;
> >  		}
> > -		if (atomic)
> > +		if (atomic) {
> >  			udelay(100);
> > -		else
> > -			schedule();
> > +		} else {
> > +			/*
> > +			 * Avoid rescheduling in the first 10 ms to avoid
> > +			 * timeouts for SMBus like devices
> > +			 */
> > +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> > +				udelay(10);
> > +			else
> > +				schedule();
> > +		}
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.40.1
> 
> If we want to be sure that the ADS1015 I2C ADC will never timeout, we
> would have to add a patch to disable preemption during transmission.
> This would look like this:
> 
> @@ -1244,6 +1248,12 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  	bool is_lastmsg = false;
>  	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
>  
> +	/* If we are in SMBus mode we need to do the transfer atomically */
> +	if (i2c_imx->smbus_mode) {
> +		preempt_disable();
> +		atomic = true;
> +	}
> +
>  	/* Start I2C transfer */
>  	result = i2c_imx_start(i2c_imx, atomic);
>  	if (result) {
> @@ -1320,6 +1330,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  	if (i2c_imx->slave)
>  		i2c_imx_slave_init(i2c_imx);
>  
> +	if (i2c_imx->smbus_mode)
> +		preempt_enable();
> +
>  	return (result < 0) ? result : num;
>  }
> 
> However, we are aware that disabling preemption is not a good idea. So
> we were discussing how this is normally handled with SMBus devices? Is
> it just expected that SMBus devices will timeout in rare cases?
> 
> For our use case, the problem would be solved if we could get rid of the
> schedule call and replace it with a udelay. It seems that the i.MX8M
> Mini I2C controller needs a few ms to clear the IBB flag. In the
> reference manual, they write:
> > I2C bus busy bit. Indicates the status of the bus. NOTE: When I2C is
> > enabled (I2C_I2CR[IEN] = 1), it continuously polls the bus data (SDA)
> > and clock (SCL) signals to determine a Start or Stop condition. Bus is
> > idle. If a Stop signal is detected, IBB is cleared. Bus is busy. When
> > Start is detected, IBB is set.
> Unfortunately, it is not clear how often they poll. In our tests the
> issue disappeard when we used udelay instead of usleep or schedule for
> the first 10 ms.

I'm not really happy with this variant. It can be acceptable in some
use cases, but may have not pleasant side effects on other. Since this
is still valid case, I would prefer to have a UAPI to enable polling
mode as optional optimization with a warning that it may affect latency
on other corner of the system.

> Since we know we don't have a multi-master configuration, another way
> would be to not test for IBB and just start the transfer. We saw that
> other drivers use the multi-master device tree property to determine if
> multi-master is supported. Here another quote from the reference manual:
> > On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be
> > tested to determine whether the serial bus is free.
> We assume this means it is not necessary to test for IBB if we know we
> are in a single-master configuration.

I interpret this part of documentation in the same way, so it will be
great if you can implement this option.

Regards,
Oleksij
Stefan Eichenberger June 24, 2024, 3:49 p.m. UTC | #8
Hi Oleksij,

On Sat, Jun 22, 2024 at 07:02:39AM +0200, Oleksij Rempel wrote:
> Hi Stefan,
> 
> On Fri, Jun 21, 2024 at 05:22:46PM +0200, Stefan Eichenberger wrote:
> > Hi Andi, Andrew, Wolfram, Oleksij,
> > 
> > After some internal discussion we still have some questions which are
> > blocking us from solving the issue.
> > 
> > On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote:
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > 
> > > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to
> > > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C
> > > bus is idle. The imx i2c driver will call schedule when waiting for the
> > > bus to become idle after switching to master mode. When the i2c
> > > controller switches to master mode it pulls SCL and SDA low, if the
> > > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL
> > > clocking, it will timeout and ignore all signals until the next start
> > > condition occurs (SCL and SDA low). This can occur when the system load
> > > is high and schedule returns after more than 25 ms.
> > > 
> > > This rfc tries to solve the problem by using a udelay for the first 10
> > > ms before calling schedule. This reduces the chance that we will
> > > reschedule. However, it is still theoretically possible for the problem
> > > to occur. To properly solve the problem, we would also need to disable
> > > interrupts during the transfer.
> > > 
> > > After some internal discussion, we see three possible solutions:
> > > 1. Use udelay as shown in this rfc and also disable the interrupts
> > >    during the transfer. This would solve the problem but disable the
> > >    interrupts. Also, we would have to re-enable the interrupts if the
> > >    timeout is longer than 1ms (TBD).
> > > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a
> > >    timeout, we try again.
> > > 3. We use the suggested solution and accept that there is an edge case
> > >    where the timeout can happen.
> > > 
> > > There may be a better way to do this, which is why this is an RFC.
> > > 
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > ---
> > >  drivers/i2c/busses/i2c-imx.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > > index 3842e527116b7..179f8367490a5 100644
> > > --- a/drivers/i2c/busses/i2c-imx.c
> > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > @@ -503,10 +503,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
> > >  				"<%s> I2C bus is busy\n", __func__);
> > >  			return -ETIMEDOUT;
> > >  		}
> > > -		if (atomic)
> > > +		if (atomic) {
> > >  			udelay(100);
> > > -		else
> > > -			schedule();
> > > +		} else {
> > > +			/*
> > > +			 * Avoid rescheduling in the first 10 ms to avoid
> > > +			 * timeouts for SMBus like devices
> > > +			 */
> > > +			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
> > > +				udelay(10);
> > > +			else
> > > +				schedule();
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > -- 
> > > 2.40.1
> > 
> > If we want to be sure that the ADS1015 I2C ADC will never timeout, we
> > would have to add a patch to disable preemption during transmission.
> > This would look like this:
> > 
> > @@ -1244,6 +1248,12 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
> >  	bool is_lastmsg = false;
> >  	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
> >  
> > +	/* If we are in SMBus mode we need to do the transfer atomically */
> > +	if (i2c_imx->smbus_mode) {
> > +		preempt_disable();
> > +		atomic = true;
> > +	}
> > +
> >  	/* Start I2C transfer */
> >  	result = i2c_imx_start(i2c_imx, atomic);
> >  	if (result) {
> > @@ -1320,6 +1330,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
> >  	if (i2c_imx->slave)
> >  		i2c_imx_slave_init(i2c_imx);
> >  
> > +	if (i2c_imx->smbus_mode)
> > +		preempt_enable();
> > +
> >  	return (result < 0) ? result : num;
> >  }
> > 
> > However, we are aware that disabling preemption is not a good idea. So
> > we were discussing how this is normally handled with SMBus devices? Is
> > it just expected that SMBus devices will timeout in rare cases?
> > 
> > For our use case, the problem would be solved if we could get rid of the
> > schedule call and replace it with a udelay. It seems that the i.MX8M
> > Mini I2C controller needs a few ms to clear the IBB flag. In the
> > reference manual, they write:
> > > I2C bus busy bit. Indicates the status of the bus. NOTE: When I2C is
> > > enabled (I2C_I2CR[IEN] = 1), it continuously polls the bus data (SDA)
> > > and clock (SCL) signals to determine a Start or Stop condition. Bus is
> > > idle. If a Stop signal is detected, IBB is cleared. Bus is busy. When
> > > Start is detected, IBB is set.
> > Unfortunately, it is not clear how often they poll. In our tests the
> > issue disappeard when we used udelay instead of usleep or schedule for
> > the first 10 ms.
> 
> I'm not really happy with this variant. It can be acceptable in some
> use cases, but may have not pleasant side effects on other. Since this
> is still valid case, I would prefer to have a UAPI to enable polling
> mode as optional optimization with a warning that it may affect latency
> on other corner of the system.
> 
> > Since we know we don't have a multi-master configuration, another way
> > would be to not test for IBB and just start the transfer. We saw that
> > other drivers use the multi-master device tree property to determine if
> > multi-master is supported. Here another quote from the reference manual:
> > > On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be
> > > tested to determine whether the serial bus is free.
> > We assume this means it is not necessary to test for IBB if we know we
> > are in a single-master configuration.
> 
> I interpret this part of documentation in the same way, so it will be
> great if you can implement this option.

Perfect thanks a lot for the feedback. I will try to implement this
option and run some tests to make sure it will not break anything.

Best regards,
Stefan
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 3842e527116b7..179f8367490a5 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -503,10 +503,18 @@  static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 				"<%s> I2C bus is busy\n", __func__);
 			return -ETIMEDOUT;
 		}
-		if (atomic)
+		if (atomic) {
 			udelay(100);
-		else
-			schedule();
+		} else {
+			/*
+			 * Avoid rescheduling in the first 10 ms to avoid
+			 * timeouts for SMBus like devices
+			 */
+			if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10)))
+				udelay(10);
+			else
+				schedule();
+		}
 	}
 
 	return 0;