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.
Change-Id: I9f86d19538c75f2361287a136e4aa7e3351189e7
Signed-off-by: Arjun Gopalan <[email protected]>
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index f56c8ba..7b006e5 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -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 <[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:
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 82a6739..9780784 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -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,
On Fri, Aug 30, 2013 at 9:20 PM, Arjun Gopalan <[email protected]> wrote:
>
> 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.
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.
On Sun, Sep 08, 2013 at 02:27:11PM +0200, Ohad Ben-Cohen wrote:
> Hi Arjun,
>
> On Fri, Aug 30, 2013 at 9:20 PM, Arjun Gopalan <[email protected]> wrote:
> >
> > 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.
>
> 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.
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?
> 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.
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
On Mon, Sep 9, 2013 at 2:57 PM, Thierry Reding <[email protected]> wrote:
> If I understand correctly, the way that the services should be announced
> is via RSC_VDEV entries in the resource table?
Yes.
> 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?
Yeah, it generally does.
> > 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.
>
> 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'm not aware of any.
> 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.
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.
On Mon, Sep 09, 2013 at 10:06:24PM +0200, Ohad Ben-Cohen wrote:
> On Mon, Sep 9, 2013 at 2:57 PM, Thierry Reding <[email protected]> wrote:
[...]
> > Looking at the remoteproc core, it seems that there's no direct way to
> > change the firmware running on a remoteproc at runtime.
>
> 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).
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.
> Does that help?
Yes, thanks very much for confirming.
Thierry
On 09/08/2013 05:27 AM, Ohad Ben-Cohen wrote:
> Hi Arjun,
>
> On Fri, Aug 30, 2013 at 9:20 PM, Arjun Gopalan <[email protected]> wrote:
>> 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.
> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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