[DM][MTD] Add new common MTD drivers #11002
Conversation
Signed-off-by: GuEe-GUI <2991707448@qq.com>
1. CFI-Nor flash DM driver. 2. SPI-Nor flash DM driver. Signed-off-by: GuEe-GUI <2991707448@qq.com>
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-12-02 13:20 CST)
📝 Review Instructions
|
There was a problem hiding this comment.
Pull request overview
This PR adds two new common MTD (Memory Technology Device) drivers for RT-Thread's Device Manager (DM) framework: a CFI-compliant NOR flash driver and an SPI NOR flash driver. These drivers provide standardized interfaces for flash memory devices commonly found in embedded systems, targeting QEMU, Raspberry Pi 3/4, and Rockchip 3500 platforms.
Key Changes:
- Added CFI (Common Flash Interface) NOR flash driver with comprehensive support for Intel and AMD command sets
- Added SPI NOR flash driver leveraging SFUD (Serial Flash Universal Driver) framework
- Updated build configuration to conditionally compile new drivers based on Kconfig options
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| components/drivers/mtd/mtd-spi-nor.c | New SPI NOR flash driver providing device-tree based probe functionality and SFUD integration |
| components/drivers/mtd/mtd-cfi.h | Header file defining CFI flash commands, status flags, query structures, and vendor-specific constants |
| components/drivers/mtd/mtd-cfi.c | Complete CFI NOR flash driver implementation with read/write/erase operations, vendor-specific fixups, and geometry detection |
| components/drivers/mtd/SConscript | Build script updates to include new CFI and SPI NOR drivers when enabled |
| components/drivers/mtd/Kconfig | Configuration options for enabling CFI and SPI NOR flash drivers with appropriate dependencies |
| /* Only check every once in a while */ | ||
| if ((length & 0xffff) < fdev->portwidth) | ||
| { | ||
| res = -RT_ERROR; | ||
| goto _out_lock; | ||
| } | ||
| } | ||
|
|
||
| if (!length) | ||
| { | ||
| res = length; | ||
| goto _out_lock; | ||
| } | ||
|
|
||
| /* Handle unaligned tail bytes */ | ||
| word.w32 = 0; | ||
| ptr = (rt_uint8_t *)wp; | ||
|
|
||
| for (i = 0; i < fdev->portwidth && length > 0; ++i) | ||
| { | ||
| cfi_flash_add_byte(fdev, &word, *data++); | ||
| --length; | ||
| } | ||
|
|
||
| for (; i < fdev->portwidth; ++i) | ||
| { | ||
| cfi_flash_add_byte(fdev, &word, cfi_flash_read8(ptr + i)); | ||
| } | ||
|
|
||
| if (!(res = cfi_flash_write_word(fdev, wp, word))) | ||
| { | ||
| res = length; |
There was a problem hiding this comment.
Incorrect write function implementation / 写函数实现不正确
English: The write function has multiple issues with return values and logic:
-
Lines 620-625: This code appears to be misplaced or incorrectly implemented. The comment says "Only check every once in a while" but the condition will trigger frequently and always returns an error (
-RT_ERROR), which will cause most writes to fail. This logic seems wrong. -
Lines 628-631: When
length == 0after the aligned write loop, the function returnslength(which is 0), but it should return the number of bytes successfully written, not 0. -
Line 651: Similarly returns
length(which is 0 after processing tail bytes) instead of the number of bytes written.
The function should track and return the total number of bytes successfully written, not the remaining length.
中文: 写函数在返回值和逻辑上存在多个问题：
-
第620-625行：此代码似乎被误放或实现错误。注释说"只是偶尔检查一次"，但条件会频繁触发并总是返回错误（
-RT_ERROR），这会导致大多数写入失败。这个逻辑似乎是错误的。 -
第628-631行：当对齐写入循环后
length == 0时，函数返回length（为0），但应该返回成功写入的字节数，而不是0。 -
第651行：类似地返回
length（处理尾部字节后为0），而不是写入的字节数。
函数应该跟踪并返回成功写入的总字节数，而不是剩余长度。
| static void cfi_flash_dev_remove(struct rt_device *dev, struct cfi_flash_device *fdev) | ||
| { | ||
| if (fdev->sect[0]) | ||
| { | ||
| rt_iounmap((void *)fdev->sect[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resource leak: mutex not cleaned up / 资源泄漏：互斥锁未清理
English: The mutex initialized at line 1224 with rt_mutex_init() is never detached/destroyed. The cfi_flash_dev_remove() function (lines 1246-1252) should call rt_mutex_detach() to properly clean up the mutex resource when the device is removed.
中文: 第1224行使用 rt_mutex_init() 初始化的互斥锁从未被分离/销毁。cfi_flash_dev_remove() 函数（第1246-1252行）应该调用 rt_mutex_detach() 以在设备移除时正确清理互斥锁资源。
Suggestion / 建议:
static void cfi_flash_dev_remove(struct rt_device *dev, struct cfi_flash_device *fdev)
{
rt_mutex_detach(&fdev->rw_lock);
if (fdev->sect[0])
{
rt_iounmap((void *)fdev->sect[0]);
}
}| @@ -0,0 +1,98 @@ | |||
| /* | |||
There was a problem hiding this comment.
🟡 [PR Title/PR 标题]: Title format needs correction / 标题格式需要修正
English: PR title should follow format: [module][subsystem] Description with lowercase prefix.
Current title: [DM][MTD] Add new common MTD drivers
Suggested title: [dm][mtd] Add new common MTD drivers
中文: PR 标题应遵循格式：小写的 [模块][子系统] 描述。
当前标题：[DM][MTD] Add new common MTD drivers
建议标题：[dm][mtd] Add new common MTD drivers
| */ | ||
| { "at25df321a" }, { "at25df641" }, { "at26df081a" }, | ||
| { "mx25l4005a" }, { "mx25l1606e" }, { "mx25l6405d" }, { "mx25l12805d" }, | ||
| { "mx25l25635e" },{ "mx66l51235l" }, |
There was a problem hiding this comment.
Spacing issue after comma / 逗号后缺少空格
English: Missing space after comma on line 58. Should be { "mx25l25635e" }, { "mx66l51235l" }, following RT-Thread coding style.
中文: 第58行逗号后缺少空格。应为 { "mx25l25635e" }, { "mx66l51235l" }, 以符合 RT-Thread 编码规范。
| { "mx25l25635e" },{ "mx66l51235l" }, | |
| { "mx25l25635e" }, { "mx66l51235l" }, |
| { "n25q064" }, { "n25q128a11" }, { "n25q128a13" }, { "n25q512a" }, | ||
| { "s25fl256s1" }, { "s25fl512s" }, { "s25sl12801" }, { "s25fl008k" }, | ||
| { "s25fl064k" }, | ||
| { "sst25vf040b" },{ "sst25vf016b" },{ "sst25vf032b" },{ "sst25wf040" }, |
There was a problem hiding this comment.
Spacing issue after comma / 逗号后缺少空格
English: Missing spaces after commas on line 62. Should be { "sst25vf040b" }, { "sst25vf016b" }, { "sst25vf032b" }, { "sst25wf040" }, following RT-Thread coding style.
中文: 第62行逗号后缺少空格。应为 { "sst25vf040b" }, { "sst25vf016b" }, { "sst25vf032b" }, { "sst25wf040" }, 以符合 RT-Thread 编码规范。
| { "sst25vf040b" },{ "sst25vf016b" },{ "sst25vf032b" },{ "sst25wf040" }, | |
| { "sst25vf040b" }, { "sst25vf016b" }, { "sst25vf032b" }, { "sst25wf040" }, |
| { | ||
| rt_uint8_t *buffer; | ||
|
|
||
| /* Reset, unknow reset's cmd now, so try Intel and AMD both. */ |
There was a problem hiding this comment.
Typo in comment / 注释中的拼写错误
English: "unknow" should be "unknown" on line 721.
中文: 第721行 "unknow" 应为 "unknown"。
| /* Reset, unknow reset's cmd now, so try Intel and AMD both. */ | |
| /* Reset, unknown reset's cmd now, so try Intel and AMD both. */ |
| { | ||
| goto _fail; | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential memory leak on error path / 潜在的内存泄漏
English: The query allocation at line 1043 is not freed in all error paths. Specifically, if cfi_flash_detect() returns false (line 1058), the code jumps to _fail which frees query (line 1241), but there's also an early return at line 1263 in cfi_flash_probe() that could skip cleanup. Additionally, if rt_dm_dev_set_name_auto() or rt_mutex_init() fails after successful probe, the query memory is already freed but the function could still fail later.
However, looking more carefully, the query is freed at line 1241 which is after the _fail label, so this should be correct for the cfi_flash_dev_probe function. The main concern is ensuring query is properly freed in all paths.
Actually, I need to reconsider: the query is allocated and freed within cfi_flash_dev_probe, so the scope is correct. But the query should be freed even on success path - it's only needed during probe, not after. Currently it's only freed on the error path.
中文: 第1043行分配的 query 在成功路径中没有被释放。query 仅在探测期间需要，成功后应该释放。当前实现只在错误路径（第1241行）释放了 query。
Suggestion / 建议:
// After line 1228, before return RT_EOK:
rt_free(query);
return RT_EOK;| rt_free(query); |
| rt_err_t err; | ||
| rt_ubase_t sect, sect_end; | ||
| struct cfi_flash_device *fdev = raw_to_cfi_flash_device(dev); | ||
|
|
||
| rt_mutex_take(&fdev->rw_lock, RT_WAITING_FOREVER); | ||
|
|
||
| sect = cfi_flash_find(fdev, fdev->sect[0] + offset); | ||
| sect_end = cfi_flash_find(fdev, fdev->sect[0] + offset + length); | ||
|
|
||
| for (; sect <= sect_end; ++sect) | ||
| { | ||
| if (fdev->protect[sect]) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| switch (fdev->vendor) | ||
| { | ||
| case CFI_CMDSET_INTEL_PROG_REGIONS: | ||
| case CFI_CMDSET_INTEL_STANDARD: | ||
| case CFI_CMDSET_INTEL_EXTENDED: | ||
| cfi_flash_write_cmd(fdev, sect, 0, FLASH_CMD_CLEAR_STATUS); | ||
| cfi_flash_write_cmd(fdev, sect, 0, FLASH_CMD_BLOCK_ERASE); | ||
| cfi_flash_write_cmd(fdev, sect, 0, FLASH_CMD_ERASE_CONFIRM); | ||
| break; | ||
|
|
||
| case CFI_CMDSET_AMD_STANDARD: | ||
| case CFI_CMDSET_AMD_EXTENDED: | ||
| cfi_flash_unlock_seq(fdev, sect); | ||
| cfi_flash_write_cmd(fdev, sect, fdev->addr_unlock1, AMD_CMD_ERASE_START); | ||
| cfi_flash_unlock_seq(fdev, sect); | ||
| cfi_flash_write_cmd(fdev, sect, 0, fdev->cmd_erase_sector); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
|
|
||
| err = cfi_flash_full_status_check(fdev, sect, fdev->erase_blk_tout); | ||
| } | ||
|
|
||
| rt_mutex_release(&fdev->rw_lock); | ||
|
|
||
| return err; |
There was a problem hiding this comment.
Uninitialized variable used on error path / 错误路径使用了未初始化的变量
English: The variable err is used in the return statement at line 706, but it's only initialized inside the for-loop at line 701. If the loop executes zero times (when sect > sect_end from the start), or if all sectors are protected, err will be uninitialized.
中文: 变量 err 在第706行的返回语句中使用，但它只在第701行的 for 循环内初始化。如果循环执行零次（当 sect > sect_end 时），或者所有扇区都被保护，err 将是未初始化的。
Suggestion / 建议:
rt_err_t err = RT_EOK; // Initialize err at declaration (line 663)1 participant
拉取/合并请求描述：(PR description)
[
QEMU
Raspi3/4
Rockchip 3500
]
当前拉取/合并请求的状态 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