[WIP] Factorise stm32 and sam0 linker scripts #7729

kYc0o · 2017-10-13T01:54:04Z

This PR aims to remove all linker scripts for the stm32 and sam0 CPU families, by replacing them by a common linker script which accept ROM and RAM size variables. It also sets the CPU_ID and ccmram (but not used in main cortex_common.ld) for the stm32 family.

Besides removing all redundant linker scripts, this PR also allows to set an offset length at the beginning of the ROM, which is needed for some boards with embedded bootloaders. It also allows to skip a given ROM area for custom bootloaders.

I did some tests for some boards, but I think a wide testing might be required for the affected boards, which are quite a few.

Here there is a list which should be completed if possible before merging this PR.
Once all or most of the boards are tested, I'll add a last commit which removes all the unnecessary linker scripts. Any help is very much appreciated!

I'd like to merge first this one before #7727 and #7725, and rather ask the authors to adapt for this PR.

kYc0o · 2017-10-13T02:17:34Z

Well I pushed the removal of all linker scripts and let Murdock take a look. He agrees so there's no harm on removing them.

Thus, only testing is left.

jnohlgard

I like the idea of reducing the number of scripts, but I am a bit sceptical to extending the board makefiles on every board like this. This complicates the makefile creation for new board ports. The boot loader offset is fine in a board makefile but the cpu properties for rom and ram length should be more centralized. Is there any way to deduce the rom and ram sizes from the model number?
I started working on a similar thing for kinetis but I have not had time to finish it. Those CPUs have the rom size as part of the model number.

jnohlgard · 2017-10-13T04:49:56Z

cpu/stm32_common/ldscripts/stm32_common.ld

+
+MEMORY
+{
+    rom (rx)    : ORIGIN = 0x08000000 + boot_offset, LENGTH = rom_length


If you subtract boot offset from the length you will have one less source of mistakes in the makefiles.

jnohlgard · 2017-10-13T04:50:58Z

cpu/sam0_common/ldscripts/sam0_common.ld

 MEMORY
 {
-    rom (rx)    : ORIGIN = 0x00002000, LENGTH = 256K-0x2000
-    ram (rwx)   : ORIGIN = 0x20000000, LENGTH = 32K
+    rom (rx)    : ORIGIN = 0x00000000 + boot_offset, LENGTH = rom_length


If you subtract boot offset from the length you will have one less source of mistakes in the makefiles.

Yes! I thought about it but wanted to make it explicit... maye is not necessary so I'll change with your suggestion.

haukepetersen

Very nice effort, I like it! But please do it in a clean way -> use the logical CPU structure that is there instead of starting with ugly else-if blocks!

haukepetersen · 2017-10-13T08:12:17Z

makefiles/arch/cortexm.inc.mk

@@ -16,7 +16,16 @@ export CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)

 export ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
 export LINKFLAGS += -L$(RIOTCPU)/$(CPU)/ldscripts -L$(RIOTCPU)/cortexm_common/ldscripts
-export LINKER_SCRIPT ?= $(CPU_MODEL).ld
+
+ifneq (,$(filter stm32%,$(CPU_FAM)))


this seems wrong. There should be no multiplexing for different families in the shared cortex code tree. Instead, this should be handled by the families makefiles!

To be more precise: leave export LINKER_SCRIPT ?= $(CPU_MODEL).ld in this file. But move the other two options to the corresponding cpu family makefile.includes. As we use ?= for assignment here, and the family makefiles are passed before this one, the more specific values prevail.

Yes! I prefer your suggestion, I was looking where to put it but didn't think about ?= (it always happen...). Will fix.

haukepetersen · 2017-10-13T08:16:06Z

boards/arduino-mkr-common/Makefile.include

@@ -17,7 +21,7 @@ ifeq ($(PROGRAMMER),jlink)
 else
  # by default, we use BOSSA to flash this board and take into account the
  # preinstalled Arduino bootloader.
-  export LINKER_SCRIPT ?= $(RIOTCPU)/sam0_common/ldscripts/$(CPU_MODEL)_arduino_bootloader.ld
+  #export LINKER_SCRIPT ?= $(RIOTCPU)/sam0_common/ldscripts/$(CPU_MODEL)_arduino_bootloader.ld


what is this about? Just commenting out seems not the right way to proceed here?!

Oops, sorry, I commented it out first to test and forgot to remove it, since actually the file doesn't exist anymore in this PR.

haukepetersen · 2017-10-13T08:24:15Z

Two more comments:
a) any reason not to split this PR, one for each CPU family that is adapted?
b) I think we can carry this a bit further: I will propose a fix removing the cpuid from the stm32's linkerscripts -> so there is no need to have them separately anymore.

haukepetersen · 2017-10-13T08:45:36Z

done, see #7731

kYc0o · 2017-10-13T10:22:19Z

Is there any way to deduce the rom and ram sizes from the model number?

Unfortunately not, for some models the trailing letters mean the memory sizes but for some other models the letters are completely different.

I agree it's a bit of overhead setting so many variables (two, after #7731 gets merged), and indeed maybe not in the right place, except for the offset.

What I suggest is maybe still use the CPU_MODEL variable to evaluate the model and set the sizes accordingly, at cpu level. However this will lead to several ifneq().

aabadie

Found some remaining commented lines that could be removed.

Just one remark: would it be possible to just introduce news variables like RAM_LENGTH, ROM_LENGTH, ROM_OFFSET etc in the board Makefile and use them in makefiles/arch/cortexm.inc.mk ?

LINKFLAGS += $(LINKFLAGPREFIX)--defsym=offset=$(ROM_OFFSET)
LINKFLAGS += $(LINKFLAGPREFIX)--defsym=rom_length=$(ROM_LENGTH)
LINKFLAGS += $(LINKFLAGPREFIX)--defsym=ram_length=$(RAM_LENGTH)

aabadie · 2017-10-13T10:30:02Z

boards/opencm904/Makefile.include

@@ -3,7 +3,7 @@ export CPU = stm32f1
 export CPU_MODEL = stm32f103cb

 # custom linkerscript
-export LINKER_SCRIPT = stm32f103cb_opencm904.ld
+#export LINKER_SCRIPT = stm32f103cb_opencm904.ld


This needs to be removed as well

aabadie · 2017-10-13T10:30:16Z

boards/spark-core/Makefile.include

@@ -3,7 +3,7 @@ export CPU = stm32f1
 export CPU_MODEL = stm32f103cb

 # the spark-core uses its own custom linkerscript...
-export LINKER_SCRIPT = stm32f103cb_sparkcore.ld
+#export LINKER_SCRIPT = stm32f103cb_sparkcore.ld


jnohlgard · 2017-10-13T10:47:36Z

@aabadie I was thinking the same thing. Also RAM and ROM base addresses could be configurable. This would let us eliminate both the Kinetis and the stm32 ldscripts.
Defaults can be provided for the base addresses by the cortexm_common makefile, while still allowing override from the CPU makefiles.

kYc0o · 2017-10-13T13:44:33Z

I like that too. I will do it and let's see how it looks like.

However I still have the question of where to define those variables, if at the board level or split them up at both board and cpu level with some ifneq.

aabadie

Nice changes!
And nearly done, except the remaining commented out lines and @gebart comment about subtracting offset from rom size.

aabadie · 2017-10-13T17:21:14Z

cpu/sam0_common/Makefile.include

@@ -7,6 +7,14 @@ export CFLAGS += -DDONT_USE_CMSIS_INIT
 # for the sam[drl] CPUs we hold all linkerscripts in the sam0 common folder
 export LINKFLAGS += -L$(RIOTCPU)/sam0_common/ldscripts

+ifdef ROM_OFFSET
+LINKFLAGS += $(LINKFLAGPREFIX)--defsym=offset=$(ROM_OFFSET)


you need 2 spaces indent here

aabadie · 2017-10-13T17:21:32Z

cpu/stm32_common/Makefile.include

@@ -8,5 +8,14 @@ USEMODULE += periph_common
 # include stm32 common functions and stm32 common periph drivers
 USEMODULE += stm32_common stm32_common_periph

+ifdef ROM_OFFSET
+LINKFLAGS += $(LINKFLAGPREFIX)--defsym=offset=$(ROM_OFFSET)


2 spaces indent

aabadie · 2017-10-13T17:24:36Z

boards/arduino-mkr-common/Makefile.include

@@ -6,6 +6,10 @@ export CPU_MODEL = samd21g18a
 export PORT_LINUX ?= /dev/ttyACM0
 export PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))

+export ROM_OFFSET = 0x2000


This should only be defined when using bossa programmer, with jlink programmer the bootloader is overwritten.

Oh yes! I forgot, will change.

kYc0o · 2017-10-13T17:25:34Z

Nice changes!
And nearly done, except the remaining commented out lines and @gebart comment about subtracting offset from rom size.

It's ongoing, I'm pushing just to see if Murdock complains, since I need to be sure all boards still compile without issues.

I'll say explicitly when it's ready again. Thanks for the feedback @aabadie !

kYc0o · 2017-10-13T18:04:20Z

I addressed most of the comments, it remains to move all the defines for ROM and RAM size at the cpu level.

aabadie · 2017-10-13T18:06:32Z

Thinking of it, there's also a README in cpu//sam0_common/ldscripts/ that could be removed by this PR

kYc0o · 2017-10-13T18:10:24Z

Thinking of it, there's also a README in cpu//sam0_common/ldscripts/ that could be removed by this PR

It's removed.

kYc0o · 2017-10-16T14:25:16Z

Well I think it's done, can I squash to start a new clean review? @gebart @haukepetersen @aabadie ?

…mily

…_MODEL

aabadie · 2017-11-16T12:04:20Z

#7739 has been merged and #7799 is close to be merged as well. Looking at the changes I think this PR is now a duplicate.
Do you think we can close ?

kYc0o · 2017-11-16T12:06:44Z

I'd like to close it when #7799 gets merged. Is it OK for you?

aabadie · 2017-11-16T12:11:25Z

Is it OK for you?

More or less, we want to do some clean up in opened PRs, then no.
IIUC, this PR is #7739 + #7799 and #7739 is already merged but if you really think this PR provides other important things that #7799 doesn't, then keep it open (you'll need to rebase though). Do you think, we'll merge it one day ?

kYc0o · 2017-11-16T13:23:41Z

No, I don't think this would be merged one day, I just wanted to leave it open for reference, but I have no problems closing it. Thus, let it be!

aabadie · 2017-11-16T13:24:25Z

Thanks @kYc0o !

kYc0o added this to the Release 2017.10 milestone Oct 13, 2017

kYc0o assigned keestux, haukepetersen, vincent-d, aabadie, lebrush and kaspar030 Oct 13, 2017

kYc0o force-pushed the factorise_stm32_linker_scripts branch from 2b1b89c to ffdd8d5 Compare October 13, 2017 02:05

jnohlgard requested changes Oct 13, 2017

View reviewed changes

haukepetersen requested changes Oct 13, 2017

View reviewed changes

haukepetersen mentioned this pull request Oct 13, 2017

cpu/stm32: optimized definition of CPUID_ADDR #7731

Merged

aabadie requested changes Oct 13, 2017

View reviewed changes

kYc0o force-pushed the factorise_stm32_linker_scripts branch from 960dc26 to 820d665 Compare October 13, 2017 16:52

aabadie requested changes Oct 13, 2017

View reviewed changes

aabadie reviewed Oct 13, 2017

View reviewed changes

kYc0o removed this from the Release 2017.10 milestone Oct 16, 2017

kYc0o added 8 commits October 17, 2017 15:44

cpu/cortexm_common/Makefile.include: define linker vars for all cortexm

25d8023

cpu/cortexm_common/ldscripts: add common linker script for cortexm fa…

8ba5b3b

…mily

cpu/stm32*/Makefile.include: add ROM and RAM lengths according to CPU…

5d0e3b2

…_MODEL

cpu/sam0_common/Makefile.include: add CPU ROM and RAM lengths

02d0e48

cpu/stm32_common/Makefile.include: add CPU ROM and RAM lengths

9f82477

boards: add rom boot offset for boards w/bootloader

3285721

cpu/sam0_commmon/ldscripts: remove all samd/l family linker scripts

6c74b52

cpu/stm32*/ldscripts: remove all stm32 family linker scripts

5a31d9c

kYc0o force-pushed the factorise_stm32_linker_scripts branch from 3e3736e to 5a31d9c Compare October 17, 2017 14:11

kYc0o mentioned this pull request Oct 17, 2017

ld: refactor sam0 ldscripts #7739

Merged

6 tasks

kYc0o removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 17, 2017

kYc0o mentioned this pull request Oct 23, 2017

ld: refactor stm32 linker scripts #7799

Merged

kYc0o closed this Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Factorise stm32 and sam0 linker scripts #7729

[WIP] Factorise stm32 and sam0 linker scripts #7729

kYc0o commented Oct 13, 2017 •

edited

Loading

kYc0o commented Oct 13, 2017 •

edited

Loading

jnohlgard left a comment

jnohlgard Oct 13, 2017

jnohlgard Oct 13, 2017

kYc0o Oct 13, 2017

haukepetersen left a comment

haukepetersen Oct 13, 2017

haukepetersen Oct 13, 2017

kYc0o Oct 13, 2017

haukepetersen Oct 13, 2017

kYc0o Oct 13, 2017

haukepetersen commented Oct 13, 2017

haukepetersen commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie left a comment

aabadie Oct 13, 2017

aabadie Oct 13, 2017

jnohlgard commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie left a comment

aabadie Oct 13, 2017

aabadie Oct 13, 2017

aabadie Oct 13, 2017

kYc0o Oct 13, 2017

kYc0o commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie commented Oct 13, 2017

kYc0o commented Oct 13, 2017

kYc0o commented Oct 16, 2017

aabadie commented Nov 16, 2017

kYc0o commented Nov 16, 2017

aabadie commented Nov 16, 2017

kYc0o commented Nov 16, 2017

aabadie commented Nov 16, 2017

[WIP] Factorise stm32 and sam0 linker scripts #7729

[WIP] Factorise stm32 and sam0 linker scripts #7729

Conversation

kYc0o commented Oct 13, 2017 • edited Loading

kYc0o commented Oct 13, 2017 • edited Loading

jnohlgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Oct 13, 2017

haukepetersen commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnohlgard commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o commented Oct 13, 2017

kYc0o commented Oct 13, 2017

aabadie commented Oct 13, 2017

kYc0o commented Oct 13, 2017

kYc0o commented Oct 16, 2017

aabadie commented Nov 16, 2017

kYc0o commented Nov 16, 2017

aabadie commented Nov 16, 2017

kYc0o commented Nov 16, 2017

aabadie commented Nov 16, 2017

kYc0o commented Oct 13, 2017 •

edited

Loading

kYc0o commented Oct 13, 2017 •

edited

Loading