Open crasbe opened 3 months ago
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.
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.
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 :sweat_smile:
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.
@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-R407Not 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 thetarget-export-variables
injlink.inc.mk
forflash%
or whatever your target forpinreset
is. Defining the variable inMakefile.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 :sweat_smile:
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)
Changing
flash%
toflash 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 defineflash 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 seeLINK_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)
2) 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
3) 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 atarget-export-non-empty-variables
separate function removing the need for theifneq
.-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 :sweat_smile:
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.
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:
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.On the left side, the UICR is shown before enabling the pinreset and on the right side it is shown after the pinreset.
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 callnrfjprog --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 aJLINK_POST_FLASH
define for dist/tools/jlink.sh, that writes the right values into the UICR. So far nobody seems to useJLINK_POST_FLASH
for anything. 2) Activate the pinreset withnrfjprog
. 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 addJLINK_POST_FLASH
to the command like this:We can check the burn file that is created by makefiles/tools/jlink.inc.mk has the right post-flash operation:
HOWEVER when I add
JLINK_POST_FLASH = Write4 0x10001200 00000012 00000012
to the boards/nrf52840dk/Makefile.include and just callBOARD=nrf52840dk make flash term
, it does not work. What's odd though is that it does work when I callJLINK_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 addingJLINK_PRE_FLASH=''
or similar to the command line, ther
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