Skip to content

Siwx91x dma driver fixes #88705

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.

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

Merged
merged 3 commits into from
Apr 22, 2025

Conversation

smalae
Copy link
Collaborator

@smalae smalae commented Apr 16, 2025

The following fixes are done in the DMA driver:

  1. Introduced a new variable in the dma_siwx91x_channel_info
    structure to provide a clean way to differentiate transfer
    directions. This enhancement is utilized to trigger software
    requests specifically for memory-to-memory transfers
  2. Addressed an issue where regular/non-scatter-gather DMA
    transfers were not explicitly using the primary DMA descriptor
    structure. This ensures a smooth regular DMA transfer after
    any scatter-gather transfer.
  3. Addressed an issue where alignment of dma desc varaible
    of dma0 is corrected to 1024 instead of 512.

@@ -37,6 +37,7 @@ struct dma_siwx91x_channel_info {
dma_callback_t dma_callback; /* User callback */
void *cb_data; /* User callback data */
RSI_UDMA_DESC_T *sg_desc_addr_info; /* Scatter-Gather table start address */
uint8_t transfer_direction; /* mem<->mem ot per<->mem */
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth to consider giving a named type for the enum and just use that everywhere instead of the mix of uint8_t and int. Btw, you don't gain anything with using a smaller type in the above struct due to implicit padding, i.e. the struct size would be exactly the same if you used int. Even if you did need a smaller type, you can also get that with an enum by adding __packed to the definition. I'd also consider adding a third possible value to make the code look more intuitive (e.g. you can return an actual enum value instead of -EINVAL):

enum transfer_dir {
	TRANSFER_MEM_TO_MEM,
	TRANSFER_TO_OR_FROM_PER,
	TRANSFER_DIR_INVALID = -1,
};

Perhaps out of scope for this PR, but if you want to make these symbol names more compact, I believe a common convention is to shorten "transfer" to "xfer'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated it according to the suggestion.

@smalae smalae force-pushed the siwx91x_dma_driver_fixes branch from d7dc92b to 697b353 Compare April 17, 2025 09:03
@smalae smalae requested a review from jhedberg April 17, 2025 09:06
@@ -464,8 +478,7 @@ static int siwx91x_dma_start(const struct device *dev, uint32_t channel)
}

/* 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) {
if (data->zephyr_channel_info[channel].xfer_direction == TRANSFER_MEM_TO_MEM) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curiosity, in which case the original condition didn't work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case of transfers using alternate descriptors, this condition is true for all the transfers and triggers the SW request even for peripheral transfers.

@@ -206,13 +208,13 @@ static int siwx91x_sg_chan_config(const struct device *dev, RSI_UDMA_HANDLE_T ud
struct dma_siwx91x_data *data = dev->data;
RSI_UDMA_DESC_T *sg_desc_base_addr = NULL;
uint8_t transfer_type;
int ret;
int xfer_dir;
Copy link
Member

Choose a reason for hiding this comment

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

enum xfer_dir dir;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

ret = siwx91x_transfer_direction(config->channel_direction);
if (ret < 0) {
xfer_dir = siwx91x_transfer_direction(config->channel_direction);
if (xfer_dir < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

if (dir == TRANSFER_DIR_INVALID) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

return -EINVAL;
}
transfer_type = ret ? UDMA_MODE_PER_SCATTER_GATHER : UDMA_MODE_MEM_SCATTER_GATHER;
transfer_type = xfer_dir ? UDMA_MODE_PER_SCATTER_GATHER : UDMA_MODE_MEM_SCATTER_GATHER;
Copy link
Member

@jhedberg jhedberg Apr 17, 2025

Choose a reason for hiding this comment

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

if (dir == TRANSFER_TO_OR_FROM_PER) {
	transfer_type = UDMA_MODE_PER_SCATTER_GATHER;
} else {
	transfer_type = UDMA_MODE_MEM_SCATTER_GATHER;
}

(xfer_dir is not a boolean so don't treat it like that)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

@@ -28,15 +28,17 @@

LOG_MODULE_REGISTER(si91x_dma, CONFIG_DMA_LOG_LEVEL);

enum {
enum xfer_dir_t {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zephyr tends to apply the Coding Style of the Linux kernel. So, _t is only used for typedefs. In addition, we try to avoid typedefs and use plain enum xx or struct yy instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

@smalae
Copy link
Collaborator Author

smalae commented Apr 21, 2025

@jerome-pouiller @jhedberg . I have added one more commit to this PR, which corrects the alignment of sram_desc_addr from 512 to 1024. This issue was identified during SPI testing with the latest DMA changes introduced by the memory_partition PR. Kindly review.

jhedberg
jhedberg previously approved these changes Apr 22, 2025
@smalae smalae force-pushed the siwx91x_dma_driver_fixes branch from 57a798f to e773c95 Compare April 22, 2025 06:21
@smalae smalae requested a review from jhedberg April 22, 2025 06:22
jhedberg
jhedberg previously approved these changes Apr 22, 2025
};

struct dma_siwx91x_channel_info {
dma_callback_t dma_callback; /* User callback */
void *cb_data; /* User callback data */
RSI_UDMA_DESC_T *sg_desc_addr_info; /* Scatter-Gather table start address */
int xfer_direction; /* mem<->mem ot per<->mem */
Copy link
Member

Choose a reason for hiding this comment

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

One more int -> enum dma_xfer_dir here

smalae added 3 commits April 22, 2025 14:25
Introduced a new variable in the `dma_siwx91x_channel_info`
structure to provide a clean way to differentiate transfer
directions. This enhancement is utilized to trigger software
requests specifically for memory-to-memory transfers

Signed-off-by: Sai Santhosh Malae <[email protected]>
Addressed an issue where regular/non-scatter-gather DMA
transfers were not explicitly using the primary DMA descriptor
structure. This ensures a smooth regular DMA transfer after
any scatter gather transfer.

Signed-off-by: Sai Santhosh Malae <[email protected]>
Addressed an issue where alignment of dma desc varaible
of dma0 is corrected to 1024 instead of 512.

Signed-off-by: Sai Santhosh Malae <[email protected]>
@smalae smalae force-pushed the siwx91x_dma_driver_fixes branch from e773c95 to e08a1ad Compare April 22, 2025 08:59
@smalae smalae requested a review from jhedberg April 22, 2025 09:00
@kartben kartben merged commit 0d547d4 into zephyrproject-rtos:main Apr 22, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: DMA Direct Memory Access platform: Silabs Silicon Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants