Add ipm driver for AMD-Xilinx platforms. #61008

tnmysh · 2023-07-31T19:34:13Z

This PR introduces new IPM driver for AMD-Xilinx platforms.

The sample demo is available in openamp-system-reference repository here that is using this driver to communicate with host processor: https://github.com/OpenAMP/openamp-system-reference/tree/main/examples/zephyr/rpmsg_multi_services

This PR also extends MPU SRAM_PRIV region from 64M to 2G for this demo to work.

openamp shared memory regions can be anywhere in DDR memory withing 2G range. Current SRAM_PRIV region is 64M which prevents access of shared memory (vrings) by RPU (cortex-r5) if it is out of 64M range. This patch allows vrings to be in DDR within 2G address space. Developed-by: Dan Millea Commited by: Tanmay Shah Signed-off-by: Dan Milea <dan.milea@windriver.com> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>

github-actions

Hello @TanmayShah-xilinx, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

uLipe

I have some question and initial minor nitpicks here.

drivers/ipm/Kconfig

drivers/ipm/ipm_xlnx_ipi.c

tnmysh · 2023-08-07T16:02:32Z

@carlocaione @uLipe requesting reviews.

tnmysh · 2023-08-07T16:13:01Z

@uLipe I agree to all the comments, but I will wait for more comments before addressing them. Please let me know explicitly once you are done reviewing this patch set, so I can start working on all the comments at the same time.

carlocaione

Why not using the MBOX API instead that is supporting multiple channels out-of-the-box instead of having to tweak the IPM API?

carlocaione · 2023-08-08T12:45:43Z

drivers/ipm/ipm_xlnx_ipi.c

+#define READ8_REG(reg, offset)       sys_read8((reg) + (offset))
+#define WRITE8_REG(reg, offset, val) sys_write8(val, (reg) + (offset))


useless macros, remove those.

carlocaione · 2023-08-08T12:47:09Z

drivers/ipm/ipm_xlnx_ipi.c

+	uint32_t remote_ipi_ch_bit;
+};
+
+static ALWAYS_INLINE void set_bit(const uint32_t addr, uint32_t offset, uint8_t bit)


Please, use sys_set_bit & co. Same for clear and test.

uLipe · 2023-08-08T13:48:19Z

@carlocaione , I talked to @tnmysh , and the reason of providing the IPM first is to avoid breaking changes on the zephyr system reference applications from openAMP, the samples consumes the IPM driver in the same way the current openamp in-tree samples does ( excluding the new static vrings based, that already use mbox).

Can we move forward with this last one driver? And put the mbox driver on the backlog as we did for the IVSHMEM IPM?

Thanks :)

carlocaione · 2023-08-08T13:49:26Z

Can we move forward with this last one driver? And put the mbox driver on the backlog as we did for the IVSHMEM IPM?

Alright, but something still needs to be addressed in this driver (see my comments).

uLipe · 2023-08-08T13:51:31Z

Got it, thank you!

tnmysh · 2023-08-08T14:54:03Z

Thanks @carlocaione and @uLipe for reviews. If you are done reviewing this patchset, I will address all the comments. If you need more time for reviews please let me know.

uLipe · 2023-08-11T13:44:57Z

@tnmysh I'm done with my review for now :)

carlocaione · 2023-08-12T08:40:51Z

Thanks @carlocaione and @uLipe for reviews. If you are done reviewing this patchset, I will address all the comments. If you need more time for reviews please let me know.

yup, waiting for the new revision.

tnmysh · 2023-08-14T20:05:42Z

@carlocaione, @uLipe

Changes in v2:

use DT_HAS_XLNX_ZYNQMP_IPI_MAILBOX_ENABLED as dependency
in Kconfig
move platform info to private header
remove redundant macros to access registers
use sys_write* and sys_read* APIS to read and write register.
use sys_set_bit, sys_test_bit to set and test bit.
add critical section when sending ipi data

Add ipm driver to use Inter Processor Interrupts on Xilinx ZynqMP platform. This patch also adds sample application that shows use of xlnx ipm driver. This driver uses default arm gic interrupt controller and works only for lockstep mode of cortex-r5f cluster for now. In split mode the cortex-r5 cluster will have two r5f cores and they are expected to work in AMP mode. If both r5f cores run simultaneouly, only one of the core is able to receive IPI interrupts at this time and it will be the one that started later. In future this limitation shall be removed. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>

tnmysh · 2023-08-14T20:27:42Z

Added header xlnx_ipi_ipm.h file in latest push.

uLipe · 2023-08-14T21:02:31Z

drivers/ipm/ipm_xlnx_ipi.c

+#include <zephyr/logging/log.h>
+
+#include "ipm_xlnx_ipi.h"
+LOG_MODULE_REGISTER(ipm_xlnx_ipi, CONFIG_IPM_LOG_LEVEL);


Nitpick: please group the log include files with LOG_MODULE_REGISTER macro.

uLipe

Just left a minor comment, overall LGTM

carlocaione

LGTM just a couple of comments

carlocaione · 2023-08-15T09:30:46Z

drivers/ipm/ipm_xlnx_ipi.c

+	const struct xlnx_ipi_child_config *cdev_conf;
+
+	cdev_conf = dev->config;


I need to remove this. Since, this PR is merged, I will create separate change Thanks.

carlocaione · 2023-08-15T09:36:38Z

drivers/ipm/ipm_xlnx_ipi.c

+		return -EMSGSIZE;
+	}
+
+	key = irq_lock();


any reason to use irq_lock over something SMP safe like using a spinlock?

Hi Carlo, Thanks for reviews.

Actually this driver can be used in SMP mode or AMP mode.
For AMP mode, we may not need spinlocks. For SMP mode we will need spinlocks. I will add that support in separate patch.

Thanks.

@carlocaione @uLipe

I tried added spinlocks, but didn't work as expected. ipi_send can be called from interrupt context. Instead I will have to add workqueue support for each child's callback that are currently being called from ISR. This will be taken care later when SMP use cases are enabled. For now would like to go ahead with simple irq_lock();

github-actions

Hi @tnmysh!
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! 🪁

tnmysh requested review from galak, bbolen and stephanosio as code owners July 31, 2023 19:34

tnmysh changed the title ~~Add AMD-Xilinx Platforms ipm driver~~ Add AMD-Xilinx platforms ipm driver Jul 31, 2023

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: IPM Inter-Processor Mailbox platform: Xilinx Xilinx labels Jul 31, 2023

zephyrbot requested review from dcpleung, henrikbrixandersen and ibirnbaum July 31, 2023 19:34

tnmysh changed the title ~~Add AMD-Xilinx platforms ipm driver~~ Add ipm driver for AMD-Xilinx platforms. Jul 31, 2023

github-actions bot reviewed Jul 31, 2023

View reviewed changes

tnmysh mentioned this pull request Jul 31, 2023

examples: zephyr: add qemu_cortex_r5 board support OpenAMP/openamp-system-reference#14

Merged

uLipe requested changes Aug 7, 2023

View reviewed changes

uLipe requested a review from carlocaione August 7, 2023 16:11

carlocaione requested changes Aug 8, 2023

View reviewed changes

tnmysh force-pushed the xlnx_ipi_ipm branch from 3ab9d12 to 77a9a59 Compare August 14, 2023 20:02

tnmysh force-pushed the xlnx_ipi_ipm branch from 77a9a59 to 5bf566b Compare August 14, 2023 20:25

uLipe reviewed Aug 14, 2023

View reviewed changes

uLipe approved these changes Aug 14, 2023

View reviewed changes

carlocaione approved these changes Aug 15, 2023

View reviewed changes

fabiobaltieri merged commit 09e2a4e into zephyrproject-rtos:main Aug 15, 2023
17 checks passed

github-actions bot reviewed Aug 15, 2023

View reviewed changes

ibirnbaum mentioned this pull request Sep 30, 2024

soc: xlnx: zynqmp: overhaul the Cortex-R MPU setup #79221

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ipm driver for AMD-Xilinx platforms. #61008

Add ipm driver for AMD-Xilinx platforms. #61008

tnmysh commented Jul 31, 2023

github-actions bot left a comment

uLipe left a comment

tnmysh commented Aug 7, 2023

tnmysh commented Aug 7, 2023

carlocaione left a comment

carlocaione Aug 8, 2023

carlocaione Aug 8, 2023

uLipe commented Aug 8, 2023 •

edited

Loading

carlocaione commented Aug 8, 2023

uLipe commented Aug 8, 2023

tnmysh commented Aug 8, 2023

uLipe commented Aug 11, 2023

carlocaione commented Aug 12, 2023

tnmysh commented Aug 14, 2023

tnmysh commented Aug 14, 2023

uLipe Aug 14, 2023

uLipe left a comment

carlocaione left a comment

carlocaione Aug 15, 2023

tnmysh Aug 15, 2023

carlocaione Aug 15, 2023

tnmysh Aug 15, 2023

tnmysh Aug 16, 2023

github-actions bot left a comment

		#define READ8_REG(reg, offset) sys_read8((reg) + (offset))
		#define WRITE8_REG(reg, offset, val) sys_write8(val, (reg) + (offset))

		const struct xlnx_ipi_child_config *cdev_conf;

		cdev_conf = dev->config;

Add ipm driver for AMD-Xilinx platforms. #61008

Add ipm driver for AMD-Xilinx platforms. #61008

Conversation

tnmysh commented Jul 31, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

uLipe left a comment

Choose a reason for hiding this comment

tnmysh commented Aug 7, 2023

tnmysh commented Aug 7, 2023

carlocaione left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uLipe commented Aug 8, 2023 • edited Loading

carlocaione commented Aug 8, 2023

uLipe commented Aug 8, 2023

tnmysh commented Aug 8, 2023

uLipe commented Aug 11, 2023

carlocaione commented Aug 12, 2023

tnmysh commented Aug 14, 2023

tnmysh commented Aug 14, 2023

Choose a reason for hiding this comment

uLipe left a comment

Choose a reason for hiding this comment

carlocaione 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

github-actions bot left a comment

Choose a reason for hiding this comment

uLipe commented Aug 8, 2023 •

edited

Loading