Implement second core on LPC54114 #7161

stanislav-poboril · 2018-04-23T14:57:26Z

Introduces support for the Cortex-M0+ core of LPC54114 in addition to the existing support for the Cortex-M4 core. Introduces support for mailbox communication between the two cores. This PR replaces #5960, which has been abandoned due to RPMsg-Lite solution not chosen as the standard AMP solution in Zephyr. So it contains the same commits (except for RPMsg-Lite related ones) and it's also rebased to the recent code in master branch.

The new mailbox (ipm_mcux) driver could be possibly used by the chosen AMP solution - OpenAMP - to enable assymetric multiprocessing.

Adds support for the slave core (Cortex M0+) on LPC54114
Adds an IPM shim driver using the mcux mailbox driver
Adds IPM samples

galak

startup_LPC54114_cm4.S doesn't seem to exist in SDK MCUx 2.2.1

dbkinder · 2018-04-23T17:52:56Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+Segger J-Link firmware variant.
+
+Before you start using Zephyr on the LPCXpresso54114, download and run
+`LPCScrypt`_ to update the LPC-Link2 firmware to the latest version, currently


You have a link here without defining the target, with something like:

.. _LPCScrypt: https://www.nxp.com/support/developer-resources/software-development-tools/lpc-developer-resources-/lpc-microcontroller-utilities/lpcscrypt-v1.8.2:LPCSCRYPT

Ok, should be fixed now.

dbkinder · 2018-04-23T17:59:26Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+=========
+
+You can debug an application in the usual way. Here is an example for the
+:ref:`ipm_mcux` application.


For this :ref: to work, your pull request also needs to create a .rst doc in samples/subsys/ipc/ipm_mcux with a label ipm_mcux. (No such document exists.)

dbkinder · 2018-04-23T18:00:17Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+``Firmware_JLink_LPC-Link2_20160923.bin``. Serial communication problems, such
+as dropping characters, have been observed with older versions of the firmware.
+
+The code for the secondary core is linked into primary core binary file.


into the primary

dbkinder · 2018-04-23T18:01:13Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+as dropping characters, have been observed with older versions of the firmware.
+
+The code for the secondary core is linked into primary core binary file.
+Startup code copies secondary core's code into appropriate location in RAM


copies the secondary core's code into an appropriate

galak

Various areas that need cleaning up.

galak · 2018-04-23T20:03:51Z

boards/arm/lpcxpresso54114_m0/pinmux.c

+#include <fsl_common.h>
+#include <fsl_lpc_iocon.h>
+
+#define IOCON_PIO_DIGITAL_EN	0x80u


these defines should be somewhere common.

Ok, fixed. This was not in MCUxpresso SDK device header file, but I have added the file pin_mux.h from SDK to ext. It defines these things.

galak · 2018-04-23T20:05:33Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+
+   ***** BOOTING ZEPHYR OS v1.10.99 - BUILD: Feb  7 2018 20:32:27 *****
+   Hello World from MASTER! arm
+   Received: 1


The Recieved:1 ... should probalby be removed from here.

I have replaced by

Received: 1
...
Received: 99

to not include all lines.

galak · 2018-04-23T20:07:09Z

boards/arm/lpcxpresso54114_m0/lpcxpresso54114_m0.yaml

+type: mcu
+arch: arm
+ram: 64
+flash: 256


should this be 64 since its pointing to sram1?

galak · 2018-04-23T20:07:10Z

boards/arm/lpcxpresso54114_m0/lpcxpresso54114_m0.yaml

+name: NXP LPCXpresso54114
+type: mcu
+arch: arm
+ram: 64


Should this be 32 since its pointing at sram2?

galak · 2018-04-23T20:07:55Z

boards/arm/lpcxpresso54114_m0/board.h

+#include <soc.h>
+
+/* Red LED */
+#define RED_GPIO_NAME	CONFIG_GPIO_MCUX_LPC_PORT0_NAME


Would be nice if this moved to being in dts.

Blinky demo no longer builds even for master core on master branch.
I have moved it into DTS but commented it out. Need to add missing gpio sections in soc DTS.
I have tried to add it but haven't been able to make it work.
So blinky demo still does not work.

leds are fixed and in dts now

galak · 2018-04-23T20:08:49Z

boards/arm/lpcxpresso54114/lpcxpresso54114.dts

@@ -17,7 +17,7 @@
 	};

 	chosen {
-		zephyr,sram = &sram1;
+		zephyr,sram = &sram0;


add a comment in commit message about this change.

Ok, modified commit message.

galak · 2018-04-23T20:10:42Z

arch/arm/soc/nxp_lpc/lpc54xxx/dts.fixup

@@ -6,8 +6,6 @@

 /* SoC level DTS fixup file */

-#define CONFIG_NUM_IRQ_PRIO_BITS		ARM_V7M_NVIC_E000E100_ARM_NUM_IRQ_PRIORITY_BITS


Do this as:

#if defined(CONFIG_SOC_LPC54114_M0) #define CONFIG_NUM_IRQ_PRIO_BITS ARM_V6M_NVIC_E000E100_ARM_NUM_IRQ_PRIORITY_BITS #else #define CONFIG_NUM_IRQ_PRIO_BITS ARM_V7M_NVIC_E000E100_ARM_NUM_IRQ_PRIORITY_BITS #endif

Ok, changed.

galak · 2018-04-23T20:12:14Z

ext/hal/nxp/mcux/devices/LPC54114/incbin.S

+; * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+; * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+; * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+; */


Not really sure the value of having this imported from the SDK. Can you guys just do a version that is the arch/soc code that is Apache licensed.

There isn't really anything unique here.

This is not changed yet. The file content is copied from SDK and the license text says it has to stay there. So would have to rewrite it with different implementation actually. Otherwise our license scans on SDK would probably detect this match with Zephyr.
Why is it necessary to not have it in ext and have it in soc with Apache license?

Well for one, we probably want a solution that is not just NXP specific. We have other SoCs that will probably support multiple cores and we want something that works for all of them.

Changed commit message as the file is actually from SDK 2.2.0, without Clear BSD license.

stanislav-poboril · 2018-04-24T08:39:51Z

startup_LPC54114_cm4.S doesn't seem to exist in SDK MCUx 2.2.1

I don't have a 2.2.1 SDK package, but I have downloaded 2.3.1 and it is there in devices/LPC54114/gcc/startup_LPC54114_cm4.S. The file amp_startup.S leaves stuff not necessary in Zephyr out, but the rest is based on the SDK file. I will change the commit message to mention mcux 2.3.1 instead of 2.2.1.

galak · 2018-04-24T13:09:46Z

I don't have a 2.2.1 SDK package, but I have downloaded 2.3.1 and it is there in devices/LPC54114/gcc/startup_LPC54114_cm4.S. The file amp_startup.S leaves stuff not necessary in Zephyr out, but the rest is based on the SDK file. I will change the commit message to mention mcux 2.3.1 instead of 2.2.1.

I think it should be 2 commits, include saying 2.3.1. So first import the file exactly as is from the SDK, than modify it in a separate commit for what we need in Zephyr.

stanislav-poboril · 2018-04-24T16:49:27Z

I think it should be 2 commits, include saying 2.3.1. So first import the file exactly as is from the SDK, than modify it in a separate commit for what we need in Zephyr.

Separated into two commits.

dbkinder · 2018-04-24T17:20:53Z

samples/subsys/ipc/ipm_mcux/README.rst

+Overview
+********
+
+This application will use mailbox to send messages from one processor core


How about:

The :ref:`lpcxpresso54114` board has two core processors (M4F and dual M0). This sample application uses a mailbox to send messages from one processor core to the other.

dbkinder · 2018-04-24T17:26:20Z

samples/subsys/ipc/ipm_mcux/README.rst

+   ***** Booting Zephyr OS v1.11.0-764-g4e3007a *****
+   Hello World from MASTER! arm
+   Received: 1
+   Received: 2


Let's do the same here with:

Received 1 ... Received 99

dbkinder · 2018-04-24T17:30:13Z

samples/subsys/ipc/ipm_mcux/README.rst

@@ -0,0 +1,138 @@
+.. _ipm-mcux-sample:


Since this is a new documentation area in the samples, you'll need to create an index/TOC page named samples/subsys/ipc/ipc.rst with the contents:

.. _ipc_samples: IPC Samples ########### .. toctree:: :maxdepth: 1 :glob: **/*

(This will fix the "not included in any TOC" error.)

Added. Is there a way to validate the rst docs myself before pushing? Thanks.

Yes. You can generate the documentation locally by following the instructions in http://docs.zephyrproject.org/README.html. In GitHub, you can get an approximation of what a document will look like by clicking on the view button -- you'll see the rendered reST format, but the Sphinx directives won't be recognized.

stanislav-poboril · 2018-04-25T10:06:31Z

Thank you for your comments. Everything should fixed now (except for the external file incbin.S which will stay external). Could you review once more and approve or have further comments? Thanks.

galak · 2018-04-26T18:21:28Z

ext/hal/nxp/mcux/devices/LPC54114/amp_startup.S

+ */
+
+/* ---------------------------------------------------------------------------------------*/
+/*  @file:    startup_LPC54114_cm4.S                                                      */


Can we import the version from SDK 2.3.0 as its BSD licensed and not BSD-clear licensed

galak

Some additional review comments/cleanups.

galak · 2018-04-26T18:30:53Z

arch/arm/soc/nxp_lpc/lpc54xxx/Kconfig.defconfig.lpc54114_m0

+	string
+	default lpc54114_m0
+
+config CPU_CORTEX_M_HAS_VTOR


this should be a select under config SOC_LPC54114_M0

galak · 2018-04-26T18:30:53Z

arch/arm/soc/nxp_lpc/lpc54xxx/soc.h

@@ -23,6 +23,17 @@ extern "C" {
 #include <device.h>
 #include <misc/util.h>
 #include <fsl_common.h>
+
+/* Address of RAM, where the image for core1 should be copied */


The addresses seem like the should be board specific.

The address should be shared by two boards in fact: the M4 version and M0 version. But these are separate boards in Zephyr now, so unfortunately there will stay some sort of duplication. But for sure it can be done at least better than now.

galak · 2018-04-26T18:30:53Z

ext/hal/nxp/mcux/CMakeLists.txt

@@ -5,7 +5,12 @@ string(TOUPPER ${CONFIG_SOC} MCUX_DEVICE)
 if("${MCUX_DEVICE}" STREQUAL "LPC54114")
  set(MCUX_CPU CPU_${CONFIG_SOC_PART_NUMBER}_cm4)
 else()
-  set(MCUX_CPU CPU_${CONFIG_SOC_PART_NUMBER})
+  if("${MCUX_DEVICE}" STREQUAL "LPC54114_M0")
+  set(MCUX_CPU CPU_${CONFIG_SOC_PART_NUMBER}_cm0plus)


nit: should ident these for if block

galak · 2018-04-26T18:30:53Z

arch/arm/soc/nxp_lpc/lpc54xxx/soc.c

@@ -35,6 +35,8 @@

 static ALWAYS_INLINE void clkInit(void)
 {
+
+#ifdef CONFIG_SOC_LPC54114


Wonder if this would be a bit more clear as #ifndef CONFIG_SOC_LPC54114_M0, or do we have a CONFIG_SOC_LPC54114_M4 define?

Maybe a comment, about doing this only on the master/m4

Do we want to change the name of the original soc to append _M4 so it's explicit?

I think renaming to _M4 would be good.

Done rename to _M4.

Note that SOCs are _M4 and _M0 now, while board has no suffix for M4 configuration and board with M0 configuration has suffix for M0. This is because the name of the board already existed without _M4 suffix before.

galak · 2018-04-26T18:33:37Z

ext/hal/nxp/mcux/CMakeLists.txt

+
+enable_language(C ASM)
+
+zephyr_sources_ifdef(CONFIG_SOC_LPC54114 devices/${MCUX_DEVICE}/amp_startup.S)


Any reason not to keep the same path & filename as the SDK?

Fixed, kept the same name.

MaureenHelm · 2018-04-26T21:19:06Z

arch/arm/soc/nxp_lpc/lpc54xxx/Kconfig.defconfig.lpc54114

@@ -32,4 +32,12 @@ config USART_MCUX_LPC

 endif # SERIAL

+config SLAVE_CORE_MCUX


This needs to move to Kconfig.soc

MaureenHelm · 2018-04-26T21:23:53Z

arch/arm/soc/nxp_lpc/lpc54xxx/linker.ld

+/* Specify the memory areas */
+MEMORY
+{
+  m_core1_image         (RX)  : ORIGIN = 0x00030000, LENGTH = 0x00010000


Can these values come from dts?

MaureenHelm · 2018-04-26T21:27:29Z

arch/arm/soc/nxp_lpc/lpc54xxx/soc.c

@@ -35,6 +35,8 @@

 static ALWAYS_INLINE void clkInit(void)
 {
+
+#ifdef CONFIG_SOC_LPC54114


Do we want to change the name of the original soc to append _M4 so it's explicit?

MaureenHelm · 2018-04-26T21:31:21Z

dts/arm/nxp/nxp_lpc54xxx.dtsi

+			/* reduced from 0x40000 to 0x30000
+			 * to leave room for secondary core
+			 */
+			reg = <0 0x30000>;


Would it be better to use partitions instead, like we do for mcuboot?

The partition names currently available for mcuboot might be a little misleading for this case, but I can try to change it.

ioannisg · 2018-04-30T10:22:19Z

arch/arm/core/cortex_m/Kconfig

+config PLATFORM_SPECIFIC_INIT
+	bool
+	prompt "Enable platform (SOC) specific startup hook"
+	default n


Some help text, here, would be very nice, since this will be a generic Cortex-M K-option (even though the option is self-descriptive)

Added a simple help text.

ioannisg · 2018-04-30T10:58:31Z

arch/arm/soc/nxp_lpc/lpc54xxx/Kconfig.series

@@ -7,7 +7,8 @@

 config SOC_SERIES_LPC54XXX
 	bool "LPC LPC54xxx Series MCU"
-	select CPU_CORTEX_M4
+	select CPU_CORTEX_M


Aren't CPU_CORTEX_M explicit selections obsolete, now @galak ?

Removed CPU_CORTEX_M, as it is automatically selected by including CPU_CORTEX_M4 or CPU_CORTEX_M0PLUS elsewhere.

ioannisg · 2018-04-30T11:22:33Z

drivers/ipm/Kconfig

+	default n
+	depends on IPM && HAS_MCUX
+	help
+	Driver for MCUX mailbox


Fix indentation for help message

dbkinder

Doc changes look OK. Thanks.

The new file startup_LPC54114_cm4.S is a copy of the file devices/LPC54114/gcc/startup_LPC54114_cm4.S from mcux 2.2.0. It contains platform specific initialization code for both cores. Origin: NXP MCUXpresso SDK 2.2.0 URL: mcux.nxp.com Maintained-by: External Signed-off-by: Stanislav Poboril <[email protected]>

The instructions performed by standard Zephyr startup files are removed from the file startup_LPC54114_cm4.S. Introduced the section _PlatformInit which will be called when platform specific initialization is needed. Signed-off-by: Stanislav Poboril <[email protected]>

Add soc configuration support and dts files for nxp_lpc54xxx_m0. Adjusted nxp_lpc54xxx soc linker, configuration and dts files for the presence of slave core. Added startup hook support in ARM Cortext-M arch to allow the initialization of slave core from startup and reset. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

Add lpcxpresso54114_m0 board dts, yaml, fixup, docs, configuration, pinmux and header files. The primary core changed to use sram0 instead of sram1 now. The secondary core uses sram1 for its code and sram2 as a sram. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

Add driver for MCUX mailbox which can be used for lpcxpresso54114 and other lpc and kinetis socs. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

Add ext code to be able to include image of slave core in primary core image. Origin: NXP MCUXpresso SDK 2.2.0 URL: mcux.nxp.com Maintained-by: External Signed-off-by: Stanislav Poboril <[email protected]>

Add MCUX IPM sample application. It can be run on lpcxpresso54114 board at the moment. First, the slave core image has to be built from samples/subsys/ipc/ipm_mcux/remote/. Then, buid primary core image from samples/subsys/ipc/ipm_mcux/master/. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

stanislav-poboril · 2018-05-16T12:37:29Z

This PR is replaced by #7558 (already merged) and #7560.

stanislav-poboril requested review from dbkinder and MaureenHelm as code owners April 23, 2018 14:57

galak reviewed Apr 23, 2018

View reviewed changes

stanislav-poboril mentioned this pull request Apr 23, 2018

Introduce OpenAMP/RPMsg-Lite on LPC54114 #5960

Closed

dbkinder suggested changes Apr 23, 2018

View reviewed changes

galak added area: ARM ARM (32-bit) Architecture area: Boards platform: NXP NXP EXT Has change or related to ext/ (obsolete) labels Apr 23, 2018

galak requested changes Apr 23, 2018

View reviewed changes

dbkinder suggested changes Apr 24, 2018

View reviewed changes

galak reviewed Apr 26, 2018

View reviewed changes

galak requested changes Apr 26, 2018

View reviewed changes

galak reviewed Apr 26, 2018

View reviewed changes

MaureenHelm reviewed Apr 26, 2018

View reviewed changes

ioannisg requested changes Apr 30, 2018

View reviewed changes

dbkinder approved these changes May 7, 2018

View reviewed changes

galak mentioned this pull request May 8, 2018

openamp sample integration/app #7416

Merged

stanislav-poboril added 7 commits May 14, 2018 09:07

mcux: Add MCUX IPM driver for lpc and kinetis socs

b0bad56

Add driver for MCUX mailbox which can be used for lpcxpresso54114 and other lpc and kinetis socs. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

lpc: Add code to include slave core binary for lpc54114 from mcux 2.2.0

cf80d08

Add ext code to be able to include image of slave core in primary core image. Origin: NXP MCUXpresso SDK 2.2.0 URL: mcux.nxp.com Maintained-by: External Signed-off-by: Stanislav Poboril <[email protected]>

galak mentioned this pull request May 16, 2018

Add Support for building and booting multicore images on LPC54114 #7560

Merged

stanislav-poboril closed this May 16, 2018

stanislav-poboril deleted the lpc54114_multicore branch May 21, 2018 08:19

		@@ -6,8 +6,6 @@

		/* SoC level DTS fixup file */

		#define CONFIG_NUM_IRQ_PRIO_BITS ARM_V7M_NVIC_E000E100_ARM_NUM_IRQ_PRIORITY_BITS


		enable_language(C ASM)

		zephyr_sources_ifdef(CONFIG_SOC_LPC54114 devices/${MCUX_DEVICE}/amp_startup.S)

		@@ -32,4 +32,12 @@ config USART_MCUX_LPC

		endif # SERIAL

		config SLAVE_CORE_MCUX

Implement second core on LPC54114 #7161

Implement second core on LPC54114 #7161

Conversation

stanislav-poboril commented Apr 23, 2018

galak 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galak 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

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

stanislav-poboril Apr 24, 2018 • edited Loading

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

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

stanislav-poboril commented Apr 24, 2018

galak commented Apr 24, 2018

stanislav-poboril commented Apr 24, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanislav-poboril commented Apr 25, 2018

Choose a reason for hiding this comment

galak 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

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

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

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

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

stanislav-poboril commented May 16, 2018

stanislav-poboril Apr 24, 2018 •

edited

Loading