i2c: stm32: dma enhancement

[sgilbert182]

Add DMA enhancement for stm32 i2c_v2 driver

Fix #34354

[github-actions]

Hello @sgilbert182, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[erwango]

@sgilbert182 Thanks for this contribution. We'll try to review in best delays. Meanwhile,please have a look at feedback reported by compliance scripts
Also, would you mind providing some details on testing (which platforms, which tests, ..) ?

[sgilbert182]

Hi, no worries. I tested this on a g4

[erwango]

@sgilbert182 Quick hint: Clang format is not mandatory (and I agree this isn't clear at all, these warning are recent and should be deactivated IMO), if you want to make your life easier, you can drop format related changes.

[erwango]

To be clear on formatting changes (and hopefully we can focus on the content afterwards):

You should fix those (not fixing will block CI)

You are not requested to fix those (and even sometimes, it is better not to apply):

[sgilbert182]

To be clear on formatting changes (and hopefully we can focus on the content afterwards):

You should fix those (not fixing will block CI)

You are not requested to fix those (and even sometimes, it is better not to apply):

I seem to have some issues with Clang Format, I run it locally and it produces results that fail CI. Looks like I need to make the changes manually

[erwango]

Initial review attempt:

Mains request before going further:
Did you consider implementing I2C_CALLBACK which is supposed to be the async version of I2C API while you're at?
If adding DMA support it would be more logical to make use of this API which allows truly async transfers rather than adding DMA support on a blocking API.
Reason I ask is to avoid current state of STM32 SPI driver which is in a similar state today with a sync API allowing use of DMA and ASYNC API effectively blocking.

(Note also that driver is supposed to get further changes to support RTIO API, so let's limit the changes to where it makes real sense)

Then let's club all DMA related additions to the driver in a single commit to make it easier to review.

Finally, if possible, let's rebase on top of #82015 (once merged) in order to get rid of clang-format suggestions.

[sgilbert182]

Mains request before going further: Did you consider implementing I2C_CALLBACK which is supposed to be the async version of I2C API while you're at? If adding DMA support it would be more logical to make use of this API which allows truly async transfers rather than adding DMA support on a blocking API. Reason I ask is to avoid current state of STM32 SPI driver which is in a similar state today with a sync API allowing use of DMA and ASYNC API effectively blocking.

I did not, I was porting over changes I was using for a project but I can take a look at this when I have time.

(Note also that driver is supposed to get further changes to support RTIO API, so let's limit the changes to where it makes real sense)

I will revert this

Then let's club all DMA related additions to the driver in a single commit to make it easier to review.

Finally, if possible, let's rebase on top of #82015 (once merged) in order to get rid of clang-format suggestions.

should I close this MR until this can be rebased and I've had a chance to look at the callback mechanism?

[erwango]

I did not, I was porting over changes I was using for a project but I can take a look at this when I have time.

Would be great. Thanks

should I close this MR until this can be rebased and I've had a chance to look at the callback mechanism?

Not required. But for clarity, you can move it to draft.

[erian747]

For devices with DCACHE enabled and rx buffer is in cached mem, it think a cache invalidation should be done here
Another option would be to issue a log warning when transfer is started that memory should be non-cacheable like it is done in uart-stm32.c

[sgilbert182]

I've added cache support, slightly modified from the UART implementation see if it makes sense where I put it. I don't have a platform at hand to test it though. I'll have to dig through my box of bits and see if I have something

[erian747]

Use clean cache operation before starting DMA write and use invalidate cache after read DMA finishes
Also note that clean/invalidate addresses shall align with cache line size, 32 bytes

[sgilbert182]

Good catch, corrected

[erwango]

Thanks @sgilbert182 for the update.

Some comments on the code. Regarding the PR it self, there will be some more work required:

• Blocking: We don't merge fixup commits. So please directly fix initial commits.
• Not blocking: Regarding formatting, the clang formatting suggestions have now been disabled (they'll disappear if you rebase on top of latest main). So ideally, I'd suggest to drop the formatting changes.

[sgilbert182]

Hi @erwango, thanks for your comments

I've attempted to fix the initial commits (sorry for the flurry of forced pushes) but there's still a couple of commits I see as step changes rather than just fixes but you might feel differently. Let me know if you're happy or what still needs adjusting.

• Blocking: We don't merge fixup commits. So please directly fix initial commits.

I've rebased but should the formatting changes be kept so the file is more in line with current style?

• Not blocking: Regarding formatting, the clang formatting suggestions have now been disabled (they'll disappear if you rebase on top of latest main). So ideally, I'd suggest to drop the formatting changes.

[erwango]

Thanks for the update.

As mentioned on the code style, my personal taste would be to drop them, but they are done on a dedicated commit and maintain readability, so I don't have objective reason to block.

Regarding implementation steps, to me they clearly falls under points 3 and 5 in https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-requirements, so they should be squashed.

Last, I didn't came back on this in last review, but in my initial review I was mentioning that use of I2C_CALLBACK was preferred for DMA support (following the same experience that we had on SPI). I won't block on this point as I guess this new API variant can still be added later.

[sgilbert182]

I've condensed the formats to a single commit, I could drop it here and create a follow up MR with those formatting changes but it feels heavy handed.

As mentioned on the code style, my personal taste would be to drop them, but they are done on a dedicated commit and maintain readability, so I don't have objective reason to block.

All fixits should now be merged into the initial commits.

Regarding implementation steps, to me they clearly falls under points 3 and 5 in https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-requirements, so they should be squashed.

I have had a little time over the last few days and have attempted to add the async support but I haven't enough time to fully exercise it. It's a single commit so can be dropped and I can create a new MR when I have more time to work with it.

Last, I didn't came back on this in last review, but in my initial review I was mentioning that use of I2C_CALLBACK was preferred for DMA support (following the same experience that we had on SPI). I won't block on this point as I guess this new API variant can still be added later.