drivers: dma: esp32s3: Add DMA support for esp32s3 #57495
Conversation
LucasTambor
requested review from
nashif,
galak,
glaubermaroto and
dcpleung
as code owners
zephyrbot
added
area: Devicetree Binding
zephyrbot
requested review from
andyross,
marekmatej,
sylvioalves,
teburd and
uLipe
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
f6a437d
to
a45d74a
Compare
tests/drivers/dma/chan_blen_transfer/boards/esp32s3_devkitm.conf
Outdated
Show resolved
Hide resolved
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
a45d74a
to
0d9ba76
Compare
|
maybe put files in |
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
0d9ba76
to
6801041
Compare
|
@marekmatej done ;) |
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
6801041
to
edd33a8
Compare
drivers/dma/dma_esp32_gdma.c
Outdated
| static int dma_esp32_config_rx_descriptor(struct dma_esp32_channel *dma_channel, | ||
| struct dma_block_config *block) | ||
| { | ||
| memset(&dma_channel->desc, 0, sizeof(dma_channel->desc)); | ||
| dma_channel->desc.buffer = (void *)block->dest_address; | ||
| dma_channel->buf_original = (uint8_t *)block->dest_address; | ||
| k_free(dma_channel->buf_temp); |
There was a problem hiding this comment.
You are freeing buf_temp which is allocated in the dma_esp32_config_tx_descriptor. Why cannot it be allocated for the driver lifetime?
| @@ -577,7 +624,7 @@ DMA_ESP32_DEFINE_IRQ_HANDLER(4) | |||
| }, \ | |||
| }; \ | |||
| \ | |||
| DEVICE_DT_INST_DEFINE(idx, &dma_esp32_init, NULL, &dma_data_##idx, &dma_config_##idx, \ | |||
| DEVICE_DT_INST_DEFINE(idx, &dma_esp32_init, NULL, &dma_data_##idx, &dma_config_##idx, \ | |||
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
edd33a8
to
c54a1c6
Compare
| static int dma_esp32_config_rx_descriptor(struct dma_esp32_channel *dma_channel, | ||
| struct dma_block_config *block) | ||
| { | ||
| memset(&dma_channel->desc, 0, sizeof(dma_channel->desc)); | ||
| dma_channel->desc.buffer = (void *)block->dest_address; | ||
| dma_channel->buf_original = (uint8_t *)block->dest_address; | ||
| k_free(dma_channel->buf_temp); | ||
| dma_channel->buf_temp = NULL; | ||
| if (!esp_ptr_dma_capable((uint32_t *)block->dest_address)) { |
There was a problem hiding this comment.
Why are we trying to allocate on behalf of callers here, if an invalid buffer for DMA is being provided this seems like a bug that should be asserted on.
I'm a little confused by the code here to malloc/free a temporary buffer when that's not the case. Is there a particular scenario esp32 expects this to occur?
There was a problem hiding this comment.
We are freeing the channel's temporary buffer every time a new descriptor needs to be created. In case the original buffer is in a valid DMA region, the k_free can handle a NULL pointer without issue and no allocation is needed. In case it is in an invalid region, we create a temporary one to handle the transactions, freeing it and reallocating according the size required by the transactions. I'm trying to deal with both scenarios here, since the original buffer location depends on the application or driver using it.
Did I answer your question? I'm not shure haha
There was a problem hiding this comment.
I guess what I'm suggesting is this is a bug then in the driver or application that should be caught. There's an RFC for managing buffers #57602
At the moment this would be, to my best knowledge, the only DMA driver that attempts to fix the buffer you asked a copy to by allocating from the heap. I would suggest pushing that issue up the stack as this is a surprising place to have that happen.
There was a problem hiding this comment.
Oh nice! I will follow up this RFC and contribute in whatever way I can. But since it is an ongoing discussion, should we block the merge of this commit? This strategy of dealing with buffer is around for quite a while, this commit only add support for esp32s3 and small fixes.
There was a problem hiding this comment.
I'd point out that generally Zephyr is trying to follow misra, and in misra-c "dynamic allocation is not allowed" See our guidelines https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#main-rules
So I think this is something that needs to be revisited but today should be considered a programming error trying to ask DMA to work with a buffer that doesn't conform to DMA requirements.
Add GDMA support for esp32s3. Remove suspend/resume since they are optional and do the same as start/stop. Fix possible null pointer derreference. Signed-off-by: Lucas Tamborrino <[email protected]>
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
c54a1c6
to
0728128
Compare
According to the coding guidelines "dynamic allocation is not allowed". This commit removes handling invalid DMA capable buffers by allocating temporary buffer in a valid memory region, considering them as errors. Signed-off-by: Lucas Tamborrino <[email protected]>
Add esp32s3 overlays for dma tests. Signed-off-by: Lucas Tamborrino <[email protected]>
LucasTambor
force-pushed
the
feature/s3_dma
branch
from
0728128
to
4ecb7b2
Compare
Add GDMA support for esp32s3.
Remove suspend/resume since they are optional and do the same as start/stop.
Fix possible null pointer derreference.