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.

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

examples: dual_qemu_ivhshmem: add RPMsg over IVSHMEM sample code. #11

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

uLipe
Copy link
Collaborator

@uLipe uLipe commented Jun 21, 2023

The additions include a backend to glue the Zephyr IVSHMEM device driver into the openAMP code making it usable to send data between two QEMU instances using the RPMsg protocol.

Also a custom shell command in the host side application is provided to send string messages for a number of times allowing to perform stress test of the communication.

This sample has a limitation regarding the order of initialization of the QEMU host and side instances, the host side must be initialized first, followed by the remote side please refer the README of this sample for more info.

@uLipe uLipe marked this pull request as draft June 21, 2023 19:54
west.yml Outdated Show resolved Hide resolved
@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from 9bc11a7 to 5eca56b Compare June 21, 2023 20:28
@uLipe uLipe marked this pull request as ready for review June 21, 2023 20:34
@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from 5eca56b to 209836d Compare June 21, 2023 20:40
@uLipe uLipe changed the title WIP: examples: rpmsg_over_ivshmem: add RPMsg over IVSHMEM sample code. WIP: examples: dual_qemu_ivhshmem: add RPMsg over IVSHMEM sample code. Jun 21, 2023
@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch 2 times, most recently from bba9e7e to 25fdf05 Compare June 22, 2023 14:59
@uLipe
Copy link
Collaborator Author

uLipe commented Jun 22, 2023

@wmamills , @danmilea , @arnopo just tagging you to follow-up, but feel free to review and reproduce this one.

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from 25fdf05 to b1bb14e Compare June 23, 2023 11:44
@uLipe uLipe changed the title WIP: examples: dual_qemu_ivhshmem: add RPMsg over IVSHMEM sample code. examples: dual_qemu_ivhshmem: add RPMsg over IVSHMEM sample code. Jun 23, 2023
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.

2 minor comments else LGTM

west.yml Outdated Show resolved Hide resolved
@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from b1bb14e to c233361 Compare June 26, 2023 13:43
@uLipe uLipe requested a review from arnopo June 26, 2023 13:43
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.

It seems that a coding rule in Zephyr is to put the code in the src folder. This could be the case for the rpmsg_ivshmem_backend folder, but keeping it that way works for me too.

Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

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

Hello,

Thanks for your patch.
minor comments related to coding style, otherwise looks good to me.

Thanks,
Tanmay

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from c233361 to 998a4a2 Compare July 3, 2023 14:00
@uLipe uLipe requested review from tnmysh and arnopo July 3, 2023 14:01
Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

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

LGTM. Just can we mention vendor-id was taken from zephyr-(version) in README ? That will be good for user to understand how vendor-id is defined.

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from d0d46c3 to a86d734 Compare July 5, 2023 16:17
@uLipe uLipe requested a review from tnmysh July 5, 2023 16:17
@arnopo
Copy link
Collaborator

arnopo commented Jul 13, 2023

@uLipe I run zephyr checkpatch.pl on your PR. there are some errors reported... could you fix them, please?

@TanmayShah-xilinx: any new concerns on this PR?

@uLipe
Copy link
Collaborator Author

uLipe commented Jul 13, 2023

@arnopo , yes sure, I will fix and run checkpatch.pl before pushing again

@uLipe
Copy link
Collaborator Author

uLipe commented Jul 17, 2023

@arnopo , @TanmayShah-xilinx just adressed the zephyr style issues.

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.

I still have issues with checkpatch.
could you squash the fixes and re-run the checkpatch?

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from 2ee492e to d5912c0 Compare July 17, 2023 16:04
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.

Please keep a separate patch for the update of the zephyr version. FYI in OpenAMp we use to fix the issue in the commit directly an not add fixup! on top of the series during review step

Please , could you fix the isfollowing remaining issue report by checkpatch

CHECK: Blank lines aren't necessary after an open brace '{'
#462: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:21:
+{
+

WARNING: line length of 102 exceeds 10 columns
#496: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:55:
+	printf("Hello %s - Host Side, the communication over RPMsg is ready to use!\n", CONFIG_BOARD);

CHECK: Alignment should match open parenthesis
#500: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:59:
+static int cmd_rpmsg_ivshmem_send(const struct shell *sh,
+			   size_t argc, char **argv)

WARNING: line length of 109 exceeds 80 columns
#516: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:75:
+			printf("failed to send message over RPMsg-IVSHMEM to the Remote side: %d\n", status);


CHECK: Alignment should match open parenthesis
#534: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:93:
+SHELL_STATIC_SUBCMD_SET_CREATE(sub_rpmsg_ivshmem,
+						SHELL_CMD_ARG(send, NULL,

CHECK: Alignment should match open parenthesis
#535: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:94:
+						SHELL_CMD_ARG(send, NULL,
+								"Usage: rpmsg_ivshmem send <string> <number of messages>",


CHECK: Alignment should match open parenthesis
#541: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:100:
+SHELL_CMD_ARG_REGISTER(rpmsg_ivshmem, &sub_rpmsg_ivshmem,
+				"Commands to use RPMsg over IVSHMEM",

WARNING: 'supress' may be misspelled - perhaps 'suppress'?
#643: FILE: examples/zephyr/dual_qemu_ivshmem/remote/prj.conf:16:
+# Enable IVSHMEM shell when debugging the driver, default OFF to supress warnings when not used.

WARNING: line length of 104 exceeds 100 columns
#705: FILE: examples/zephyr/dual_qemu_ivshmem/remote/src/main.c:53:
+	printf("Hello %s - Remote Side, the communication over RPMsg is ready to use!\n", CONFIG_BOARD);

CHECK: No space is necessary after a cast
#760: FILE: examples/zephyr/dual_qemu_ivshmem/rpmsg_ivshmem_backend/rpmsg_ivshmem_backend.c:48:
+			.virt       = (void *) 0,

CHECK: Please don't use multiple blank lines
#921: FILE: examples/zephyr/dual_qemu_ivshmem/rpmsg_ivshmem_backend/rpmsg_ivshmem_backend.c:209:
+
+

CHECK: Alignment should match open parenthesis
#927: FILE: examples/zephyr/dual_qemu_ivshmem/rpmsg_ivshmem_backend/rpmsg_ivshmem_backend.c:215:
+	k_thread_create(&ivshmem_ev_loop_thread,
+		ivshmem_ev_loop_stack,

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from d5912c0 to 6f65e75 Compare July 19, 2023 00:03
@uLipe uLipe requested a review from arnopo July 19, 2023 00:03
@uLipe
Copy link
Collaborator Author

uLipe commented Jul 19, 2023

It is strange, my checkpatch.pl (under zephyr/scripts) did not catch these style issues, I addressed them manually btw.

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch 4 times, most recently from 83bb6db to f1c8cec Compare July 20, 2023 16:28
@uLipe
Copy link
Collaborator Author

uLipe commented Jul 20, 2023

@arnopo , PTAL again, I ran the checkpatch using the same command you are using, the remaining issues are gone, but it stills report duplicate sign-off, I checked manually the patch and I did not find a reason for this being reported.

@arnopo
Copy link
Collaborator

arnopo commented Jul 20, 2023

@arnopo , PTAL again, I ran the checkpatch using the same command you are using, the remaining issues are gone, but it stills report duplicate sign-off, I checked manually the patch and I did not find a reason for this being reported.

You can ignore the signature duplication. It because the patch 11.patch from github contain both commits.

From your last version I can see only 3 check remaining that could be fixed, else it is ok

CHECK: Alignment should match open parenthesis
#498: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:58:
+static int cmd_rpmsg_ivshmem_send(const struct shell *sh, size_t argc,
+				char **argv)

CHECK: Alignment should match open parenthesis
#532: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:92:
+SHELL_STATIC_SUBCMD_SET_CREATE(sub_rpmsg_ivshmem,
+		SHELL_CMD_ARG(send, NULL,

CHECK: Alignment should match open parenthesis
#533: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:93:
+		SHELL_CMD_ARG(send, NULL,
+				"Usage: rpmsg_ivshmem send <string> <number of messages>",

CHECK: Alignment should match open parenthesis
#538: FILE: examples/zephyr/dual_qemu_ivshmem/host/src/main.c:98:
+SHELL_CMD_ARG_REGISTER(rpmsg_ivshmem, &sub_rpmsg_ivshmem,
+		"Commands to use RPMsg over IVSHMEM",


@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch 2 times, most recently from f3f12bb to 575ab2b Compare July 20, 2023 17:10
@uLipe
Copy link
Collaborator Author

uLipe commented Jul 20, 2023

It is strange, no matter how I align these, or change my editor settings, these three error never disappear, I even copied the alignment from other Zephyr samples and subsystems with no luck, as well trying to align mixing spaces and tabs, is there something I'm missing? @arnopo

@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch 6 times, most recently from 4b7184f to 04f4f14 Compare July 20, 2023 21:15
The additions include a backend to glue the Zephyr IVSHMEM
device driver into the openAMP code making it usable to send
data between two QEMU instances using the RPMsg protocol.

Also a custom shell command in the host side application
is provided to send string messages for a number of times.

Signed-off-by: Felipe Neves <felipe.neves@linaro.org>
This revision contains the IVSHMEM infrastructure
needed to make the RPMsg over it to work.

Signed-off-by: Felipe Neves <felipe.neves@linaro.org>
@uLipe uLipe force-pushed the feature/zephyr-rpmsg-ivshmem branch from 04f4f14 to 5b94086 Compare July 20, 2023 21:18
@uLipe
Copy link
Collaborator Author

uLipe commented Jul 20, 2023

@arnopo the problem was in my editor local config, I was able to fix the alignment warning, thank you.

@uLipe uLipe requested a review from arnopo July 20, 2023 21:20
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.

LGTM

Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

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

LGTM

@tnmysh
Copy link
Collaborator

tnmysh commented Aug 5, 2023

@wmamills if all look good, can we merge this change? Me and @arnopo have approved.

@arnopo arnopo merged commit 9c75250 into OpenAMP:main Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants