i2c: stm32: dma enhancement #81814

sgilbert182 · 2024-11-23T17:43:28Z

Add DMA enhancement for stm32 i2c_v2 driver

github-actions · 2024-11-23T17:44:54Z

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 · 2024-11-25T08:25:59Z

@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 · 2024-11-25T16:31:04Z

Hi, no worries. I tested this on a g4

erwango · 2024-11-25T16:36:51Z

@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 · 2024-11-26T10:08:54Z

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 · 2024-11-26T10:25:26Z

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.

erwango · 2024-11-26T10:25:26Z

drivers/i2c/Kconfig.stm32

+config I2C_DMA
+	bool "DMA support"
+	depends on I2C_STM32_V2
+	help
+	  Enable DMA support for the STM32 I2C driver.


Suggested change

config I2C_DMA

bool "DMA support"

depends on I2C_STM32_V2

help

Enable DMA support for the STM32 I2C driver.

config I2C_STM32_DMA

bool "STM32 I2C DMA support"

depends on I2C_STM32_V2

select DMA

help

Enable DMA support for the STM32 I2C driver.

Limited to `st,stm32-i2c-v2` compatible SoCs for now.

Optionally we should add select CACHE_MANAGEMENT if CPU_HAS_DCACHE if tested on H7/F7.
Otherwise, let's specify it is not supported for now on those (depends on !DCACHE and provide details in help section)

erwango · 2024-11-26T10:31:24Z

drivers/i2c/i2c_ll_stm32.c

+                data->current.is_err = 0;
+				dma_stop(dma_dev, channel);


Indentation

erwango · 2024-11-26T10:31:32Z

drivers/i2c/i2c_ll_stm32.c

+    switch (status) {
+        case DMA_STATUS_COMPLETE:
+                data->current.is_err = 0;
+				dma_stop(dma_dev, channel);


Indentation

erwango · 2024-11-26T10:42:23Z

drivers/i2c/i2c_ll_stm32.c

-		uint8_t *next_msg_flags = NULL;
-
-		if (num_msgs > 1) {
-			next = current + 1;
-			next_msg_flags = &(next->flags);
-		}
-		ret = stm32_i2c_transaction(dev, *current, next_msg_flags, slave);
+		ret = stm32_i2c_transaction(dev, *current, next ? &(next->flags) : NULL, slave);


Let's not mix non related DMA changes within this PR.

erwango · 2024-11-26T10:44:58Z

drivers/i2c/i2c_ll_stm32.c

+	k_sem_init(&data->dma_rx_sem, 1, K_SEM_MAX_LIMIT);
+	k_sem_init(&data->dma_tx_sem, 1, K_SEM_MAX_LIMIT);


See definition for K_SEM_MAX_LIMIT

This is intended for use when a semaphore does not have an explicit maximum limit, and instead is just used for counting purposes.

Are you sure this is the intended behavior in this case ?

Note that we probably have other bad usages elsewhere.

I used this to catch an unknown number of threads accessing the same I2C bus, do you have any known good usages I can follow instead?

k_sem_init(&data->dma_rx_sem, 1, 1); k_sem_init(&data->dma_tx_sem, 1, 1);

erwango · 2024-11-26T10:52:55Z

drivers/i2c/i2c_ll_stm32.c

+        case DMA_STATUS_BLOCK:
+                break;


Support of DMA_STATUS_BLOCK is related to enabling circular mode. If not supported for now (support could be enabled later) let's generate an assert in this case

Would you prefer an assert or a log_err?

erwango · 2024-11-26T11:05:20Z

drivers/i2c/i2c_ll_stm32_v2.c

+static int configure_dma(const struct device *dma_dev, uint32_t dma_channel,
+			 struct dma_config *dma_cfg, struct dma_block_config *blk_cfg,
+			 uint32_t source_address, uint32_t dest_address, struct i2c_msg *msg,
+			 struct k_sem *dma_sem, const char *direction)


It is suboptimal to use more than 4 parameters in a function on ARM platforms. 4 first params are accessed from registers (r0-r3), other params will be accessed from stack.
Any way to do this differently ?

Glad you mentioned it, I've been playing with re-jigging the structs which may help here... so yes!

structs re-jigged

sgilbert182 · 2024-11-26T11:25:57Z

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 · 2024-11-26T12:49:49Z

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.

Add option to enable DMA support Signed-off-by: Simon Gilbert <[email protected]>

Add initial DMA settings structs to stm32 i2c config and data structs Signed-off-by: Simon Gilbert <[email protected]>

Add macrobatics to pull DMA settings from device tree Signed-off-by: Simon Gilbert <[email protected]>

Add DMA header files Signed-off-by: Simon Gilbert <[email protected]>

Add DMA options (phandle-array and names) to yaml file Signed-off-by: Simon Gilbert <[email protected]>

Tidy up of i2c_stm32_transfer and added TX and RX semaphore inits

Added stop DMA fot transmit complete or other master end conditions

Clang format Signed-off-by: Simon Gilbert <[email protected]>

Add assert catches in DMA callbacks Signed-off-by: Simon Gilbert <[email protected]>

use structs to pass dma device and config settings more easily Signed-off-by: Simon Gilbert <[email protected]>

Second pass at applying Clang Format Signed-off-by: Simon Gilbert <[email protected]>

sgilbert182 requested a review from ydamigos as a code owner November 23, 2024 17:43

zephyrbot added area: I2C platform: STM32 ST Micro STM32 labels Nov 23, 2024

zephyrbot requested review from erwango, FRASTM, gautierg-st, GeorgeCGV, marwaiehm-st, mathieuchopstm and teburd November 23, 2024 17:44

zephyrbot assigned erwango Nov 23, 2024

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from 1adf73d to 601d56f Compare November 23, 2024 18:55

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch 2 times, most recently from 357f005 to d465675 Compare November 25, 2024 16:29

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from d465675 to 57b1147 Compare November 25, 2024 19:18

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from 57b1147 to 057bc2b Compare November 26, 2024 10:37

erwango requested changes Nov 26, 2024

View reviewed changes

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from 057bc2b to 94c6e1d Compare November 26, 2024 12:29

sgilbert182 marked this pull request as draft November 26, 2024 13:43

sgilbert182 added 3 commits November 26, 2024 17:37

i2c: stm32: Add config option to enable DMA support

5ebd513

Add option to enable DMA support Signed-off-by: Simon Gilbert <[email protected]>

i2c: stm32: Add dma settings structs to config and data structs

253557b

Add initial DMA settings structs to stm32 i2c config and data structs Signed-off-by: Simon Gilbert <[email protected]>

i2c: stm32: Add macros to get settings from device tree

ee3915e

Add macrobatics to pull DMA settings from device tree Signed-off-by: Simon Gilbert <[email protected]>

sgilbert182 added 5 commits November 26, 2024 17:37

i2c: stm32: Add dma header files

5b8e89f

Add DMA header files Signed-off-by: Simon Gilbert <[email protected]>

i2c: stm32: Add DMA settings to yaml file

63538cc

Add DMA options (phandle-array and names) to yaml file Signed-off-by: Simon Gilbert <[email protected]>

i2c: stm32: Minor refactor of i2c_stm32_transfer function

ff5602b

Tidy up of i2c_stm32_transfer and added TX and RX semaphore inits

i2c: stm32: Added DMA stop for various i2c comms conditions

207cf56

Added stop DMA fot transmit complete or other master end conditions

i2c: stm32: Clang format

b394ecb

Clang format Signed-off-by: Simon Gilbert <[email protected]>

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from 94c6e1d to c14037a Compare November 26, 2024 17:37

sgilbert182 added 2 commits November 27, 2024 08:51

i2c: stm32: Add assert catches in callbacks

dee3c6e

Add assert catches in DMA callbacks Signed-off-by: Simon Gilbert <[email protected]>

i2c: stm32: refactor use of DMA device config and settings

9a6a2f5

use structs to pass dma device and config settings more easily Signed-off-by: Simon Gilbert <[email protected]>

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from f9bdf86 to cfd0b0a Compare November 27, 2024 09:01

i2c: stm32: Clang format

8c030ea

Second pass at applying Clang Format Signed-off-by: Simon Gilbert <[email protected]>

sgilbert182 force-pushed the feature/stm32-i2c-dma-enhancement branch from cfd0b0a to 8c030ea Compare November 27, 2024 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i2c: stm32: dma enhancement #81814

i2c: stm32: dma enhancement #81814

sgilbert182 commented Nov 23, 2024 •

edited

Loading

github-actions bot commented Nov 23, 2024

erwango commented Nov 25, 2024

sgilbert182 commented Nov 25, 2024

erwango commented Nov 25, 2024

erwango commented Nov 26, 2024

sgilbert182 commented Nov 26, 2024

erwango left a comment •

edited

Loading

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

erwango Nov 26, 2024

sgilbert182 Nov 26, 2024

sgilbert182 Nov 26, 2024

sgilbert182 commented Nov 26, 2024

erwango commented Nov 26, 2024

		k_sem_init(&data->dma_rx_sem, 1, K_SEM_MAX_LIMIT);
		k_sem_init(&data->dma_tx_sem, 1, K_SEM_MAX_LIMIT);

i2c: stm32: dma enhancement #81814

Are you sure you want to change the base?

i2c: stm32: dma enhancement #81814

Conversation

sgilbert182 commented Nov 23, 2024 • edited Loading

github-actions bot commented Nov 23, 2024

erwango commented Nov 25, 2024

sgilbert182 commented Nov 25, 2024

erwango commented Nov 25, 2024

erwango commented Nov 26, 2024

sgilbert182 commented Nov 26, 2024

erwango left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgilbert182 commented Nov 26, 2024

erwango commented Nov 26, 2024

sgilbert182 commented Nov 23, 2024 •

edited

Loading

erwango left a comment •

edited

Loading