cpu+board frdm-k22f: Add NXP FRDM-K22F development board by jnohlgard · Pull Request #6994 · RIOT-OS/RIOT

jnohlgard · 2017-05-02T09:18:29Z

This PR adds support for the NXP Kinetis K22F CPU and the FRDM-K22F development board.

Some common settings for FRDM development boards were moved to boards/frdm-common to reduce the maintenance effort. The existing FRDM-K64F board support has been updated to use the shared files.

Depends on ~~#6916~~ ~~#6993~~

aabadie

Codewise looks good. I only have a few nitpicks. Is there someone with the hardware that could test ?

This will be ok after usual rebase/squash and comments addressed.

Thanks for the hard work on supporting NXP @gebart :)

aabadie · 2017-06-22T14:22:38Z

cpu/k22f/include/cpu_conf.h

+#endif
+
+/**
+ * @brief   ARM Cortex-M specific CPU configuration


Could be @name ?

aabadie · 2017-06-22T14:22:51Z

cpu/k22f/include/cpu_conf.h

+/**
+ * @name GPIO pin mux function numbers
+ */
+/** @{ */


This can be moved in the same doxygen block above

aabadie · 2017-06-22T14:23:02Z

cpu/k22f/include/cpu_conf.h

+/**
+ * @name GPIO interrupt flank settings
+ */
+/** @{ */


aabadie · 2017-06-22T14:23:11Z

cpu/k22f/include/cpu_conf.h

+/**
+ * @name Timer hardware information
+ */
+/** @{ */


aabadie · 2017-06-22T14:24:36Z

cpu/kinetis_common/include/periph_cpu.h

 * @brief UART module configuration options
 */
 typedef struct {
-    UART_Type *dev;               /**< Pointer to module hardware registers */


Any particular reason for replacing by void * ?

Yes, this is part of the commit to support both UART and LPUART in the same driver, and there are other CPUs which only have one or the other, so I used a void pointer as the least common denominator.

That's interesting. I'm wondering if the same strategy could also be applied to STM32. Some macros make the driver implementation harder to read/understand btw.

The macros are used as a way to eliminate the dispatcher function if a board only supports one or the other type of hardware.

jnohlgard · 2017-07-11T12:35:54Z

I will try to address all comments during this week

haukepetersen

Nice one! My main issue is the added support for the lpuart which I think might be better PRed separately, but otherwise only minor things.

Did just try to flash the board with a newly pulled openOCD version (0.10.0+dev-00167-g29cfe9c (2017-07-13-18:35)), but without success -> using the jlink interface I get an Selected transport is not supported.... But then I did not try hard and did not try to update the firmware on the flash chip...

haukepetersen · 2017-07-13T14:04:13Z

boards/frdm-common/Makefile.include

+# watchdog automatically. However, current stable releases of Ubuntu and Debian
+# have only version 0.9.0 and older OpenOCD packages (Ubuntu 17.04, Debian Jessie)
+# Set this to 0 to avoid the extra manual step of disabling the watchdog.
+export USE_OLD_OPENOCD ?= 1


I wonder: how about we make a recent version of OpenOCD mandatory if you want to use these boards? This would simplify the configuration quite a bit and I think it is completely ok to expect developers to have more or less recent tool versions... Maybe we can put something in place like OPENOCD_MIN_VER := 0.10.0 in the board's Makefile and add some code in dist/tools/openocd/openocd.sh to check that version and give some verbose error message telling the developter to update OpenOCD? This could also be useful for other boards...

Though this might as well be address in a follow-up PR...

I don't like the idea of requiring a newer version than what is available in the distributions. For the newer CPUs (e.g. kw41z) there's no choice, but the older CPUs should be possible to use the distribution provided openocd.

I'm fine with that, I just wanted to think that thought loud once :-)

haukepetersen · 2017-07-13T14:06:41Z

boards/frdm-k22f/Makefile.include

+# define the cpu used by the board
+export CPU = k22f
+export CPU_MODEL = mk22fn512vlh12
+export CPU_FAMILY = kx


I would prefer moving the CPU_FAMILY define into cpu/k22f/Makefile.include, as it is tied to that CPU...

haukepetersen · 2017-07-13T16:28:11Z

boards/frdm-k22f/include/periph_conf.h

+#define KINETIS_MCG_ERC_OSCILLATOR   1
+#define KINETIS_MCG_ERC_FRDIV        3           /* ERC divider = 256 */
+#define KINETIS_MCG_ERC_RANGE        1
+#define KINETIS_MCG_ERC_FREQ         40000000u


parens around the value?

haukepetersen · 2017-07-13T16:29:11Z

boards/frdm-k22f/include/periph_conf.h

+},                           \
+}
+#define LPTMR_NUMOF             (1U)
+#define LPTMR_CONFIG { \


allignment of \ chars is somewhat inconsistent (sorry for being picky :-) )

haukepetersen · 2017-07-13T16:31:09Z

boards/frdm-k22f/include/periph_conf.h

+    { ADC0, GPIO_PIN(PORT_B,  0),  8 },
+    { ADC0, GPIO_PIN(PORT_B,  1),  9 },
+    { ADC0, GPIO_PIN(PORT_C,  1), 15 },
+    { ADC0, GPIO_PIN(PORT_C,  2),  4 },


I'd prefer to name the parameters : { .dev = ADC0, .pin = ... },

haukepetersen · 2017-07-13T16:32:22Z

boards/frdm-k64f/Makefile.include

 # define the cpu used by the FRDM-K64F board
 export CPU = k64f
 export CPU_MODEL = mk64fn1m0vll12
+export CPU_FAMILY = kx


same here: move to CPUs Makefile.include

haukepetersen · 2017-07-13T16:40:02Z

cpu/kinetis_common/include/periph_cpu.h

@@ -322,6 +330,7 @@ typedef struct {
    volatile uint32_t *scgc_addr; /**< Clock enable register, in SIM module */
    uint8_t scgc_bit;             /**< Clock enable bit, within the register */
    uint8_t mode;                 /**< UART mode: data bits, parity, stop bits */


I think I would prefer to PR the 'UART extension' in a separate PR...

Is there any benefit in doing this in two steps, other than bureaucratic? This CPU supports both UART and LPUART, the KW41Z (other PR) supports only LPUART. The already supported kinetis CPUs support AFAIK, only UART. The LPUART should probably be the default when possible because it is working in STOP and VLPS modes (async operation), to allow lower power consumption.

haukepetersen · 2017-07-13T16:41:28Z

cpu/kinetis_common/isr_kinetis.c

 WEAK_DEFAULT void isr_i2s0_tx(void);
 WEAK_DEFAULT void isr_llwu(void);
 WEAK_DEFAULT void isr_lptmr0(void);
+WEAK_DEFAULT void isr_lpuart0(void);


this is added to the common vectors file, but never used in this file, right?!

Should be used in the periph UART driver, I'll look it up.

This is the weak default for the function used in vectors.c (https://github.com/gebart/RIOT/blob/pr/frdm-k22f/cpu/k22f/vectors.c#L76) for when the LPUART module is not used by the UART driver.

I see, never mind then.

haukepetersen · 2017-07-13T16:42:18Z

cpu/kinetis_common/periph/uart.c

@@ -23,13 +23,27 @@
 * @}


as stated above, I think the support for the LPUART should be PRed separately...

jnohlgard · 2017-07-13T18:11:25Z

@haukepetersen did you try flashing with the default setting for FRDM_IFACE? My frdm-k22f board came flashed with a DAP interface firmware. The frdm-kw41z, on the other hand, came with a jlink firmware. The default settings in the board Makefile.include reflect what my boards have had from the factory.

jnohlgard · 2017-07-14T06:51:42Z

addressed comments

jnohlgard · 2017-07-14T07:11:36Z

Split the LPUART change into a separate PR #7362
I think all comments have now been addressed.

@haukepetersen @aabadie OK to squash?

aabadie

Untested-ACK (to not block this PR)

haukepetersen · 2017-07-17T09:40:30Z

Ok, turned out that I was not able to flash because I don't have a frdm-k22f, but I tried to flash a frdm-kw41z...

haukepetersen

untested ACK. But confirmed the frdm-k64f to be still working with this PR.

haukepetersen · 2017-07-17T09:42:00Z

please squash and merge at will one Murdock is happy.

…DM boards

Newer CPUs have alternate clock sources on ADICLK=1 instead of Bus/2

jnohlgard · 2017-07-18T06:40:34Z

squashed, rebased

jnohlgard · 2017-07-18T06:54:32Z

Murdock is happy -> go

jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels May 2, 2017

jnohlgard added this to the Release 2017.07 milestone May 2, 2017

jnohlgard assigned kYc0o May 2, 2017

jnohlgard mentioned this pull request May 2, 2017

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

Merged

jnohlgard force-pushed the pr/frdm-k22f branch from e2e0ba8 to b2aac65 Compare May 2, 2017 10:41

jnohlgard mentioned this pull request May 10, 2017

kinetis: MCG improvements #6978

Merged

jnohlgard force-pushed the pr/frdm-k22f branch from b2aac65 to 6358dd1 Compare May 30, 2017 06:55

jnohlgard mentioned this pull request May 30, 2017

drivers: Add support for KW41Z builtin transceiver #7107

Closed

jnohlgard force-pushed the pr/frdm-k22f branch 2 times, most recently from 98d195f to de76dac Compare June 1, 2017 05:16

jnohlgard mentioned this pull request Jun 8, 2017

Tracker: Kinetis development board support #7154

Closed

jnohlgard force-pushed the pr/frdm-k22f branch from de76dac to f6578d9 Compare June 21, 2017 11:27

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 Jun 21, 2017

jnohlgard force-pushed the pr/frdm-k22f branch from f6578d9 to 72ad5d4 Compare June 21, 2017 11:31

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

aabadie requested changes Jun 22, 2017

View reviewed changes

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

haukepetersen requested changes Jul 13, 2017

View reviewed changes

jnohlgard force-pushed the pr/frdm-k22f branch from de0e44d to db85db0 Compare July 14, 2017 06:51

jnohlgard force-pushed the pr/frdm-k22f branch from db85db0 to 7927a2e Compare July 14, 2017 07:06

jnohlgard mentioned this pull request Jul 14, 2017

kinetis: Add support for LPUART module in parallel with UART module #7362

Merged

aabadie approved these changes Jul 14, 2017

View reviewed changes

haukepetersen approved these changes Jul 17, 2017

View reviewed changes

Joakim Nohlgård added 6 commits July 18, 2017 08:39

boards/frdm-common: Introduce frdm-common for shared files for all FR…

156c7ec

…DM boards

kinetis: Add support for alternate location for ADC1 clock gate

676df9a

kinetis: Handle ADC modules without extra ADICLK divider

de8f0b6

Newer CPUs have alternate clock sources on ADICLK=1 instead of Bus/2

cpu/k22f: Add Freescale/NXP Kinetis K22F

4ceb71e

cpu/k22f: Add GPIO compatibility definitions to vendor headers

57b32f5

boards/frdm-k22f: Add NXP FRDM-K22F development board

912e497

jnohlgard force-pushed the pr/frdm-k22f branch from 7927a2e to 912e497 Compare July 18, 2017 06:39

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

jnohlgard 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 Jul 18, 2017

jnohlgard merged commit 0f494bb into RIOT-OS:master Jul 18, 2017

jnohlgard deleted the pr/frdm-k22f branch July 18, 2017 06:54

jnohlgard mentioned this pull request Jul 18, 2017

frdm-k64f: Remove old openocd.cfg #7380

Merged

Conversation

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

Uh oh!

Uh oh!

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

jnohlgard commented Jul 11, 2017

Uh oh!

haukepetersen 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 Jul 13, 2017

Uh oh!

jnohlgard commented Jul 14, 2017

Uh oh!

jnohlgard commented Jul 14, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jul 17, 2017

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jul 17, 2017

Uh oh!

jnohlgard commented Jul 18, 2017

Uh oh!

jnohlgard commented Jul 18, 2017

Uh oh!

jnohlgard commented May 2, 2017 •

edited

Loading