Re: [RFC PATCH 04/10] drivers: add RPMsg framework

Hi Tanmay,

On Tue, 25 Jul 2023 at 08:08, Tanmay Shah tanmay.shah@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@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@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@amd.com +S: Maintained +F: drivers/rpmsg/* +F: include/rpmsg.h +F: test/dm/rpmsg.c

SANDBOX M: Simon Glass sjg@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@1 {
  

•           compatible = "sandbox,test-rpmsg";
  

•   };
  

•   rpmsg_2: rpmsg@2 {
  

•           compatible = "sandbox,test-rpmsg";
  

•   };
  

•   rproc_1: rproc@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@wizery.com
• • Brian Swetland swetland@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