Re: [RFC PATCH 03/10] remoteproc: add remoteproc virtio transport driver

Hi Tanmay,

On Tue, 25 Jul 2023 at 08:08, Tanmay Shah tanmay.shah@amd.com wrote:

Remoteproc virtio is virtio transport layer driver for remoteproc. Implement virtio config ops used by actual virtio driver using remoteproc virtio device

Ported from the Linux kernel version 6.4-rc2 (d848a4819d85), Introduced in kernel version v3.10 (ac8954a41393) file: drivers/remoteproc/remoteproc_virtio.c.

Signed-off-by: Tanmay Shah tanmay.shah@amd.com

MAINTAINERS | 6 + drivers/remoteproc/Kconfig | 11 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/rproc-uclass.c | 42 ++- drivers/remoteproc/rproc_virtio.c | 422 ++++++++++++++++++++++++++++++ drivers/virtio/virtio_ring.c | 16 ++ include/remoteproc.h | 66 +++-- include/rproc_virtio.h | 29 ++ include/virtio.h | 3 + include/virtio_ring.h | 17 ++ 10 files changed, 593 insertions(+), 20 deletions(-) create mode 100644 drivers/remoteproc/rproc_virtio.c create mode 100644 include/rproc_virtio.h

Can you split this patch up a bit? It seems to add a new method and lots of other stuff.

diff --git a/MAINTAINERS b/MAINTAINERS index 87991cccdd..c4a32a0956 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1319,6 +1319,12 @@ S: Maintained T: git https://source.denx.de/u-boot/custodians/u-boot-nand-flash.git F: drivers/mtd/nand/raw/

+REMOTEPROC +M: Tanmay Shah tanmay.shah@amd.com +S: Maintained +F: drivers/remoteproc/rproc_virtio.c +F: include/rproc_virtio.h

RISC-V M: Rick Chen rick@andestech.com M: Leo ycliang@andestech.com diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 27e4a60ff5..b758c248e4 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -102,4 +102,15 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.

+config REMOTEPROC_VIRTIO

•   bool "Support remoteproc virtio devices"
  

•   select REMOTEPROC
  

•   select VIRTIO
  

•   depends on DM
  

•   help
  

•     Say 'y' here to add support of remoteproc virtio devices.
  

•     rproc_virtio is virtio transport layer driver. The transport
  

•     drivers provide a set of ops for the real virtio device
  

•     driver to call.
  

endmenu diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index fbe9c172bc..61fdb87efb 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -16,3 +16,4 @@ obj-$(CONFIG_REMOTEPROC_TI_K3_R5F) += ti_k3_r5f_rproc.o obj-$(CONFIG_REMOTEPROC_TI_POWER) += ti_power_proc.o obj-$(CONFIG_REMOTEPROC_TI_PRU) += pru_rproc.o obj-$(CONFIG_REMOTEPROC_TI_IPU) += ipu_rproc.o +obj-$(CONFIG_REMOTEPROC_VIRTIO) += rproc_virtio.o diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index d697639cdd..3aebaf6187 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -14,6 +14,7 @@ #include <malloc.h> #include <virtio_ring.h> #include <remoteproc.h> +#include <rproc_virtio.h> #include <asm/io.h> #include <dm/device-internal.h> #include <dm.h> @@ -279,6 +280,33 @@ static int rproc_config_pagetable(struct udevice *dev, unsigned int virt, return 0; }

+/**

• • rproc_find_res_by_name() - After parsing the resource table add the mappings
• • @dev: device we finished probing
• • @name: name of rproc_mem_entry resource
• • Return: If failed NULL, else first carveout entry with matching name.
• */

+struct rproc_mem_entry *rproc_find_res_by_name(struct udevice *dev,

•                                          const char *name)
  

+{

•   struct rproc *rproc = rproc_get_cfg(dev);
  

•   struct rproc_mem_entry *mapping = NULL;
  

•   int ret;
  

•   if (!rproc)
  

•           return NULL;
  

•   list_for_each_entry(mapping, &rproc->mappings.node, node) {
  

•           ret = strcmp(mapping->name, name);
  

•           if (!ret)
  

Do we need ret?

•                   return mapping;
  

•   }
  

•   debug("%s: %s carveout not found\n", dev->name, name);
  

We typically put a blank line before the final return

•   return NULL;
  

+}

UCLASS_DRIVER(rproc) = { .id = UCLASS_REMOTEPROC, .name = "remoteproc", @@ -688,6 +716,7 @@ static int alloc_vring(struct udevice *dev, struct fw_rsc_vdev *rsc, int i) static int handle_vdev(struct udevice *dev, struct fw_rsc_vdev *rsc, int offset, int avail) {

•   struct rproc *rproc;
    int i, ret;
    void *pa;
  
  

@@ -720,7 +749,18 @@ static int handle_vdev(struct udevice *dev, struct fw_rsc_vdev *rsc, }

    /*

•    * allocate the vrings
  

•    * If virtio device creation is supported, then prefer to get vrings
  

•    * during find_vq op
  

•    */
  

•   rproc = rproc_get_cfg(dev);
  

•   if (rproc && rproc->support_rpmsg_virtio &&
  

•       !(IS_ENABLED(CONFIG_SANDBOX))) {
  

Why the special case for sandbox? Can't it work like real hardware?

•           return rproc_virtio_create_dev(dev, rsc, offset);
  

•   }
  

•   /*
  

•    * allocate the vrings traditional way
     */
  

Single-line comment style

    for (i = 0; i < rsc->num_of_vrings; i++) {
            ret = alloc_vring(dev, rsc, i);

diff --git a/drivers/remoteproc/rproc_virtio.c b/drivers/remoteproc/rproc_virtio.c new file mode 100644 index 0000000000..5e7cdfa12f --- /dev/null +++ b/drivers/remoteproc/rproc_virtio.c @@ -0,0 +1,422 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Remote Processor Messaging transport
• • Copyright (C) 2011 Texas Instruments, Inc.
• • Copyright (C) 2011 Google, Inc.
• • Copyright (C) 2023, Advanced Micro Devices Inc.
• • VirtIO RPMsg transport driver
• • Ported from Linux drivers/remoteproc/remoteproc_virtio.c
• */

+#include <common.h> +#include <dm.h> +#include <log.h> +#include <remoteproc.h> +#include <rproc_virtio.h> +#include <virtio.h> +#include <virtio_ring.h> +#include <virtio_types.h> +#include <asm/io.h> +#include <dm/device_compat.h> +#include <dm/device-internal.h> +#include <linux/bug.h> +#include <linux/compat.h> +#include <linux/err.h>

+/**

• • rproc_flush_dcache() - flush shared memory regions
• • @dev: valid remoteproc virtio device
• • Some platforms have dcache enabled. If dcache is on then data written
• • to shared memory is actually written to cache memory. This function
• • checks if cache is on or not and if it is on, then it performs
• • flush and invalidate operation on address range or reserved memory
• • created by platform driver for remoteproc
• • Return: none
• */

+void rproc_flush_dcache(struct udevice *dev) +{

•   struct rproc *rproc = rproc_get_cfg(dev->parent);
  

•   struct rproc_mem_entry *mapping = NULL;
  

•   struct list_head *mapping_node = NULL;
  

•   /* If dcache is off, don't perform cache operation */
  

•   if (dcache_status() == false)
  

if (!dcache_status())

•           return;
  

•   /*
  

•    * If cache is on, then flush cache
  

•    * specially reserved mem regions for,
  

•    * resource table, vrings and vdevbuffer of platform dev
  

•    */
  

•   list_for_each(mapping_node, &rproc->mappings.node) {
  

•           mapping = container_of(mapping_node, struct rproc_mem_entry,
  

•                                  node);
  

•           /*
  

•            * First flush data so current data from cache is written
  

•            * to actual memory from cache. Then invalidate cache so
  

•            * next time data will be accessed from actual memory
  

•            */
  

•           flush_dcache_range(mapping->da, mapping->da + mapping->len);
  

•           invalidate_dcache_range(mapping->da, mapping->da + mapping->len);
  

•   }
  

+}

[..]

Regards, Simon