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

arm/rp2040,rp2350: Allow configuring XOSC startup delay, and set similarly to pico-sdk #15598

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

ids1024
Copy link
Contributor

@ids1024 ids1024 commented Jan 18, 2025

Summary

On some boards, the 1ms startup delay for the XOSC isn't sufficient, and NuttX can fail to start. (Depending on how the board was was reset.) To fix this, the delay has to be increased of affected boards, so this adds a configuration in board.h.

raspberrypi/pico-sdk#457 changed this in pico-sdk, and talks a bit about why this is needed.

Impact

For the Adafruit RP2040 boards and the Seeed XIAO, affected boards should now boot properly. At the cost of about 64ms delay on start.

Behavior should be entirely unchanged on other boards.

Testing

  • adafruit-feather-rp2040:usbnsh works on my unit before and after this change; so the one I have isn't affected, or it's very intermittent
  • I needed this fix for NuttX to start properly on power on reset or when plugged in on my Feather RP2350. I'll be opening another PR to add that board.

Previously this used a hard-coded value of 12 MHz, but this presumably
should use `BOARD_XOSC_FREQ`. Similarly to how it is done in pico-sdk.

It looks like all boards are currently defined to use 12 MHz XOSC
clocks, so this won't change behavior with any of them.

Signed-off-by: Ian Douglas Scott <[email protected]>
This follows the formula in the "Startup Delay" section of the
datasheets, where `BOARD_XOSC_STARTUPDELAY` is the time in milliseconds,
which is then multiplied by the clock fequency in kHz, divided by 256
cycles, and rounded up.

This differs from `pico-sdk`, which just multiplied the value of the
expression it previously used by a `PICO_XOSC_STARTUP_DELAY_MULTIPLIER`.
This seems less correct, though it should be essentially the same except
rounding. (`BOARD_XOSC_STARTUPDELAY_MULTIPLIER` also wouldn't fit nicely
the way the `#define`s are aligned.)

This sets `BOARD_XOSC_STARTUPDELAY` to `1` for all boards, so this will
produce the same output and not change behavior.

Signed-off-by: Ian Douglas Scott <[email protected]>
This matches how `PICO_XOSC_STARTUP_DELAY_MULTIPLIER` is set in
pico-sdk.

This should fix issues on startup for some units of these boards. It
seems not all are affected. See
raspberrypi/pico-sdk#457 and the issue linked
there for more information.

Signed-off-by: Ian Douglas Scott <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: S The size of the change in this PR is small labels Jan 18, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 18, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be expanded for clarity.

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change. The link to the pico-sdk PR provides valuable context.
  • Impact Addressed: The impact section covers several key areas (user, hardware, compatibility).
  • Testing Provided: Testing results are included, although they could be more robust.

Areas for Improvement:

  • Summary: While the summary is good, explicitly mentioning the new configuration option added to board.h would be beneficial. For example: "This PR adds a CONFIG_RP2040_XOSC_STARTUP_DELAY_MS configuration option in board.h to allow boards to specify a longer startup delay for the XOSC."
  • Impact: While the impact on the user, hardware, and compatibility is mentioned, several sections are marked as "NO" without explanation. Even if there's no impact, explicitly stating "NO - No change to the build process" is better for clarity. The impact on documentation should be addressed – does this new config option need to be documented? Security and forward/backward compatibility implications should also be explicitly addressed, even if the answer is "NO - No security implications."
  • Testing: While testing is provided, it's limited. Ideally, you'd provide logs demonstrating the failure before the change and the successful boot after the change. The current testing description suggests the initial board works, making the test less convincing. Find a board that consistently exhibits the failure to showcase the fix effectively. Include specifics of the build host (OS, compiler version). Clearly state which target architecture and board configurations were tested.

Example of Improved Testing Section:

Testing:

Build Host(s): macOS Ventura 13.5, clang version 14.0.6
Target(s): arm, adafruit-feather-rp2040:usbnsh, seeed-xiao:nsh

Testing logs before change (adafruit-feather-rp2350 -  Note: This board will be added in a separate PR):

```

Testing logs after change (adafruit-feather-rp2350):

<Insert log showing successful boot here>

Testing logs before/after change (adafruit-feather-rp2040, seeed-xiao): (Note: These boards did not exhibit the failure consistently but were tested to ensure the change did not introduce regressions)

<Insert relevant portions of logs, focusing on XOSC initialization>


By addressing these points, the PR will be even stronger and easier for reviewers to assess.

@anchao anchao merged commit c937899 into apache:master Jan 18, 2025
25 checks passed
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 Board: arm Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants