feat(k230): add PDMA driver support #10587
Conversation
📌 Code Review Assignment🏷️ Tag: bsp_k230Reviewers: unicornx Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-08-10 13:56 CST)
📝 Review Instructions
|
eatvector
changed the title
eatvector
changed the title
eatvector
changed the title
eatvector
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds PDMA (Peripheral Direct Memory Access) driver support for the K230 BSP, implementing data transfer between system memory and peripheral ports. The driver provides configurable 8-channel DMA operations with common APIs for other device drivers to utilize.
Key changes include:
- Complete PDMA driver implementation with hardware abstraction layer
- Support for 8 configurable DMA channels with interrupt handling
- Linked List Transfer (LLT) mechanism for efficient data movement
- Comprehensive channel management with atomic operations and hardware locking
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bsp/k230/drivers/interdrv/pdma/drv_pdma.h | Header file defining PDMA structures, enums, and API declarations |
| bsp/k230/drivers/interdrv/pdma/drv_pdma.c | Main driver implementation with channel management and interrupt handling |
| bsp/k230/drivers/interdrv/pdma/SConscript | Build script for PDMA driver compilation |
| bsp/k230/board/Kconfig | Configuration options for enabling PDMA and individual channels |
| bsp/k230/.config | Updated configuration file with PDMA options (currently disabled) |
| } | ||
|
|
||
| /* Ensure LLT is visible to DMA */ | ||
| rt_hw_cpu_dcache_clean((uint64_t*)llt_list, sizeof(pdma_llt_t) * list_num); |
There was a problem hiding this comment.
Casting to 'uint64_t*' for cache operation is potentially unsafe and not portable. The function should accept 'void*' or the correct pointer type. Consider using '(void*)llt_list' instead.
| rt_hw_cpu_dcache_clean((uint64_t*)llt_list, sizeof(pdma_llt_t) * list_num); | |
| rt_hw_cpu_dcache_clean((void*)llt_list, sizeof(pdma_llt_t) * list_num); |
Copilot uses AI. Check for mistakes.
| LOG_I("PDMA initialized. Enabled channels: ["); | ||
| for (rt_uint8_t ch = 0; ch < PDMA_CH_MAX; ch++) | ||
| { | ||
| if (PDMA_CH_MENUCONFIG_ENABLED(ch)) | ||
| { | ||
| LOG_RAW("%d ", ch); | ||
| } | ||
| } | ||
| LOG_RAW("]"); |
There was a problem hiding this comment.
[nitpick] Using LOG_RAW in a loop without proper formatting can result in unclear log output. Consider building a formatted string first or using a single log statement with proper formatting.
| LOG_I("PDMA initialized. Enabled channels: ["); | |
| for (rt_uint8_t ch = 0; ch < PDMA_CH_MAX; ch++) | |
| { | |
| if (PDMA_CH_MENUCONFIG_ENABLED(ch)) | |
| { | |
| LOG_RAW("%d ", ch); | |
| } | |
| } | |
| LOG_RAW("]"); | |
| char enabled_channels[4 * PDMA_CH_MAX + 1] = {0}; // Enough for "NNN " per channel | |
| char *ptr = enabled_channels; | |
| int len = 0; | |
| for (rt_uint8_t ch = 0; ch < PDMA_CH_MAX; ch++) | |
| { | |
| if (PDMA_CH_MENUCONFIG_ENABLED(ch)) | |
| { | |
| len = snprintf(ptr, sizeof(enabled_channels) - (ptr - enabled_channels), "%d ", ch); | |
| if (len > 0) | |
| ptr += len; | |
| } | |
| } | |
| LOG_I("PDMA initialized. Enabled channels: [%s]", enabled_channels); |
Copilot uses AI. Check for mistakes.
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
5 times, most recently
from
4aba546 to
22d46ac
Compare
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
3 times, most recently
from
eaeab7c to
4815a87
Compare
|
@DannyRay019 请一起 review 一下，谢谢 |
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
3 times, most recently
from
4a573f4 to
f438b09
Compare
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
from
f438b09 to
31d4a16
Compare
There was a problem hiding this comment.
除了下面针对具体代码上的一些零碎的问题之外，我还有以下几个 general 的问题：
- 我对照了一下 vendor/canaan 的代码，
https://gitee.com/canmv-k230/rtsmart/blob/canmv_k230/kernel/bsp/maix3/drivers/interdrv/pdma/drv_pdma.h貌似你提交的部分是少了一个rt_dma_chan_done，不知道你参考的 vendor 代码是哪里，这个 API 你为何没有？ - 你是如何测试这个新加的 pdma 驱动的？是否可以写一个 utest？对于其他的 master 上新增的驱动，我们都会写一个 utest，参考 master 上
bsp/k230/drivers/utest
There was a problem hiding this comment.
这个文件需要简化，请看一下 bsp/k230/drivers/interdrv/pwm/SConscript 的实现。
| rt_size_t page_size[PDMA_CH_MAX]; /**< Page size for each channel */ | ||
| rt_bool_t alloc_page[PDMA_CH_MAX]; /**< Page allocation status for each channel */ | ||
| k230_pdma_chan_cb_t cb[PDMA_CH_MAX]; /**< Callback functions for each channel */ | ||
| rt_atomic_t configured_map; /**< Bitmap of configured channels */ |
| .alloc_page = { [0 ... PDMA_CH_MAX-1] = RT_FALSE }, | ||
| .cb = { [0 ... PDMA_CH_MAX-1] = { .callback = RT_NULL } }, | ||
| .configured_map = 0 | ||
| }; |
There was a problem hiding this comment.
是否可以直接简化成：
static pdma_controller_t pdma_ctrl = {0};| /* Request hardware lock */ | ||
| if (kd_request_lock(HARDLOCK_PDMA)) | ||
| { | ||
| pdma_ctrl.hardlock = -1; |
| #include "ioremap.h" | ||
| #include "drv_hardlock.h" | ||
| #include "drv_pdma.h" | ||
| #include <rtdbg.h> |
There was a problem hiding this comment.
建议加上 DBG_TAG 和 DBG_LVL 的定义，具体参考 bsp/k230/drivers/interdrv/sdio/drv_sdhci.c
| if (PDMA_CH_INT_IS_TRIGGERED(ch, PDMA_PDONE_INT)) | ||
| { | ||
| success = RT_TRUE; /* Mark transfer successful */ | ||
| callback = (k230_pdma_callback_t)rt_atomic_load(&pdma_ctrl.cb[ch].callback); /* Store callback pointer */ |
| else if (PDMA_CH_INT_IS_TRIGGERED(ch, PDMA_PTOUT_INT)) | ||
| { | ||
| success = RT_FALSE; /* Mark transfer failed */ | ||
| callback = (k230_pdma_callback_t)rt_atomic_load(&pdma_ctrl.cb[ch].callback); /* Store callback pointer */ |
| #define PDMA_CH7_ENABLED 1 | ||
| #else | ||
| #define PDMA_CH7_ENABLED 0 | ||
| #endif |
There was a problem hiding this comment.
我建议在 pdma_controller_t 中为每个 channel 定义一个 menuconfig 的成员，并在 rt_hw_pdma_device_init 中基于 BSP_USING_PDMA_CHANNELx 设置每个 channel 是否可用。
好处是不用再定义一套 PDMA_CHx_ENABLED 的宏，也不用写上面这段代码，PDMA_CH_MENUCONFIG_ENABLED 这些宏也可以去掉，需要时直接根据判断 pdma_controller_t 中为每个 channel 新加的 menuconfig 的成员即可。
There was a problem hiding this comment.
我建议在
pdma_controller_t中为每个 channel 定义一个 menuconfig 的成员，并在rt_hw_pdma_device_init中基于BSP_USING_PDMA_CHANNELx设置每个 channel 是否可用。 好处是不用再定义一套PDMA_CHx_ENABLED的宏，也不用写上面这段代码，PDMA_CH_MENUCONFIG_ENABLED这些宏也可以去掉，需要时直接根据判断pdma_controller_t中为每个 channel 新加的 menuconfig 的成员即可。
嗯，我觉得这种方法更好，简洁直观
| rt_bool_t alloc_page[PDMA_CH_MAX]; /**< Page allocation status for each channel */ | ||
| k230_pdma_chan_cb_t cb[PDMA_CH_MAX]; /**< Callback functions for each channel */ | ||
| rt_atomic_t configured_map; /**< Bitmap of configured channels */ | ||
| } pdma_controller_t; |
There was a problem hiding this comment.
是否可以再抽象一个 per_channel 的 config 结构体。
这个 per_channel 的 config 结构体除了包括现有代码中的三项外，还可以加上我建议的新加的 menuconfig 成员。另外 configured_map 是否也可以改成 per_channel 的方式，虽然浪费一点内存，但是代码可读性会好一些。
There was a problem hiding this comment.
是否可以再抽象一个 per_channel 的 config 结构体。 这个 per_channel 的 config 结构体除了包括现有代码中的三项外，还可以加上我建议的新加的 menuconfig 成员。另外 configured_map 是否也可以改成 per_channel 的方式，虽然浪费一点内存，但是代码可读性会好一些。
好的，我觉得有道理
| rt_size_t page_size[PDMA_CH_MAX]; /**< Page size for each channel */ | ||
| rt_bool_t alloc_page[PDMA_CH_MAX]; /**< Page allocation status for each channel */ | ||
| k230_pdma_chan_cb_t cb[PDMA_CH_MAX]; /**< Callback functions for each channel */ | ||
| rt_atomic_t configured_map; /**< Bitmap of configured channels */ |
There was a problem hiding this comment.
我们是否有必要记住一个 channel 是否已经配置过？如果没有配置过，驱动就按照默认的 可行的 配置处理好了。
另外从 k230_pdma_start 函数中的注释我了解之所以要检查是否配置过是为了避免 uncloseable。我建议相关注释最好放在结构体定义上而不是写在函数中。一般我们会先看结构体定义，此时就了解了我们为什么要这个成员的原因。特别的，这个 configured_map 的含义从变量名上其实是不太好理解的，所以在结构体中说明会更方便大家理解代码，否则要通读函数才能知道（还好只有一处而且你在函数中写了注释 :-)）
There was a problem hiding this comment.
我们是否有必要记住一个 channel 是否已经配置过？如果没有配置过，驱动就按照默认的 可行的 配置处理好了。
另外从 k230_pdma_start 函数中的注释我了解之所以要检查是否配置过是为了避免 uncloseable。我建议相关注释最好放在结构体定义上而不是写在函数中。一般我们会先看结构体定义，此时就了解了我们为什么要这个成员的原因。特别的，这个 configured_map 的含义从变量名上其实是不太好理解的，所以在结构体中说明会更方便大家理解代码，否则要通读函数才能知道（还好只有一处而且你在函数中写了注释 :-)）
嗯好的，这个在开启dma通道前需要检测是否配置成功我感觉是有必要的，因为之前测试发现，如果不配置相关寄存器以及传输链表，直接开启，这个通道后面就关不了，硬件一直处于busy状态，只能重启系统。我会按照您所说修改注释。
十分感谢您的意见，这几天我将结合您的意见进行优化修改。 之前我主要参考了k230_sdk项目中的代码实现。vendor/canaan目录下相关代码也看过，rt_dma_chan_done函数的主要功能是通过等待DMA中断事件来实现同步。 不过我认为用户完全可以自行注册DMA中断回调函数来实现这种同步。当DMA中断触发时，系统会将传输状态或超时信息作为参数传递给这个回调函数。开发者可以在自定义的中断回调中灵活地实现事件通知机制，相关的等待逻辑也可以在用户层代码中自主实现。考虑到这种实现方式具有更好的灵活性，所以我没有专门实现这个函数，也没有在中断处理中发送事件。 这个测试只能在特定的设备端口和ddr之间进行，之前我使用uart的端口和ddr之间数据传输进行开发调试，我思考一下增加一个合适的测试用例。 |
|
修改完毕后可以点击如下图所示重新发起 review |
好的 |
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
增加bsp k230 下的pdma驱动，实现系统内存与外设端口之间的数据传输，支持配置八个dma通道，提供常用的dma 通道操作接口，供其他设备驱动调用。
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up