Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: dma: siwx917: Enable scatter-gather transfer support #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

drivers: dma: siwx917: Enable scatter-gather transfer support #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99c06c1
drivers: dma: siwx917: Enable scatter-gather transfer support
smalae Dec 11, 2024
3f3c34e
drivers: dma: siwx917: Review comment changes
smalae Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ jobs:
-DEXTRA_CONF_FILE=$(pwd)/tests/drivers/dma/chan_blen_transfer/boards/siwx917_rb4338a.conf \
-v --inline-logs

- name: Build Scatter-Gather DMA test
working-directory: zephyr-silabs
shell: bash
run: |
west twister -s drivers.dma.scatter_gather -p siwx917_rb4338a -- \
-DTC_OVERLAY_FILE=$(pwd)/tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.overlay \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you meant -DDTC_OVERLAY_FILE=.... BTW, are you sure this syntax works? I believe the proper syntax is -x DTC_OVERLAY_FILE=....

(we also need to change Build DMA test)

-DEXTRA_CONF_FILE=$(pwd)/tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.conf \
-v --inline-logs

- name: Build Bluetooth samples
working-directory: zephyr-silabs
shell: bash
Expand Down
9 changes: 9 additions & 0 deletions .github/workflows/upstream-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@ jobs:
-DEXTRA_CONF_FILE=$(pwd)/tests/drivers/dma/chan_blen_transfer/boards/siwx917_rb4338a.conf \
-v --inline-logs

- name: Build Scatter-Gather DMA test
working-directory: zephyr-silabs
shell: bash
run: |
west twister -s drivers.dma.scatter_gather -p siwx917_rb4338a -- \
-DTC_OVERLAY_FILE=$(pwd)/tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.overlay \
-DEXTRA_CONF_FILE=$(pwd)/tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.conf \
-v --inline-logs

- name: Build Bluetooth samples
working-directory: zephyr-silabs
shell: bash
Expand Down
7 changes: 6 additions & 1 deletion drivers/dma/Kconfig.siwx917
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ menuconfig DMA_SILABS_SIWX917
help
Enable the High Power(HP)/Ultra Low Power(ULP) DMA driver for the Silabs SiWx917 SoC series.

if DMA_SILABS_SIWX917
config DMA_SILABS_SIWX917_COMMON_INIT_PRIORITY
int "Common initialization priority"
depends on DMA_SILABS_SIWX917
default 42

config DMA_SILABS_SIWX917_SG_BUFFER_COUNT
int "The maximum allowable number of buffers for scatter-gather transfers"
default 10
endif
226 changes: 207 additions & 19 deletions drivers/dma/dma_silabs_siwx917.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,36 @@
#include <errno.h>
#include <zephyr/irq.h>
#include <zephyr/sys/util.h>
#include <zephyr/sys/sys_io.h>
#include <zephyr/device.h>
#include <zephyr/drivers/dma.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/bitarray.h>
#include <zephyr/types.h>
#include "rsi_rom_udma_wrapper.h"
#include "rsi_rom_udma.h"
#include "rsi_udma.h"
#include "sl_status.h"

#define DT_DRV_COMPAT silabs_siwx917_dma
#define DMA_MAX_TRANSFER_COUNT 1024
#define DMA_CH_PRIORITY_HIGH 1
#define DMA_CH_PRIORITY_LOW 0
#define VALID_BURST_LENGTH 0
#define UDMA_ADDR_INC_NONE 0X03
#define DT_DRV_COMPAT silabs_siwx917_dma
#define DMA_MAX_TRANSFER_COUNT 1024
#define DMA_CH_PRIORITY_HIGH 1
#define DMA_CH_PRIORITY_LOW 0
#define VALID_BURST_LENGTH 0
#define UDMA_ADDR_INC_NONE 0x03
#define UDMA_MODE_PER_ALT_SCATTER_GATHER 0x07

LOG_MODULE_REGISTER(si91x_dma, CONFIG_DMA_LOG_LEVEL);

struct dma_sg_descriptor_allocator {
/* DMA descriptors in contiguous memory */
RSI_UDMA_DESC_T sg_transfer_desc_table[CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT];
/* Pointer to bitmap representing the allocation status of descriptors
* with each bit indicating the status of a single descriptor
*/
sys_bitarray_t *free_desc;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to introduce struct dma_sg_descriptor_allocator ? You can probably just add sg_transfer_desc_table and free_desc to struct dma_siwx917_data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, struct sys_mem_blocks is designed to do exactly what you try to do. You can look at drivers/dma/dma_silabs_ldma.c for an example.


struct dma_siwx917_config {
UDMA0_Type *reg; /* UDMA register base address */
uint8_t channels; /* UDMA channel count */
Expand All @@ -34,10 +47,13 @@ struct dma_siwx917_config {

struct dma_siwx917_data {
UDMA_Channel_Info *chan_info;
dma_callback_t dma_callback; /* User callback */
void *cb_data; /* User callback data */
RSI_UDMA_DATACONTEXT_T dma_rom_buff; /* Buffer to store UDMA handle */
/* related information */
dma_callback_t dma_callback; /* User callback */
void *cb_data; /* User callback data */
struct dma_sg_descriptor_allocator
*sg_transfer_desc_block; /* Pointer to scatter-gather descriptors block */
RSI_UDMA_DATACONTEXT_T dma_rom_buff; /* Buffer to store UDMA handle
* related information
*/
};

static inline int siwx917_dma_is_peripheral_request(uint32_t dir)
Expand Down Expand Up @@ -87,6 +103,146 @@ static inline int siwx917_dma_addr_adjustment(uint32_t adjustment)
}
}

/* Releases a range of scatter-gather descriptors */
static inline void release_sg_desc_blocks(sys_bitarray_t *desc_alloc, uint32_t start_index,
uint32_t block_count)
{
sys_bitarray_clear_region(desc_alloc, block_count, start_index);
}

/* Requests the index of contiguous memory for scatter-gather descriptor table */
static int request_sg_desc_base_addr(sys_bitarray_t *desc_alloc, uint32_t block_count)
smalae marked this conversation as resolved.
Show resolved Hide resolved
{
uint32_t i;

/* Find contiguous free blocks */
for (i = 0; i <= CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT - block_count; i++) {
if (sys_bitarray_is_region_cleared(desc_alloc, block_count, i)) {
break;
}
}
if (i > CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT - block_count) {
/* No contiguous free blocks present */
return -EINVAL;
}
/* Mark the blocks as allocated */
if (sys_bitarray_set_region(desc_alloc, block_count, i) < 0) {
return -EINVAL;
}
return i;
}

/* Sets up the scatter-gather descriptor table for a DMA transfer */
static int set_scatter_gather_desc(RSI_UDMA_DESC_T *descs, const struct dma_config *config)
{
struct dma_block_config *block_addr = config->head_block;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const struct dma_block_config *block_addr

volatile RSI_UDMA_CHA_CONFIG_DATA_T *cfg;

for (int i = 0; i < config->block_count; i++) {
cfg = &descs[i].vsUDMAChaConfigData1;
/* Set the source and destination end addresses */
descs[i].pSrcEndAddr =
(uint32_t *)(block_addr->source_address +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pSrcEndAddr is a void *

(block_addr->block_size - config->source_data_size));
descs[i].pDstEndAddr =
(uint32_t *)(block_addr->dest_address +
(block_addr->block_size - config->dest_data_size));
/* Set the source and destination data sizes */
cfg->srcSize = siwx917_dma_data_width(config->source_data_size);
cfg->dstSize = siwx917_dma_data_width(config->dest_data_size);
/* Calculate the number of DMA transfers required */
block_addr->block_size /= config->source_data_size;
if (block_addr->block_size > DMA_MAX_TRANSFER_COUNT) {
return -EINVAL;
}
/* Set the total number of DMA transfers */
cfg->totalNumOfDMATrans = block_addr->block_size - 1;
/* Set the transfer type based on whether it is a peripheral request */
cfg->transferType = siwx917_dma_is_peripheral_request(config->channel_direction)
? UDMA_MODE_PER_ALT_SCATTER_GATHER
: UDMA_MODE_MEM_ALT_SCATTER_GATHER;
/* Set the arbitration size */
cfg->rPower = ARBSIZE_1;
if (siwx917_dma_addr_adjustment(block_addr->source_addr_adj) < 0 ||
siwx917_dma_addr_adjustment(block_addr->dest_addr_adj) < 0) {
return -EINVAL;
}
/* Set source and destination address increments */
cfg->srcInc = siwx917_dma_addr_adjustment(block_addr->source_addr_adj)
? UDMA_SRC_INC_NONE
: siwx917_dma_data_width(config->source_data_size);
cfg->dstInc = siwx917_dma_addr_adjustment(block_addr->dest_addr_adj)
? UDMA_DST_INC_NONE
: siwx917_dma_data_width(config->dest_data_size);
/* Move to the next block */
block_addr = block_addr->next_block;
}
smalae marked this conversation as resolved.
Show resolved Hide resolved
if (block_addr != NULL) {
/* next_block address for last block must be null */
return -EINVAL;
}
/* Set the transfer type for the last descriptor */
descs[config->block_count - 1].vsUDMAChaConfigData1.transferType =
siwx917_dma_is_peripheral_request(config->channel_direction) ? UDMA_MODE_BASIC
: UDMA_MODE_AUTO;
return 0;
}

/* Configure DMA for scatter-gather transfer */
static int dma_scatter_gather_config(const struct device *dev, RSI_UDMA_HANDLE_T udma_handle,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you uniformize the names of your functions? Maybe:

siwx917_dma_sg_config()
siwx917_dma_sg_fill_desc()
siwx917_dma_sg_allocate_descs()
siwx917_dma_sg_release_descs()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, some functions in this file are named siwx917_dma_, others dma_siwx917_ and others dma_. I suggest siwx917_dma_ everywhere (if you do, place that change in a separate commit). Rationale:

  • When we see a call to a function, it is important to clearly understand if it is a Zephyr function or a driver specific function. So, we have to prefix symbols with siwx917.
  • It could be handy to remove dma_ prefix if the driver growth (since everything in this file is related to DMA, it is not absolutely required to keep dma_ prefix in the names). Typically, we could name the functions related to scatter/gather: siwx917_sg_config(), siwx917_sg_fill_desc(), ...

uint32_t channel, struct dma_config *config)
{
uint8_t transfer_type = UDMA_MODE_MEM_SCATTER_GATHER;
const struct dma_siwx917_config *cfg = dev->config;
struct dma_siwx917_data *data = dev->data;
RSI_UDMA_DESC_T *sg_desc_base_addr = NULL;
int block_alloc_start_index;

if (siwx917_dma_is_peripheral_request(config->channel_direction) == 1) {
transfer_type = UDMA_MODE_PER_SCATTER_GATHER;
} else if (siwx917_dma_is_peripheral_request(config->channel_direction) < 0) {
return -EINVAL;
}
if (siwx917_dma_data_width(config->source_data_size) < 0 ||
siwx917_dma_data_width(config->dest_data_size) < 0) {
return -EINVAL;
}
if (config->block_count > CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT) {
return -EINVAL;
}
/* Request start index for scatter-gather descriptor table */
block_alloc_start_index = request_sg_desc_base_addr(data->sg_transfer_desc_block->free_desc,
config->block_count);
if (block_alloc_start_index < 0) {
return -EIO;
}
sg_desc_base_addr =
&data->sg_transfer_desc_block->sg_transfer_desc_table[block_alloc_start_index];
if (set_scatter_gather_desc(sg_desc_base_addr, config)) {
return -EINVAL;
}
/* This channel information is used to distinguish scatter-gather transfers and
* free the allocated descriptors in sg_transfer_desc_block
*/
data->chan_info[channel].SrcAddr = 0;
data->chan_info[channel].DestAddr = 0;
data->chan_info[channel].Cnt = config->block_count;
data->chan_info[channel].Size = block_alloc_start_index;
RSI_UDMA_InterruptClear(udma_handle, channel);
RSI_UDMA_ErrorStatusClear(udma_handle);
if (cfg->reg == UDMA0) {
sys_write32((BIT(channel) | M4SS_UDMA_INTR_SEL), (mem_addr_t)&M4SS_UDMA_INTR_SEL);
} else {
sys_write32((BIT(channel) | cfg->reg->UDMA_INTR_MASK_REG),
(mem_addr_t)&cfg->reg->UDMA_INTR_MASK_REG);
}
sys_write32(BIT(channel), (mem_addr_t)&cfg->reg->CHNL_PRI_ALT_SET);
sys_write32(BIT(channel), (mem_addr_t)&cfg->reg->CHNL_REQ_MASK_CLR);
RSI_UDMA_SetChannelScatterGatherTransfer(udma_handle, channel, config->block_count,
sg_desc_base_addr, transfer_type);
return 0;
}

static int dma_channel_config(const struct device *dev, RSI_UDMA_HANDLE_T udma_handle,
uint32_t channel, struct dma_config *config,
UDMA_Channel_Info *channel_info)
Expand Down Expand Up @@ -184,8 +340,16 @@ static int dma_siwx917_configure(const struct device *dev, uint32_t channel,
return -EINVAL;
}

/* Configure dma channel for transfer */
status = dma_channel_config(dev, udma_handle, channel, config, data->chan_info);
if (config->head_block->source_gather_en || config->head_block->dest_scatter_en ||
config->cyclic) {
/* Configure DMA for a Scatter-Gather transfer */
status = dma_scatter_gather_config(dev, udma_handle, channel, config);
} else {
/* Configure dma channel for transfer */
status = dma_channel_config(dev, udma_handle, channel, config, data->chan_info);
}
data->dma_callback = config->dma_callback;
data->cb_data = config->user_data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data->dma_callback was not initialized in the original code? How did that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a miss. It was actually implemented but mistakenly removed while squashing commits. Additionally, Zephyr test cases for DMA mem-to-mem transfers only print in the callback but do not clear a flag or semaphore to indicate transfer completion. This caused the test case to pass by comparing the data after a timeout period without relying on the callback. This bug was identified as part of testing the SG test.

if (status) {
return status;
}
Expand Down Expand Up @@ -259,12 +423,12 @@ static int dma_siwx917_start(const struct device *dev, uint32_t channel)
if (RSI_UDMA_ChannelEnable(udma_handle, channel) != 0) {
return -EINVAL;
}

/* Check if the transfer type is memory-memory */
if (udma_table[channel].vsUDMAChaConfigData1.srcInc != UDMA_SRC_INC_NONE &&
udma_table[channel].vsUDMAChaConfigData1.dstInc != UDMA_DST_INC_NONE) {
/* Apply software trigger to start transfer */
cfg->reg->CHNL_SW_REQUEST |= BIT(channel);
sys_write32((BIT(channel) | cfg->reg->CHNL_SW_REQUEST),
(mem_addr_t)&cfg->reg->CHNL_SW_REQUEST);
}
return 0;
}
Expand Down Expand Up @@ -298,7 +462,7 @@ static int dma_siwx917_get_status(const struct device *dev, uint32_t channel,
return -EINVAL;
}
/* Read the channel status register */
if (cfg->reg->CHANNEL_STATUS_REG & BIT(channel)) {
if (sys_read32((mem_addr_t)&cfg->reg->CHANNEL_STATUS_REG) & BIT(channel)) {
stat->busy = 1;
} else {
stat->busy = 0;
Expand Down Expand Up @@ -326,6 +490,7 @@ static int dma_siwx917_init(const struct device *dev)
.udma_irq_num = cfg->irq_number,
.desc = cfg->sram_desc_addr,
};
size_t offset;

udma_handle = UDMAx_Initialize(&udma_resources, udma_resources.desc, NULL,
(uint32_t *)&data->dma_rom_buff);
Expand All @@ -339,6 +504,15 @@ static int dma_siwx917_init(const struct device *dev)
if (UDMAx_DMAEnable(&udma_resources, udma_handle) != 0) {
return -EBUSY;
}
/* Allocate the bitmap for representing the allocation status of SG descriptors */
if (sys_bitarray_alloc(data->sg_transfer_desc_block->free_desc,
CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT, &offset) < 0) {
return -EINVAL;
}
if (sys_bitarray_clear_region(data->sg_transfer_desc_block->free_desc,
CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT, offset) < 0) {
return -EINVAL;
}
return 0;
}

Expand All @@ -353,8 +527,9 @@ static void dma_siwx917_isr(const struct device *dev)
};
uint8_t channel;

/* Disable the IRQ to prevent the ISR from being triggered by */
/* interrupts from other DMA channels */
/* Disable the IRQ to prevent the ISR from being triggered by
* interrupts from other DMA channels
*/
irq_disable(cfg->irq_number);
channel = find_lsb_set(cfg->reg->UDMA_DONE_STATUS_REG);
/* Identify the interrupt channel */
Expand All @@ -363,12 +538,19 @@ static void dma_siwx917_isr(const struct device *dev)
}
/* find_lsb_set() returns 1 indexed value */
channel -= 1;
if (data->chan_info[channel].SrcAddr == 0 && data->chan_info[channel].DestAddr == 0) {
/* A Scatter-Gather transfer is completed, free the allocated descriptors */
release_sg_desc_blocks(data->sg_transfer_desc_block->free_desc,
data->chan_info[channel].Size, data->chan_info[channel].Cnt);
data->chan_info[channel].Cnt = 0;
data->chan_info[channel].Size = 0;
}
if (data->chan_info[channel].Cnt == data->chan_info[channel].Size) {
if (data->dma_callback) {
/* Transfer complete, call user callback */
data->dma_callback(dev, data->cb_data, channel, 0);
}
cfg->reg->UDMA_DONE_STATUS_REG = BIT(channel);
sys_write32(BIT(channel), (mem_addr_t)&cfg->reg->UDMA_DONE_STATUS_REG);
} else {
/* Call UDMA ROM IRQ handler. */
ROMAPI_UDMA_WRAPPER_API->uDMAx_IRQHandler(&udma_resources, udma_resources.desc,
Expand All @@ -377,7 +559,8 @@ static void dma_siwx917_isr(const struct device *dev)
if (udma_resources.desc[channel].vsUDMAChaConfigData1.srcInc != UDMA_SRC_INC_NONE &&
udma_resources.desc[channel].vsUDMAChaConfigData1.dstInc != UDMA_DST_INC_NONE) {
/* Set the software trigger bit for starting next transfer */
cfg->reg->CHNL_SW_REQUEST |= BIT(channel);
sys_write32((BIT(channel) | cfg->reg->CHNL_SW_REQUEST),
(mem_addr_t)&cfg->reg->CHNL_SW_REQUEST);
}
}
out:
Expand All @@ -396,8 +579,13 @@ static const struct dma_driver_api siwx917_dma_driver_api = {

#define SIWX917_DMA_INIT(inst) \
static UDMA_Channel_Info dma##inst##_channel_info[DT_INST_PROP(inst, dma_channels)]; \
SYS_BITARRAY_DEFINE_STATIC(free_desc##inst, CONFIG_DMA_SILABS_SIWX917_SG_BUFFER_COUNT); \
static struct dma_sg_descriptor_allocator dma##inst##_desc_allocator = { \
.free_desc = &free_desc##inst, \
}; \
static struct dma_siwx917_data dma##inst##_data = { \
.chan_info = dma##inst##_channel_info, \
.sg_transfer_desc_block = &dma##inst##_desc_allocator, \
}; \
static void siwx917_dma##inst##_irq_configure(void) \
{ \
Expand Down
2 changes: 2 additions & 0 deletions tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CONFIG_DMA_SG_CHANNEL_NR=2
CONFIG_DMA_SG_XFER_SIZE=1024
15 changes: 15 additions & 0 deletions tests/drivers/dma/scatter_gather/boards/siwx917_rb4338a.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright (c) 2024 Silicon Laboratories Inc.
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
aliases {
dma0 = &udma0;
};
};

&udma0 {
status = "okay";
};