lsm6dsv16x: add RTIO async and fifo stream

[avisconti]

Add RTIO async and RTIO stream functionalities that enables, among all the other things, the sensor data streaming from FIFO. RTIO stream is using both SENSOR_TRIG_FIFO_WATERMARK and SENSOR_TRIG_FIFO_FULL triggers. The decoder currently only supports following FIFO tags:

  - LSM6DSV16X_XL_NC_TAG
  - LSM6DSV16X_GY_NC_TAG
  - LSM6DSV16X_TEMP_NC_TAG

In DT it is possible to configure following FIFO params:

  - fifo-watermark (defaults to 32)
  - accel-fifo-batch-rate (defaults to LSM6DSV16X_DT_XL_NOT_BATCHED)
  - gyro-fifo-batch-rate (defaults to LSM6DSV16X_DT_GY_NOT_BATCHED)
  - temp-fifo-batch-rate (defaults to LSM6DSV16X_DT_TEMP_NOT_BATCHED)

Since I do not have any boards with a chipset with I2C/SPI rtio supported zephyr driver, I tested this solution using I2C rtio default driver. Waiting for #71961 to be reviewed, as well as a proper solution for STM32 SPI.

Moreover, this PR does not contain any specific sample (in the future I may add one), but I'm using a modified version of samples/sensor/accel_polling to test the FIFO streaming.

[teburd]

looking pretty good to me, I do think the workq could be avoided here with a little effort to do the reads asynchronously, or configure the sensor asynchronously (though I get this is a little bit of a hassle).

The workq does add some code size in trade for convenience.

[teburd]

16 is quite a large number of submissions and completions to keep in the pool Each one is going to be ~32bytes, 16*32 512B of potential space here.

Likely only needs 2 to 4 depending on what kind of transactions you are doing.

[avisconti]

ok

[teburd]

Same comment as above, I'd recommend reducing the number of submissions (and completions) in the pool to something like 2 or 4 here. Likely 4 for doing the typical write a register, read a value, do a callback.

[avisconti]

But other sensor drivers are using 8/16. Maybe I could fix it to 4?

[teburd]

If they are using 16, this is likely wasteful... I'd try fixing it to 4

[teburd]

Not required, but one possibility here is to look at the current request options and sensor config to assign an appropriate callback here avoiding some branching.

Potentially you could directly read the FIFO perhaps because the watermark is set, you can see if the data is requested with it.

Other possibilities to reduce branching are likely here but I get the current example drivers don't do this.

[avisconti]

Sorry, this is the only thing I'm not addressing. If it is really requested I'll explore it. :(

[teburd]

Understandable on the branching front

probably worth ignoring the response of the callback

check_fifo_status_reg->flags |= RTIO_SQE_NO_RESPONSE;

I'm still wondering about callbacks and responses, but today they do result in a completion response.

[teburd]

I had forgotten @JordanYates added a helper for this,

rtio_sqe_prep_callback_no_cqe()

https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/rtio/rtio.h#L618

[avisconti]

re-pushed with some fix in order to (hopefully) pass CI. Still need to go thru some of the review comments...

[XenuIsWatching]

I actually also did the same thing (almost in the same way here) except with I3C with FIFO streaming with I3C RTIO.... I'll get to creating a PR on top of this maybe this week or next... but I still need a review on #79346

[avisconti]

I actually also did the same thing (almost in the same way here) except with I3C with FIFO streaming with I3C RTIO.... I'll get to creating a PR on top of this maybe this week or next... but I still need a review on #79346

OK, I'm going to review #79346 right now. Thanks

[avisconti]

I actually also did the same thing (almost in the same way here) except with I3C with FIFO streaming with I3C RTIO.... I'll get to creating a PR on top of this maybe this week or next... but I still need a review on #79346

BTW, what hardware are you using? It seems that you already have a rtio-enabled I3C bus driver.

[teburd]

Some small noteworthy things I took notice of. Hopefully the last round

[XenuIsWatching]

if I run with the fifo_th interrupt, all of sudden I this error after running for less than a minute. I don't know much about the internals of RTIO as to why...
If I run with the fifo_full interrupt which doesn't trigger as often, I don't see it

@teburd do you have any ideas?

[00:00:10.651,000] <err> LSM6DSV16X_RTIO: Bus error: -140

[teburd]

-ECANCELLED implies an error or intentional cancellation of that submission or one before it in the same chain/transaction. Beyond that its not obvious, I'd add some logging.

[XenuIsWatching]

I'm not doing any intentional cancellation so it must be error... I'll try to do some digging on my own here

[XenuIsWatching]

@teburd somehow increasing cq_sz in RTIO_DEFINE(name, sq_sz, cq_sz) from 4 to 8 makes the error not happen anymore, I'm not quite sure how that is used

[teburd]

cq_sz is the pool of available completions, likely the gpio interrupt for handling the sensor fifo events is being unmasked before clearing out the results of the last gpio interrupt in some manner. One possible source could be caused the callback submissions generating completions and not dealing with those if the flag isn't set to not do that. I get that its maybe surprising behavior on that front, @JordanYates I believe ran into something similar in #76351

[teburd]

rtio_sqe_prep_callback_no_cqe() is helpful for the first case

https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/rtio/rtio.h#L618

[avisconti]

@XenuIsWatching not able to replicate. Can you give me more details on your tests? (i.e. fifo watermark, batching odr, ...). Also, I'm using I2C, but you are using I3C, so maybe not replicable on my side.

[XenuIsWatching]

yes, I am using I3C here

[XenuIsWatching]

I'm using zephyr 3.7 (just with this patched in), fifo watermark is 0x20, batching odr is 960Hz for accel and gyro along with running on I3C w/ IBI interrupts on my own commit (i'll try to get draft commit up soon, it's just that it has so many dependencies on unmerged PRs)

adding check_fifo_status_reg->flags |= RTIO_SQE_NO_RESPONSE; didn't fix it for me :(, but still having the cqe_sz set to 8 does....

[teburd]

Understandable on the branching front

probably worth ignoring the response of the callback

check_fifo_status_reg->flags |= RTIO_SQE_NO_RESPONSE;

I'm still wondering about callbacks and responses, but today they do result in a completion response.

[XenuIsWatching]

We'd really need someway to change this at runtime.... but I think that's a more general issue here and outside the scope of this PR...

[avisconti]

Yes, sure. I'm planning to add something to change at runtime watermark and maybe also FIFO batching rate, even if I think that the former is more important

[avisconti]

@teburd I seem to remember that you had already approved this PR previously before the latest small changes. Can you possibly take a look again at this latest stage?