[RFC PATCH 04/10] drivers: add RPMsg framework
Simon Glass
sjg at chromium.org
Thu Jul 27 02:50:14 CEST 2023
Hi Tanmay,
On Tue, 25 Jul 2023 at 08:08, Tanmay Shah <tanmay.shah at amd.com> wrote:
>
> RPMsg framework is used to communicate to remote processor
> using rpmsg protocol.
>
> This framework is ported from the Linux kernel
> directory: drivers/rpmsg/
> kernel version: 6.4-rc2 (d848a4819d85)
>
> Signed-off-by: Tanmay Shah <tanmay.shah at amd.com>
> ---
> MAINTAINERS | 7 +
> arch/sandbox/dts/test.dts | 8 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/rpmsg/Kconfig | 31 +++
> drivers/rpmsg/Makefile | 10 +
> drivers/rpmsg/rpmsg-uclass.c | 156 ++++++++++++
> drivers/rpmsg/rpmsg_internal.h | 52 ++++
> drivers/rpmsg/sandbox_test_rpmsg.c | 88 +++++++
> drivers/rpmsg/virtio_rpmsg_bus.c | 384 +++++++++++++++++++++++++++++
> drivers/virtio/virtio-uclass.c | 1 +
> include/dm/uclass-id.h | 1 +
> include/rpmsg.h | 140 +++++++++++
> include/rproc_virtio.h | 8 +-
> include/virtio.h | 4 +-
> include/virtio_ring.h | 15 ++
> test/dm/Makefile | 1 +
> test/dm/rpmsg.c | 41 +++
> 18 files changed, 947 insertions(+), 3 deletions(-)
> create mode 100644 drivers/rpmsg/Kconfig
> create mode 100644 drivers/rpmsg/Makefile
> create mode 100644 drivers/rpmsg/rpmsg-uclass.c
> create mode 100644 drivers/rpmsg/rpmsg_internal.h
> create mode 100644 drivers/rpmsg/sandbox_test_rpmsg.c
> create mode 100644 drivers/rpmsg/virtio_rpmsg_bus.c
> create mode 100644 include/rpmsg.h
> create mode 100644 test/dm/rpmsg.c
Reviewed-by: Simon Glass <sjg at chromium.org>
Again there is a lot going on in this patch and it could use a split.
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c4a32a0956..876a7fdbdf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1365,6 +1365,13 @@ F: drivers/usb/gadget/f_rockusb.c
> F: cmd/rockusb.c
> F: doc/README.rockusb
>
> +RPMSG
> +M: Tanmay Shah <tanmay.shah at amd.com>
> +S: Maintained
> +F: drivers/rpmsg/*
> +F: include/rpmsg.h
> +F: test/dm/rpmsg.c
> +
> SANDBOX
> M: Simon Glass <sjg at chromium.org>
> S: Maintained
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index b5509eee8c..fca1a591fb 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -1247,6 +1247,14 @@
> compatible = "sandbox,sandbox-rng";
> };
>
> + rpmsg_1: rpmsg at 1 {
> + compatible = "sandbox,test-rpmsg";
> + };
> +
> + rpmsg_2: rpmsg at 2 {
> + compatible = "sandbox,test-rpmsg";
> + };
> +
> rproc_1: rproc at 1 {
> compatible = "sandbox,test-processor";
> remoteproc-name = "remoteproc-test-dev1";
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index a25f6ae02f..69700f1f83 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -112,6 +112,8 @@ source "drivers/reset/Kconfig"
>
> source "drivers/rng/Kconfig"
>
> +source "drivers/rpmsg/Kconfig"
> +
> source "drivers/rtc/Kconfig"
>
> source "drivers/scsi/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 3bc6d279d7..68e8d8b065 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -109,6 +109,7 @@ obj-y += mfd/
> obj-y += mtd/
> obj-y += pwm/
> obj-y += reset/
> +obj-y += rpmsg/
> obj-y += input/
> obj-y += iommu/
> # SOC specific infrastructure drivers.
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> new file mode 100644
> index 0000000000..4efb8dfcd7
> --- /dev/null
> +++ b/drivers/rpmsg/Kconfig
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2023, Advanced Micro devices, Inc.
> +
> +menu "RPMsg drivers"
> +
> +# RPMsg gets selected by drivers as needed
> +# All users should depend on DM
> +config RPMSG
> + bool
> + depends on DM
> +
> +config VIRTIO_RPMSG_BUS
> + bool "virtio rpmsg bus"
> + depends on DM
> + select RPMSG
> + select REMOTEPROC_VIRTIO
> + help
> + Say 'y' here to enable virtio based RPMsg. RPMsg allows
> + U-Boot drivers to communicate with remote processors.
Please expand abbreviations in Kconfig
> +
> +config RPMSG_SANDBOX
> + bool "RPMsg driver for sandbox platform"
> + depends on DM
> + select RPMSG
> + depends on SANDBOX
> + help
> + Say 'y' here to add sandbox driver for RPMsg framework used
> + for dummy communication with remote processor on sandbox platform
> +
> +endmenu
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> new file mode 100644
> index 0000000000..21611725ea
> --- /dev/null
> +++ b/drivers/rpmsg/Makefile
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2023, Advanced Micro Devices, Inc.
> +
> +obj-$(CONFIG_RPMSG) += rpmsg-uclass.o
> +
> +obj-$(CONFIG_RPMSG_SANDBOX) += sandbox_test_rpmsg.o
> +
> +# virtio driver for rpmsg
> +obj-$(CONFIG_VIRTIO_RPMSG_BUS) += virtio_rpmsg_bus.o
> diff --git a/drivers/rpmsg/rpmsg-uclass.c b/drivers/rpmsg/rpmsg-uclass.c
> new file mode 100644
> index 0000000000..3e749a5827
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg-uclass.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * remote processor messaging bus
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <rpmsg.h>
> +#include <virtio.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/uclass.h>
> +#include <dm/uclass-internal.h>
> +
> +#include "rpmsg_internal.h"
> +
> +int rpmsg_init(int core_id)
> +{
> + struct udevice *udev = NULL;
> + int ret;
> +
> + ret = uclass_find_device_by_seq(UCLASS_RPMSG, core_id, &udev);
> + if (ret) {
> + debug("can't find rpmsg dev for core_id %d\n", core_id);
> + return ret;
> + }
> +
> + ret = device_probe(udev);
> + if (ret)
> + debug("failed to probe rpmsg dev, ret = %d\n", ret);
If you use uclass_get_device_by_seq() it does the probe for you.
> +
> + return ret;
> +}
> +
> +static int rpmsg_find_device(int core_id, struct udevice **rpdev)
> +{
> + int core_count;
> +
> + core_count = uclass_id_count(UCLASS_RPMSG);
> + if (core_id >= core_count) {
> + debug("invalid core id = %d\n", core_id);
> + return -EINVAL;
> + }
> +
> + return uclass_find_device(UCLASS_RPMSG, core_id, rpdev);
> +}
> +
> +/**
> + * rpmsg_send() - send a message across to the remote processor
> + * @core_id: remote processor core id
> + * @data: payload of message
> + * @len: length of payload
> + *
> + * This function sends @data of length @len on the @core_id endpoint.
> + * The message will be sent to the remote processor which the @core_id
> + * belongs to, using @ept's address and its associated rpmsg
> + * device destination addresses.
> + * In case there are no TX buffers available, the function will fail
> + * immediately
> + *
> + * Return: 0 on success and an appropriate error value on failure.
> + */
> +int rpmsg_send(int core_id, void *data, int len)
> +{
> + struct udevice *rpdev = NULL;
> + const struct rpmsg_device_ops *ops;
> + int ret;
> +
> + ret = rpmsg_find_device(core_id, &rpdev);
> + if (ret) {
> + debug("no rpmsg device for core = %d, ret = %d\n", core_id, ret);
> + return ret;
> + }
> + if (!rpdev)
> + return -ENODEV;
> +
> + ops = (const struct rpmsg_device_ops *)device_get_ops(rpdev);
Create an rpmsg_get_ops() macro like done in other subsystems.
> + if (!ops) {
> + debug("send op not registered for device %s\n", rpdev->name);
> + return -EINVAL;
> + }
> +
> + return ops->send(rpdev, data, len);
> +}
> +
[..]
> +void rpmsg_debug_data(int core_id, int vq_id)
> +{
> + struct udevice *rpdev = NULL;
> + const struct rpmsg_device_ops *ops;
> + int ret;
> +
> + if (vq_id > 1)
> + debug("vq_id %d not supported\n", vq_id);
Return error?
> +
> + ret = rpmsg_find_device(core_id, &rpdev);
> + if (ret || !rpdev) {
> + debug("no rpmsg device for core = %d, ret = %d\n", core_id, ret);
> + return;
> + }
> +
> + ops = (const struct rpmsg_device_ops *)device_get_ops(rpdev);
> + if (!ops || !ops->debug_data) {
Generally !ops is not allowed, so we can refuse to bind or probe in
that case and avoid this check in every method.
> + debug("recv op not registered for device %s\n", rpdev->name);
> + return;
> + }
> +
> + ops->debug_data(rpdev, vq_id);
> +}
> +
> +int rpmsg_uclass_init(struct uclass *class)
> +{
> + int ret;
> +
> + /* make sure virtio framework is initialized */
> + ret = virtio_init();
I suppose this is OK. It creates a dependency on virtio just to init
this uclass.
> + if (ret)
> + debug("virtio init failed, %d\n", ret);
> +
> + return ret;
> +}
> +
> +UCLASS_DRIVER(rpmsg_bus) = {
> + .name = "rpmsg_bus",
> + .id = UCLASS_RPMSG,
> + .init = rpmsg_uclass_init,
> + .flags = DM_UC_FLAG_SEQ_ALIAS,
> +};
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> new file mode 100644
> index 0000000000..4f7bf9fb90
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * remote processor messaging bus internals
> + *
> + * Copyright (C) 2011 Texas Instruments, Inc.
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + *
> + * Ohad Ben-Cohen <ohad at wizery.com>
> + * Brian Swetland <swetland at google.com>
> + */
> +
> +#ifndef __RPMSG_INTERNAL_H__
> +#define __RPMSG_INTERNAL_H__
> +
> +#include <rpmsg.h>
> +#include <virtio.h>
> +
> +/**
> + * struct rpmsg_hdr - common header for all rpmsg messages
> + * @src: source address
> + * @dst: destination address
> + * @reserved: reserved for future use
> + * @len: length of payload (in bytes)
> + * @flags: message flags
> + * @data: @len bytes of message payload data
> + *
> + * Every message sent(/received) on the rpmsg bus begins with this header.
> + */
> +struct rpmsg_hdr {
> + __rpmsg32 src;
> + __rpmsg32 dst;
> + __rpmsg32 reserved;
> + __rpmsg16 len;
> + __rpmsg16 flags;
> + u8 data[];
> +} __packed;
> +
> +/**
> + * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @send: send data to core
> + * @recv: recv data to buf. data limited to buf_len bytes and buf is expected
> + * to have that much memory allocated
> + * @debug_data: calls virtqueue_dump debug utility to print vq data
> + */
> +struct rpmsg_device_ops {
> + int (*send)(struct udevice *rpdev, void *data, int len);
> + int (*recv)(struct udevice *rpdev, rpmsg_rx_cb_t cb);
> + void (*debug_data)(struct udevice *rpdev, int vq_id);
We normally write out the function docs in full here.
Need to add declarations for rpmsg_send(), etc. here.
Looks like you have memory leaks in the error paths of your init
functions, but perhaps it doesn't matter.
[..]
> diff --git a/include/rpmsg.h b/include/rpmsg.h
> new file mode 100644
> index 0000000000..bed61240ca
> --- /dev/null
> +++ b/include/rpmsg.h
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _RPMSG_H_
> +#define _RPMSG_H_
> +
> +/**
> + * struct dm_rpmsg_uclass_pdata - platform data for a CPU
> + * @name: Platform-specific way of naming the RPMsg platform device
> + * @driver_plat_data: driver specific platform data that may be needed.
> + *
> + * This can be accessed with dev_get_uclass_plat() for any UCLASS_RPMSG
> + * device.
> + *
> + */
> +struct dm_rpmsg_uclass_pdata {
> + const char *name;
> + void *driver_plat_data;
> +};
> +
> +typedef __u16 __bitwise __rpmsg16;
> +typedef __u32 __bitwise __rpmsg32;
> +typedef __u64 __bitwise __rpmsg64;
> +
> +#define RPMSG_ADDR_ANY 0xFFFFFFFF
> +
> +#define RPMSG_NAME_SIZE 32
> +
> +static inline bool rpmsg_is_little_endian(void)
> +{
> +#ifdef __LITTLE_ENDIAN
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> +static inline u16 __rpmsg16_to_cpu(bool little_endian, __rpmsg16 val)
> +{
> + if (little_endian)
> + return le16_to_cpu((__force __le16)val);
> + else
> + return be16_to_cpu((__force __be16)val);
> +}
> +
> +static inline __rpmsg16 __cpu_to_rpmsg16(bool little_endian, u16 val)
> +{
> + if (little_endian)
> + return (__force __rpmsg16)cpu_to_le16(val);
> + else
> + return (__force __rpmsg16)cpu_to_be16(val);
> +}
> +
> +static inline u32 __rpmsg32_to_cpu(bool little_endian, __rpmsg32 val)
> +{
> + if (little_endian)
> + return le32_to_cpu((__force __le32)val);
> + else
> + return be32_to_cpu((__force __be32)val);
> +}
> +
> +static inline __rpmsg32 __cpu_to_rpmsg32(bool little_endian, u32 val)
> +{
> + if (little_endian)
> + return (__force __rpmsg32)cpu_to_le32(val);
> + else
> + return (__force __rpmsg32)cpu_to_be32(val);
> +}
> +
> +static inline u64 __rpmsg64_to_cpu(bool little_endian, __rpmsg64 val)
> +{
> + if (little_endian)
> + return le64_to_cpu((__force __le64)val);
> + else
> + return be64_to_cpu((__force __be64)val);
> +}
> +
> +static inline __rpmsg64 __cpu_to_rpmsg64(bool little_endian, u64 val)
> +{
> + if (little_endian)
> + return (__force __rpmsg64)cpu_to_le64(val);
> + else
> + return (__force __rpmsg64)cpu_to_be64(val);
> +}
> +
> +static inline u16 rpmsg16_to_cpu(__rpmsg16 val)
> +{
> + return __rpmsg16_to_cpu(rpmsg_is_little_endian(), val);
> +}
> +
> +static inline __rpmsg16 cpu_to_rpmsg16(u16 val)
> +{
> + return __cpu_to_rpmsg16(rpmsg_is_little_endian(), val);
> +}
> +
> +static inline u32 rpmsg32_to_cpu(__rpmsg32 val)
> +{
> + return __rpmsg32_to_cpu(rpmsg_is_little_endian(), val);
> +}
> +
> +static inline __rpmsg32 cpu_to_rpmsg32(u32 val)
> +{
> + return __cpu_to_rpmsg32(rpmsg_is_little_endian(), val);
> +}
> +
> +static inline u64 rpmsg64_to_cpu(__rpmsg64 val)
> +{
> + return __rpmsg64_to_cpu(rpmsg_is_little_endian(), val);
> +}
> +
> +static inline __rpmsg64 cpu_to_rpmsg64(u64 val)
> +{
> + return __cpu_to_rpmsg64(rpmsg_is_little_endian(), val);
> +}
> +
> +typedef int (*rpmsg_rx_cb_t)(void *buf, int msg_len, u32 msgs_received);
> +
> +#if IS_ENABLED(CONFIG_RPMSG)
> +
> +int rpmsg_init(int core_id);
> +int rpmsg_send(int core_id, void *data, int len);
> +int rpmsg_recv(int core_id, rpmsg_rx_cb_t cb);
function comments?
> +
> +#else
> +int rpmsg_init(int core_id)
> +{
> + return -ENODEV;
-ENOSYS
> +}
> +
> +int rpmsg_send(int core_id, void *data, int len)
> +{
> + return -ENODEV;
> +}
> +
> +int rpmsg_recv(int core_id, rpmsg_rx_cb_t cb)
> +{
> + return -ENODEV;
> +}
> +
> +#endif /* IS_ENABLED(CONFIG_RPMSG) */
> +
> +#endif /* _RPMSG_H */
[..]
Regards,
Simon
More information about the U-Boot
mailing list