Re: Sony IMX290/462 image sensors I2C xfer peculiarity

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Krzysztof Hałasa]

Hi,

adding more people to Cc: as this is more general stuff than my specific
image sensor setup.

Is there any reason for the following (meta) patch to not be applied?

Currently, every i.MX8MP atomic I2C transfer starts with 100 us delay
(just after the START condition). At 400 kHz bus (384 kHz or whatever),
this is equivalent to several tens of bits. Is this delay needed?

This is on NXP 5.15 branch, but it seems the mainline is identical here.

With this patch, the 1-byte (quasi) atomic image sensor register reads
(16-bit address + 8-bit value) are down to ca. 160 us, and writes take
120 us.

It seems one bit on the bus takes ca. 2.66 us (hardware), and the delay
between consecutive bytes is ca. 4.82 us (I guess CPU takes a fair share
of this). This is i.MX8MP @ apparently 1200 MHz (1600 MHz with freq
scaler).

Fire away.
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -534,xx +534,xx @@
 static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool atomic)
 {
     unsigned long orig_jiffies = jiffies;
     unsigned int temp;
 
     while (1) {
         temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 
         /* check for arbitration lost */
         if (temp & I2SR_IAL) {
             i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
             return -EAGAIN;
         }
 
         if (for_busy && (temp & I2SR_IBB)) {
             i2c_imx->stopped = 0;
             break;
         }
         if (!for_busy && !(temp & I2SR_IBB)) {
             i2c_imx->stopped = 1;
             break;
         }
         if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
             dev_dbg(&i2c_imx->adapter.dev,
                 "<%s> I2C bus is busy\n", __func__);
             return -ETIMEDOUT;
         }
         if (atomic)
-            udelay(100);
+            udelay(1);
         else
             schedule();
     }
 
     return 0;
 }
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Stefan Lengfeld]

Hi Krzysztof,

> Currently, every i.MX8MP atomic I2C transfer starts with 100 us delay
> (just after the START condition). At 400 kHz bus (384 kHz or whatever),
> this is equivalent to several tens of bits. Is this delay needed?

I cannot answer whether the delay is needed for atomic transfer or not.
But I can give a bit of context for I2C atomic transfers in general.

These where only introduced for a very narrow and special uses shutting
down the device/power with external PMICs in the kernel's shutdown
handlers.

E.g. the see code in the file 'drivers/i2c/i2c-core.h':

    /*
     * We only allow atomic transfers for very late communication, e.g. to access a
     * PMIC when powering down. Atomic transfers are a corner case and not for
     * generic use!
     */
    static inline bool i2c_in_atomic_xfer_mode(void)
    {
            return system_state > SYSTEM_RUNNING && irqs_disabled();
    }

So I wonder why this delay is a problem for your described you case.
The e-mail title talks about an image sensor. Why and in which use case
this sensor needs an atomic transfer? 

My understand is that an ordinary I2C device would just use normal (and
sleepable) I2C transfers while the device is in use.

Kind regards,
Stefan

On Wed, Oct 11, 2023 at 11:50:12AM +0200, Krzysztof Hałasa wrote:
> Hi,
> 
> adding more people to Cc: as this is more general stuff than my specific
> image sensor setup.
> 
> Is there any reason for the following (meta) patch to not be applied?
> 
> Currently, every i.MX8MP atomic I2C transfer starts with 100 us delay
> (just after the START condition). At 400 kHz bus (384 kHz or whatever),
> this is equivalent to several tens of bits. Is this delay needed?
> 
> This is on NXP 5.15 branch, but it seems the mainline is identical here.
> 
> With this patch, the 1-byte (quasi) atomic image sensor register reads
> (16-bit address + 8-bit value) are down to ca. 160 us, and writes take
> 120 us.
> 
> It seems one bit on the bus takes ca. 2.66 us (hardware), and the delay
> between consecutive bytes is ca. 4.82 us (I guess CPU takes a fair share
> of this). This is i.MX8MP @ apparently 1200 MHz (1600 MHz with freq
> scaler).
> 
> Fire away.
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -534,xx +534,xx @@
>  static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool atomic)
>  {
>      unsigned long orig_jiffies = jiffies;
>      unsigned int temp;
>  
>      while (1) {
>          temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  
>          /* check for arbitration lost */
>          if (temp & I2SR_IAL) {
>              i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
>              return -EAGAIN;
>          }
>  
>          if (for_busy && (temp & I2SR_IBB)) {
>              i2c_imx->stopped = 0;
>              break;
>          }
>          if (!for_busy && !(temp & I2SR_IBB)) {
>              i2c_imx->stopped = 1;
>              break;
>          }
>          if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>              dev_dbg(&i2c_imx->adapter.dev,
>                  "<%s> I2C bus is busy\n", __func__);
>              return -ETIMEDOUT;
>          }
>          if (atomic)
> -            udelay(100);
> +            udelay(1);
>          else
>              schedule();
>      }
>  
>      return 0;
>  }
> -- 
> Krzysztof "Chris" Hałasa
> 
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
> 

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Krzysztof Hałasa]

Stefan,

> I cannot answer whether the delay is needed for atomic transfer or not.
> But I can give a bit of context for I2C atomic transfers in general.
>
> These where only introduced for a very narrow and special uses shutting
> down the device/power with external PMICs in the kernel's shutdown
> handlers.

Well, I guess I'm abusing this code a bit.

The problem is I use Sony IMX290 and IMX462 image sensors, and they have
an apparently hard-coded timeout of about 2^18 their master clock cycles
(= ca. 7 ms with my setup). After the timeout they simply disconnect
from the I2C bus. Of course, this isn't mentioned in the docs.
Unfortunately, "normal" I2C accesses take frequently more than those
7 ms (mostly due to scheduling when all CPU cores are in use). So I
hacked the IMX I2C driver a bit and now all accesses to the sensor use
the atomic paths and local_irq_save() (inside the driver only).

> My understand is that an ordinary I2C device would just use normal (and
> sleepable) I2C transfers while the device is in use.

You are spot-on here :-) Now I use IMX 290 and 462.

OTOH I wonder if such issues are limited to those sensors only.

Thanks for your immediate response,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Alexander Stein]

Hi,

Am Mittwoch, 11. Oktober 2023, 13:25:55 CEST schrieb Krzysztof Hałasa:
> Stefan,
> 
> > I cannot answer whether the delay is needed for atomic transfer or not.
> > But I can give a bit of context for I2C atomic transfers in general.
> > 
> > These where only introduced for a very narrow and special uses shutting
> > down the device/power with external PMICs in the kernel's shutdown
> > handlers.
> 
> Well, I guess I'm abusing this code a bit.
> 
> The problem is I use Sony IMX290 and IMX462 image sensors, and they have
> an apparently hard-coded timeout of about 2^18 their master clock cycles
> (= ca. 7 ms with my setup). After the timeout they simply disconnect
> from the I2C bus. Of course, this isn't mentioned in the docs.
> Unfortunately, "normal" I2C accesses take frequently more than those
> 7 ms (mostly due to scheduling when all CPU cores are in use). So I
> hacked the IMX I2C driver a bit and now all accesses to the sensor use
> the atomic paths and local_irq_save() (inside the driver only).

I assume that the master clock is running independently to I2C from the SoC 
the sensor is attached to. Your calculations indicate you are assuming ~400kHz 
I2C clock frequency.
But nothing is preventing that sensor from running on a 100kHz I2C bus. Even 
this "atomic" hack will not be sufficient in that case.

Best regards,
Alexander

> > My understand is that an ordinary I2C device would just use normal (and
> > sleepable) I2C transfers while the device is in use.
> 
> You are spot-on here :-) Now I use IMX 290 and 462.
> 
> OTOH I wonder if such issues are limited to those sensors only.
> 
> Thanks for your immediate response,


-- 
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
http://www.tq-group.com/

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Krzysztof Hałasa]

Hi Alexander,

Alexander Stein <alexander.stein@ew.tq-group.com> writes:

> I assume that the master clock is running independently to I2C from the SoC 
> the sensor is attached to. Your calculations indicate you are assuming ~400kHz 
> I2C clock frequency.

This is correct.

> But nothing is preventing that sensor from running on a 100kHz I2C bus. Even 
> this "atomic" hack will not be sufficient in that case.

It will be sufficient. Even if all times are 4x longer (they shouldn't
since the CPU won't get slower), they wouldn't reach, say, 1200 us.
There would still be quite a lot of a margin (the timeout is 7 ms).
Even with the faster MCLK (these sensors can operate on 37.125 and on
74.250 MHz) and IF the timeout is still 2^18 in the latter case (meaning
3.5 ms), it would most probably work.

Of course, the atomic hack is not meant for general consumption (at
least for now in its current shape), only the udelay() reduction
(100 -> 1) could probably go in.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Stefan Lengfeld]

Hi Chris,

> > My understand is that an ordinary I2C device would just use normal (and
> > sleepable) I2C transfers while the device is in use.
> 
> You are spot-on here :-) Now I use IMX 290 and 462.
> 
> OTOH I wonder if such issues are limited to those sensors only.

Hmm, yes. I know no other I2C device that has these timeout issues. (*)

> The problem is I use Sony IMX290 and IMX462 image sensors, and they have
> an apparently hard-coded timeout of about 2^18 their master clock cycles
> (= ca. 7 ms with my setup). After the timeout they simply disconnect
> from the I2C bus. Of course, this isn't mentioned in the docs.

hmm. I have no idea about this sensor and your setup. So I can just give hints:

This timeout seems strange. If this 7 ms timeout is required, it would mean
that I2C masters require to fullfill real-time/deadline requirements.  For
"small" I2C master in microcontrolles this seems ok-ish, but for general
operating systems real-time requirements are hard.  The real-time patches for
linux just landed recently and it still requires fine tuning the system for the
required deadlines.

Maybe you just hit a corner case or a bug, that can be avoid, in the I2C
device.  Maybe check with the manufacturer directly?

> Unfortunately, "normal" I2C accesses take frequently more than those
> 7 ms (mostly due to scheduling when all CPU cores are in use).

Yes, correctly. There are multiple cases in which I2C transactions to the same
device can be preempted/delayed: A busy system, as you said, or when some other driver
in the kernel accesses another I2C device on the same bus. This will lock the
bus/I2C adapter for the duration of its transfer.

Do you know the I2C repeated start feature [1]? This allows to batch together
multiple I2C read/writes in a single transfer. And in the best case, this
transfer is executed in one go without a delay in between. At least in the
kernel it's guaranteed that no other driver can go in between with another
transfer.

Kind regards,
Stefan

[1]: https://www.i2c-bus.org/repeated-start-condition/

(*) Fun answer: Actually external watchdogs have timeouts. But the timeout
duration is in the range of seconds, not milliseconds. And timeout expiration
is expected (in error cases ;-).

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Wolfram Sang]

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


> Do you know the I2C repeated start feature [1]? This allows to batch together
> multiple I2C read/writes in a single transfer. And in the best case, this
> transfer is executed in one go without a delay in between. At least in the
> kernel it's guaranteed that no other driver can go in between with another
> transfer.

If the HW does rep_start properly, it is even guaranteed on the bus
because the bus is never seen as free by other participants. Check
"START and STOP" conditions in the I2C specs.


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

Re: Sony IMX290/462 image sensors I2C xfer peculiarity

[Krzysztof Hałasa]

Hi Stefan,

> Maybe you just hit a corner case or a bug, that can be avoid, in the I2C
> device.  Maybe check with the manufacturer directly?

I don't have direct contact at Sony, I guess I can try to escalate this
through the part supplier, but I won't hold my breath.

> Do you know the I2C repeated start feature [1]? This allows to batch together
> multiple I2C read/writes in a single transfer. And in the best case, this
> transfer is executed in one go without a delay in between. At least in the
> kernel it's guaranteed that no other driver can go in between with another
> transfer.

Sure, imx290.c sensor driver use repeated STARTs. In fact, it only makes
things worse.

The timeout counter seems to start with the regular START (falling edge
of SDA), repeated STARTs don't reset it. After 2^18 + 8 or + 9 MCLK
cycles, the sensor simply disconnects from the bus, generating
pseudo-STOP if it was in the middle of its 0 bit (0->1 SDA change while
SCL high) or starting sending pseudo-1 bits otherwise (0xFF data on read
or negative ACK on write). This way repeated START -> longer transfer ->
higher probability of failure. Not that it really matters.

I don't know about in-sensor race conditions, for example on WRITE to
the chip, when the ACK it interrupted by the timeout (this can be
detected by the CPU, but not reliably, depending on actual timings).

OTOH with my "always use atomic xfers with these sensors" hack to the
i.MX I2C driver, it seems to work correctly (at least as far as I2C is
concerned).

I wonder if we could/should add some special handling of these sensors
in the mainline kernel. local_irq_save() and the atomic path do the
trick, but it would have to be done in all I2C drivers (or at least in
ones used with these sensors). If no other devices need such treatment,
well... Everyone can (possibly) use a non-official hack.

Thanks for your input,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa