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 STM32H5 QSPI Driver #15351

Merged
merged 1 commit into from
Dec 28, 2024
Merged

Conversation

kywwilson11
Copy link
Contributor

Summary

This is an implementation of QSPI for the STM32H5, using the OCTOSPI peripheral. The driver is an adaptation of the STM32H7 QSPI driver, with changes made to accommodate differences in the STM32H5 OCTOSPI register set vs the STM32H7 QSPI registers.

Impact

This will only impact the STM32H5 driver and only if QSPI is enabled.

Testing

Testing was completed using the existing w25q driver as well as utilizing the nxffs file system. Additionally, a logic scope was used to verify correct operation of the QSPI interface. Attached are images of measurements taken with the logic scope and a brief functional test utilizing the w25q driver and nxffs.

page_program_1
page_program_2
read_id
read_status_register_2
fast_read_quad_io_1
fast_read_quad_io_2
qspi_func_test.txt

Register addresses are defined. Bit defintions are still needed.

Bitmaps for OCTOSPI peripheral register

Incremental updates to qspi files

Redefined QSPI/OCTOSPI registers for STM32H5

Fixed register definitions for the STM32H5. However, more work still needs to be done. Some bits that shared registers on the STM32H7 have different register locations on the H5. This still needs to be accounted for.

Fixed more differences vs STM32H7 qspi

Added ifdef for including stm32_dma.h in stm32_qspi.c. Added stm32_qspi.c to Make.defs.

Register fixes. SPI activity but not able to format device yet.

Fixed DCYC mask

Set HCLK frequency to correct value. Undid ccrconfig debug.

Tested Interrupt Mode (single SPI). Added alternate bytes to meminfo and cmdinfo structures. Updated Kconfig variables for STM32H5.

Fixed base register in hardware/stm32_qspi.h. Updated qspi_dumpregs.

The base register was previously set to STM32_QUADSPI_BASE, changed to the correctly named STM32_OCTOSPI1_BASE. However, these defines for the OCTOSPI registers are not even used. Instead qspi_putreg and qspi_getreg utilize the priv->base value to access OCTOSPI registers.

Removed altbytes code, left as before. Moved QSPI clock selection to stm32h5xx_rcc.c

Changed STM32H5_QUADSPI to STM32H5_QSPI1

Added hook to define QSPI_CLK_FREQUENCY as STM32_QSPI_FREQUENCY from board.h

Removed changes to nuttx qspi.h

style fixes
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Dec 26, 2024
@kywwilson11 kywwilson11 changed the title Added incomplete octospi hardware defintions. Added STM32H5 QSPI Driver Dec 26, 2024
@kywwilson11 kywwilson11 changed the title Added STM32H5 QSPI Driver Add STM32H5 QSPI Driver Dec 26, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 26, 2024

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements, but needs improvement in providing specific details. Here's a breakdown:

Strengths:

  • Clear Summary of the Change: The summary explains what was changed, why, and how.
  • Impact Assessment Started: The impact section begins to address the scope of the change.
  • Testing Evidence Provided: Logic analyzer captures and a functional test log demonstrate effort in verifying the changes.

Weaknesses and Required Improvements:

  • Incomplete Impact Assessment: The impact section is too brief. It needs to specifically address all the points in the template. For example:

    • Impact on user: Even if the answer is NO, explicitly state "NO". If QSPI wasn't available before, this IS a change for the user and needs describing.
    • Impact on build: Will any Kconfig options need to be added/modified to enable this driver? If so, explain how.
    • Impact on hardware: Be specific. "STM32H5" is a family. List the specific chip(s) tested. Mention any board(s) used.
    • Impact on documentation: If documentation updates are needed (they likely are!), mention that they will be provided in a separate commit or are included in the PR. If not needed, explicitly state "NO".
    • Impact on security, compatibility, etc.: Address all impact points, even if the answer is "NO." Explicitly stating "NO" clarifies that these aspects were considered.
  • Missing Build/Target Details (Testing): The testing section lacks crucial information. Provide the following:

    • Host OS: e.g., Ubuntu 22.04
    • Host Compiler: e.g., GCC 11.2.0
    • Target Architecture/Board/Configuration: e.g., STM32H563ZI, Nucleo-H563ZI, nsh configuration
    • Before/After Logs: The attached text file is good, but ideally, include relevant snippets directly in the PR description. Show specific commands and their output before and after the change, highlighting the differences. What happens if QSPI is used before this change? An error? What happens after? Success?
  • Missing "Before" Test Logs: While "after" logs are present, there are no explicit "before" logs demonstrating the previous behavior (e.g., the lack of QSPI functionality or errors encountered). This makes it harder to assess the effectiveness of the change.

Example of Improved Testing Section:

I confirm that changes are verified on local setup and works as intended:

* Build Host(s): Ubuntu 22.04, x86_64, GCC 11.2.0
* Target(s): STM32H563ZI, Nucleo-H563ZI board, `nsh` configuration

Testing logs before change (QSPI not implemented):

nsh> qspi_test
ERROR: QSPI driver not initialized


Testing logs after change (QSPI functional):

nsh> qspi_test
QSPI initialized successfully.
W25QXX ID: 0xEF4018
NXFFS mounted successfully.
Wrote 1024 bytes to NXFFS.
Read 1024 bytes from NXFFS.
Data verified.


[Full test log](https://github.com/user-attachments/files/18254399/qspi_func_test.txt)  <-- Still link to full log

By addressing these weaknesses and providing more specific details, the PR will better meet the NuttX requirements and significantly improve its reviewability. Remember, the goal is to make it as easy as possible for reviewers to understand and verify the changes.

kywwilson11 added a commit to stbenn/nuttx that referenced this pull request Dec 26, 2024
There were two simple errors when running ci tests on pull apache#15351. These errors were unrelated to that pull. After investigating I found two simple errors. 1. The len variable was likely a typo that should have been the function argument, ien. 2. The spinlock_t lock was defined in a section dependent on CONFIG_SERIAL_TERMIOS. However, the spinlock_t lock was used in code that did not require the SERIAL_TERMIOS config. Therefore errors resulted.
kywwilson11 added a commit to stbenn/nuttx that referenced this pull request Dec 27, 2024
There were two simple errors when running ci tests on pull apache#15351. These errors were unrelated to that pull. After investigating I found two simple errors. 1. The len variable was likely a typo that should have been the function argument, ien. 2. The spinlock_t lock was defined in a section dependent on CONFIG_SERIAL_TERMIOS. However, the spinlock_t lock was used in code that did not require the SERIAL_TERMIOS config. Therefore errors resulted.

imxrt_edma_s missing lock variable

CI build tests noted that the variable g_edma was referencing a lock variable that did not exist in the structure. This change adds that missing lock as a spinlock_t type.
kywwilson11 added a commit to stbenn/nuttx that referenced this pull request Dec 27, 2024
There were two simple errors when running ci tests on pull apache#15351. These errors were unrelated to that pull. After investigating I found two simple errors. 1. The len variable was likely a typo that should have been the function argument, ien. 2. The spinlock_t lock was defined in a section dependent on CONFIG_SERIAL_TERMIOS. However, the spinlock_t lock was used in code that did not require the SERIAL_TERMIOS config. Therefore errors resulted.

imxrt_edma_s missing lock variable

CI build tests noted that the variable g_edma was referencing a lock variable that did not exist in the structure. This change adds that missing lock as a spinlock_t type.

Remove unused flags variable from imxrt_dmach_stop
kywwilson11 added a commit to stbenn/nuttx that referenced this pull request Dec 27, 2024
There were two simple errors when running ci tests on pull apache#15351. These errors were unrelated to that pull. After investigating I found two simple errors. 1. The len variable was likely a typo that should have been the function argument, ien. 2. The spinlock_t lock was defined in a section dependent on CONFIG_SERIAL_TERMIOS. However, the spinlock_t lock was used in code that did not require the SERIAL_TERMIOS config. Therefore errors resulted.

imxrt_edma_s missing lock variable

CI build tests noted that the variable g_edma was referencing a lock variable that did not exist in the structure. This change adds that missing lock as a spinlock_t type.

Remove unused flags variable from imxrt_dmach_stop

Removed extra =
xiaoxiang781216 pushed a commit that referenced this pull request Dec 28, 2024
There were two simple errors when running ci tests on pull #15351. These errors were unrelated to that pull. After investigating I found two simple errors. 1. The len variable was likely a typo that should have been the function argument, ien. 2. The spinlock_t lock was defined in a section dependent on CONFIG_SERIAL_TERMIOS. However, the spinlock_t lock was used in code that did not require the SERIAL_TERMIOS config. Therefore errors resulted.

imxrt_edma_s missing lock variable

CI build tests noted that the variable g_edma was referencing a lock variable that did not exist in the structure. This change adds that missing lock as a spinlock_t type.

Remove unused flags variable from imxrt_dmach_stop

Removed extra =
@xiaoxiang781216 xiaoxiang781216 merged commit 1cfab89 into apache:master Dec 28, 2024
18 of 25 checks passed
@kywwilson11 kywwilson11 deleted the stm32h5_octospi branch December 30, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture 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.

3 participants