nuttxpr commented on PR #14813: URL: https://github.com/apache/nuttx/pull/14813#issuecomment-2479285087
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR description has some good points but is missing key information to fully meet the NuttX requirements. Here's a breakdown of what's missing and how to improve it: **Missing/Needs Improvement:** * **Summary:** While it explains the "why," it lacks detail on "what" and "how." Be more specific. Which files were changed in the arch directory? Which functions were modified? What exact lines of code were altered regarding the PLL configuration logic? Mentioning the simplification based on `board.h` is good, but exemplify it. * **Related Issue:** If this PR addresses a specific issue in the NuttX or NuttX-apps repositories, provide links. Even if it's a general improvement, consider creating an issue first to discuss the change before implementing it. This fosters community involvement and helps prevent duplicated work. * **Impact:** * **User Impact:** While it mentions ease of configuration, it doesn't explain *how* a user might be impacted. Does this simplify anything for the end-user of an application running on NuttX? * **Build Impact:** If only source code changed, the build process *shouldn't* change, but explicitly stating "NO" clarifies this. * **Hardware Impact:** You've specified the target, but haven't explicitly stated which architectures/boards/drivers are affected *beyond* the STM32H5. If it's only the H5, say so explicitly. If it's intended to be more general, explain that and list potential impacts on other platforms. * **Documentation:** Does this change require updates to any NuttX documentation? If so, were those updates included in the PR? * **Security Impact:** Even if there's no security impact, explicitly state "NO." * **Compatibility:** State explicitly whether this breaks any backward compatibility. Does code relying on the old PLL configuration method need to be changed? * **Testing:** * **Before/After Logs:** The provided testing information is insufficient. "Serial com port continued to work" isn't a log. Provide *actual* log output demonstrating the system clock configuration before and after the change. Show clock frequencies, register values, or any other relevant data that proves the PLL is configured correctly. Include commands used to obtain these logs. **Example of Improved PR Description:** ```markdown ## Summary This PR simplifies the PLL clock configuration logic in the STM32H5 architecture by relying on fully defined PLL register values in `board.h`. Previously, the code contained separate conditional logic for configuring individual PLL components. This change removes those conditions and assumes that `board.h` provides complete register definitions, as demonstrated in the STM32H7 port. The primary changes are in `arch/arm/src/stm32h5/stm32_rcc.c` (functions `stm32_rcc_enableperiph()` and `stm32_stdclockconfig()`), and `boards/arm/stm32h5/nucleo-h563zi/src/stm32_bringup.c` (modification to the board initialization sequence). Specifically, the code previously handled individual PLL register settings like this (example): ```c // Old code (simplified example) if (STM32_PLL1CFG_PLL1M & RCC_PLL1CFG_PLL1M_MASK) { // ... } ``` Now, it assumes a fully defined register value: ```c // New code (simplified example) putreg32(STM32_PLL1CFG_VALUE, STM32_RCC_PLL1CFG); ``` ## Impact * **New feature added:** NO * **Existing feature changed:** YES (PLL clock configuration logic simplified) * **Impact on user:** NO (The change is internal to the clock configuration and should not affect user applications directly unless they directly manipulate PLL registers). * **Impact on build:** NO * **Impact on hardware:** YES (STM32H5 architecture. Specifically, the Nucleo-H563ZI board was tested, but the changes apply to the STM32H5 family.) * **Impact on documentation:** NO (The change is internal and does not affect user-facing APIs or configuration procedures). * **Impact on security:** NO * **Impact on compatibility:** Potentially YES (Backward compatibility may be broken if custom board configurations were relying on the previous, more granular PLL register setting methods. Those configurations will need to be updated to provide fully defined PLL register values in `board.h`.) * **Anything else to consider:** This simplification makes it easier to manage PLL settings in `board.h` and aligns the STM32H5 port with the approach used in the STM32H7 port. ## Testing I confirm that changes are verified on a local setup and work as intended: * **Build Host:** Ubuntu 24.04 on WSL 2 (Windows 11), arm-none-eabi-gcc (15:13.2.rel1-2) 13.2.1 20231009 * **Target:** STM32H5 (Nucleo-H563ZI), `nsh` configuration **Testing logs before change:** ``` nsh> sysinfo ... (Output showing PLL1 frequency before the change) ... nsh> cat /proc/sys/clkinfo <-- Or any other relevant command to show clock config ... (Detailed clock configuration output) ... ``` **Testing logs after change:** ``` nsh> sysinfo ... (Output showing PLL1 frequency after the change – should be the same/correct) ... nsh> cat /proc/sys/clkinfo <-- Show that the clock configuration is as expected ... (Detailed clock configuration output) ... ``` ``` By providing this level of detail, reviewers can understand the changes thoroughly, evaluate the impact accurately, and verify the testing results effectively. This will greatly increase the chances of your PR being accepted. Remember to always consult the Contributing Guidelines and consider engaging with the NuttX community before submitting significant changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
