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

boards/nrf52*dk: Enable Pinreset after Flashing #20965

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Nov 7, 2024

Contribution description

As outlined in #20775, there was a silicon change in the nRF52 chips at some revision making the UICR register non-persistent. This means that the applied configuration is deleted when the flash is erased and programmed.
The reset pin is configured in this UICR register as well, so after flashing RIOT to (any) nRF52 chip without a bootloader, the reset pin won't work anymore.

This PR adds a JLINK_POST_FLASH command which configures the UICR->PSELRESET registers while the nRF52 chip is still unlocked from Flashing.

The JLINK_POST_FLASH variable is added to the builsystem sanity check as well, so it is evaluated alongside JLINK_PRE_FLASH. I'm not sure if that was required, but now it is consistent.

Request for comments: The documentation should be updated accordingly and the old tool in dist/tools/nrf52_resetpin_cfg is obsolete and could be deleted. This PR works with all silicon revisions of the nRF52 and the tool does not.
Should I do that in this PR or do a separate PR?

Testing procedure

You can use any application you want and flash it to either a nRF52DK board (with the nRF52832DK) or an nRF52840DK from Nordic and press the Reset button.
The board should reset.

BOARD=nrf52dk make -C examples/default flash term

Issues/PRs references

This fixes #20775.

@crasbe crasbe requested a review from aabadie as a code owner November 7, 2024 16:59
@github-actions github-actions bot added Area: tools Area: Supplementary tools Area: boards Area: Board ports labels Nov 7, 2024
@crasbe crasbe changed the title boards/nrf52 boards/nrf52*dk: Enable Pinreset after Flashing Nov 7, 2024
@crasbe crasbe force-pushed the pr/nRF52_pinreset branch from 1a2dcae to dbad618 Compare November 7, 2024 17:08
@crasbe
Copy link
Contributor Author

crasbe commented Nov 7, 2024

Request for comments: The documentation should be updated accordingly and the old tool in dist/tools/nrf52_resetpin_cfg is obsolete and could be deleted. This PR works with all silicon revisions of the nRF52 and the tool does not.
Should I do that in this PR or do a separate PR?

I am currently working on another PR that updates the nRF documentation (and checks off some points from the list in #20775).
Therefore it's probably best to remove the old tool in this PR (because it fits this one best), but only merge this PR after the documentation update. Then there is no step in between where the documentation is pointing to something that does not exist anymore.

@mguetschow
Copy link
Contributor

Nice, thank you! Just tested this locally with an nRF52840dk of a newer revision, where the reset button doesn't work on master, and does with this PR.

the old tool in dist/tools/nrf52_resetpin_cfg is obsolete and could be deleted. This PR works with all silicon revisions of the nRF52 and the tool does not.

For feature parity, we would need to support BOARD=dwm1001 with this approach, too: https://github.com/RIOT-OS/RIOT/blob/master/dist/tools/nrf52_resetpin_cfg/Makefile#L17

Also, this approach only works with PROGRAMMER=jlink, right? What about OpenOCD?

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 8, 2024
@riot-ci
Copy link

riot-ci commented Nov 8, 2024

Murdock results

✔️ PASSED

0c89794 dist/tools/nrf52_resetpin_cfg: add revision warning

Success Failures Total Runtime
3509 0 3509 08m:12s

Artifacts

@crasbe
Copy link
Contributor Author

crasbe commented Nov 8, 2024

the old tool in dist/tools/nrf52_resetpin_cfg is obsolete and could be deleted. This PR works with all silicon revisions of the nRF52 and the tool does not.

For feature parity, we would need to support BOARD=dwm1001 with this approach, too: https://github.com/RIOT-OS/RIOT/blob/master/dist/tools/nrf52_resetpin_cfg/Makefile#L17

That is interesting, I did not have the dwm1001 on my radar. The documentation of the dwm1001 does not mention the reset tool.
One board that I actually forgot was the waveshare-nrf52840-eval-kit (which is not part of the tool :D ).

Also, this approach only works with PROGRAMMER=jlink, right? What about OpenOCD?

Yes, currently this only works with JLink.

I tried to look into how to implement the UICR programming in OpenOCD, but currently OpenOCD does not work with the embedded JLink on the nRF52840. Maybe you can try it to see if it works for you?

Apparently, the new(er?) versions of the J-Link use a different product ID, which is not recognized by "libjaylink" which OpenOCD uses.

PROGARMMER=openocd BOARD=nrf52840dk make -C examples/hello-world flash
...
### Flashing Target ###
Open On-Chip Debugger 0.11.0
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
swd
Error: No J-Link device found.

To get some more debug information, you can add change line 340 of dist/tools/openocd/openocd.sh:

- sh -c "${OPENOCD} \
+ sh -c "${OPENOCD} -d3 \

This yields the following:

...
Debug: 87 2 target.c:1639 handle_target_init_command(): Initializing targets...
Debug: 88 2 semihosting_common.c:99 semihosting_common_init():  
Debug: 89 2 jlink.c:647 jlink_init(): Using libjaylink 0.2.0 (compiled with 0.2.0).
Debug: 90 12 jlink.c:526 jaylink_log_handler(): Found 0 USB device(s).
Error: 91 12 jlink.c:683 jlink_init(): No J-Link device found.
Debug: 92 12 command.c:628 run_command(): Command 'init' failed with error code -100
User : 93 12 command.c:694 command_run_line():
...

libjaylink v0.2.0 recognizes the following Product IDs (https://github.com/syntacore/libjaylink/blob/master/libjaylink/discovery_usb.c#L46-L65), and Segger J-Link firmware from "2024 Oct 9 11:01" uses 1060:

chris@ThinkPias:~$ lsusb
...
Bus 003 Device 005: ID 1366:1060 SEGGER J-Link
...

Maybe this is an issue I could raise with OpenOCD since the last release of libjaylink was in 2016...
What a rabbit hole 😅

Edit: This is actually not true, the Github repo is just outdated. This seems to be the active repository for libjaylink: https://gitlab.zapb.de/libjaylink/libjaylink

This still wouldn't fix our issue because the ID 1060 is not part of that lib either... BUT it seems to be under active development, so maybe I can open an issue or PR there.


This is a good opportunity to remove the remarks about the OpenOCD version in boards/common/nrf52/Makefile.include.
Since version 0.11 (the current one is 0.12), the nRF52 devices are supported.

# setup OpenOCD for flashing. Version 0.10 of OpenOCD doesn't contain support
# for nrf52dk and nrf52840dk boards. To use OpenOCD with these a version
# build from source (master > 2018, August the 13rd) is required.

@crasbe
Copy link
Contributor Author

crasbe commented Nov 8, 2024

Okay.. this is becoming a pretty deep rabbit hole, I'll open a separate issue about it.

@mguetschow
Copy link
Contributor

Okay.. this is becoming a pretty deep rabbit hole, I'll open a separate issue about it.

A proposal to move this forward for now without too much side-tracking:

  1. Do not remove the nrf52_resetpin_cfg tool for now, as it still has the advantage that older revisions can be flashed and configured using openOCD. Update the tool's README mentioning that it only works for older revisions (the exact revision number was linked in cpu/nrf52: Add Make Option to Enable Pinreset #20775)
  2. Update this to include all nRF52 boards with reset pin that are supported by RIOT and flashed with J-Link.
  3. leave the rest to cpu/nrf52: OpenOCD not working with newer Revision Silicon and nRF52840 J-Link #20968

@crasbe
Copy link
Contributor Author

crasbe commented Nov 8, 2024

Alright, that sounds like a good plan.

To point 3: Would you recommend doing a second PR with the OpenOCD fixes? That would probably make sense, right?
I interpret your response from #20968 that I should add it to this PR :D
Okay, separate PR for the OpenOCD fixes and include the reset addition to this one :D

@mguetschow
Copy link
Contributor

To point 3: Would you recommend doing a second PR with the OpenOCD fixes? That would probably make sense, right?

I think so too, since it is pretty disjoint. Maybe the OpenOCD fixes could go first and then this PR could build on top of it and include the OpenOCD resetbutton configuration trick.

@crasbe
Copy link
Contributor Author

crasbe commented Nov 19, 2024

I went through the RIOT Board list and this board appears to have a hardware reset button as well: https://doc.riot-os.org/group__boards__reel.html

I'll add the JLink and OpenOCD commands to that as well, after that I think this is ready to be merged.
Then I can open the Documentation PR, so we don't run into a hen-egg-problem.

@mguetschow
Copy link
Contributor

Sounds good! Would you mind also addressing number 1 from above?

@crasbe
Copy link
Contributor Author

crasbe commented Nov 19, 2024

Sounds good! Would you mind also addressing number 1 from above?

Sure, I didn't notice the second part of that 😅

@crasbe crasbe requested a review from jia200x as a code owner November 19, 2024 16:08
@github-actions github-actions bot added the Area: doc Area: Documentation label Nov 19, 2024
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

LGTM then, thanks a lot!

@mguetschow mguetschow added this pull request to the merge queue Nov 21, 2024
@maribu
Copy link
Member

maribu commented Nov 21, 2024

added this pull request to the merge queue 6 hours ago

360 min is the timeout for the merge queue, I'll drop this from the queue and add it again to not waste a 3 hrs Murdock CI pass.

@maribu maribu removed this pull request from the merge queue due to a manual request Nov 21, 2024
@maribu maribu added this pull request to the merge queue Nov 21, 2024
@maribu maribu removed this pull request from the merge queue due to a manual request Nov 21, 2024
@maribu maribu added this pull request to the merge queue Nov 21, 2024
Merged via the queue into RIOT-OS:master with commit ab67824 Nov 21, 2024
26 checks passed
@crasbe
Copy link
Contributor Author

crasbe commented Nov 21, 2024

Thanks everyone ❤️

@crasbe crasbe deleted the pr/nRF52_pinreset branch November 21, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: doc Area: Documentation Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpu/nrf52: Add Make Option to Enable Pinreset
4 participants