ld: refactor sam0 ldscripts by kYc0o · Pull Request #7739 · RIOT-OS/RIOT

kYc0o · 2017-10-17T14:28:28Z

This PR is a subset of #7729, I split it to ease review and understand better the concept.

The goal is to get rid of every specific linker script associated to a specific CPU model, and factorise it to allow CPU specific definitions like start address and boot offset.

This would give us flexibility to:

Avoid definition of specific linker scripts on boards with embedded bootloader (in case we don't want to overwrite it),
Define variables for our own bootloader, as well as the address space on which we want to write the code on ROM, and
Remove all linker scripts for the whole Cortex-M family (in subsequent PRs), and replace them by address and length memory definitions.

Boards to be tested:

jnohlgard

This is a good initiative for reducing the maintenance burden.

Some questions about the implementation:

Symbol names, should we use a prefix to avoid name clashes with c code? Sometimes linker scripts use single or double underscores to avoid collisions with variables and functions.
Special segment types. How do we specify special segments, like the CCM ram in stm32, without polluting the generic linker script?

jnohlgard · 2017-10-17T15:52:13Z

boards/arduino-mkr-common/Makefile.include

  # 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 ROM_OFFSET = 0x2000


Do we want this to be unconditionally assigned or do we want ?= instead?

Yes, I agree it's better with the conditional ?=. Will change.

jnohlgard · 2017-10-17T15:52:45Z

boards/opencm904/Makefile.include

 PORT_LINUX ?= /dev/ttyACM0
 PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))

+export ROM_OFFSET = 0x3000


jnohlgard · 2017-10-17T15:55:38Z

cpu/cortexm_common/Makefile.include

 export IMAGE_HDR_SIZE ?= 512
+
+ifneq (,$(ROM_OFFSET))
+  LINKFLAGS += $(LINKFLAGPREFIX)--defsym=rom_offset=$(ROM_OFFSET)


I would actually prefer to just assign this to 0 by default and always pass the linker symbol on the command line.
Fewer conditionals in the makefiles make them easier to read IMO.

jnohlgard · 2017-10-17T15:56:55Z

cpu/cortexm_common/ldscripts/cortexm.ld

+{
+    rom (rx)    : ORIGIN = rom_start_addr + boot_offset, LENGTH = rom_length - boot_offset
+    ram (rwx)   : ORIGIN = ram_start_addr,               LENGTH = ram_length
+    ccmram (rwx): ORIGIN = ccmram_start_addr,            LENGTH = ccmram_length


This is an stm32 specific kind of memory. Is there not another way to get this segment into the linker?

Actually this segment is not used at all in our current cortexm_base.ld

Maybe we can discuss this when we figure out how to use this RAM? I mean to remove it from the MEMORY section but still define it to know which CPUs implement it, and then when we use this segment add it either in this linker script or somewhere else.

jnohlgard · 2017-10-17T16:01:16Z

cpu/sam0_common/Makefile.include

+  export RAM_LEN = 0x8000
+endif
+
+ROM_START_ADDR ?= 0x00000000


This could actually go in the cortex m makefile. These addresses are sensible defaults for any cortex m CPU and can be overridden in the CPU specific makefiles where they differ instead.

Hmmm... As far as I understand the memory spaces are not defined by ARM in an explicit way, but it's up to the silicon vendors to allocate their ROM and RAM to any address.

In RIOT we have, for ROM, at least 3 examples:

cc2538 at 0x00200000

stm32 family at 0x08000000

kinetis, nordic and sam at 0x00000000

For RAM is even more varied:

cc2538 generally at 0x20000000, but for some models at 0x20004000

kinetis, stm32 and sam0 at 0x20000000

NXP at 0x10000000, some at 0x40000000

sam3x8e at 0x20070000

Thus, in my opinion use standard values seem dangerous.

alright, I agree, it's generalized enough in this place.

jnohlgard · 2017-10-17T16:05:11Z

cpu/sam0_common/Makefile.include

 CFLAGS += -DCPU_FAM_$(shell echo $(CPU_FAM) | tr 'a-z-' 'A-Z_')

+# Set ROM and RAM lengths according to CPU model
+ifneq (,$(filter samd21g18a samd21j18a saml21j18a samr21g18a,$(CPU_MODEL)))


This is fine here for now, but you could also try to use a wildcard or parse the size from the model number if you're feeling experimental. I'm guessing the 18 in the model is the rom size? (2**18 = 0x40000 = 256 KB)

You're right. All SAMXxxY18 model have 256KB of flash.

I thought about this but my skills in make made me feel insecure. Besides the fact that I think is better to explicitly state which are the memory capabilities of a given CPU model.

If you really want this I'll ask my "make guru" colleagues how to do it.

no, like I wrote above, this is fine here now. This can be refactored later.

BTW SAML21J18B is missing from the line.

Do we support such mcu?

Yes, SAML21-XPRO board can be equipped with SAML21J18A or SAML21J18B. I also heard Rev C is on the way.
SAML21J18A were only early engineering sample, Atmel does NOT sell them. Only SAML21J18B are sold.

Oh, I didn't find it in the current base code, however it is in the vendor headers. I'll add it.

jnohlgard · 2017-10-17T16:05:31Z

cpu/sam0_common/Makefile.include

+endif
+
+ROM_START_ADDR ?= 0x00000000
+RAM_START_ADDR ?= 0x20000000


Good default for any cortex m

jnohlgard · 2017-10-17T16:10:49Z

cpu/cortexm_common/ldscripts/cortexm.ld

+ * @{
+ *
+ * @file
+ * @brief           Memory definitions for the cortexm family


"Cortex-M family"

jnohlgard · 2017-10-17T16:12:00Z

boards/spark-core/Makefile.include


 export INCLUDES += -I$(RIOTCPU)/$(CPU)/include/ -I$(RIOTBOARD)/$(BOARD)/include/

+export ROM_OFFSET = 0x5000


dylad · 2017-10-17T17:18:46Z

SAML21 has a low-power embedded SRAM in addition to the standard SRAM.
Does the current solution allow an easy way to define it ?

kYc0o · 2017-10-18T13:11:44Z

SAML21 has a low-power embedded SRAM in addition to the standard SRAM.
Does the current solution allow an easy way to define it ?

I think a MEMORY definition either in the generic cortexm.ld script or in one located at the sam0_common/ldscripts folder would allow it.

kYc0o · 2017-10-24T13:35:57Z

Symbol names, should we use a prefix to avoid name clashes with c code? Sometimes linker scripts use single or double underscores to avoid collisions with variables and functions.

I don't have a strong opinion on this, but it seems legit to me. Which one you prefer the most?

Special segment types. How do we specify special segments, like the CCM ram in stm32, without polluting the generic linker script?

I removed this special segment on the generic script. I think it make sense to put the specific regions in a specific linker script for each family (I didn't see a specific CPU implementing weird regions on RAM or ROM, but rather a whole CPU family), as I propose in my last fixup at #7799.

jnohlgard · 2017-10-24T14:02:29Z

@kYc0o I suggest single underscore prefixes since we already have some symbols with that prefix in the cortexm_common ldscript

kYc0o · 2017-10-24T15:32:44Z

Added underscores to variables.

jnohlgard

Looks fine, could you just see if it still works without the export? (see comments)
I realized we are exporting a lot of stuff that we don't need to when we had that problem with Makefile.dep last week..

jnohlgard · 2017-10-31T18:38:43Z

boards/arduino-mkr-common/Makefile.include

  # 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 ROM_OFFSET ?= 0x2000


I don't think it is necessary to export this, could you try without export?

jnohlgard · 2017-10-31T19:11:21Z

cpu/sam0_common/Makefile.include


+# Set ROM and RAM lengths according to CPU model
+ifneq (,$(filter samd21g18a samd21j18a saml21j18a samr21g18a,$(CPU_MODEL)))
+  export ROM_LEN = 0x40000


could you try this without export?

kYc0o · 2017-11-03T14:24:51Z

@gebart fixed. Murdock agrees so without export it works well.

jnohlgard

looks fine now 👍

jnohlgard · 2017-11-04T22:08:56Z

please squash

kaspar030 · 2017-11-07T10:21:02Z

@kYc0o ping please squash

kYc0o · 2017-11-07T10:22:35Z

oops, almost forgot.

kYc0o · 2017-11-07T10:26:40Z

squashed.

kaspar030 · 2017-11-07T10:33:13Z

Need to fix feather-m0.

kYc0o · 2017-11-07T10:34:26Z

Oh it got merged before this one... Will fix.

kaspar030

sorry, some more change requests

kaspar030 · 2017-11-07T10:54:35Z

boards/feather-m0/Makefile.include

@@ -17,7 +17,7 @@ ifeq ($(PROGRAMMER),jlink)
 else
  # by default, we use BOSSA to flash this board to take into account the


the comment needs updating

kaspar030 · 2017-11-07T10:56:06Z

cpu/sam0_common/Makefile.include


-# for the sam[drl] CPUs we hold all linkerscripts in the sam0 common folder
-export LINKFLAGS += -L$(RIOTCPU)/sam0_common/ldscripts
+# For Cortex-M cpu we use the cortexm.ld linker script


reword: "use shared Cortex-M linker script"

or maybe common

kaspar030 · 2017-11-07T10:57:10Z

boards/arduino-mkr-common/Makefile.include

@@ -17,7 +17,7 @@ ifeq ($(PROGRAMMER),jlink)
 else
  # by default, we use BOSSA to flash this board and take into account the


here and below: comment needs updating

I don't think so, BOSSA is the default flash tool (for these boards).

I updated it accordingly.

Ok, I was looking at fixed version but the initial comment is still there ;)

kYc0o · 2017-11-07T13:40:59Z

@kaspar030 I addressed your comments, I suppose I could have squashed directly but forgot it...

…mily

aabadie · 2017-11-07T20:49:50Z

So I guess @kaspar030 just has to ACK and we go

aabadie · 2017-11-08T14:30:08Z

@kaspar030, can you ACK this one ?

aabadie · 2017-11-08T16:48:22Z

pinging again @kaspar030, just in case :)

aabadie · 2017-11-09T15:23:22Z

@kaspar030 can we dismiss your review and merge this one ?

kaspar030

ACK.

kYc0o · 2017-11-13T13:42:19Z

Thanks! the story continues in #7799 ...

kYc0o added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Oct 17, 2017

kYc0o assigned jnohlgard, haukepetersen, aabadie and kaspar030 Oct 17, 2017

kYc0o added 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 force-pushed the factorise_sam0_ldscripts branch 2 times, most recently from 696a9c0 to ada52bc Compare October 17, 2017 15:16

jnohlgard reviewed Oct 17, 2017

View reviewed changes

kYc0o mentioned this pull request Oct 23, 2017

ld: refactor stm32 linker scripts #7799

Merged

kYc0o force-pushed the factorise_sam0_ldscripts branch from 504239e to 4f6352b Compare October 24, 2017 13:32

jnohlgard approved these changes Oct 31, 2017

View reviewed changes

jnohlgard approved these changes Nov 4, 2017

View reviewed changes

kYc0o force-pushed the factorise_sam0_ldscripts branch from 236178b to a889df3 Compare November 7, 2017 10:26

kYc0o force-pushed the factorise_sam0_ldscripts branch from a889df3 to 0815de0 Compare November 7, 2017 10:40

kaspar030 changed the title ~~ld: Factorise sam0 ldscripts~~ ld: refactor sam0 ldscripts Nov 7, 2017

kaspar030 requested changes Nov 7, 2017

View reviewed changes

kYc0o added 5 commits November 7, 2017 15:05

cpu/cortexm_common/Makefile.include: define linker length vars if set

4d681ac

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

d25fd64

…mily

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

4c86d95

boards: add rom boot offset for boards w/bootloader

a7a41ad

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

ec9897f

kYc0o force-pushed the factorise_sam0_ldscripts branch from 5a5ec37 to ec9897f Compare November 7, 2017 14:06

kaspar030 approved these changes Nov 13, 2017

View reviewed changes

kaspar030 merged commit f6d7e54 into RIOT-OS:master Nov 13, 2017

kYc0o deleted the factorise_sam0_ldscripts branch November 13, 2017 13:43

aabadie mentioned this pull request Nov 16, 2017

[WIP] Factorise stm32 and sam0 linker scripts #7729

Closed

50 tasks


		export INCLUDES += -I$(RIOTCPU)/$(CPU)/include/ -I$(RIOTBOARD)/$(BOARD)/include/

		export ROM_OFFSET = 0x5000

		@@ -17,7 +17,7 @@ ifeq ($(PROGRAMMER),jlink)
		else
		# by default, we use BOSSA to flash this board to take into account the

Conversation

kYc0o commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Oct 17, 2017

Uh oh!

kYc0o commented Oct 18, 2017

Uh oh!

kYc0o commented Oct 24, 2017

Uh oh!

jnohlgard commented Oct 24, 2017

Uh oh!

kYc0o commented Oct 24, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Nov 3, 2017

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Nov 4, 2017

Uh oh!

kaspar030 commented Nov 7, 2017

Uh oh!

kYc0o commented Nov 7, 2017

Uh oh!

kYc0o commented Nov 7, 2017

Uh oh!

kaspar030 commented Nov 7, 2017

Uh oh!

kYc0o commented Nov 7, 2017

kYc0o commented Oct 17, 2017 •

edited

Loading