Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial new component: a GRLIB NAND Flash Controller 2 driver. This driver significantly expands the system's storage capabilities by enabling robust and compliant interaction with NAND flash devices. It handles low-level flash operations, bad block management, and adapts to various flash configurations through ONFI 4.0 parameter parsing, ensuring reliable data storage and retrieval for embedded systems. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
efc4cc0 to
2923ac3
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new GRLIB NANDFCTRL2 flash driver, including data and metadata interfaces, and integrates it into the build system and linker scripts. The changes enable support for NAND flash memory, providing functionalities for reading, writing, erasing, and managing bad blocks. The implementation includes ONFI 4.0 parameter page parsing and timing configuration. Several areas require attention regarding clarity, hardware register documentation, and adherence to configuration best practices for device-specific parameters. Additionally, a critical compilation error was identified in the header file for an undefined struct, and significant memory allocation changes in linker scripts require justification.
|
|
||
| totalSparesz = (nand->info.size / nand->info.writesz) * nand->info.spareavail; | ||
|
|
||
| if ((offs >= nand->info.size) || (len > totalSparesz) || ((offs % nand->info.writesz) != 0U)) { |
There was a problem hiding this comment.
The validation (offs % nand->info.writesz) != 0U implies that offs must be page-aligned. However, the meta_read and meta_write functions are intended to operate on the spare area (metadata). If offs represents an offset within the logical address space of the metadata device, it should be aligned to nand->info.spareavail or some other metadata-specific unit, not nand->info.writesz (which is the data page size). This logic is ambiguous and could lead to incorrect behavior or unexpected validation failures. Please clarify the intended meaning of offs for the metadata device and adjust the validation accordingly.
|
|
||
| #define ADDR_DDR 0x10000 | ||
| #define SIZE_PLO 0x40000 | ||
| #define SIZE_PLO 0x1000000 |
There was a problem hiding this comment.
The SIZE_PLO has been significantly increased from 0x40000 to 0x1000000. This is a substantial change in memory allocation for the loader. While it might be necessary for the new NAND driver, such a large increase should be explicitly justified in the pull request description to ensure efficient memory usage and avoid unnecessary resource consumption.
|
|
||
| #define ADDR_DDR 0x00000 | ||
| #define SIZE_SBI 0x10000 | ||
| #define SIZE_PAYLOAD 0x1000000 |
2923ac3 to
07024fb
Compare
Unit Test Results9 553 tests 8 961 ✅ 53m 27s ⏱️ Results for commit 07024fb. |
YT: CSAT-158
07024fb to
ba9395d
Compare
YT: CSAT-158
Description
Motivation and Context
Types of changes
How Has This Been Tested?
riscv64-gr765-vcu118Checklist:
Special treatment