Openamp v2018.10 #11355
Openamp v2018.10 #11355
Conversation
|
Fixes #9921 |
Codecov Report
@@ Coverage Diff @@
## master #11355 +/- ##
=======================================
Coverage 53.51% 53.51%
=======================================
Files 239 239
Lines 26983 26983
Branches 6518 6518
=======================================
Hits 14440 14440
Misses 9879 9879
Partials 2664 2664Continue to review full report at Codecov.
|
| ${PLATFORM_DIR}/platform.c | ||
| ${PLATFORM_DIR}/platform_ops.c | ||
| ${PLATFORM_DIR}/resource_table.c) | ||
| target_sources(app PRIVATE src/main_master.c) |
There was a problem hiding this comment.
I think we should have a "master" folder since we have one for "remote". And a proper README.rst per sample app.
There was a problem hiding this comment.
I don't have any particular issue with this, so master/main.c? instead of src/main_master.c? How much do you expect to exist in the master dir beyond the source and a README.rst in both master and remote?
There was a problem hiding this comment.
I think it makes more clear to have a master/src/main.c and remote/src/main.c, especially for beginners. For master and remote folders we should have a README.rst giving more details what the sample app does alongside with the CMakeLists.txt and prj.conf.
As soon as this and #8527 get merged I will work to get this working on the i.MX7.
There was a problem hiding this comment.
What do you expect in the remote/README.rst?
There was a problem hiding this comment.
Introducing to it, how it works. Explaining it is not using remoteproc, etc.
galak
force-pushed
the
openamp-v2018.10
branch
from
8af38e2 to
561baab
Compare
|
@arnop2 & @wjliang Would appreciate you guys taking a look at this. |
galak
force-pushed
the
openamp-v2018.10
branch
from
561baab to
3882f6e
Compare
samples/subsys/ipc/openamp/common.h
Outdated
| #define VRING_ALIGNMENT 4 | ||
| #define VRING_SIZE 16 | ||
|
|
||
| #define RSC_TABLE_ADDRESS 0x04000000 |
There was a problem hiding this comment.
Should the resource table address covered by the shm? as this is beyond the SHM_START_ADDR, just wonder where is this memory mapped?
There was a problem hiding this comment.
What do you mean by mapped?
| static K_SEM_DEFINE(data_sem, 0, 1); | ||
| static K_SEM_DEFINE(data_rx_sem, 0, 1); | ||
|
|
||
| static void platform_ipm_callback(void *context, u32_t id, volatile void *data) |
There was a problem hiding this comment.
Platforms need a mailbox to be able to run this example. Suggest to add the limitation in README.rst file.
galak
force-pushed
the
openamp-v2018.10
branch
from
3882f6e to
3769d5e
Compare
| struct virtio_dispatch dispatch = { | ||
| .get_status = virtio_get_status, | ||
| .get_features = virtio_get_features, | ||
| .set_features = virtio_set_features, |
There was a problem hiding this comment.
I believe that the .set_features callback is not needed for the remote:
https://github.com/OpenAMP/open-amp/blob/master/lib/remoteproc/remoteproc_virtio.c#L167
There was a problem hiding this comment.
Or maybe there is an inconsistency in the OpenAMP implementation:
OpenAMP/open-amp#139
There was a problem hiding this comment.
For now, as RPMsg is the only virtio user, and the demo doesn't make use of .set_features on the remote side. Implementation is not required. But prefer to have comments to the virtio dispatch ops structure to explain why there is no implementation for those functions.
| @@ -135,7 +135,7 @@ int metal_sys_init(const struct metal_init_params *params) | |||
| strerror(errno)); | |||
| return -errno; | |||
| } | |||
| if (sizeof(int) != fread(&seed, sizeof(int), 1, urandom)) { | |||
| if (fread(&seed, 1, sizeof(seed), urandom) <= 0) { | |||
There was a problem hiding this comment.
I do not know if that was intention of the author, however we assume success if at least one byte of seed has been taken from urandom.
galak
force-pushed
the
openamp-v2018.10
branch
from
3769d5e to
7f7e6be
Compare
|
Found the following issues, please fix and resubmit: License/Copyright issuesIn most cases you do not need to do anything here, especially if the files reported below are going into ext/ and if license was approved for inclusion into ext/ already. Fix any missing license/copyright issues. The license exception if a JFYI for the maintainers and can be overriden when merging the pull request.
|
galak
force-pushed
the
openamp-v2018.10
branch
3 times, most recently
from
7f7e6be to
2a43738
Compare
| static void virtio_set_features(struct virtio_device *vdev, | ||
| u32_t features) | ||
| { | ||
| } |
There was a problem hiding this comment.
better to comments on why it is empty?
| return 0; | ||
| } | ||
|
|
||
| SYS_INIT(init_status_flag, PRE_KERNEL_1, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT); |
There was a problem hiding this comment.
Only see init_status_flag() called from master, it suggests master needs to load first, better to have some comments on the init_status_flags.
samples/subsys/ipc/openamp/common.h
Outdated
| #define VRING_ALIGNMENT 4 | ||
| #define VRING_SIZE 16 | ||
|
|
||
| #define OPENAMP_STATUS_ADDR 0x04000000 |
There was a problem hiding this comment.
Could be nice to have CONFIG or dtsmemory region to declare the address and size, as platform dependent
There was a problem hiding this comment.
Will re-work that as a follow on PR that looks to support more platforms with this sample.
|
|
||
| /* Since we are using name service, we need to wait for a response | ||
| * from NS setup and than we need to process it | ||
| */ |
There was a problem hiding this comment.
just for you information, NS announcement is optional, you can also define a fixed source and destination address on end point creation.
galak
force-pushed
the
openamp-v2018.10
branch
from
2a43738 to
fa8147a
Compare
galak
force-pushed
the
openamp-v2018.10
branch
from
fa8147a to
8d57d87
Compare
|
@MaureenHelm did you or anyone from NXP want to review this? |
galak
force-pushed
the
openamp-v2018.10
branch
from
8d57d87 to
e3aa39d
Compare
Since there's an official release lets update to it. Signed-off-by: Kumar Gala <[email protected]>
Update to new OpenAMP v2018.10 release. This release allows us to utilize just rpmsg without remoteproc. The API set has changed and requires updates to the openamp sample. Additionally, the changes in this release reduce the code size footprint, and support a static allocation memory model. Signed-off-by: Kumar Gala <[email protected]>
galak
force-pushed
the
openamp-v2018.10
branch
from
e3aa39d to
753d192
Compare
The v2018.10 release of OpenAMP reworks the API set and splits the remoteproc vs rpmsg interfaces so one can use rpmsg without remoteproc. This helps drastically reduce the code footprint utilized by OpenAMP. The remote see around 4k reduction in code size. Signed-off-by: Kumar Gala <[email protected]>
galak
force-pushed
the
openamp-v2018.10
branch
from
753d192 to
6fb4b57
Compare
Update Zephyr to utilize OpenAMP (and libmetal) v2018.10 release. The v2018.10 release reduces code footprint and reworks the API set to decouple rpmsg and remoteproc. We update the sample app to take advantage of rpmsg and see around a 4k reduction in footprint.