kinetis: Support all models in one directory by jnohlgard · Pull Request #7882 · RIOT-OS/RIOT

jnohlgard · 2017-10-26T23:42:28Z

The purpose of this PR is to reduce code duplication, and reduce the effort required to add support for new boards using different NXP Kinetis CPUs.

Parts of the header files are script generated, which will make it easier to add new CPUs using only the vendor header.

Based on ~~#7878~~ ~~#7761~~ ~~#7362~~

haukepetersen · 2017-10-27T08:15:32Z

Nice one! Exceptional diff -> +29,098 −51,264! Will look into the base PRs first, but looking forward to get this merged!

smlng · 2017-10-27T09:54:23Z

I agree, really nice work @gebart!

Do you use newer vendor headers, because I saw MK22F25612.h is remove with some >12K lines and reintroduces with >8K lines, while others stayed the same (e.g. MK22F51212.h) which git correctly identified to be moved.

smlng · 2017-10-27T09:59:22Z

tried to flash gnrc_networking onto PhyNode (pba-d-01-kw2x) and got an error for target make flash:

make: *** No rule to make target `/Volumes/devel/github/smlng/RIOT/cpu/kinetis_common/dist/wdog-disable.s', needed by `/Volumes/devel/github/smlng/RIOT/cpu/kinetis_common/dist/wdog-disable.bin'.  Stop.

smlng · 2017-10-27T10:08:26Z

I commented the watchdog part in cpu/kinetis/Makefile.include and also fixed PRE_FLASH_CHECK_SCRIPT in boards/pba-d-01-kwx2/Makefile.include to point to kinetis instead of kinetis_common. Now it works.

jnohlgard · 2017-10-27T14:04:41Z

I will update this PR when #7878 is merged.

jnohlgard · 2017-10-31T12:38:40Z

rebased, waiting for #7362

jnohlgard · 2017-11-08T16:03:33Z

Deps have been merged. Ready for review

smlng

Looks good to me, tested successfully with PhyNode (pba-d-01-kw2x).

Just one question, see comment - applies to all usages of $(CPU).

smlng · 2017-11-08T19:47:24Z

boards/frdm-k22f/Makefile.dep

 endif

-include $(RIOTCPU)/k22f/Makefile.dep
+include $(RIOTCPU)/$(CPU)/Makefile.dep


why use $(CPU) here instead of kinetis as in all other places, e.g., Makefile.features

I mean, why not use $(CPU) when it is available? The reason for hard coding the name in Makefile.features is that it is loaded before Makefile.include in the board directory, so CPU has not been assigned a value yet.

smlng · 2017-11-08T19:53:25Z

please fix Murdock warnings and squash. Some more test reports with other boards would be great, too.

smlng

IMHO moving xtimer config from board to cpu is not a good idea and violates RIOTs common config scheme, hence I'd say this should remain with the board.

jnohlgard · 2017-11-09T07:03:07Z

@smlng you are right, let's have that discussion in a separate PR.

kaspar030 · 2017-11-09T08:34:12Z

cpu/kinetis/include/cpu_conf.h

+#endif /* defined(KINETIS_SERIES_x) */
+
+#ifndef MCU_MEM_MAP_VERSION
+#error Missing vendor header for the chosen CPU_MODEL


Maybe make this a simple #else fallback error case above?

I used this to avoid having to repeat the same else condition in each of the cpu_conf_kinetis_x.h files. The macro MCU_MEM_MAP_VERSION is defined for all Kinetis vendor memory map headers regardless of family or series.

kaspar030 · 2017-11-09T08:35:27Z

cpu/kinetis/kinetis-info.mk

@@ -0,0 +1,146 @@
+# Split the part number into qualification, family, subfamily, core, memory, temperature, package, speed
+KINETIS_INFO := $(shell printf '%s' '$(CPU_MODEL)' | tr 'a-z' 'A-Z' |sed -E -e 's/^(M|K|S9)K([ELMSVW]?|EA)([0-9]?)([0-9]?)([A-Z])([NX]?)([0-9][0-9M]?[0-9]?)([ABZ]?)(.*)$$/\1 \2 \3 \4 \5 \6 \7 \8:\9/' -e 's/^([^ ]*)  /\1 K /' -e 's/^([^:]*):([CMV])(..)([0-9]*).*$$/\1 \2 \3 \4/' -e 's/  / _ /g' -e 's/  / _ /g')


smlng

some minor stuff

smlng · 2017-11-09T08:21:21Z

cpu/kinetis/include/cpu_conf_kinetis_l.h

 #endif

-#endif /* CPU_CONF_H */
+#endif /* CPU_CONF_KINETIS_K_H */


copy-paste: /CPU_CONF_KINETIS_K_H/CPU_CONF_KINETIS_L_H/

smlng · 2017-11-09T08:22:01Z

cpu/kinetis/include/cpu_conf_kinetis_m.h

+}
+#endif
+
+#endif /* CPU_CONF_KINETIS_K_H */


dito, should be CPU_CONF_KINETIS_M_H

smlng · 2017-11-09T08:22:22Z

cpu/kinetis/include/cpu_conf_kinetis_v.h

+}
+#endif
+
+#endif /* CPU_CONF_KINETIS_K_H */


and ... you know the drill 😉

smlng · 2017-11-09T08:22:32Z

cpu/kinetis/include/cpu_conf_kinetis_w.h

+}
+#endif
+
+#endif /* CPU_CONF_KINETIS_K_H */


another one

smlng · 2017-11-09T08:33:21Z

cpu/kinetis/kinetis-info.mk

@@ -0,0 +1,146 @@
+# Split the part number into qualification, family, subfamily, core, memory, temperature, package, speed
+KINETIS_INFO := $(shell printf '%s' '$(CPU_MODEL)' | tr 'a-z' 'A-Z' |sed -E -e 's/^(M|K|S9)K([ELMSVW]?|EA)([0-9]?)([0-9]?)([A-Z])([NX]?)([0-9][0-9M]?[0-9]?)([ABZ]?)(.*)$$/\1 \2 \3 \4 \5 \6 \7 \8:\9/' -e 's/^([^ ]*)  /\1 K /' -e 's/^([^:]*):([CMV])(..)([0-9]*).*$$/\1 \2 \3 \4/' -e 's/  / _ /g' -e 's/  / _ /g')


wow, nice but rather long one-line command. First thought: missing space between | sed, second: maybe put this into a script in cpu/kinetis/dist/? The latter is a non-blocking suggestion 😄

fixed the space. I would still prefer to keep this inline inside the makefile. It's only a oneliner and it is easier to have everything in one place, since all the parsing of that processed line also happen in kinetis-info.mk

smlng · 2017-11-09T08:34:40Z

cpu/kinetis/kinetis-info.mk

+  $(info Max speed:         $(KINETIS_SPEED))
+  $(info Temperature range: $(KINETIS_MASKREV))
+  $(info Revision:          $(KINETIS_MASKREV))
+  $(info Package code:      $(KINETIS_PACKAGE))


btw output for PhyNode pba-d-01-kw2x is

Kinetis CPU info: M W 2 1 D _ 256 _ V HA 5 Qualification: M Core: D Series: W Family: 2 Subfamily: 1 ROM size: 256 RAM size: 32 (256/8) SRAM_L: 16 (256/8/2) Max speed: 5 Temperature range: _ Revision: _ Package code: HA

Is qualification M correct, would've thought it should be K, because series and family do match

Qualification is M for mainstream parts, P for prerelease/preliminary, and some others for different certifications, for example the automotive parts have some other code there.

btw, fixed the Temperature range debug print. It is supposed to be "V" for the mkw21d256vha5 (V is -40 -- +105 C)

smlng · 2017-11-09T09:32:08Z

cpu/kinetis/ldscripts/kinetis.ld

+{
+    vectors (rx)   : ORIGIN = _rom_start_addr,          LENGTH = 0x400
+    flashsec (rx)  : ORIGIN = _rom_start_addr + 0x400,  LENGTH = 0x10
+    rom (rx)       : ORIGIN = _rom_start_addr + 0x410,  LENGTH = _rom_length - 0x410


wouldn't it be better/nicer to use variables and expressions (+) here instead of repeating magic numbers, i.e., to ease changes. Something like _vectors_len = 0x400, _flashsec_len = 0x10, and _rom_offset = _vectors_len + _flashsec_len.

good suggestion, will update

jnohlgard · 2017-11-09T12:32:48Z

While we are cleaning up the Kinetis directory, I removed the xxx_NUMOF guards in the periph drivers according to #7981

smlng · 2017-11-09T15:36:37Z

tested again with PhyNode, pba-d-01-kw2x -> still good.

smlng · 2017-11-09T19:07:26Z

please squash reasonably

… module

jnohlgard · 2017-11-10T09:44:52Z

@smlng squashed

smlng · 2017-11-10T13:42:18Z

PhyNode still works, ACK & GO!

jnohlgard · 2017-11-10T14:09:46Z

@smlng @kaspar030 @haukepetersen Thanks for a very good review effort 🎉

jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Oct 26, 2017

jnohlgard assigned haukepetersen Oct 26, 2017

jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 28, 2017

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from 8c03723 to 4895dcb Compare October 28, 2017 14:49

jnohlgard mentioned this pull request Oct 30, 2017

cpu/cortex-m variants: Remove _estack declarations #7908

Merged

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from bb1e7a5 to ec61890 Compare October 31, 2017 12:34

jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 31, 2017

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch 2 times, most recently from c1390d6 to c487373 Compare November 8, 2017 15:29

jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 8, 2017

smlng approved these changes Nov 8, 2017

View reviewed changes

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from 5807575 to 9ed62ec Compare November 8, 2017 21:50

smlng suggested changes Nov 8, 2017

View reviewed changes

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from 9ed62ec to 6c22fd9 Compare November 9, 2017 07:02

kaspar030 reviewed Nov 9, 2017

View reviewed changes

smlng suggested changes Nov 9, 2017

View reviewed changes

haukepetersen mentioned this pull request Nov 9, 2017

periph: remove file guards from peripheral drivers #7981

Closed

46 tasks

This was referenced Nov 9, 2017

cpu+board: Add support for NXP Kinetis KW41Z and FRDM-KW41Z development board #6995

Merged

kinetis: Implement low power modes #7897

Closed

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from 6ef755a to b73eb67 Compare November 9, 2017 13:54

smlng approved these changes Nov 9, 2017

View reviewed changes

jnohlgard mentioned this pull request Nov 10, 2017

make: use -isystem for CPU vendor header includes #7993

Closed

Joakim Nohlgård added 6 commits November 10, 2017 10:39

kinetis: Extract detailed information from CPU_MODEL

0dabad2

kinetis: Use one ldscript for all CPUs

cb8e3f1

kinetis: Unify all Kinetis CPUs in one directory

22c52bd

kinetis: Add script for generating vendor header include lines

19d407a

kinetis: Update RNGA driver configuration to automatically detect the…

925a908

… module

kinetis: Remove periph driver xxx_NUMOF guards

33e751e

jnohlgard force-pushed the pr/kinetis-one-cpu-dir branch from b73eb67 to 33e751e Compare November 10, 2017 09:43

smlng merged commit 8f4b21b into RIOT-OS:master Nov 10, 2017

jnohlgard deleted the pr/kinetis-one-cpu-dir branch November 10, 2017 14:53

smlng mentioned this pull request Dec 11, 2017

kinetis: linker fail to link on Raspbian/Debian Jessie #8238

Closed

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

		@@ -0,0 +1,146 @@
		# Split the part number into qualification, family, subfamily, core, memory, temperature, package, speed
		KINETIS_INFO := $(shell printf '%s' '$(CPU_MODEL)' \| tr 'a-z' 'A-Z' \|sed -E -e 's/^(M\|K\|S9)K([ELMSVW]?\|EA)([0-9]?)([0-9]?)([A-Z])([NX]?)([0-9][0-9M]?[0-9]?)([ABZ]?)(.)$$/\1 \2 \3 \4 \5 \6 \7 \8:\9/' -e 's/^([^ ]) /\1 K /' -e 's/^([^:]):([CMV])(..)([0-9]).*$$/\1 \2 \3 \4/' -e 's/ / _ /g' -e 's/ / _ /g')

Conversation

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

Uh oh!

Uh oh!

haukepetersen commented Oct 27, 2017

Uh oh!

smlng commented Oct 27, 2017

Uh oh!

smlng commented Oct 27, 2017

Uh oh!

smlng commented Oct 27, 2017

Uh oh!

jnohlgard commented Oct 27, 2017

Uh oh!

jnohlgard commented Oct 31, 2017

Uh oh!

jnohlgard commented Nov 8, 2017

Uh oh!

smlng 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!

smlng commented Nov 8, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Nov 9, 2017

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!

smlng 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!

jnohlgard commented Nov 9, 2017

Uh oh!

smlng commented Nov 9, 2017

Uh oh!

smlng commented Nov 9, 2017

jnohlgard commented Oct 26, 2017 •

edited

Loading