SPI driver for KINETIS K20.

[SpaceCoaster]

Hi,

Attached are three patches.

1. SPI low level driver for the KINETIS K20 processors.
2. Board files for the MCHCK board with a KINETIS MK20DX128 processor.
3. SPI demo program for the MCHCK board driving an AT45DB081 data flash.

The SPI driver for master mode, is restricted to 8 bit and it doesn't use DMA. This SPI device is very different from the SPI device used in the KL series processors and the driver would not be common between the two series.

I am quite pleased with the way it worked out. Chibios and the chip played nicely together.

Cheers,
Derek

[colin]

All right, awesome to see more Kinetis development. Thanks. Here's a quick patch review.

0001-KINETIS-SPI-driver.patch

Good to see the non-ASCII copyright symbol and curly quotes in the file header comment replaced with proper ASCII characters . The non-ASCII symbols caused minor annoyances for me before.

Probably should remove the old commented-out #define constant for SPI0

Code: Select all

+//#define SPI0            (*(KINETISK_SPI_t *)0x4002C000)

since the proper SPI0 pointer is defined earlier:

Code: Select all

#define SPI0                    ((SPI_TypeDef *)     SPI0_BASE)

and also delete the commented-out lines for register pointers that are not used since we use SPI0->regname instead of SPI0_regname. I think all the following lines in the patch should be removed: (note these lines are note all consecutive)

Code: Select all

+/***********  Bits definition for SPIx_MCR register  *************/
+//#define SPI0_MCR        (*(volatile uint32_t *)0x4002C000) // DSPI Module Configuration Register
+//#define SPI0_TCR        (*(volatile uint32_t *)0x4002C008) // DSPI Transfer Count Register
+//#define SPI0_CTAR0      (*(volatile uint32_t *)0x4002C00C) // DSPI Clock and Transfer Attributes Register, In Master Mode
+//#define SPI0_CTAR0_SLAVE    (*(volatile uint32_t *)0x4002C00C) // DSPI Clock and Transfer Attributes Register, In Slave Mode
+//#define SPI0_SR         (*(volatile uint32_t *)0x4002C02C) // DSPI Status Register
+//#define SPI0_RSER       (*(volatile uint32_t *)0x4002C030) // DSPI DMA/Interrupt Request Select and Enable Register
+//#define SPI0_PUSHR      (*(volatile uint32_t *)0x4002C034) // DSPI PUSH TX FIFO Register In Master Mode
+//#define SPI0_PUSHR_SLAVE    (*(volatile uint32_t *)0x4002C034) // DSPI PUSH TX FIFO Register In Slave Mode
+
+/***********  Bits definition for SPIx_POPR register  *************/
+//#define SPI0_POPR       (*(volatile uint32_t *)0x4002C038) // DSPI POP RX FIFO Register
+
+/***********  Bits definition for SPIx_TXFRn register  *************/
+//#define SPI0_TXFR0      (*(volatile uint32_t *)0x4002C03C) // DSPI Transmit FIFO Registers
+//#define SPI0_TXFR1      (*(volatile uint32_t *)0x4002C040) // DSPI Transmit FIFO Registers
+//#define SPI0_TXFR2      (*(volatile uint32_t *)0x4002C044) // DSPI Transmit FIFO Registers
+//#define SPI0_TXFR3      (*(volatile uint32_t *)0x4002C048) // DSPI Transmit FIFO Registers
+
+/***********  Bits definition for SPIx_RXFRn register  *************/
+//#define SPI0_RXFR0      (*(volatile uint32_t *)0x4002C07C) // DSPI Receive FIFO Registers
+//#define SPI0_RXFR1      (*(volatile uint32_t *)0x4002C080) // DSPI Receive FIFO Registers
+//#define SPI0_RXFR2      (*(volatile uint32_t *)0x4002C084) // DSPI Receive FIFO Registers
+//#define SPI0_RXFR3      (*(volatile uint32_t *)0x4002C088) // DSPI Receive FIFO Registers
 

os/hal/ports/KINETIS/K20x/spi_lld.h:
I think it's not correct to say that these settings are "24 MHz" or "27 kHz" in the LLD, since it's really just a divisor for the microcontroller clock, which frequency may vary depending on the target configuration. Right?
You could call it "KINETIS_SPI_TAR_24MHZ_FOR_48MHZ_SYSCLK" or "KINETIS_SPI_TAR_SYSCLK_DIV_2" or something, if I understand correctly.

Code: Select all

+/* TAR settings for n bits at 24 Mhz */
+#define KINETIS_SPI_TAR_24MHZ(n)    SPIx_CTARn_FMSZ((n) - 1) | \
+    SPIx_CTARn_CPOL | SPIx_CTARn_CPHA | \
+    SPIx_CTARn_DBR | SPIx_CTARn_PBR(0b00) | SPIx_CTARn_BR(0b0000) | \
+    SPIx_CTARn_CSSCK(0b0000) | SPIx_CTARn_ASC(0b0000) | SPIx_CTARn_DT(0b0000)
+
+/* TAR settings for n bits at 27kHz for debugging */
+#define KINETIS_SPI_TAR_SLOW(n)     SPIx_CTARn_FMSZ(((n) - 1)) | \
+    SPIx_CTARn_CPOL | SPIx_CTARn_CPHA | \
+    SPIx_CTARn_DBR | SPIx_CTARn_PBR(0b11) | SPIx_CTARn_BR(0b1001) | \
+    SPIx_CTARn_CSSCK(0b1011) | SPIx_CTARn_ASC(0b0111) | SPIx_CTARn_DT(0b1011)
+
+#define KINETIS_SPI_TAR_8BIT_FAST   KINETIS_SPI_TAR_24MHZ(8)
+#define KINETIS_SPI_TAR_8BIT_SLOW   KINETIS_SPI_TAR_SLOW(8)
+
+#define KINETIS_SPI_TAR0_DEFAULT    KINETIS_SPI_TAR_24MHZ(8)
+#define KINETIS_SPI_TAR1_DEFAULT    KINETIS_SPI_TAR_24MHZ(8)

Probably should be use hexadecimal literals instead of non-standard gcc binary literals in this code. And note, there's no need for extra parentheses around a literal constant in a #define.

Code: Select all

+#define KINETIS_SPI_PCS_NONE        (0b00000)
+#define KINETIS_SPI_PCS0            (0b00001)
+#define KINETIS_SPI_PCS1            (0b00010)
+#define KINETIS_SPI_PCS2            (0b00100)
+#define KINETIS_SPI_PCS3            (0b01000)
+#define KINETIS_SPI_PCS4            (0b10000)
+#define KINETIS_SPI_PCS_ALL         (0b11111)

[colin]

0002-KINETIS-Support-for-MCHCK-K20-board.patch

os/hal/boards/MCHCK_K20/board.c:
Trivial alignment issue on PTD5 due to a stray tab character.

Code: Select all

+        /* PTD0*/ PAL_MODE_UNCONNECTED,     /* PTD1*/ PAL_MODE_UNCONNECTED,     /* PTD2*/ PAL_MODE_UNCONNECTED,
+        /* PTD3*/ PAL_MODE_UNCONNECTED,     /* PTD4*/ PAL_MODE_UNCONNECTED,    /* PTD5*/ PAL_MODE_UNCONNECTED,
+        /* PTD6*/ PAL_MODE_UNCONNECTED,     /* PTD7*/ PAL_MODE_UNCONNECTED,     /* PTD8*/ PAL_MODE_UNCONNECTED,
+        /* PTD9*/ PAL_MODE_UNCONNECTED,     /*PTD10*/ PAL_MODE_UNCONNECTED,     /*PTD11*/ PAL_MODE_UNCONNECTED,

os/hal/boards/MCHCK_K20/board.h:

1. Fix board name in comment s/Freescale Freedom.*/MCHK whatever/
2. Remove commented-out KINETIS_SYSCLK, _MCG_MODE, _XTAL_FREQUENCY here which are typically defined in the application mcuconf.h. Do we want to allow board.h to provide defaults if mcuconf.h doesn't specify?

Code: Select all

+/*
+ * Setup for Freescale Freedom K20D50M board.
+ */
+
+/*
+ * Board identifier.
+ */
+#define BOARD_MCHCK_MX20DX128
+#define BOARD_NAME                  "MCHCK MX20DX128"
+
+/* External 8 MHz crystal with PLL for 48 MHz core/system clock. */
+//#define KINETIS_SYSCLK_FREQUENCY    48000000UL
+//#define KINETIS_MCG_MODE            KINETIS_MCG_MODE_PEE
+//#define KINETIS_XTAL_FREQUENCY      8000000UL

[SpaceCoaster]

Colin,

Thanks for the code review. All those changes look good.

Derek

[colin]

0003-KINETIS-SPI-demo-for-MCHCK-K20-board.patch

demos/KINETIS/RT-MCHCK-K20-SPI/Makefile:

Move -std=gnu99 to USE_COPT instead of USE_OPT so it applies only to C and not C++ modules. Granted there may not be any C++ in the demo but it's just good practice, maybe someone will copy this file as a starting point and add C++ files later.

Just a comment, maybe leave it as-is. But any idea why CRT0_INIT_STACKS=0 is defined? I assume you copied this from the other K20 demos (RT-FREEDOM-K20D50M and RT-TEENSY3), but it's not clear why those platforms would specify this. It looks like CRT0_INIT_STACKS fills the main and process stacks with zeros at startup so it must be merely a performance optimization to skip this. It seems it must be very few bytes of code space and runtime cycles. Shall we leave it off and get the default value which is TRUE? I assume that the reason the stack initialization code exists is that it's for predictability, so there is no chance for garbage left over after soft reset or something to affect subsequent behavior in case of referencing unassigned locations on the stack.

Code: Select all

+# Compiler options here.
+ifeq ($(USE_OPT),)
+  USE_OPT = -O2 -ggdb -fomit-frame-pointer -falign-functions=16 -DCRT0_INIT_STACKS=0 -std=gnu99
+endif
+
+# C specific options here (added to USE_OPT).
+ifeq ($(USE_COPT),)
+  USE_COPT =
+endif
+
+# C++ specific options here (added to USE_OPT).
+ifeq ($(USE_CPPOPT),)
+  USE_CPPOPT = -fno-rtti
+endif

[SpaceCoaster]

Colin,

I checked out the CRT0_INIT_STACKS and it looks OK to initialize the stack. The code looks good and it runs fine with the stacks being initialized. Fabio commented about it in another thread.

One thing that might cause a problem. In my Makefile I have

Code: Select all

USE_PROCESS_STACKSIZE = 0x200

In GCC/rules.mk I have

Code: Select all

ifeq ($(USE_PROCESS_STACKSIZE),)
  LDOPT := $(LDOPT),--defsym=__process_stack_size__=0x400
else
  LDOPT := $(LDOPT),--defsym=__process_stack_size__=$(USE_PROCESS_STACKSIZE)
endif

in MK20DX128.ld I have

Code: Select all

__process_stack_size__  = 0x0400;

The final result in ch.dmp is that the __process_stack_size__ ends up as 0x200 but 0x400 is actually allocated on the stack.
This would work as long as you decrease the stack size in the Makefile.

Increasing the size to 0x600 in the Makefile and you end up with __process_stack_base__ and __process_stack_end__ being separated by 0x400 while __process_stack_size__ is set to 0x600.

I am not sure if this is actually a problem but it certainly looks odd/wrong.

Cheers,
Derek

[SpaceCoaster]

OK, everything looks better if the lines.

Code: Select all

__main_stack_size__     = 0x0400;
__process_stack_size__  = 0x0400;


are taken out of KL25Z128.ld and MK20DX128.ld

[utzig]

Colin,

CRT0_INIT_STACKS=0 was previously defined because with the stack initialisation the watchdog would fire before getting to __early_init where it was disabled. Giovanni changed the order some weeks ago after I told him of this problem so now __early_init is called before initialising the stack. I should have changed the code back then...

Also, I'm almost done with the I2C code. Just need to make it interrupt driven now, which should be easy and then finish writing a driver for MMA8451 to be added to both freedom demos. Thankfully I2C works the same on both KL2x and K2x...

I will review this in the following days, thanks!

[SpaceCoaster]

Fabio,

Great, that explains a lot.

I am looking forward to using the I2C code. I have a few chips I want to try it with. I think I might even have an MMA8451.

Cheers,
Derek

[SpaceCoaster]

Updated patches for SPI to include Colin's comments.