nrf52: use cortexm.ld script when applicable by bergzand · Pull Request #11127 · RIOT-OS/RIOT

bergzand · 2019-03-07T14:20:30Z

Contribution description

Split off from #11126, this reworks the nrf52840 and nrf52832 makefiles to use the cortexm.ld script when the nordic_softdevice_ble is not included in the build. The common linker script is not used when the nordic_softdevice_ble is
included

the nrf52832xxaa.ld included a boot_offset in the calculation, not sure if it was used or not.

Testing procedure

Builds with the nrf52840 and nrf52832 should still work.

Issues/PRs references

Required for #11126

cladmi · 2019-03-07T15:29:11Z

It looks like the boot_offset was for introduced for multislot.ld introduced by #7209

So need to adapt makefiles/mcuboot.mk I guess. I will verify and if needed to make the file working with this PR or both maybe.

cladmi · 2019-03-07T15:34:15Z

Indeed tests/mcuboot is currently broken with this. I will look for it.

bergzand · 2019-03-07T15:34:38Z

Indeed tests/mcuboot is currently broken with this. I will look for it.

Thanks!

cladmi · 2019-03-07T15:42:00Z

Testing command: have make term open for nrf52dk and run BOARD=nrf52dk make -C tests/mcuboot/ clean mcuboot-flash it will print

2019-03-07 16:41:08,983 - INFO # Hello MCUBoot!
2019-03-07 16:41:08,987 - INFO # You are running RIOT on a(n) nrf52dk board.
2019-03-07 16:41:08,990 - INFO # This board features a(n) nrf52 MCU.
2019-03-07 16:41:08,992 - INFO # The startup address is: 0x8200

You would need to add this before changing the linker to have both supported:

diff --git a/makefiles/mcuboot.mk b/makefiles/mcuboot.mk
index 09987798c..a20cc686b 100644
--- a/makefiles/mcuboot.mk
+++ b/makefiles/mcuboot.mk
@@ -22,6 +22,7 @@ $(MCUBOOT_KEYFILE):
        $(Q)$(IMGTOOL) keygen -k $@ -t rsa-2048
 endif

+mcuboot: ROM_OFFSET=$$(($(MCUBOOT_SLOT0_SIZE) + $(IMAGE_HDR_SIZE)))
 mcuboot: mcuboot-create-key link
        @$(COLOR_ECHO)
        @$(COLOR_ECHO) '$(COLOR_PURPLE)Re-linking for MCUBoot at $(MCUBOOT_SLOT0_SIZE)...$(COLOR_RESET)'

And then after changing the linker script you can remove these lines:

diff --git a/makefiles/mcuboot.mk b/makefiles/mcuboot.mk
index 09987798c..86bf9b092 100644
--- a/makefiles/mcuboot.mk
+++ b/makefiles/mcuboot.mk
@@ -26,9 +26,7 @@ mcuboot: mcuboot-create-key link
        @$(COLOR_ECHO)
        @$(COLOR_ECHO) '$(COLOR_PURPLE)Re-linking for MCUBoot at $(MCUBOOT_SLOT0_SIZE)...$(COLOR_RESET)'
        @$(COLOR_ECHO)
-       $(Q)$(_LINK) $(LINKFLAGPREFIX)--defsym=offset="$$(($(MCUBOOT_SLOT0_SIZE) + $(IMAGE_HDR_SIZE)))" \
-       $(LINKFLAGPREFIX)--defsym=length="$$(($(MCUBOOT_SLOT1_SIZE) - $(IMAGE_HDR_SIZE)))" \
-       $(LINKFLAGPREFIX)--defsym=image_header="$(IMAGE_HDR_SIZE)" -o $(ELFFILE) && \
+       $(Q)$(_LINK) -o $(ELFFILE) && \
        $(OBJCOPY) $(OFLAGS) -Obinary $(ELFFILE) $(BINFILE) && \
        $(IMGTOOL) sign --key $(MCUBOOT_KEYFILE) --version $(IMAGE_VERSION) --align \
        $(MCUBOOT_IMAGE_ALIGN) -H $(IMAGE_HDR_SIZE) $(BINFILE) $(SIGN_BINFILE)

You can also remove cpu/cortexm_common/ldscripts/multislot.ld when you remove cpu/nrf52/ldscripts/nrf52832xxaa.ld.

cladmi · 2019-03-07T15:45:19Z

I can push them to your branch if you want.

bergzand · 2019-03-07T15:46:41Z

I can push them to your branch if you want.

Please do so, I think you should know by now how skilled I am with makefiles :-)

bergzand · 2019-03-07T15:47:08Z

Although I should be able to manage this much, anyway feel free to push :-)

cladmi · 2019-03-07T15:55:18Z

I checked just in case, it cannot currently be used for the softdevice case as it would require to configure _ram_offset in the same way as _rom_offset with also the _ram_length handling. So would be for another pull request :)

cladmi · 2019-03-07T15:56:46Z

I will run the whole test suite with this configuration on nrf52dk.

cladmi · 2019-03-07T16:35:04Z

The test suit have the same failure as master:

Failures during test:
- [tests/gnrc_ipv6_ext](tests/gnrc_ipv6_ext/test.failed)
- [tests/gnrc_rpl_srh](tests/gnrc_rpl_srh/test.failed)
- [tests/pkg_fatfs_vfs](tests/pkg_fatfs_vfs/test.failed)

examples/default and such do not work but it is also the case in master #11091

cladmi · 2019-03-11T15:45:26Z

I tested for the nrf52dk and the resulting binary is the same by comparing to the master (origin) commit:

cd examples/hello-world
RIOT_CI_BUILD=1 BOARD=nrf52dk make all clean-intermediates; mv bin bin_master

sha1sum bin_master/nrf52dk/* bin_pr/nrf52dk/* | sort
403673ef4e0e21f307387297f38afce4b8a94207  bin_pr/nrf52dk/hello-world.elf
4f3119f27fe3b694a3618effe55d48f1967b71dc  bin_master/nrf52dk/hello-world.elf
73f21fd1f29af794a440c0cc4656215cd606ca6f  bin_master/nrf52dk/hello-world.bin
73f21fd1f29af794a440c0cc4656215cd606ca6f  bin_pr/nrf52dk/hello-world.bin
792c2c40097879103ec2cdcf6f382fa777d1c5a2  bin_master/nrf52dk/hello-world.map
adcf866733ccbe766209576636dca491508183a5  bin_pr/nrf52dk/hello-world.map

diff -r bin_master/ bin_pr/                                                                                                                                                                                                                                                           
Binary files bin_master/nrf52dk/hello-world.elf and bin_pr/nrf52dk/hello-world.elf differ                                                                                                                                                                                                 
diff -r bin_master/nrf52dk/hello-world.map bin_pr/nrf52dk/hello-world.map
2605a2606,2609
>                 0x0000000000000000                _rom_start_addr = 0x0
>                 0x0000000020000000                _ram_start_addr = 0x20000000
>                 0x0000000000080000                _rom_length = 0x80000
>                 0x0000000000010000                _ram_length = 0x10000
2616,2618c2620,2622
<                 0x0000000000080000                rom_length = 0x80000
<                 0x0000000000000000                boot_offset = DEFINED (offset)?offset:0x0
<                 [0x0000000000080000]                rom_length = DEFINED (length)?length:rom_length
---
>                 0x0000000000000000                _rom_offset = DEFINED (_rom_offset)?_rom_offset:0x0
>                 0x0000000000080000                _fw_rom_length = DEFINED (_fw_rom_length)?_fw_rom_length:(_rom_length - _rom_offset)
>                 0x0000000000000001                ASSERT ((_fw_rom_length <= (_rom_length - _rom_offset)), Specified firmware size does not fit in ROM)

Are there other things that need to be checked ?

boards/common/nrf52/Makefile.include

Comments addressed

cladmi · 2019-03-11T17:24:22Z

For me the changes look ok and good on my local tests with the nrf52dk.
Are there other tests to run for this ?

bergzand · 2019-03-12T08:50:07Z

I don't have anything specific for this. I would assume that if the binaries still work it should be okay. @kYc0o Do you have any suggestions for testing this?

cladmi · 2019-03-12T11:25:01Z

I will check all examples with nrf52 based boards.

kaspar030 · 2019-03-12T12:07:36Z

I will check all examples with nrf52 based boards.

CI should do most if the flag is enabled, for nrf52dk.

kaspar030 · 2019-03-12T12:13:17Z

CI should do most if the flag is enabled, for nrf52dk.

sorry, no, not the examples...

kYc0o · 2019-03-12T12:26:46Z

I don't have anything specific for this. I would assume that if the binaries still work it should be okay. @kYc0o Do you have any suggestions for testing this?

It looks ok for me and if the test tests/cortexm_common_ldscript/ succeeds everything should be ok to merge it.

cladmi · 2019-03-12T13:32:39Z

I meant checking the .bin sha1 for examples.
I just want to add other tests than just saying "CI is ok", test/mcuboot would have been broken by trusting the CI only.

And no worry examples/default is correctly still not working with this PR as it is in the base commit.

cladmi · 2019-03-12T14:01:05Z

I compared the BINFILE sha1sum in all supported examples for all the nrf52 boards acd52832 nrf52840-mdk nrf52840dk nrf52dk ruuvitag thingy52 and it is the same as the reference commit.

The BINFILE is also the same in tests/mcuboot for the nrf52dk, the signed file is different but it is because of the randomness of the signature.

cladmi

I agree with the changes and tested. Please squash.

cladmi · 2019-03-12T14:01:57Z

As I also contributed to this PR maybe somebody else should also ~~hack~~ ACK.

bergzand · 2019-03-12T14:09:23Z

hack

Hack and ACK? 😆

kYc0o

Double (H)ACK.

cladmi · 2019-03-12T17:04:32Z

Please squash for the final check.

Prepare mcuboot for changing `nrf52` to use `cortexm.ld`. It sets the offset for linking the image.

The common linker script is not used when the nordic_softdevice_ble is included

Linkerscript for `nrf52dk` has been updated to use `cortexm.ld` so does not need the configuration anymore for the removed `cpu/nrf52/ldscripts/nrf52832xxaa.ld`.

bergzand · 2019-03-13T10:58:45Z

@cladmi Squashed, please verify if I put the commits in the right order :)

cladmi · 2019-03-13T11:53:06Z

The order is good for me.

I re-tested mcuboot-flash and it works BOARD=nrf52dk make -C tests/mcuboot/ mcuboot-flash
Otherwise the rebased version is the same as the other one so just waiting for murdock.

Small non related issue:

For some reason, my board was in a state where I needed to erase the rom to have it working. But after that, it worked properly

# Erase the rom with an interactive application
JLINK_PRE_FLASH=erase BOARD=nrf52dk make -C tests/shell  flash

bergzand · 2019-03-13T14:51:19Z

Seems to be an issue with the nrf52dk board hooked up to the CI here.

bergzand · 2019-03-13T18:29:02Z

All green, who would like to press the big green button‽

kYc0o · 2019-03-13T18:41:01Z

Everything good here -> go!

bergzand added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: cpu Area: CPU/MCU ports labels Mar 7, 2019

bergzand requested review from cladmi, haukepetersen and kYc0o March 7, 2019 14:20

bergzand mentioned this pull request Mar 7, 2019

nrf52: Add riotboot support #11126

Merged

4 tasks

cladmi mentioned this pull request Mar 7, 2019

makefiles/tools/jlink.inc.mk: use FLASHFILE #11130

Merged

3 tasks

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 11, 2019

cladmi previously requested changes Mar 11, 2019

View reviewed changes

boards/common/nrf52/Makefile.include Outdated Show resolved Hide resolved

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 11, 2019

cladmi added this to the Release 2019.04 milestone Mar 11, 2019

cladmi approved these changes Mar 12, 2019

View reviewed changes

kYc0o approved these changes Mar 12, 2019

View reviewed changes

cladmi and others added 3 commits March 13, 2019 11:57

makefiles/mcuboot.mk: prepare for using cortexm.ld

1e1611c

Prepare mcuboot for changing `nrf52` to use `cortexm.ld`. It sets the offset for linking the image.

nrf52: use cortexm.ld script when applicable

3163b8d

The common linker script is not used when the nordic_softdevice_ble is included

makefiles/mcuboot.mk: remove compatibility with old ldscript

a3df00c

Linkerscript for `nrf52dk` has been updated to use `cortexm.ld` so does not need the configuration anymore for the removed `cpu/nrf52/ldscripts/nrf52832xxaa.ld`.

bergzand force-pushed the pr/nrf52/ref_ldscripts branch from 5daeede to a3df00c Compare March 13, 2019 10:58

kYc0o merged commit de72073 into RIOT-OS:master Mar 13, 2019

Conversation

bergzand commented Mar 7, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

bergzand commented Mar 7, 2019

Uh oh!

cladmi commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

bergzand commented Mar 7, 2019

Uh oh!

bergzand commented Mar 7, 2019

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

cladmi commented Mar 7, 2019

Uh oh!

cladmi commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cladmi commented Mar 11, 2019

Uh oh!

bergzand commented Mar 12, 2019

Uh oh!

cladmi commented Mar 12, 2019

Uh oh!

kaspar030 commented Mar 12, 2019

Uh oh!

kaspar030 commented Mar 12, 2019

Uh oh!

kYc0o commented Mar 12, 2019

Uh oh!

cladmi commented Mar 12, 2019

Uh oh!

cladmi commented Mar 12, 2019

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergzand commented Mar 12, 2019

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 12, 2019

Uh oh!

bergzand commented Mar 13, 2019

Uh oh!

cladmi commented Mar 13, 2019

Small non related issue:

Uh oh!

bergzand commented Mar 13, 2019

Uh oh!

bergzand commented Mar 13, 2019

Uh oh!

kYc0o commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi commented Mar 7, 2019 •

edited

Loading

cladmi commented Mar 11, 2019 •

edited

Loading

cladmi commented Mar 12, 2019 •

edited

Loading