bsp: 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-18 13:06 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
4 times, most recently
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状态，只能重启系统。我会按照您所说修改注释。
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
7 times, most recently
from
6a13f31 to
eee04cd
Compare
您好，已经按照您的建议进行修改，增加了一个对应的utest，您有空的时候麻烦再审核一下 |
您好，我是plct的实习生，负责移植rtthread上k230相关的驱动的，我看你移植了pdma的驱动，目前uart驱动需要加入dma支持，我目前在做这个，想和您沟通一下，你看方便给个联系方式吗 |
可以啊，欢迎一起学习交流，方便的话可以加我微信：wx_17830801987 |
| * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING | ||
| * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ |
There was a problem hiding this comment.
我不确定这个 pdma 的测试代码是否来自 canaan。如果是你自己开发的，可以去掉。
There was a problem hiding this comment.
我不确定这个 pdma 的测试代码是否来自 canaan。如果是你自己开发的，可以去掉。
嗯，是自己新增加的
| { | ||
| rt_free(pdma_event); | ||
| LOG_I("Thanks for feeding me, I'm still alive!"); | ||
| return RT_EOK; |
There was a problem hiding this comment.
cleanup 操作建议把所有 channel 都强制 stop & release一次， 避免出现中途出错导致 dma 通道异常。如果这期间有出错也打印出来。
bsp/k230/drivers/utest/test_pdma.c
Outdated
| #include "board.h" | ||
| #include "drv_pdma.h" | ||
|
|
||
| #define UART_IRQ 0x10 |
bsp/k230/drivers/utest/test_pdma.c
Outdated
|
|
||
| static rt_event_t pdma_event = RT_NULL; | ||
|
|
||
| void pdma_call_back(rt_uint8_t ch, rt_bool_t is_done) |
bsp/k230/drivers/utest/test_pdma.c
Outdated
| PDMA_EVENT_NONE, | ||
| PDMA_EVENT_COMPLETE, | ||
| PDMA_EVENT_TIMEOUT | ||
| } pdma_event_t; |
There was a problem hiding this comment.
建议作为测试的一部分，类型和宏都加上 "TEST_"/"test_" 以防和其他冲突
bsp/k230/drivers/utest/test_pdma.c
Outdated
| rt_uint32_t len = 192; | ||
|
|
||
| /* Allocate aligned buffer and prepare test pattern */ | ||
| uint8_t *buf = rt_malloc_align(len, 64); |
There was a problem hiding this comment.
我看了一下 TRM。k230 的 dma 传输对内存的地址要求好像是 4 字节对齐即可。你这里要求 64 字节对齐是不是要求太高了？TRM 上只是说每个 channel 有 64 字节的 buffer 而已。你这里对传输数据的字节对齐是什么考虑？
There was a problem hiding this comment.
我看了一下 TRM。k230 的 dma 传输对内存的地址要求好像是 4 字节对齐即可。你这里要求 64 字节对齐是不是要求太高了？TRM 上只是说每个 channel 有 64 字节的 buffer 而已。你这里对传输数据的字节对齐是什么考虑？
好的我改一下，这个地方四字节对齐就足够了
There was a problem hiding this comment.
我看了一下 TRM。k230 的 dma 传输对内存的地址要求好像是 4 字节对齐即可。你这里要求 64 字节对齐是不是要求太高了？TRM 上只是说每个 channel 有 64 字节的 buffer 而已。你这里对传输数据的字节对齐是什么考虑？
这个地方我又回去思考了一下，之前按照64字节对齐，是参考的k230_sdk中的写法，他们是按64字节，也就是缓存行大小对齐。我改成按四字节对齐后发现如果多运行几次在释放buf时会出现非法地址问题。后面排查了一下，发现是代码中申请的buf 前面还存了一个指针ptr，后面释放内存时会访问这个指针指向的内存。
如果不按缓存行大小64B地址对齐，buf可能和ptr在同一个缓存行中。由于k230是通过软件维护dma导致的缓存和内存不一致性问题，所以在进行读串口接受缓存数据时，dma 把数据搬运到内存buf后需要使数据对应的缓存行失效，但由于ptr和buf 在同一个缓存行里，会导致ptr缓存中的新值也被错误失效，在后面读取ptr时实际获取的是内存中存储的旧值，这可能是一个非法地址，从而导致造成后面的非法访存。
总结起来就是，如果buf不按缓存行对齐，可能导致后面使用缓存管理指令时让不该被失效的数据缓存值也被失效，从而丢失新值。所以一般这种通过软件维护dma一致性的系统里申请dma 内存时都会要求地址和大小按照缓存行大小对齐，来避免出现类似上面的问题。
There was a problem hiding this comment.
了解了。
我建议你为 64 定义一个宏，然后在定义宏的地方统一加一下说明。
否则现在我看你在调用 rt_malloc_align 的地方写了两次注释，着实有些浪费。
There was a problem hiding this comment.
建议在 test_pdma.c 的开头统一描述一下测试用例。参考 bsp/k230/drivers/utest/test_wdt.c
因为 pdma 的测试，特别是 TX/RX 测试是需要人工参与检查的，所以可以在测试用例描述中说明一下，譬如对 test_pdma_tx() 要求在串口上看到 ....
There was a problem hiding this comment.
建议在
test_pdma.c的开头统一描述一下测试用例。参考bsp/k230/drivers/utest/test_wdt.c因为 pdma 的测试，特别是 TX/RX 测试是需要人工参与检查的，所以可以在测试用例描述中说明一下，譬如对
test_pdma_tx()要求在串口上看到 ....
好的
| LOG_E("Channel %d not properly configured", ch); | ||
| PDMA_UNLOCK(); | ||
| rt_hw_interrupt_enable(level); | ||
| return -RT_ERROR; |
There was a problem hiding this comment.
对于未配置过就 start 的情况，我们是否可以采用什么默认的配置对通道配置一下？我看你这里是强制退出了。
There was a problem hiding this comment.
对于未配置过就 start 的情况，我们是否可以采用什么默认的配置对通道配置一下？我看你这里是强制退出了。
嗯，这个地方我再思考一下
There was a problem hiding this comment.
对于未配置过就 start 的情况，我们是否可以采用什么默认的配置对通道配置一下？我看你这里是强制退出了。
hello,我又修改了一下，麻烦您有空的时候评审一下。另外这个未配置就开启的情况开启传输我感觉还是维持原来的实现更合适，配置通道传输参数的话得绑定一个设备，支持device_sel_e 中定义的枚举类型。我觉得设置默认绑定设备看起来很奇怪，而且有可能默认配置导致硬件出现一些奇怪的问题。所以我感觉在开启通道传输时，检测到未配置传输参数时返回错误码是更安全的做法。
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
4 times, most recently
from
50c7ce5 to
1fe2465
Compare
There was a problem hiding this comment.
第三次 review。
基本没啥问题了，我也 approve 了。我只是补充了一些小的建议，你如果有空就改一下（如果 pr 还没有merge 的话，因为具体负责 merge 的是其他人）
另外有关 commit 的编写，我也建议你改一下，譬如标题建议统一改成 “bsp: k230: xxxxx". 其他具体要求参考 https://github.com/plctlab/plct-rt-thread/blob/notes/0.notes/20241212-github-tips.md#2-how-to-write-git-commit-message.
bsp/k230/drivers/utest/test_pdma.c
Outdated
| rt_uint32_t len = 192; | ||
|
|
||
| /* Allocate aligned buffer and prepare test pattern */ | ||
| uint8_t *buf = rt_malloc_align(len, 64); |
There was a problem hiding this comment.
了解了。
我建议你为 64 定义一个宏，然后在定义宏的地方统一加一下说明。
否则现在我看你在调用 rt_malloc_align 的地方写了两次注释，着实有些浪费。
好的 |
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
from
1fe2465 to
58c8de3
Compare
eatvector
changed the title
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
4 times, most recently
from
1e540d4 to
2601b2c
Compare
This commit introduces the PDMA (Peripheral DMA) driver for K230 SoC, providing essential DMA capabilities for peripheral data transfers. Key features implemented: 1. PDMA channel request/release management 2. Channel start/stop control 3. Interrupt callback registration 4. Data transfer between device ports and DDR memory The driver includes: - Core driver implementation (drv_pdma.c/h) - Build system support (SConscript) - Basic test cases (test_pdma.c) Tested with: - PDMA channel request/release - DDR to UART TX FIFO transfers - UART RX FIFO to DDR transfers Signed-off-by: eatvector <[email protected]>
eatvector
force-pushed
the
feat/add-k230-pdma-driver
branch
from
2601b2c to
49267ab
Compare
|
@Rbb666 大佬，麻烦您有空的时候评审合并一下这个提交，如果觉得没有其他问题的话，unicornx大佬已经评审过了 |
拉取/合并请求描述：(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