Silabs siwx91x gpdma driver #93556
Silabs siwx91x gpdma driver #93556
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
github-actions
bot
added
manifest
manifest-hal_silabs
DNM (manifest)
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
3 times, most recently
from
15f1e38 to
e8d84a9
Compare
zephyrbot
added
area: DMA
zephyrbot
requested review from
asmellby,
jerome-pouiller,
jhedberg,
Martinhoff-maker,
mnkp,
nordic-krch,
rettichschnidi,
teburd and
yonsch
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
from
e8d84a9 to
395aa21
Compare
| if (blen < 4) { | ||
| return AHBBURST_SIZE_1; |
There was a problem hiding this comment.
that feels not right, if a user want a blen of 2 (I don't know why), you have a burst size of 1, it's tricky for him no ?
There was a problem hiding this comment.
Yes, the final burst length (blen) is calculated as the least value between ahb_burst and the source/destination burst length. Code is updated now.
| static ATOMIC_DEFINE(dma_channels_atomic_##inst, \ | ||
| DT_INST_PROP(inst, silabs_dma_channel_count)); \ | ||
| SYS_MEM_BLOCKS_DEFINE_STATIC(desc_pool_##inst, 32, \ | ||
| CONFIG_GPDMA_SILABS_SIWX91X_DESCRIPTOR_COUNT, 512); \ |
There was a problem hiding this comment.
alignment of the mem block needs to be that high ? descriptor for gpdma have such constraint ? or is it because of the usage of contiguous sys mem block ?
There was a problem hiding this comment.
Most tests in WiseConnect align descriptor arrays to 512 or 1024 bytes. However, I changed the alignment value to 32 bytes and ran the tests, which appear to work correctly. The alignment has now been updated to 32 bytes.
| #define SIWX91X_GPDMA_INIT(inst) \ | ||
| static ATOMIC_DEFINE(dma_channels_atomic_##inst, \ | ||
| DT_INST_PROP(inst, silabs_dma_channel_count)); \ | ||
| SYS_MEM_BLOCKS_DEFINE_STATIC(desc_pool_##inst, 32, \ |
There was a problem hiding this comment.
sizeof(RSI_GPDMA_DESC_T)
There was a problem hiding this comment.
Each descriptor block address must be 32-byte aligned for GPDMA. Since sizeof(RSI_GPDMA_DESC_T) is 20 bytes, without 32-byte alignment for each block, some test cases are failing.
|
|
||
| if (channel_int_status & abort_mask) { | ||
| RSI_GPDMA_AbortChannel(GPDMA_HANDLE, channel); | ||
| cfg->reg->GLOBAL.INTERRUPT_STAT_REG = abort_mask; |
There was a problem hiding this comment.
for all reg operation, please use sys_read(...) and sys_write(...)
| data->chan_info[channel].desc_count); | ||
| data->chan_info[channel].chan_base_desc = NULL; | ||
| data->chan_info[channel].desc_count = 0; | ||
| cfg->reg->GLOBAL.INTERRUPT_STAT_REG = done_mask; |
There was a problem hiding this comment.
don't you only want to clean the interrupt for the channel ?
There was a problem hiding this comment.
Writing 0 to these register bits has no effect.
| return 0; | ||
| } | ||
|
|
||
| static void siwx91x_gpdma_isr(const struct device *dev) |
There was a problem hiding this comment.
how do you handle it if you have multiple channel isr ?
There was a problem hiding this comment.
The ISR will be triggered multiple times for multiple channel interrupts. The ISR will continue to be retriggered until all channel bit fields in cfg->reg->GLOBAL.INTERRUPT_STAT_REG are cleared. I tested this scenario in the SPI transfer case
|
|
||
| data->chan_info[channel].dma_callback = config->dma_callback; | ||
| data->chan_info[channel].cb_data = config->user_data; | ||
| atomic_set_bit(data->dma_ctx.atomic, channel); |
There was a problem hiding this comment.
The atomic bitset here is being used elsewhere for channel acquire/release calls by default. If the bit isn't set while configuring, and the caller is preempted, its possible to race on obtaining and configuring the channel.
There was a problem hiding this comment.
Agreed, this should be handled through channel request and release APIs. Removed this call here.
| return -EINVAL; | ||
| } | ||
|
|
||
| if (!atomic_test_bit(data->dma_ctx.atomic, channel)) { |
There was a problem hiding this comment.
You shouldn't need these atomic ops in start/stop
| xfer_cfg->chnlCtrlConfig.srcFifoMode = 1; | ||
| } | ||
|
|
||
| if (sys_mem_blocks_alloc_contiguous(data->gpdma_desc_pool, 1, &gpdma_desc_addr)) { |
There was a problem hiding this comment.
The mem_block allocator does not have an internal lock. The only acceptable lock you can use here (as DMA APIs should absolutely be ISR ok) would be a spin lock.
Alternatively you could have statically allocated descriptors for each channel and use them as needed. Though this might leave more "dead" descriptors around could be done without locks.
It'd be nicer to me to see a struct descriptor type with a well understood size rather than a void * being used.
There was a problem hiding this comment.
I implemented a spin lock and updated the descriptor type for 'gpdma_desc_addr'. I will use mem_block as it handles the allocation/deallocation of memory blocks better than statically allocated ones.
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
3 times, most recently
from
d71fb6e to
b80bcc8
Compare
Clock driver changes required for initializing the GPDMA clock for the siwx91x driver Signed-off-by: Sai Santhosh Malae <[email protected]>
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
from
b80bcc8 to
7078699
Compare
| key = k_spin_lock(&data->mem_blocks_lock); | ||
|
|
||
| if (sys_mem_blocks_alloc_contiguous(data->gpdma_desc_pool, 1, | ||
| (void **)&gpdma_desc_addr)) { | ||
| if (pPrevDesc == NULL) { | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
This looks like a bug to me. You're returning from the function before unlocking the spin lock.
| uint16_t max_xfer_size = GPDMA_DESC_MAX_TRANSFER_SIZE - config->source_data_size; | ||
| const struct dma_block_config *block_addr = config->head_block; | ||
| RSI_GPDMA_DESC_T *gpdma_desc_addr = NULL; | ||
| RSI_GPDMA_DESC_T *pPrevDesc = NULL; |
There was a problem hiding this comment.
Is there a good reason to use CamelCase and Hungarian notation for this variable name? (which violates the Zephyr coding style)
There was a problem hiding this comment.
No, it is updated now
Implement GPDMA driver for siwx91x device Signed-off-by: Sai Santhosh Malae <[email protected]>
1. Create a YAML file for gpdma node 2. Add GPDMA node in the siwx917.dtsi Signed-off-by: Sai Santhosh Malae <[email protected]>
Add siwx917_rb4338a board overlay and config files Signed-off-by: Sai Santhosh Malae <[email protected]>
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
from
7078699 to
d6c57ac
Compare
smalae
force-pushed
the
silabs-siwx91x-gpdma-driver
branch
from
d6c57ac to
05ff70c
Compare
|
Enable GPDMA driver for siwx91x device