ChibiOS Free Embedded RTOS

The attached patch adds support for mixing DMA and non-DMA I2C with STM32 I2Cv2. I'd like some feedback on the approach before I do the equivalent patch for I2Cv1.
The reason for the change is we have a board based on STM32F765 that has all 4 I2C ports active. It also has a pile of UARTs and SPI devices, and doesn't have enough DMA streams available to have all 4 I2C devices using DMA while also keeping the time critical SPI and UART devices using DMA.
We could just disable DMA for all I2C ports, but that seems like a waste, as some of the I2C buses will be quite active.
The downside of this patch is that a developer won't get a build error if they enable I2C DMA and forget to define all of the STM32_I2C_I2Cn_XX_DMA_STREAM macros needed for both TX and RX. What will happen instead is that DMA will be automatically (and silently) disabled for that I2C port.
We could have another define to specifically enable mixed DMA and non-DMA operation, but the ifdefs will end up pretty convoluted, so I'd like to know how you'd like this handled.
Cheers, Tridge

Just a question, do you need all those ports at the same time? DMA can be shared playing with start/stop functions.

Anyway, commenting out configuration options is not a patter we use. We are introducing more checks to make sure that configuration files are correct, see how chconf.h is handled in trunk code, this will be extended. Additionally, removing settings would break our new tool that realigns files to templates.

Giovanni

Giovanni wrote:Just a question, do you need all those ports at the same time? DMA can be shared playing with start/stop functions.

We're already doing that actually. That is how we have I2C1,2 and 3 working. We automatically generate a conflict table for DMA and we have a shared DMA module in ArduPilot HAL_ChibiOS which does the appropriate transfers between devices, locking etc.
For I2C4 we can't do it as the automatic resolver can't find a DMA stream that isn't taken already by a device that can't share DMA. For example, we can't share DMA for the ADC port, or for UARTn_RX. In principle we could extend ChibiOS to share UARTn_RX by triggering the DMA start in the interrupt for the first byte, but we haven't tried to do that yet, so the resolver marks all UARTn_RX DMA streams as non-shareable. Some other ones are non-shareable too, such as SDMMC. That could be shared in principle, but we haven't added the code into ChibiOS to do that yet. So far we can only share UART_TX, SPI_TX, SPI_RX, I2C_RX and I2C_TX.

Anyway, commenting out configuration options is not a patter we use.

I don't quite understand what you mean. The patch doesn't comment out any config options. It just interprets "STM32_I2C_USE_DMA" as "use DMA on I2C ports that have DMA channels" instead of "only allow an I2C port to be used if both the RX and TX DMA channels are defined"
So it is definitely a change in behaviour, but it also gains a lot of flexibility without adding any new options.

We are introducing more checks to make sure that configuration files are correct, see how chconf.h is handled in trunk code, this will be extended. Additionally, removing settings would break our new tool that realigns files to templates.

That's fine, but I don't quite understand how it relates to the proposed patch.
Can you suggest an alternative approach?
Cheers, Tridge

I checked if our DMA resolver script is doing an optimal job, and it is for this case. On the STM32F7xx the ITC4_TX peripheral is only available on DMA1 stream 6. The UART8_RX is also only available on DMA1 stream 6. On this board the UART8_RX is used for the 1.5 MBit link to the IOMCU (a STM32F103), which really must use DMA (especially as the protocol it uses is framed using packet timing).
So if we want to use UART8, which we must use on this board, then we can't do DMA on I2C4_TX. We could do DMA on I2C4_RX, but ChibiOS doesn't have the option to do DMA only in one direction on I2C (that is another thing I have considered adding).
Cheers, Tridge

Yes, it allocates two DMA channels even if you don't receive data. A flag could be added to the configuration structure.

On the H7 and L4+ they did something interesting, you can assign any DMA to any peripherail (in the same domain), the I2C driver could be modified to use only one DMA for both RX and TX on those platforms.

That's fine, but I don't quite understand how it relates to the proposed patch.

I meant that if you undefine anything in a configuration file the update tool would define it again and assign the default value to it. This is why commenting out is a bad idea. OK, this is not the case.

Giovanni

Giovanni wrote:Yes, it allocates two DMA channels even if you don't receive data. A flag could be added to the configuration structure.

ok, are you suggesting I add a use_dma flag to the i2c config structure to disable the use of DMA? I added a using_dma flag in the current proposed patch, but it is automatically setup in i2c_lld_init() based on the available DMA channels. Having the channels defined in the i2c driver but never used based on a config option would work, although it seems a bit odd to be defining a DMA channel that can never be used.

On the H7 and L4+ they did something interesting, you can assign any DMA to any peripherail (in the same domain), the I2C driver could be modified to use only one DMA for both RX and TX on those platforms.

that sounds very nice! I hope to get a H7 board in the next couple of months. It will be interesting to see how that works.
I need to keep supporting F4 and F7 for a long time to come though. The F7 boards for ArduPilot are just coming to market now, we can't deprecate them for a long time

This is why commenting out is a bad idea. OK, this is not the case

yep, I didn't do it that way.

tridge wrote:For I2C4 we can't do it as the automatic resolver can't find a DMA stream that isn't taken already by a device that can't share DMA.

There's actually a documentation error for I2C4 DMA - see https://community.st.com/thread/50484-documentation-error-on-dma-channels-for-i2c4-on-stm32f7. May help you.
Not sure whether its been included in Chibi yet - on a quick look I thought so.

steved wrote:There's actually a documentation error for I2C4 DMA - see https://community.st.com/thread/50484-documentation-error-on-dma-channels-for-i2c4-on-stm32f7. May help you.

yes, that is very helpful, thank you!
I'd still like to be able to run some I2C devices without DMA, but not having I2C4_TX locked to a single stream is great

that datasheet fix for I2C4_TX does allow us to get I2C4 with DMA. This is what our resolver gives now:

// auto-generated DMA mapping from dma_resolver.py

// Note: The following peripherals can't be resolved for DMA: ['TIM4_UP', 'USART2_TX', 'USART2_RX', 'USART3_RX']

#define STM32_ADC_ADC1_DMA_STREAM STM32_DMA_STREAM_ID(2, 4)
#define STM32_ADC_ADC1_DMA_CHAN 0
#define STM32_I2C_I2C1_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 5) // shared I2C1_RX,I2C4_TX
#define STM32_I2C_I2C1_RX_DMA_CHAN 1
#define STM32_I2C_I2C1_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 7) // shared I2C1_TX,I2C2_TX
#define STM32_I2C_I2C1_TX_DMA_CHAN 1
#define STM32_I2C_I2C2_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 3) // shared I2C2_RX,USART3_TX
#define STM32_I2C_I2C2_RX_DMA_CHAN 7
#define STM32_I2C_I2C2_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 7) // shared I2C1_TX,I2C2_TX
#define STM32_I2C_I2C2_TX_DMA_CHAN 7
#define STM32_I2C_I2C3_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 2) // shared I2C3_RX,I2C4_RX
#define STM32_I2C_I2C3_RX_DMA_CHAN 3
#define STM32_I2C_I2C3_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 4) // shared SPI2_TX,I2C3_TX
#define STM32_I2C_I2C3_TX_DMA_CHAN 3
#define STM32_I2C_I2C4_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 2) // shared I2C3_RX,I2C4_RX
#define STM32_I2C_I2C4_RX_DMA_CHAN 2
#define STM32_I2C_I2C4_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 5) // shared I2C1_RX,I2C4_TX
#define STM32_I2C_I2C4_TX_DMA_CHAN 2
#define STM32_SDC_SDMMC1_DMA_STREAM STM32_DMA_STREAM_ID(2, 6)
#define STM32_SDC_SDMMC1_DMA_CHAN 4
#define STM32_SPI_SPI1_RX_DMA_STREAM STM32_DMA_STREAM_ID(2, 2)
#define STM32_SPI_SPI1_RX_DMA_CHAN 3
#define STM32_SPI_SPI1_TX_DMA_STREAM STM32_DMA_STREAM_ID(2, 3)
#define STM32_SPI_SPI1_TX_DMA_CHAN 3
#define STM32_SPI_SPI2_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 1)
#define STM32_SPI_SPI2_RX_DMA_CHAN 9
#define STM32_SPI_SPI2_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 4) // shared SPI2_TX,I2C3_TX
#define STM32_SPI_SPI2_TX_DMA_CHAN 0
#define STM32_SPI_SPI4_RX_DMA_STREAM STM32_DMA_STREAM_ID(2, 0)
#define STM32_SPI_SPI4_RX_DMA_CHAN 4
#define STM32_SPI_SPI4_TX_DMA_STREAM STM32_DMA_STREAM_ID(2, 1)
#define STM32_SPI_SPI4_TX_DMA_CHAN 4
#define STM32_TIM_TIM1_UP_DMA_STREAM STM32_DMA_STREAM_ID(2, 5)
#define STM32_TIM_TIM1_UP_DMA_CHAN 6
#define STM32_UART_UART8_RX_DMA_STREAM STM32_DMA_STREAM_ID(1, 6)
#define STM32_UART_UART8_RX_DMA_CHAN 5
#define STM32_UART_UART8_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 0)
#define STM32_UART_UART8_TX_DMA_CHAN 5
#define STM32_UART_USART3_TX_DMA_STREAM STM32_DMA_STREAM_ID(1, 3) // shared I2C2_RX,USART3_TX
#define STM32_UART_USART3_TX_DMA_CHAN 4

If interrupt-driven serial is good enough for you, there are two drivers I've posted here in the past:
a) A UART driver which can use interrupts instead of DMA
b) A version of the serial driver which supports callbacks similar to the UART driver.

(I use character-by character reception for protocol decoding; hence needing the callbacks. I found the UART driver over-complicated for my needs, so modified the serial driver. One day I'll do a hybrid, with DMA for transmission only).

If either would be useful and you can't find them, let me know.

Your DMA resolver looks to be a potentially useful general tool - it can be "fun" getting the DMA assignments right.