Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for imx8mp_evk M7 core for openamp_rsc_table sample #78585

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Add support for imx8mp_evk M7 core for openamp_rsc_table sample #78585

merged 2 commits into from
Oct 15, 2024

Conversation

alxlastur
Copy link
Contributor

Add support for imx8mp_evk M7 core for openamp_rsc_table sample

@alxlastur
sample: open_amp_rsc_table: Add support for imx8mp_evk M7 core
ed6a464 
This enables openamp_rsc_table sample for imx8mp_evk on M7 core

Signed-off-by: Alexandru Lastur <[email protected]>
@zephyrbot zephyrbot added area: IPC Inter-Process Communication area: Samples Samples area: Open AMP labels Sep 17, 2024
@dbaluta dbaluta changed the title Zephyr opanamp rsc imx8mp m7 dev 1 Add support for imx8mp_evk M7 core for openamp_rsc_table sample Sep 25, 2024
@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from 685dd2d to db07b16 Compare October 10, 2024 10:19
@zephyrbot zephyrbot requested a review from doki-nordic October 10, 2024 10:19
@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from db07b16 to 5f55eb5 Compare October 10, 2024 10:24
@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from 5f55eb5 to c55d466 Compare October 10, 2024 11:04
@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from c55d466 to bbf90ae Compare October 10, 2024 11:23
dbaluta
dbaluta previously approved these changes Oct 10, 2024
View reviewed changes
@dbaluta
Copy link
Collaborator

dbaluta commented Oct 10, 2024

@alxlastur looks good to me! Ready to merge.

@arnopo please have a look.

arnopo
arnopo reviewed Oct 11, 2024
View reviewed changes
lib/open-amp/Kconfig Outdated
@@ -18,3 +18,19 @@ config OPENAMP_RSC_TABLE_NUM_RPMSG_BUFF
help
This option specifies the number of buffer used in a Vring for
interprocessor communication

config IPM_RX_CHANNEL_ID
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related to the resource table not to iPM so config should be renamed
OPENAMP_RSC_TABLE_IPM_RX_ID or something similar

lib/open-amp/resource_table.h Outdated
#define VRING0_ID 0 /* (master to remote) fixed to 0 for Linux compatibility */
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONFIG_IPM_RX_CHANNEL_ID has a default value set to 0

Having only following code

#define VRING0_ID CONFIG_IPM_RX_CHANNEL_ID

should work in all cases, no need to use #ifdef CONFIG_IPM_RX_CHANNEL_ID ( TBC by tests)

@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from bbf90ae to 9ff2b8e Compare October 14, 2024 11:58
dbaluta
dbaluta reviewed Oct 14, 2024
View reviewed changes
lib/open-amp/resource_table.h Outdated
#define VRING_RX_ADDRESS -1 /* allocated by Master processor */
#define VRING_TX_ADDRESS -1 /* allocated by Master processor */
#define VRING_BUFF_ADDRESS -1 /* allocated by Master processor */
#define VRING_ALIGNMENT 16 /* fixed to match with Linux constraint */

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you changed something here so that it appears in the diff.

-#define VRING_ALIGNMENT         16  /* fixed to match with Linux constraint */
+#define VRING_ALIGNMENT    16 /* fixed to match with Linux constraint */

This is not what we want. If you want to change the spacing or anything else we can later create a separate patch.

@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from 9ff2b8e to 987143f Compare October 14, 2024 13:47
@alxlastur
Copy link
Contributor Author

made a new push. @dbaluta @arnopo please have look when you have time.

@dleach02 dleach02 requested review from arnopo and dbaluta October 15, 2024 03:36
arnopo
arnopo requested changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A last minor update on comment

arnopo
arnopo reviewed Oct 15, 2024
View reviewed changes
lib/open-amp/resource_table.h Outdated
#define VRING1_ID \
CONFIG_OPENAMP_RSC_TABLE_IPM_TX_ID /* (remote to master) default to 1 for \
* Linux compatibility \
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiline comment format is not correct and the term master should be removed.
Furthermore the sentence default to 0/1 for Linux compatibility seems deprecated now , you can remove it.

proposal:
#define VRING0_ID CONFIG_OPENAMP_RSC_TABLE_IPM_RX_ID /* (host to remote) /
#define VRING1_ID CONFIG_OPENAMP_RSC_TABLE_IPM_TX_ID / (remote to host) */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. @arnopo please have a look. Thank you

@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from 987143f to b995459 Compare October 15, 2024 08:16
@alxlastur
samples: openamp_rsc_table: Add the option to use predefined vring ID
14b0f95 
Currently, Zephyr is always sending back notifications to
AP (e.g Linux in our case) on channel 0.

But this currently doesn't work if Linux uses other channel
id for communication. So, add option to use predefined
vring ID that can accomodate Linux used ID.

Signed-off-by: Alexandru Lastur <[email protected]>
@alxlastur alxlastur force-pushed the zephyr_opanamp_rsc_imx8mp_m7_dev_1 branch from b995459 to 14b0f95 Compare October 15, 2024 08:23
@alxlastur
Copy link
Contributor Author

@dbaluta please have a look

@carlescufi carlescufi merged commit ec2dd19 into zephyrproject-rtos:main Oct 15, 2024
23 checks passed
@github-actions GitHub Actions
Copy link

Hi @alxlastur!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnopo arnopo arnopo approved these changes

@pdgendt pdgendt pdgendt approved these changes

@carlocaione carlocaione Awaiting requested review from carlocaione

@kartben kartben Awaiting requested review from kartben

@nashif nashif Awaiting requested review from nashif

@doki-nordic doki-nordic Awaiting requested review from doki-nordic

@dbaluta dbaluta Awaiting requested review from dbaluta

Assignees

@carlocaione carlocaione

@arnopo arnopo

Labels
area: IPC Inter-Process Communication area: Open AMP area: Samples Samples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alxlastur @dbaluta @pdgendt @arnopo @carlocaione @carlescufi @zephyrbot