Skip to content
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

Add GD55 QSPI NOR Flash support #15523

Merged

Conversation

TimJTi
Copy link
Contributor

@TimJTi TimJTi commented Jan 13, 2025

Summary

This adds support for the GigaDevice QSPI GD55 NOR Flash devices

Impact

None, other than adding new device support

Testing

On a custom board using SAMA5D27C-D1G with a single GD55B01G 1Gbit device. Device has been partioned, fully and partially erased, written to and protected in various ways and checked that the protected areas report as locked correctly. Some performance measruerments taken and it behaves well.

Support has been added for the 2Gbit device "in good faith" but I have no board with this device to hand to check it.

@github-actions github-actions bot added Area: Drivers Drivers issues Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Jan 13, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 13, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

The PR, as described, partially meets the NuttX requirements. While it provides a summary and some testing information, it lacks crucial details.

Here's what's missing:

  • Summary: While the "what" is stated (adding GD55 support), the "why" and "how" are absent. Why is this support needed? What code changes were implemented to add this support (driver changes, board configuration updates, etc.)? Linking a related NuttX issue would greatly strengthen this section.
  • Impact: While stating "None, other than adding new device support" is a start, it's too broad. Be explicit. Does this impact any existing QSPI drivers? Are there any configuration options users need to be aware of? Even if the answer is truly no impact beyond the new device, explicitly stating "No impact on existing QSPI drivers" is better. Also, consider the documentation impact. Does this require documentation updates?
  • Testing: "On a custom board" is insufficient. Provide specifics:
    • Host OS and toolchain version used for building.
    • NuttX configuration used on the SAMA5D27C-D1G (e.g., sama5d27c_som1_ek_defconfig).
    • Clear "before" and "after" logs demonstrating the functionality. Ideally, the "before" logs should show the lack of GD55 support (e.g., failure to detect the device), and the "after" logs should show successful detection and basic operation (e.g., reading/writing to the flash).

In short, the PR needs more detail to meet the NuttX requirements fully. Providing more context and specifics will greatly improve the review process and increase the likelihood of acceptance.

@TimJTi TimJTi force-pushed the Add-GD55-QSPI-NOR-Flash-Support branch 3 times, most recently from 52cf8db to 3c511e7 Compare January 14, 2025 17:57
@TimJTi TimJTi force-pushed the Add-GD55-QSPI-NOR-Flash-Support branch 3 times, most recently from b8dbdc7 to b0b5d46 Compare January 16, 2025 12:21
@xiaoxiang781216
Copy link
Contributor

@TimJTi does it ready for review?

@TimJTi
Copy link
Contributor Author

TimJTi commented Jan 16, 2025

@TimJTi does it ready for review?

@xiaoxiang781216 - just doing some final checks and adjustments as I found a few minor inconsistencies. Should be marking it as ready later today

@TimJTi TimJTi force-pushed the Add-GD55-QSPI-NOR-Flash-Support branch from b0b5d46 to c7a82d4 Compare January 16, 2025 18:14
@TimJTi TimJTi marked this pull request as ready for review January 16, 2025 19:05
@xiaoxiang781216 xiaoxiang781216 merged commit acd7e44 into apache:master Jan 17, 2025
39 checks passed
@TimJTi TimJTi deleted the Add-GD55-QSPI-NOR-Flash-Support branch January 17, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants