Skip to content

Conversation

@kywwilson11
Copy link
Contributor

Summary

hardware/stm32_qspi.h - Defines for the STM32H5 QSPI driver. The peripheral on the STM32H5 is actually an OCTOSPI peripheral, however it was decided to use it as a QSPI peripheral for the following reasons:

  1. There is currently support for QSPI within Nuttx.
  2. The STM32H7 QSPI register set is very similar to the STM32H5 OCTOSPI register set and the STM32H7 series has an existing QSPI driver.
  3. OCTOSPI devices are rare as of now and are not part of our use case.

hardware/stm32h56xxx_pinmap.h - Added pin mappings for OCTOSPI GPIOs.

Impact

This change will not impact any architectures outside of the STM32H5 series. Even for the STM32H5 series, these changes will only have an impact if they are utilized.

Testing

No testing yet for these files other than ensuring that we can still build Nuttx. Further testing will be documented in a future pull request when the STM32H5 QSPI driver is added.

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: L The size of the change in this PR is large labels Dec 18, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 18, 2024

[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.

No, this PR does not fully meet the NuttX requirements. Here's why:

  • Insufficient Detail in Summary: While the summary explains the reasoning behind using QSPI terminology for an OCTOSPI peripheral, it lacks crucial information about what exactly changed. Simply stating files were modified is not enough. Describe the newly added defines in stm32_qspi.h and which pins were mapped in stm32h56xxx_pinmap.h.

  • Impact Section Lacks Specificity: Saying "no impact" or "only impact if utilized" is too vague. Be explicit. For example:

    • Impact on user: Will users need to configure anything differently to use this new functionality? Are any new Kconfig options added?
    • Impact on build: Does this change require any new build dependencies? Does it affect build size?
    • Impact on hardware: Specifically state which STM32H5 boards/configurations are affected.
    • Impact on documentation: Even if no documentation updates are required right now, state that explicitly. If documentation will be added later, mention that.
  • Testing Section is Inadequate: "Ensuring we can still build NuttX" is a bare minimum and doesn't demonstrate functionality. While full driver testing may be deferred, you should at least test that the new pin mappings don't conflict with existing ones and that the defines in stm32_qspi.h are correct. Provide some testing logs, even if they're simple. Explain how you confirmed the build works (e.g., "Successfully built nsh configuration for stm32h5-sim"). Acknowledging the lack of testing is good, but it doesn't replace the requirement to provide some level of verification.

To make this PR compliant, you need to:

  1. Expand the Summary: Detail the specific changes within each file. List the added defines and pin mappings.
  2. Clarify the Impact: Address each impact category with specifics. Even if there's "no impact", state it clearly and justify it.
  3. Improve Testing: Perform some basic verification of the changes (e.g., compile-time checks, pin conflict checks) and provide corresponding logs. Clearly state the scope of testing performed and what is deferred to later PRs.

By addressing these points, you'll ensure your PR meets the NuttX requirements and facilitates a smoother review process.

@kywwilson11
Copy link
Contributor Author

Error in the arm-12 pull_request appears to be unrelated. The error is with arch/arm/src/stm32/stm32_ethernet.c, which is unrelated to this pull request.

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a commit log. You can copy the description from the PR.

@kywwilson11
Copy link
Contributor Author

Please add a commit log. You can copy the description from the PR.

I added a new commit log with an empty commit. Let me know if this is not what you were expecting. Thank you.

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the commit message.

It looks like the CI tests are failing for some reason?

@kywwilson11
Copy link
Contributor Author

Thanks for adding the commit message.

It looks like the CI tests are failing for some reason?

Yes the CI tests are failing but it appears it is due to a board file on an unrelated architecture (legacy stm32, not stm32h5).
The file is nuttx/boards/arm/stm32/stm32f4discovery/src/stm32_ethernet.c and this error should have nothing to do with the changes I made.

Below is the error. It seems the declaration of g_phy_lock should be inside an "#ifdef HAVE_NETMONITOR" block.

Error: board/stm32_ethernet.c:84:19: error: 'g_phy_lock' defined but not used [-Werror=unused-variable] 84 | static spinlock_t g_phy_lock = SP_UNLOCKED;

@xiaoxiang781216
Copy link
Contributor

Thanks for adding the commit message.
It looks like the CI tests are failing for some reason?

Yes the CI tests are failing but it appears it is due to a board file on an unrelated architecture (legacy stm32, not stm32h5). The file is nuttx/boards/arm/stm32/stm32f4discovery/src/stm32_ethernet.c and this error should have nothing to do with the changes I made.

Below is the error. It seems the declaration of g_phy_lock should be inside an "#ifdef HAVE_NETMONITOR" block.

Error: board/stm32_ethernet.c:84:19: error: 'g_phy_lock' defined but not used [-Werror=unused-variable] 84 | static spinlock_t g_phy_lock = SP_UNLOCKED;

@hujun260 please look at this problem.

@hujun260
Copy link
Contributor

Thanks for adding the commit message.
It looks like the CI tests are failing for some reason?

Yes the CI tests are failing but it appears it is due to a board file on an unrelated architecture (legacy stm32, not stm32h5). The file is nuttx/boards/arm/stm32/stm32f4discovery/src/stm32_ethernet.c and this error should have nothing to do with the changes I made.
Below is the error. It seems the declaration of g_phy_lock should be inside an "#ifdef HAVE_NETMONITOR" block.
Error: board/stm32_ethernet.c:84:19: error: 'g_phy_lock' defined but not used [-Werror=unused-variable] 84 | static spinlock_t g_phy_lock = SP_UNLOCKED;

@hujun260 please look at this problem.

ok

…d by stm32h5_qspi driver).

Fixed comments and indent

Add commit log to OCTOSPI HW PR

Summary

hardware/stm32_qspi.h - Defines for the STM32H5 QSPI driver. The peripheral on the STM32H5 is actually an OCTOSPI peripheral, however it was decided to use it as a QSPI peripheral for the following reasons:

    There is currently support for QSPI within Nuttx.
    The STM32H7 QSPI register set is very similar to the STM32H5 OCTOSPI register set and the STM32H7 series has an existing QSPI driver.
    OCTOSPI devices are rare as of now and are not part of our use case.

hardware/stm32h56xxx_pinmap.h - Added pin mappings for OCTOSPI GPIOs.
Impact

This change will not impact any architectures outside of the STM32H5 series. Even for the STM32H5 series, these changes will only have an impact if they are utilized.
Testing

No testing yet for these files other than ensuring that we can still build Nuttx. Further testing will be documented in a future pull request when the STM32H5 QSPI driver is added.
@kywwilson11
Copy link
Contributor Author

The failure is again to do with something unrelated to this pull request. I am asking for someone to please approve this pull request.

I did look into the failure and found this:
Error: chip/imx_serial.c:300:3: error: request for member 'bits' in something not a structure or union

There is a comma missing in imx_serial.c on line 299:
.parity = CONFIG_UART1_PARITY,
.lock = SP_UNLOCKED
.bits = CONFIG_UART1_BITS,

I can do a separate pull request to fix this.

@hartmannathan
Copy link
Contributor

The failure is again to do with something unrelated to this pull request. I am asking for someone to please approve this pull request.

I did look into the failure and found this:

Error: chip/imx_serial.c:300:3: error: request for member 'bits' in something not a structure or union

There is a comma missing in imx_serial.c on line 299:

.parity = CONFIG_UART1_PARITY,

.lock = SP_UNLOCKED

.bits = CONFIG_UART1_BITS,

I can do a separate pull request to fix this.

Okay I just approved PR #15305. Just waiting for tests to finish there. Thanks for finding and fixing that.

@xiaoxiang781216 xiaoxiang781216 merged commit d59b8f2 into apache:master Dec 22, 2024
20 of 25 checks passed
@kywwilson11 kywwilson11 deleted the stm32h5_octospi_hw branch December 26, 2024 18:11
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: L The size of the change in this PR is large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants