Introduce OpenAMP/RPMsg-Lite on LPC54114 #5960

MaureenHelm · 2018-02-02T20:39:21Z

Introduces support for Open Asymmetric Multiprocessing (OpenAMP) using RPMsg-Lite, which is a lightweight implementation of the protocol targeting small memory footprint devices. Support for eRPC is not yet included due to build system issues with c++, as well as to limit the scope of changes in a single PR.

Adds support for the slave core (Cortex M0+) on LPC54114
Adds an IPM shim driver using the mcux mailbox driver
Imports RPMsg-Lite into ext/
Adds IPM and RPMsg-Lite samples

There is an outstanding issue when building the master application, such that the slave core binary cannot be found by the assembly files (incbin.S). This issue occurs only with Zephyr SDK toolchain, but not ARM gcc.

Therefore an extra manual step required between running cmake and make of the master application:

$ cmake -DBOARD=lpcxpresso54114 ..
$ cp ../../remote/build/zephyr/core1_image.bin .
$ make

Fixes #3065

codecov-io · 2018-02-02T21:01:14Z

Codecov Report

Merging #5960 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5960      +/-   ##
==========================================
- Coverage   43.77%   43.76%   -0.01%     
==========================================
  Files         485      485              
  Lines       54660    54660              
  Branches    10690    10690              
==========================================
- Hits        23926    23924       -2     
- Misses      30594    30596       +2     
  Partials      140      140

Impacted Files	Coverage Δ
tests/kernel/obj_tracing/src/philosopher.c	`72.22% <0%> (-16.67%)`	⬇️
drivers/console/native_posix_console.c	`42.1% <0%> (+1.31%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 461511e...4ce9bbc. Read the comment docs.

carlescufi · 2018-02-02T21:06:29Z

@skordal, @ioannisg FYI

skordal · 2018-02-05T07:48:54Z

drivers/ipm/ipm_mcux_mailbox.c

@@ -0,0 +1,173 @@
+/*


This file looks like an earlier version of ipm_mcux.c, it can be deleted.

skordal · 2018-02-05T08:40:39Z

ext/multicore/rpmsg_lite/lib/rpmsg_lite/porting/platform/lpc5411x/rpmsg_platform.c

+    env_lock_mutex(lock);
+
+    assert(0 <= isr_counter);
+    if (!isr_counter)


Suggest adding braces to this if, to make it clearer what goes inside it. Same goes for platform_init_interrupt() in the LPC5410x platform file.

I agree. However the files come from RPMsg-Lite project version 1.2.0. So it will be fixed in the next release of RPMsg-Lite and then it can get into Zephyr. I would not change the file in Zephyr now in order for the file content to be the same as in referenced version of RPMsg-Lite.

skordal · 2018-02-05T09:02:30Z

ext/multicore/rpmsg_lite/lib/rpmsg_lite/porting/platform/lpc5411x/rpmsg_platform_zephyr_ipm.c

+{
+    switch (RL_GET_LINK_ID(vq_id))
+    {
+        case 0:


Why is only link ID 0 supported here?

LPC54114 has two cores so there is an expectation that most of the applications would use a single link there. I am going to replace 0 with RL_PLATFORM_LPC5411x_M4_M0_LINK_ID. If somebody wants to use multiple instances of RPMsg-Lite, he or she has to define more link IDs and implement it in the switch statement.

carlescufi · 2018-02-05T10:37:03Z

I think the external library should be in ext/lib/ipc/rpmsg_lite. @MaureenHelm, @nashif thoughts?

carlescufi · 2018-02-05T11:32:55Z

@pizi-nordic FYI

carlescufi · 2018-02-05T10:35:49Z

ext/multicore/CMakeLists.txt

@@ -0,0 +1 @@
+add_subdirectory_ifdef(MULTICORE_RPMSG_LITE rpmsg_lite)


I think this should be in ext/lib/ipc

Ok. I can move ext/multicore/rpmsg_lite/ into ext/lib/ipc/rpmsg_lite. It will match naming of samples subfolders then.

carlescufi · 2018-02-05T15:44:30Z

arch/arm/core/cortex_m/prep_c.c

    !defined(CONFIG_XIP) && (CONFIG_SRAM_BASE_ADDRESS != 0)
+#if defined(CONFIG_CPU_CORTEX_M0_HAS_VECTOR_TABLE_REMAP)


I'm not sure this is going to work. CONFIG_CPU_CORTEX_M0_HAS_VECTOR_TABLE_REMAP doesn't mean it has a VTOR, but rather a mechanism to remap its table, whatever that might be. In particular I think the STM32F0 does something different than VTOR for this. @b0661, @simple2017 can you weigh in here?

The other option is that you redefine the weak function relocate_vector_table in your soc.c like the STM32F0 code has done, in order to have your own particular version that uses VTOR. I think that might be the simplest. At the same time I do realize that VTOR is the standard relocation mechanism of an M0+, so it would make sense to have it in the generic prep_c.c. I'm just not sure this is not gonna break another chip's vector table handling.

Here is the relevant STM32F0 commit if you want to take a look.

I think we should merge this PR and you could then simply set CPU_CORTEX_M_HAS_VTOR for this IC.

I don't think this gonna work on STM32F0, check here as @carlescufi mentioned. Its not really a offset register but a remap address 0x2000_0000 to address 0x0000_0000

So I will remove the changes of prep_c.c from this PR and add CPU_CORTEX_M_HAS_VTOR for LPC54414's M0+ core instead. Once the other PR is merged.

Yep. The PR is merged now.

carlescufi

nitpick, this commit: lpc: Add code to include slave core binary for lpc54114 from mcux 2.2.1

Should probably be prefixed with ext instead of lpc

nashif · 2018-02-05T15:56:59Z

I think the external library should be in ext/lib/ipc/rpmsg_lite. @MaureenHelm, @nashif thoughts?

yeah, or at least ext/lib/multicore/...

stanislav-poboril · 2018-02-06T14:43:25Z

nitpick, this commit: lpc: Add code to include slave core binary for lpc54114 from mcux 2.2.1
Should probably be prefixed with ext instead of lpc

Ok.

galak

The commit that imports the startup code for AMP doesn’t exactly match the SDK, also we are importing it from SDK 2.3.0 and the reset of the code is from SDK 2.2.1. So can we sync this with SDK 2.3.0 and either match the SDK or comment about the change of name, etc in commit message/readme.

galak

Various code review comments.

galak · 2018-02-06T15:38:03Z

arch/arm/core/cortex_m/reset.S

@@ -57,6 +60,10 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__reset)
 */
 SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start)

+#if defined(CONFIG_PLATFORM_SPECIFIC_INIT)
+    bl _PlatformInit


Why do we need to call _PlatformInit this early? Can't we just call it from nxp_lpc54114_init(). Also do we really need this implemented in ASM?

The developer of RPMsg-Lite told me it needs to be run while the code executes from address 0x0000000. This comment may give some information:

/* Setup the reset handler pointer (PC) and stack pointer value. * This is used once the second core runs its startup code. * The second core first boots from flash (address 0x00000000) * and then detects its identity (Cortex-M0, slave) and checks * registers CPBOOT and CPSTACK and use them to continue the * boot process. * Make sure the startup code for current core (Cortex-M4) is * appropriate and shareable with the Cortex-M0 core! */

This is not related to RPMsg-Lite, but to the architecture of LPC54114.
The secondary core in LPC54114 SOC needs to have a possibility of platform specific code in startup and cannot only use the generic cortex_m/reset.S

@galak Can you develop on why you disagree with this? Do you think other platforms may not need this?

galak · 2018-02-06T15:38:03Z

drivers/ipm/ipm_mcux.c

+	struct device *dev = arg;
+	struct mcux_mailbox_data *data = dev->driver_data;
+	mailbox_cpu_id_t cpu_id;
+#if defined(__CM4_CMSIS_VERSION)


Rather than having multiple ifdef blocks, can we just do something at the top of this file like:

#if defined(__CM4_CMSIS_VERION)
#define MAILBOX_ID kMAILBOX_CM4
#else
#define MAILBOX_ID kMAILBOX_CM0
#endif

galak · 2018-02-06T15:38:03Z

drivers/ipm/Kconfig

@@ -22,3 +22,10 @@ config IPM_QUARK_SE_MASTER
 	  Sets up the initial interrupt mask and clears out all
 	  channels. Should be turned on for one CPU only.

+config IPM_MCUX


Should this be IPM_MCUX_LPC (fixup below as well if we make this LPC specific)

This is for all SoC's which have HAS_MCUX defined: LPC, Kinetis, i.MX. So it is not only for LPC.

MaureenHelm · 2018-02-06T22:07:46Z

I think the external library should be in ext/lib/ipc/rpmsg_lite. @MaureenHelm, @nashif thoughts?

Fine with me

dbkinder · 2018-02-06T23:46:41Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+Before you start using Zephyr on the LPCXpresso54114, download and run
+`LPCScrypt`_ to update the LPC-Link2 firmware to the latest version. Serial
+communication problems, such as dropping characters, have been observed with
+older versions of the firmware.


can you provide a version number here (what will "older versions" mean in 3-6 months)?

The version is in the primary core doc in master branch now. The secondary core board doc has been updated to reference this main doc.

dbkinder · 2018-02-06T23:49:53Z

boards/arm/lpcxpresso54114_m0/doc/lpcxpresso54114.rst

+You can debug an application in the usual way. Here is an example for the
+:ref:`hello_world` application.
+
+.. zephyr-app-commands::


In the commit message you mentioned an "extra manual step required between running cmake and make". Does that need to be addressed here?

The need for manual step has been eliminated.

Yea. You might then want to update the commit message that says the manual step is needed.

The new file amp_startup.S is freely based on parts of the file startup_LPC54114_cm4.S from mcux 2.2.1. It contains platform specific initialization code for both cores. Origin: NXP MCUXpresso SDK 2.2.1 URL: mcux.nxp.com Maintained-by: External 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. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

Add driver for MCUX mailbox which can be used for lpcxpresso54114 and other lpc, kinetis and iMX 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.1 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]>

Add RPMsg-Lite version 1.2.0 to ext. Origin: RPMsg-Lite License: BSD 3-Clause URL: https://github.com/codeauroraforum/rpmsg-lite commit: 3ed2d34376a2d1ec04f59e4a89e201d820b5d5e5 Purpose: Introduction of lightweight implementation of the Remote Processor Messaging (RPMsg) protocol to Zephyr. Maintained-by: External Signed-off-by: Stanislav Poboril <[email protected]>

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

stanislav-poboril · 2018-02-07T22:50:07Z

The commit that imports the startup code for AMP doesn’t exactly match the SDK...

AMP startup is freely based on parts of SDK code, I have updated the commit message.

stanislav-poboril · 2018-02-07T23:06:38Z

Thanks for the comments. I have made some updates internally, @MaureenHelm will update this PR, then you could review again.

MaureenHelm · 2018-02-08T14:15:45Z

@MaureenHelm will update this PR, then you could review again.

Done

skordal · 2018-02-20T08:53:41Z

samples/subsys/ipc/ipm_mcux/master/CMakeLists.txt

+enable_language(C ASM)
+
+target_include_directories(app PRIVATE $ENV{ZEPHYR_BASE}/drivers)
+target_include_directories(app PRIVATE $ENV{ZEPHYR_BASE}/samples/subsys/ipc/ipm_mcux/master/src)


This include directory is not needed.

skordal · 2018-02-20T08:53:58Z

samples/subsys/ipc/rpmsg_lite/master/CMakeLists.txt

+
+target_include_directories(app PRIVATE $ENV{ZEPHYR_BASE}/samples/subsys/ipc/rpmsg_lite)
+target_include_directories(app PRIVATE $ENV{ZEPHYR_BASE}/drivers)
+target_include_directories(app PRIVATE $ENV{ZEPHYR_BASE}/samples/subsys/ipc/rpmsg_lite/master/src)


This include directory is not needed.

This commit adds a sample application using OpenAMP for remote procedure calls on the LPCXpresso54114. It is adapted from the RPMsg-Lite sample application added in PR zephyrproject-rtos#5960, and uses the IPM driver to provide interprocessor interrupts. Instructions for how to compile the application, including OpenAMP and libmetal is provided in the README.md file in the application's toplevel directory. Signed-off-by: Kristian Klomsten Skordal <[email protected]>

stanislav-poboril · 2018-04-23T15:06:19Z

This PR is superseded by #7161, which contains the same content except for RPMsg-Lite, which hasn't been chosen as Zephyr's preferred AMP solution.

So this PR can be cancelled and you can continue reviewing #7161 instead.

This commit adds a sample application using OpenAMP for remote procedure calls on the LPCXpresso54114. It is adapted from the RPMsg-Lite sample application added in PR zephyrproject-rtos#5960, and uses the IPM driver to provide interprocessor interrupts. Signed-off-by: Kristian Klomsten Skordal <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

This commit adds a sample application using OpenAMP for remote procedure calls on the LPCXpresso54114. It is adapted from the RPMsg-Lite sample application added in PR #5960, and uses the IPM driver to provide interprocessor interrupts. Signed-off-by: Kristian Klomsten Skordal <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

MaureenHelm added area: IPC Inter-Process Communication platform: NXP NXP labels Feb 2, 2018

MaureenHelm added this to the v1.11.0 milestone Feb 2, 2018

MaureenHelm requested review from nashif, carlescufi and galak February 2, 2018 20:39

MaureenHelm requested a review from dbkinder as a code owner February 2, 2018 20:39

skordal reviewed Feb 5, 2018

View reviewed changes

carlescufi requested changes Feb 5, 2018

View reviewed changes

galak requested changes Feb 6, 2018

View reviewed changes

dbkinder suggested changes Feb 6, 2018

View reviewed changes

stanislav-poboril added 8 commits February 7, 2018 22:49

lpc: Add support of slave core in lpcxpresso54114 board

5705370

Add lpcxpresso54114_m0 board dts, yaml, fixup, docs, configuration, pinmux and header files. Origin: Original Signed-off-by: Stanislav Poboril <[email protected]>

mcux: Add MCUX IPM driver for lpc, kinetis and iMX socs

5a05000

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

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

fa31e48

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

dbkinder approved these changes Feb 10, 2018

View reviewed changes

skordal mentioned this pull request Feb 12, 2018

[DNM][RFC] RPMsg-Lite and OpenAMP side-by-side #6153

Closed

nashif modified the milestones: v1.11.0, v1.12.0 Feb 13, 2018

skordal reviewed Feb 20, 2018

View reviewed changes

carlescufi mentioned this pull request Feb 25, 2018

arch: Add support to imx7d m4 core and colibri_imx7d_m4 board #6367

Merged

galak added the EXT Has change or related to ext/ (obsolete) label Apr 12, 2018

stanislav-poboril mentioned this pull request Apr 23, 2018

Implement second core on LPC54114 #7161

Closed

MaureenHelm closed this Apr 23, 2018

stanislav-poboril mentioned this pull request Mar 20, 2019

Add RPMsg-Lite 2.2.0 into Zephyr #14742

Closed

		@@ -0,0 +1 @@
		add_subdirectory_ifdef(MULTICORE_RPMSG_LITE rpmsg_lite)

		!defined(CONFIG_XIP) && (CONFIG_SRAM_BASE_ADDRESS != 0)
		#if defined(CONFIG_CPU_CORTEX_M0_HAS_VECTOR_TABLE_REMAP)

Introduce OpenAMP/RPMsg-Lite on LPC54114 #5960

Introduce OpenAMP/RPMsg-Lite on LPC54114 #5960

Conversation

MaureenHelm commented Feb 2, 2018

codecov-io commented Feb 2, 2018

Codecov Report

carlescufi commented Feb 2, 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

carlescufi commented Feb 5, 2018

carlescufi commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

carlescufi Feb 5, 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

stanislav-poboril Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi left a comment

Choose a reason for hiding this comment

nashif commented Feb 5, 2018

stanislav-poboril commented Feb 6, 2018

galak left a 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

MaureenHelm commented Feb 6, 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

stanislav-poboril commented Feb 7, 2018 • edited Loading

stanislav-poboril commented Feb 7, 2018

MaureenHelm commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanislav-poboril commented Apr 23, 2018

carlescufi Feb 5, 2018 •

edited

Loading

carlescufi Feb 5, 2018 •

edited

Loading

stanislav-poboril Feb 6, 2018 •

edited

Loading

stanislav-poboril commented Feb 7, 2018 •

edited

Loading