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

cpu/nrf52: Add Make Option to Enable Pinreset #20775

Open
crasbe opened this issue Jul 4, 2024 · 7 comments
Open

cpu/nrf52: Add Make Option to Enable Pinreset #20775

crasbe opened this issue Jul 4, 2024 · 7 comments

Comments

@crasbe
Copy link
Contributor

crasbe commented Jul 4, 2024

Description

In PR #10072, the nrf52_resetpin_cfg tool was introduced, allowing to program the persistent UICR register on the nRF52 boards.
Something must have changed since 2018, but when an nRF52 is programmed via the J-Link (not via bootloader), the UCIR register is always reset and therefore the pinreset does not work anymore.

Usually the programming sequence is somewhat like this:

  • Unlock (or recover) the device (this erases all of the Flash and the UICR, which holds the Pinreset configuration)
  • Perform a chiperase
  • Write the bin/hex
  • Verify
  • (optionally) write the UCIR
  • on the first reset or power down, power up, the nRF has the readback protection enabled which you can only disable by "unlocking" or "recovering" the device, deleting Flash and UICR

nrfjprog shows the issue nicely. I try to read the UICR register after powering up an nRF52840DK board. It fails because the readback protection is activated. Then I recover the device, which deletes the Flash and UICR area. After that I can read out the UICR, but it is blank.
To see how it should look like, I activate the pinreset (without powering the device down in between!) and read it out again.

chris@ThinkPias:~$ nrfjprog --readuicr asdf.hex
[error] [ Client] - Encountered error -90: Command read_device_info executed for 2 milliseconds with result -90
[error] [ Worker] - Access protection is enabled, can't read device version.
[error] [ Client] - Encountered error -90: Command read_memory_descriptors executed for 2 milliseconds with result -90
[error] [ Worker] - Can't read memory descriptors, ap-protection is enabled.
ERROR: The operation attempted is unavailable due to readback protection in
ERROR: your device. Please use --recover to unlock the device.
NOTE: For additional output, try running again with logging enabled (--log).
NOTE: Any generated log error messages will be displayed.
chris@ThinkPias:~$ nrfjprog --recover
Recovering device. This operation might take 30s.
Erasing user code and UICR flash areas.
Writing image to disable ap protect.
chris@ThinkPias:~$ nrfjprog --readuicr asdf.hex
Storing data in 'asdf.hex'.


chris@ThinkPias:~$ nrfjprog --pinresetenable
Enabling pin reset.
chris@ThinkPias:~$ nrfjprog --readuicr asdf2.hex
Storing data in 'asdf2.hex'.

On the left side, the UICR is shown before enabling the pinreset and on the right side it is shown after the pinreset.
UICR before and after pinresetenable

Programming with J-Link is pretty much the same, but nrfjprog is a lot more verbose about what it does and why it does (or does not) do something.

So essentially what I want to say is that it would be nice to be able to activate the Pinreset when flashing a RIOT image with make flash.
@benpicco proposed a software based solution in #19833, but the nRF52s do have the Pinreset registers in UICR, so why not use them? (However I am still in favor of merging that PR, it is nice to have a software option as well.)

Now... the thing is that with nrfjprog it is very easy to activate the Pinreset, just call nrfjprog --pinresetenable after programming (and before the first reset). With J-Link it is not so easy because it does not have a dedicated option to do it.
In general I see two main possibilities:

  1. Create a JLINK_POST_FLASH define for dist/tools/jlink.sh, that writes the right values into the UICR. So far nobody seems to use JLINK_POST_FLASH for anything.
  2. Activate the pinreset with nrfjprog. This would require an additional dependency, which is probably not favorable.

Option 1 seems to work. The base address for the UICR register is 0x10001000 for the nRF52840 and the offset for of the PSELRESET registers is 0x200 and 0x204. Essentially I just copied the memory content from what `nrfjprog` puts into the registers. Setting the registers with the J-Link programmer works when I add `JLINK_POST_FLASH` to the command like this:
JLINK_POST_FLASH='Write4 0x10001200 00000012 00000012' BOARD=nrf52840dk make flash term

We can check the burn file that is created by makefiles/tools/jlink.inc.mk has the right post-flash operation:

chris@ThinkPias:~/flashdev-riot/RIOT/examples/default$ cat bin/nrf52840dk/burn.seg 
loadfile /home/chris/SmartSensors/spacepatch-riot/RIOT/examples/default/bin/nrf52840dk/default.elf
Write4 0x10001200 00000012 00000012
r
g
exit

HOWEVER when I add JLINK_POST_FLASH = Write4 0x10001200 00000012 00000012 to the boards/nrf52840dk/Makefile.include and just call BOARD=nrf52840dk make flash term, it does not work. What's odd though is that it does work when I call JLINK_POST_FLASH='' BOARD=nrf52840dk make flash term.

Then I tried it with JLINK_PRE_FLASH because that is actually used by some boards, but it has the same behavior.

What's odd as well is that the 'stk3200' board, which uses JLINK_PRE_FLASH in it's boards/stk3200/Makefile.include shows the same behavior. Without adding JLINK_PRE_FLASH='' or similar to the command line, the r from the Makefile is not added to the burn.seg file.

Maybe someone has some guidance on that or generally some feedback about the approach.

Useful links

Documentation about the UICR:
https://docs.nordicsemi.com/bundle/ps_nrf52832/page/uicr.html#register.PSELRESET-0

Documentation about the Pinreset (not very much unfortunately):
https://docs.nordicsemi.com/bundle/ps_nrf52832/page/power.html#d935e411

@mguetschow
Copy link
Contributor

Thanks for opening the issue, I also already stumbled upon this.

Just for reference, here are some links from the nRF52840 PS:

Something must have changed since 2018, but when an nRF52 is programmed via the J-Link (not via bootloader), the UCIR register is always reset and therefore the pinreset does not work anymore.

An explanation for this can be found on https://docs.nordicsemi.com/bundle/ps_nrf52840/page/dif.html#d402e184: access port protect is enabled by default for newer build codes, which means that UICR will be reset on every flash. This was before only the case after manually enabling ap protect.

A work-around I was using locally is adding the following to the nrf52 boot-up code:

diff --git a/cpu/nrf52/cpu.c b/cpu/nrf52/cpu.c
index 7f34a4d231..25d90d1c1f 100644
--- a/cpu/nrf52/cpu.c
+++ b/cpu/nrf52/cpu.c
@@ -86,6 +86,19 @@ void cpu_init(void)
 
     /* trigger static peripheral initialization */
     periph_init();
+
+// valid for nrf52840dk, see RIOT/dist/tools/nrf52_resetpin_cfg/Makefile
+#define RESET_PIN 18
+    if (((NRF_UICR->PSELRESET[0] & UICR_PSELRESET_CONNECT_Msk) != (UICR_PSELRESET_CONNECT_Connected << UICR_PSELRESET_CONNECT_Pos)) ||
+        ((NRF_UICR->PSELRESET[1] & UICR_PSELRESET_CONNECT_Msk) != (UICR_PSELRESET_CONNECT_Connected << UICR_PSELRESET_CONNECT_Pos))){
+        NRF_NVMC->CONFIG = NVMC_CONFIG_WEN_Wen;
+        while (NRF_NVMC->READY == NVMC_READY_READY_Busy) {}
+        NRF_UICR->PSELRESET[0] = RESET_PIN;
+        NRF_UICR->PSELRESET[1] = RESET_PIN;
+        while (NRF_NVMC->READY == NVMC_READY_READY_Busy) {}
+        NRF_NVMC->CONFIG = NVMC_CONFIG_WEN_Ren;
+        NVIC_SystemReset();
+    }
 }
 
 /**

I think such code could be conditionally compiled in depending on the make option as option 3, with the advantage that it would be independent of the flashing tool, but the disadvantage that it would slightly increase .text, and lead to some overhead especially on the first, but also on subsequents starts.

I don't have a strong opinion on which way to go, but I just wanted to mention that option as well.

@crasbe
Copy link
Contributor Author

crasbe commented Jul 5, 2024

Something must have changed since 2018, but when an nRF52 is programmed via the J-Link (not via bootloader), the UCIR register is always reset and therefore the pinreset does not work anymore.

An explanation for this can be found on https://docs.nordicsemi.com/bundle/ps_nrf52840/page/dif.html#d402e184: access port protect is enabled by default for newer build codes, which means that UICR will be reset on every flash. This was before only the case after manually enabling ap protect.

Thank you, I was looking for this information but I searched at the wrong spot (I looked at the erratas and searched for UICR and Pinreset, but not for AP).

I think such code could be conditionally compiled in depending on the make option as option 3, with the advantage that it would be independent of the flashing tool, but the disadvantage that it would slightly increase .text, and lead to some overhead especially on the first, but also on subsequents starts.

I don't have a strong opinion on which way to go, but I just wanted to mention that option as well.

Your option 3 is very good, but I would probably approach it in a more general way (read: write a function that can make any pin a reset pin, as was intended with the PSELRESET register).

At least the nRF52DK and nRF52840DK have different pins for the reset button. For the nRF52DK it is on P0.21 (https://infocenter.nordicsemi.com/pdf/nRF52_DK_User_Guide_v2.x.x.pdf page 6) and for the nRF52840DK it is on P0.18 (https://infocenter.nordicsemi.com/pdf/nRF52840_DK_User_Guide_v1.2.pdf page 14).

Therefore I would only add the JLINK_POST_FLASH to the Makefiles of the development boards. This does not create any conflicts with potential custom boards and as a result the Reset button behaves as one would expect. I can not imagine how many users have stumbled over this and how much time was spent to find out the reason (at least for me it took more than an hour or so?).
But for this we would have to get JLINK_POST_FLASH working and I don't really know how yet...

An elegant solution would be to have something like NRF_PINRESET = (0, 18) in the Makefiles (so it can be predefined by the board files, but optional for custom boards), but I don't know how to implement it in a RIOT-y way.

@crasbe
Copy link
Contributor Author

crasbe commented Jul 5, 2024

I did some more work finding out about why the JLINK_POST_FLASH variable is ignored.
Another test I did is to see if the JLINK variable, that is defined in boards/common/nrf52/Makefile.include is used in the dist/tools/jlink/jlink.sh script by changing the backup variable _JLINK in the script to an illegal value (replace JLinkExe with JLinkExee). The backup variable should not be used because JLINK is already defined and it should work one way or another... BUT! I get the Error: J-Link appears not to be installed on your PATH error from the script.
That means that the script in fact does not use the JLINK variable but relies on it's backup variable.

After some googleing I found out, that this behavior means the variable was not exported (from the make environment to the shell script environment). But when I define JLINK_POST_FLASH on the command line, it already exists in the right environment and Make overrides the already existing variable. Therefore it works even with arbitrary values.


After a lot of digging I found it. @aabadie removed all JLINK related variables from the export list in makefiles/vars.inc.mk to speed up the build (and cleaning) process. The PR is #13567 and the related issue is #10850 by @cladmi . This did work at the time because makefiles/tools/jlink.inc.mk was still included in the boards/common/nrf52/Makefile.include file.

HOWEVER in PR #15475, this include was removed and from that point, the makefiles/tools/jlink.inc.mk script was never called again. Therefore the variables that should be exported by that script are in fact not exported at all.
I don't know how to fix this, maybe @aabadie can take a look at this. It looks like the thought behind this was that some script evaluates the PROGRAMMER variable and calls the respective scripts like jlink.inc.mk, but that got lost along the way and it did not have side effects because of the backup values in the jlink.sh script.

...

Phew, that was a lot of digging 😅

@cladmi
Copy link
Contributor

cladmi commented Jul 6, 2024

From looking in #15475 I would expect jlink.inc.mk to be loaded by https://github.com/RIOT-OS/RIOT/pull/15475/files#diff-3c8fb3a3507e1db4ca93b4d7b28883c763dc084a2ca0dc45adc907d9e4abedefR405-R407

Not sure why it would not be included just from looking at github PRs and source code.

If you would want JLINK_POST_FLASH to be available in the script, it would need to also be added to the target-export-variables in jlink.inc.mk for flash% or whatever your target for pinreset is. Defining the variable in Makefile.include is just a local Make variable and is not be exported to the script environment.
It should not be exported globally as it would export it for all the compilation targets, making it a global variable, so nothing preventing a makefile somewhere to rely on it. (can be for debug, but not at the end)

In practice, using exports there is a simplification to not have to provide and parse arguments.
This could have been a --jlink-post-flash="whatever" and provided as JLINK_FLASH_ARGS += or even a generic FLASH_ARGS += and so not require exports as it would be used as CLI argument.

Things that can help debug, are the usual "print" debug that can be added https://www.gnu.org/software/make/manual/html_node/Make-Control-Functions.html and is easier to look at that the full verbose output of make.

$(error message) to stop the execution in jlink.inc.mk to indeed check it's not included.
$(warning PROGRAMMER=$(PROGRAMMER)) just before the include to see the value.

@crasbe
Copy link
Contributor Author

crasbe commented Jul 6, 2024

@cladmi Thank you for taking time to explain this a little more to me.

From looking in #15475 I would expect jlink.inc.mk to be loaded by https://github.com/RIOT-OS/RIOT/pull/15475/files#diff-3c8fb3a3507e1db4ca93b4d7b28883c763dc084a2ca0dc45adc907d9e4abedefR405-R407

Not sure why it would not be included just from looking at github PRs and source code.

You are right, jlink.inc.mk is indeed called.

If you would want JLINK_POST_FLASH to be available in the script, it would need to also be added to the target-export-variables in jlink.inc.mk for flash% or whatever your target for pinreset is. Defining the variable in Makefile.include is just a local Make variable and is not be exported to the script environment. It should not be exported globally as it would export it for all the compilation targets, making it a global variable, so nothing preventing a makefile somewhere to rely on it. (can be for debug, but not at the end)

This is right as well, but I copied the code from JLINK_PRE_FLASH in the jlink.inc.mk file and adapted it for JLINK_POST_FLASH, which did not yield the expected result (that's why I assumed the file isn't called in the first place).

For debugging, I changed the variables.mk file to output what it would call normally:

# '$1: export $2' cannot be used alone
# By using '?=' the variable is evaluated at runtime only
_target-export-variable = $(warning $1: export $2?=)

So indeed it looks like the variables should be exported and indeed JLINK_DEVICE is being exported, because the flashing fails due to JLINK_DEVICE not being set (it can't be because I replaced the eval with a warning). But why does the export work for JLINK_DEVICE and not for JLINK_POST_FLASH?

chris@ThinkPias:~/flashdev-riot/RIOT/examples/default$ BOARD=nrf52840dk make flash -j12
/home/chris/flashdev-riot/RIOT/makefiles/tools/jlink.inc.mk:22: debug% flash% reset term-rtt: export JLINK_SERIAL?=
/home/chris/flashdev-riot/RIOT/makefiles/tools/jlink.inc.mk:25: debug% flash% reset term-rtt: export JLINK_DEVICE?=
/home/chris/flashdev-riot/RIOT/makefiles/tools/jlink.inc.mk:39: flash%: export JLINK_PRE_FLASH?=
/home/chris/flashdev-riot/RIOT/makefiles/tools/jlink.inc.mk:45: flash%: export JLINK_POST_FLASH?=
/home/chris/flashdev-riot/RIOT/makefiles/tests/tests.inc.mk:6: test test-as-root test-with-config: export TESTRUNNER_RESET_AFTER_TERM?=
Building application "default" for "nrf52840dk" with CPU "nrf52".

/home/chris/flashdev-riot/RIOT/pkg/pkg.mk:54: /.git: export GIT_CACHE_DIR?=
"make" -C /home/chris/flashdev-riot/RIOT/pkg/cmsis/ 
/home/chris/flashdev-riot/RIOT/pkg/pkg.mk:54: /.git: export GIT_CACHE_DIR?=
"make" -C /home/chris/flashdev-riot/RIOT/boards/common/init
"make" -C /home/chris/flashdev-riot/RIOT/boards/nrf52840dk
...
/home/chris/flashdev-riot/RIOT/dist/tools/jlink/jlink.sh flash /home/chris/flashdev-riot/RIOT/examples/default/bin/nrf52840dk/default.elf
### Flashing Target ###
### Flashing elf file ###
Error: No target device defined in JLINK_DEVICE env var
make: *** [/home/chris/flashdev-riot/RIOT/examples/default/../../Makefile.include:845: flash] Fehler 1

To get to the bottom of this, I replaced the flash% target in the jlink.inc.mk file for all targets, just like for JLINK_DEVICE:

# Export JLINK_POST_FLASH to flash targets only if not empty
ifneq (,$(JLINK_POST_FLASH))
  $(warning $(JLINK_POST_FLASH))
  #$(call target-export-variables,flash%,JLINK_POST_FLASH)
  $(call target-export-variables,JLINK_TARGETS,JLINK_POST_FLASH)
endif

And now it works.
I began testing the elements from the list and the one that made it work was reset. I'm not really sure why.

From what I could grasp from the Make documentation, the "flash%" is a pattern which matches with any non-empty substring. So a match would be "flash-only", but "flash" itself would not. When I change flash% to flash, it does indeed work.

To test the theory, I reverted the target back to flash% and tried out BOARD=nrf52840dk make flash-only and indeed, it worked. And now the JLINK_PRE_FLASH variable is included into the burn.seg file as well, so the export of that now works as well (not surprising because it has the same mechanism).

So... where does this leave us now? We could change flash% to flash flash% in the ifneq statements and in the JLINK_TARGETS list, which would make things work as they should.
But the intricacies of the Make system are a bit too far beyond my expertise to be honest 😅

@cladmi
Copy link
Contributor

cladmi commented Jul 6, 2024

From what I could grasp from the Make documentation, the "flash%" is a pattern which matches with any non-empty substring. So a match would be "flash-only", but "flash" itself would not. When I change flash% to flash, it does indeed work.

From https://www.gnu.org/software/make/manual/make.html#Pattern-Rules seems indeed the case.

I may have overlooked this at that time, so would be the one to blame for introducing this pattern, I do not remember. Let's say it's my fault and would be easier :D
Could also have worked differently because of different make versions but just need to make it work now.

Changing flash% to flash flash% seems like a good fix.

Either as a quickfix only here first if that unblocks you and plan to fix the rest later, or directly as a longer plan to fix all files as other targets may also be affected.
Other similar targets like debug% may also need it (do not remember the target names there).

Could even get some kind of FLASH_TARGETS common definition that would define flash flash% explaining why in a comment before and so not have to hard-code and repeat it everywhere. (they may even already exist as I see LINK_TARGETS).

Not sure who is maintaining these at the moment, I am long gone.


Out of scope:

Noting that target-export-variables could have a target-export-non-empty-variables separate function removing the need for the ifneq.

-ifneq (,$(JLINK_RESET_FILE))
-  # Export JLINK_RESET_FILE to required targets if not empty
-  $(call target-export-variables,$(JLINK_TARGETS),JLINK_RESET_FILE)
-endif
+$(call target-export-non-empty-variables,$(LINK_TARGETS),JLINK_RESET_FILE)

@crasbe
Copy link
Contributor Author

crasbe commented Jul 6, 2024

Changing flash% to flash flash% seems like a good fix.

Either as a quickfix only here first if that unblocks you and plan to fix the rest later, or directly as a longer plan to fix all files as other targets may also be affected. Other similar targets like debug% may also need it (do not remember the target names there).

At the moment I prefer the quick fix, because this is already the third tangent I'm on. Originally I just wanted to make the nRF52 documentation a bit better, then I stumbled upon this reset thing (well I knew about it before, but that's part of the documentation) and this Makefile issue is now required to fix the reset issue.

Could even get some kind of FLASH_TARGETS common definition that would define flash flash% explaining why in a comment before and so not have to hard-code and repeat it everywhere. (they may even already exist as I see LINK_TARGETS).

The makefiles/tools/openocd.inc.mk file is interesting in that regard because it seems to have every variant:

  1. The TARGETS list, that the jlink.inc.mk file has as well:
OPENOCD_TARGETS = debug% flash% reset

# Export GDB_PORT_CORE_OFFSET to required targets
$(call target-export-variables,$(OPENOCD_TARGETS),GDB_PORT_CORE_OFFSET)
  1. Explicitly exporting stuff for individual targets:
ifneq (,$(OPENOCD_CMD_RESET_HALT))
  # Export OPENOCD_CMD_RESET_HALT only to the flash targets
  $(call target-export-variables,flash,OPENOCD_CMD_RESET_HALT)
  $(call target-export-variables,flash-only,OPENOCD_CMD_RESET_HALT)
endif
  1. A list with flash targets:
OPENOCD_FLASH_TARGETS = flash flash-only flashr

ifneq (,$(IMAGE_OFFSET))
  # Export IMAGE_OFFSET only to the flash/flash-only target
  $(call target-export-variables,$(OPENOCD_FLASH_TARGETS),IMAGE_OFFSET)
endif

Not sure who is maintaining these at the moment, I am long gone.
I don't know that either, but I am glad you're taking some time to look at this. Your hints have been invaluable in getting to the bottom of this.

Out of scope:

Noting that target-export-variables could have a target-export-non-empty-variables separate function removing the need for the ifneq.

-ifneq (,$(JLINK_RESET_FILE))
-  # Export JLINK_RESET_FILE to required targets if not empty
-  $(call target-export-variables,$(JLINK_TARGETS),JLINK_RESET_FILE)
-endif
+$(call target-export-non-empty-variables,$(LINK_TARGETS),JLINK_RESET_FILE)

This is something that especially the OpenOCD script would benefit from, but I don't feel comfortable making so many changes that are somewhat hard to test (OpenOCD is used by much more platforms than I can test) and it is beyond what I originally tried to achieve 😅


For the time being, I will create a PR with the quick fix for jlink.inc.mk and maybe create an issue that sums up our findings and give a direction for a future, more general fix if someone wants to tackle that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants