permission to move definition of struct rpmsg_channel_info

Hi Ohad/Brian,

I have been working on rpmsg and I need to be able to create static rpmsg channels. Channel information needs to be specified by other drivers and for this, the drivers need access to struct rpmsg_channel_info.

So, can I move the definition of struct rpmsg_channel_info from drivers/rpmsg/virtio_rpmsg_bus.c to include/linux/rpmsg.h ?

I have a patch (enclosed inline) for the same. Please let me know if you have any thoughts on this. Thanks.

commit 6b6ddce3bbf89f0e0ae42aae799f7ff9b5c4a05b

Author: Arjun Gopalan <[email protected]>
Date:   Mon Jul 8 17:34:52 2013 -0700

rpmsg: Create static rpmsg channels

Adds support for creating static channels when rpmsg_probe() is called.

NOTE:
-----
This patch also moves the definition of struct rpmsg_channel_info from virtio_rpmsg_bus.c to linux/rpmsg.h because hardcoded static channels need to be used by other drivers.

index f56c8ba..7b006e5 100644
@@ -3,10 +3,14 @@
  *
  * Copyright (C) 2011 Texas Instruments, Inc.
  * Copyright (C) 2011 Google, Inc.
+ * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved.
  *
  * Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
  * Brian Swetland <[email protected]>
  *
+ * Supporting static rpmsg channels derived from TI's kernel
+ * Arjun Gopalan <[email protected]>
+ *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
  * may be copied, distributed, and modified under those terms.

@@ -70,18 +74,6 @@ struct virtproc_info {
     struct rpmsg_endpoint *ns_ept;
 };
 
-/**
- * struct rpmsg_channel_info - internal channel info representation
- * @name: name of service
- * @src: local address
- * @dst: destination address
- */
-struct rpmsg_channel_info {
-    char name[RPMSG_NAME_SIZE];
-    u32 src;
-    u32 dst;
-};
-
 #define to_rpmsg_channel(d) container_of(d, struct rpmsg_channel, dev)
 #define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
 

@@ -935,6 +927,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
     struct virtproc_info *vrp;
     void *bufs_va;
     int err = 0, i;
+    struct rpmsg_channel_info *ch;
 
     vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
     if (!vrp)

@@ -1004,6 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
     dev_info(&vdev->dev, "rpmsg host is online\n");
 
+    vdev->config->get(vdev, VPROC_STATIC_CHANNELS, &ch, sizeof(ch));
+
+    for (i = 0; ch && ch[i].name[0]; i++)
+        rpmsg_create_channel(vrp, &ch[i]);
+
     return 0;
 
 free_coherent:

index 82a6739..9780784 100644
@@ -95,6 +95,11 @@ enum rpmsg_ns_flags {
 
 #define RPMSG_ADDR_ANY        0xFFFFFFFF
 
+/* driver requests */
+enum {
+    VPROC_STATIC_CHANNELS,
+};
+
 struct virtproc_info;
 
 /**

@@ -117,6 +122,18 @@ struct rpmsg_channel {
     bool announce;
 };
 
+/**
+ * struct rpmsg_channel_info - internal channel info representation
+ * @name: name of service
+ * @src: local address
+ * @dst: destination address
+ */
+struct rpmsg_channel_info {
+    char name[RPMSG_NAME_SIZE];
+    u32 src;
+    u32 dst;
+};
+
 typedef void (*rpmsg_rx_cb_t)(struct rpmsg_channel *, void *, int, void *, u32);
 
 /**

Hi Arjun,

I'm not convinced how useful it is for other Linux drivers to create static rpmsg channels?

Usually these channels reflect the existence of services running on the remote processor, and their creation (or lack thereof) should be specified in the remote image. This way an rpmsg channel is published iff there is a matching remote service.

The way we were planning to add static channels functionality (I should still have preliminary patches doing this somewhere but the entire work was put on hold since TI changed its focus) is by statically publishing them in the resource table, which is coupled with a specific remote image.

Best,
Ohad.

If I understand correctly, the way that the services should be announced is via RSC_VDEV entries in the resource table?

Looking at the remoteproc core and ELF loader, it seems like the way to pass in the resource table is either via an extra ELF section or driver specific. TI drivers seem to use ELF, but the STE modem driver seems to employ some custom format.

From what I understand this resource table can also be processed by the host to provide some of the resources such as carveout memory. So that the normal way for this to work would be something like:

- load firmware image
- obtain pointer to resource table
- process resource table
- allocate resources
- update table
- upload firmware to remote processor

Upon which the remote processor probably needs to parse the resource table to set itself up. Does that sound about right?

While I see that the format of all this is pretty well documented, are there any standard tools that can be used to embed the resource table into an ELF binary? I have a pretty good idea on how it could be done, but I wonder if it might make sense to provide some kind of a standard set of headers to make things easier so that not everyone has to reinvent the wheel.

Looking at the remoteproc core, it seems that there's no direct way to change the firmware running on a remoteproc at runtime. The firmware image is bound to one specific struct rproc. Is there something fundamental that would prevent us from adding some sort of hotplug support so that we could reload the remote processor with a different firmware using a different resource table at runtime? A remote processor is only exposed via sysfs, so I assume that's probably where we'd need to hook that up to?

Perhaps I should mention that I'm investigating on using remoteproc for something semi-related to what Arjun is working on, so I expect most of these questions to be relevant to his work as well.

Thierry

Yes.

Yeah, it generally does.

I'm not aware of any.

One way is to unload and load the driver, but you can also use the bind/unbind sysfs files of your driver (echo -n device-name > /sys/..../{un}bind).

Does that help?

Best,
Ohad.

[...]

I'm not quite sure how that solves the issue with replacing the firmware image. The driver will still request the same firmware on rebind, so the only way to have a different firmware loaded would be to replace the file. That sounds sub-optimal. But it's not very important at this stage so it can be deferred until (and if) it becomes a real issue.

Yes, thanks very much for confirming.

Thierry

Hi Ohad,

Thanks for your response. Implementing a name service facility in the remote processor may be an option for us too. But, it would be nice to be able to create static rpmsg channels.

You mentioned that your patches publish rpmsg channels in the resource table which is coupled to the firmware image. In our case, we may not have a dynamic firmware that is loaded. In other words, we may not be using the Linux firmware loading mechanism at all. In that case, how do you think your patches have to be modified ?

Would we probably need static resource tables ?

On similar lines, the remoteproc core also mandates a valid firmware for every remote processor, which again won't work for our case. I have a patch to enable firmware-less remote processors but it is not in perfect shape yet because I haven't integrated virtio and remoteproc as is done in drivers/remoteproc/remoteproc_virtio. This would again possibly require something like static resource tables.

Instead, currently, I have a separate driver that overlaps with some of the functionalities offered by drivers/remoteproc/remoteproc_virtio.c.

Please let me know what your thoughts are on this. Thanks.

Regards,
Arjun