Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Ludovic, On Tue, May 22, 2012 at 3:51 PM, frq09524 ludovic.ba...@stericsson.com wrote: In my previous patch, to find the correct subdevice that match with physical memory, I used pa member of rproc_mem_entry. I'm not sure we want to find these subdevices using the physical memory of the expected region. Doing so means more work when setting up those numbers initially, and may also break if/when those number changes. What do you think about looking up those subdevices by names? E.g. before allocating the vrings, we may look for a subdevice called vrings-mem. If one exists, we'll use it when invoking the DMA API. If not, we'll just use the default rproc device. This way we don't need to change the firmware if/when we change the memory regions allocated for each purpose. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Michal, On Tue, May 22, 2012 at 8:51 AM, Michal Simek mon...@monstr.eu wrote: Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work. Sure - you'll need to actually plug the vrings allocation code there, too (i.e. this requires some coding, it's not a 1-liner). BTW: I am using kernel modules and there is no dependency for virtio_rpmsg_bus in remoteproc module in sense if you load remoteproc module you also need to load virtio_rpmsg_bus to get firmware to work. This is expected: remoteproc only provides means for booting and controlling remote processors and it doesn't boot them upfront. To actually boot a remote processor, and do something useful with it, one needs to load a module which utilizes remoteproc's API (e.g. virtio_rpmsg_bus, but YMMV). ~ # lsmod ~ # modprobe zynq_remoteproc zynq_remoteproc: Unknown symbol rproc_register (err 0) zynq_remoteproc: Unknown symbol rproc_alloc (err 0) zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0) zynq_remoteproc: Unknown symbol rproc_free (err 0) zynq_remoteproc: Unknown symbol rproc_unregister (err 0) We don't have these errors - have you tried looking into them ? Btw do you plan on submitting the code for zynq_remoteproc ? Can you see the same behavior on omap? Except the Unknown symbol errors, everything else looks good. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Ohad and Michal yes I can share my patch, remoteproc has evolved and my patch is not aligned on latest version of remote proc (especially since remoteproc: remove the single rpmsg vdev limitation). Ohad, for alignment I can take the latest branch of kernel.org (remoteproc) branch for-next? /BR Ludovic Barré On 05/22/2012 07:51 AM, Michal Simek wrote: Hi Ohad and Ludovic, 2012/5/21 Ohad Ben-Cohen o...@wizery.com mailto:o...@wizery.com Hi Michal, On Mon, May 21, 2012 at 4:02 PM, Michal Simek mon...@monstr.eu mailto:mon...@monstr.eu wrote: I have looked at it and tested on latest and greatest and the problem is still there. Ok, I see why this is happening. We're now allocating the vrings' DMA in -find_vqs() just before we boot the remote processor, and we release it on -del_vqs(). These are the virtio handlers for setting up and tearing down the vrings, so it makes sense to do so, but as a result, we still don't get the early carveout allocation you wanted. I have looked at the code and path is rproc_fw_config_virtio - rproc_handliefirtion_rsc - rproc_handle_vdev Please note that rproc_handle_vdev doesn't allocate any DMA at this point, and the allocation path is now quite different - it begins with virtio_rpmsg_bus probing and then calling into the -find_vqs() handler. For me it is necessary to ensure that dma_alloc_coherent in rproc_alloc_vring is called after carveout allocation just because dma_alloc_cohorent takes mermory from preallocated pool which starts at 0x0. ok. I also don't have any other advance mechanism how to ensure mapping for the second arm core. Probably the best way to ensure this (without it breaking unexpectedly again as remoteproc evolves) is to have separate sub-devices for different kind of memories, which remoteproc could then look up using something like device_find_child(). Ludovic is exploring this direction too, because STE also needs to bind specific memory regions with predefined purposes, without depending on the order of DMA allocations. It is good that someone else has similar requirements. I think Ludovic already has a preliminary patch which he may be able to share with you. Ludovic: Can you share your patch with us? I would like to try it. Here is log which I need to get. Look at attached patch which exactly show you what I need to do which I have tested. Yes, I see. Another temporary approach might be moving the vrings' allocations to the rproc_handle_rsc table (in the RSC_VDEV slot), but I'm not sure we want to merge this as well because this solution too is only hiding the real issue and not solving it permanently. Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work. BTW: I am using kernel modules and there is no dependency for virtio_rpmsg_bus in remoteproc module in sense if you load remoteproc module you also need to load virtio_rpmsg_bus to get firmware to work. I think that will be good to create that dependency automatically because you need to load virtio_rpmsg_bus when you problem remoteproc module. ~ # lsmod ~ # modprobe zynq_remoteproc zynq_remoteproc: Unknown symbol rproc_register (err 0) zynq_remoteproc: Unknown symbol rproc_alloc (err 0) zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0) zynq_remoteproc: Unknown symbol rproc_free (err 0) zynq_remoteproc: Unknown symbol rproc_unregister (err 0) CPU1: shutdown zynq_remoteproc 0.remoteproc-test: 0.remoteproc-test is available zynq_remoteproc 0.remoteproc-test: Note: remoteproc is still under development and considered experimental. zynq_remoteproc 0.remoteproc-test: THE BINARY FORMAT IS NOT YET FINALIZED, and backward compatibility isn't yet guaranteed. ~ # zynq_remoteproc 0.remoteproc-test: registered virtio0 (type 7) ~ # lsmod zynq_remoteproc 4346 0 - Live 0xbf01f000 remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000 virtio 2840 1 remoteproc, Live 0xbf008000 virtio_ring 4965 1 remoteproc, Live 0xbf013000 ~ # ~ # modprobe virtio_rpmsg_bus zynq_remoteproc 0.remoteproc-test: powering up 0.remoteproc-test zynq_remoteproc 0.remoteproc-test: Booting fw image test, size 2351790 zynq_remoteproc 0.remoteproc-test: remote processor 0.remoteproc-test is now up virtio_rpmsg_bus virtio0: rpmsg host is online virtio_rpmsg_bus virtio0: creating channel rpmsg-timer-statistic addr 0x50 ~ # ~ # lsmod virtio_rpmsg_bus 7575 0 - Live 0xbf024000 zynq_remoteproc 4346 1 - Live 0xbf01f000 remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000 virtio 2840 2 virtio_rpmsg_bus,remoteproc, Live 0xbf008000 virtio_ring 4965 2 virtio_rpmsg_bus,remoteproc, Live 0xbf013000 ~ # Can you see the same behavior on omap? Thanks, Michal -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Ludovic, On Tue, May 22, 2012 at 12:14 PM, frq09524 ludovic.ba...@stericsson.com wrote: Ohad, for alignment I can take the latest branch of kernel.org (remoteproc) branch for-next? Sure, it's pretty much updated sans a few simple changes. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
hi Ohad
In my previous patch, to find the correct subdevice that match with
physical memory, I used pa member of rproc_mem_entry.
but today in these 2 resources: fw_rsc_trace, fw_rsc_vdev_vring, pa
menber has been removed.
for fw_rsc_trace it's not a problem, because use rproc_da_to_va and
carveout feature.
but for fw_rsc_vdev_vring, I think use da member, like this.
dev = rproc_dma_find_dev(rproc, (phys_addr_t) vring-da);
va = dma_alloc_coherent(dev, size, dma, GFP_KERNEL);
if (!va) {
dev_err(dev, dma_alloc_coherent failed\n);
return -EINVAL;
}
are you OK ?
/BR
Ludovic Barré
On 05/22/2012 11:22 AM, Ohad Ben-Cohen wrote:
Hi Ludovic,
On Tue, May 22, 2012 at 12:14 PM, frq09524ludovic.ba...@stericsson.com wrote:
Ohad, for alignment I can take the latest branch of kernel.org (remoteproc)
branch for-next?
Sure, it's pretty much updated sans a few simple changes.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Michal, On Mon, May 21, 2012 at 4:02 PM, Michal Simek mon...@monstr.eu wrote: I have looked at it and tested on latest and greatest and the problem is still there. Ok, I see why this is happening. We're now allocating the vrings' DMA in -find_vqs() just before we boot the remote processor, and we release it on -del_vqs(). These are the virtio handlers for setting up and tearing down the vrings, so it makes sense to do so, but as a result, we still don't get the early carveout allocation you wanted. I have looked at the code and path is rproc_fw_config_virtio - rproc_handliefirtion_rsc - rproc_handle_vdev Please note that rproc_handle_vdev doesn't allocate any DMA at this point, and the allocation path is now quite different - it begins with virtio_rpmsg_bus probing and then calling into the -find_vqs() handler. For me it is necessary to ensure that dma_alloc_coherent in rproc_alloc_vring is called after carveout allocation just because dma_alloc_cohorent takes mermory from preallocated pool which starts at 0x0. ok. I also don't have any other advance mechanism how to ensure mapping for the second arm core. Probably the best way to ensure this (without it breaking unexpectedly again as remoteproc evolves) is to have separate sub-devices for different kind of memories, which remoteproc could then look up using something like device_find_child(). Ludovic is exploring this direction too, because STE also needs to bind specific memory regions with predefined purposes, without depending on the order of DMA allocations. I think Ludovic already has a preliminary patch which he may be able to share with you. Here is log which I need to get. Look at attached patch which exactly show you what I need to do which I have tested. Yes, I see. Another temporary approach might be moving the vrings' allocations to the rproc_handle_rsc table (in the RSC_VDEV slot), but I'm not sure we want to merge this as well because this solution too is only hiding the real issue and not solving it permanently. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Michal, On Sat, Mar 17, 2012 at 8:39 AM, Ohad Ben-Cohen o...@wizery.com wrote: IOW: you can probably just wait a bit until this patch is ready and take it into your tree. It will most likely bring back the behavior you need :) Does something like the attached help ? Thanks, Ohad. 0001-remoteproc-allocate-vrings-on-demand-free-when-not-n.patch Description: Binary data
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Ohad,
On Fri, Mar 16, 2012 at 4:57 PM, Michal Simek mon...@monstr.eu wrote:
In previous version I had carverout first which means that code was copyied
to physical address 0x0 and then vrigns were allocated. Current code
allocate vring first and then RTOS is not able to run from zero address.
In general, depending on specific allocation ordering is error prone
and you shouldn't do it.
Yes, I agree with you. It was in v1.
For a given device, we treat memory allocated with
dma_alloc_coherent() as homogeneous. We care about its utilization and
alignments of allocations rather than their specific location in the
total region.
For zynq platform is very important to use addresses from 0 for rtos.
The main reason why it is so important is ARM reset vectors.
If that's not the case, and there's special memory you want assigned
with a specific purpose, than it's recommended to let the code make
sure it happens explicitly, and not rely on implicit order of
allocation.
Sure.
Today there's no way to assign several memory regions with a specific
device, but a reasonable workaround that's being used is creating
several sub-devices with the special memory, and then looking them up
with stuff like device_for_each_child() or device_find_child().
About your specific issue: the current allocation scheme was chosen
for simplicity, but I do plan to change it back by postponing the
vring dma allocation until a remote processor is really powered on.
It's not a lot of memory, but this way we'd only allocate it when
really needed (so if it's CMA, others could use it).
IOW: you can probably just wait a bit until this patch is ready and
take it into your tree. It will most likely bring back the behavior you
need :)
OK. I will use v1 where allocation is based on order in resource table.
It is not ideal but it is working.
From my point of view will be the best to call dma_alloc_coherent only once
which will allocate the whole chunk of memory which is required for RTOS.
My resource table is easy:
#define TEXT_BASE 0
struct resource_table __resource resources = {
1, /* we're the first version that implements this */
3, /* number of entries in the table */
0, 0, /* reserved, must be zero */
/* offsets to entries */
{
offsetof(struct resource_table, text_cout),
offsetof(struct resource_table, rpmsg_vdev),
offsetof(struct resource_table, trace),
},
{
TYPE_CARVEOUT, 0, 0, SZ_1M, 0, 0, MEM_DATA,
},
{
TYPE_VDEV, VIRTIO_ID_RPMSG, 0,
RPMSG_IPU_C0_FEATURES, 0, 0, 0, 2, { 0, 0 },
/* no config data */
},
/* the two vrings */
{ TEXT_BASE + SZ_1M, 4096, 256, 1, 0 },
{ TEXT_BASE + SZ_1M + 0x4000, 4096, 256, 2, 0 },
{
TYPE_TRACE, TEXT_BASE + 0x4, 0x8000, 0, trace,
},
};
But as you see I need to keep that offset structure which is
that text/data starts at address 0, vrings0 on 0x10, vrigns1 on 0x104000
and trace buffer 0x14.
If dma_alloc_coherent is called just once with size taken from resource table.
(For my case for example 2MB=0x20) and offset are driven by remoteproc then
it should just work.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Michal, On Fri, Mar 16, 2012 at 4:57 PM, Michal Simek mon...@monstr.eu wrote: In previous version I had carverout first which means that code was copyied to physical address 0x0 and then vrigns were allocated. Current code allocate vring first and then RTOS is not able to run from zero address. In general, depending on specific allocation ordering is error prone and you shouldn't do it. For a given device, we treat memory allocated with dma_alloc_coherent() as homogeneous. We care about its utilization and alignments of allocations rather than their specific location in the total region. If that's not the case, and there's special memory you want assigned with a specific purpose, than it's recommended to let the code make sure it happens explicitly, and not rely on implicit order of allocation. Today there's no way to assign several memory regions with a specific device, but a reasonable workaround that's being used is creating several sub-devices with the special memory, and then looking them up with stuff like device_for_each_child() or device_find_child(). About your specific issue: the current allocation scheme was chosen for simplicity, but I do plan to change it back by postponing the vring dma allocation until a remote processor is really powered on. It's not a lot of memory, but this way we'd only allocate it when really needed (so if it's CMA, others could use it). IOW: you can probably just wait a bit until this patch is ready and take it into your tree. It will most likely bring back the behavior you need :) Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] remoteproc: resource table overhaul
Hi Ohad, Ohad Ben-Cohen wrote: The resource table is an array of 'struct fw_resource' members, where each resource entry is expressed as a single member of that array. This approach got us this far, but it has a few drawbacks: 1. Different resource entries end up overloading the same members of 'struct fw_resource' with different meanings. The resulting code is error prone and hard to read and maintain. 2. It's impossible to extend 'struct fw_resource' without breaking the existing firmware images (and we already want to: we can't introduce the new virito device resource entry with the current scheme). 3. It doesn't scale: 'struct fw_resource' must be as big as the largest resource entry type. As a result, smaller resource entries end up utilizing only small part of it. This is fixed by defining a dedicated structure for every resource type, and then converting the resource table to a list of type-value members. Instead of a rigid array of homogeneous structs, the resource table is turned into a collection of heterogeneous structures. This way: 1. Resource entries consume exactly the amount of bytes they need. 2. It's easy to extend: just create a new resource entry structure, and assign it a new type. 3. The code is easier to read and maintain: the structures' members names are meaningful. While we're at it, this patch has several other resource table changes: 1. The resource table gains a simple header which contains the number of entries in the table and their offsets within the table. This makes the parsing code simpler and easier to read. 2. A version member is added to the resource table. Should we change the format again, we'll bump up this version to prevent breakage with existing firmware images. 3. The VRING and VIRTIO_DEV resource entries are combined to a single VDEV entry. This paves the way to supporting multiple VDEV entries. 4. Since we don't really support 64-bit rprocs yet, convert two stray u64 members to u32. I don't want to look for in this patch but there is one change which completely break my remoteproc for dual core ARM. I am using ohad-github/rpmsg_3.3_rc6_TLV branch and I have also changed resource table to get it work. Here is the part of log where you can see what happen. rproc_handle_virtio_rsc: rsc type 0 rproc_handle_virtio_rsc: rsc type 3 vdev rsc: id 7, dfeatures 1, cfg len 0, 2 vrings vdev rsc: vring0: da 10, qsz 256, align 4096 vring0: va e100 dma 0 size 3000 idr 0 vdev rsc: vring1: da 104000, qsz 256, align 4096 vring1: va e1004000 dma 4000 size 3000 idr 1 zynq-rproc zynq-rproc.1: powering up cpu1_freertos zynq-rproc zynq-rproc.1: Booting fw image test, size 28 iommu not found rsc: type 0 carveout rsc: da 0, pa 0, len 10, flags 0 carveout va e110, dma 10, len 0x10 rsc: type 3 rsc: type 2 trace0 added: va e114, da 0x4, len 0x8000 phdr: type 1 da 0x0 memsz 0x1ae60 filesz 0x14014 The problem which I have is that vdev entry and vrings are handled first and in __rproc_handle_vring calls dma_alloc_coherent first which caused that virtaddr points to memory start (remapped to virt addr 0xe100) which should be used by code. In previous version I had carverout first which means that code was copyied to physical address 0x0 and then vrigns were allocated. Current code allocate vring first and then RTOS is not able to run from zero address. Thanks, Michal -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] remoteproc: resource table overhaul
The resource table is an array of 'struct fw_resource' members, where
each resource entry is expressed as a single member of that array.
This approach got us this far, but it has a few drawbacks:
1. Different resource entries end up overloading the same members of 'struct
fw_resource' with different meanings. The resulting code is error prone
and hard to read and maintain.
2. It's impossible to extend 'struct fw_resource' without breaking the
existing firmware images (and we already want to: we can't introduce the
new virito device resource entry with the current scheme).
3. It doesn't scale: 'struct fw_resource' must be as big as the largest
resource entry type. As a result, smaller resource entries end up
utilizing only small part of it.
This is fixed by defining a dedicated structure for every resource type,
and then converting the resource table to a list of type-value members.
Instead of a rigid array of homogeneous structs, the resource table
is turned into a collection of heterogeneous structures.
This way:
1. Resource entries consume exactly the amount of bytes they need.
2. It's easy to extend: just create a new resource entry structure, and assign
it a new type.
3. The code is easier to read and maintain: the structures' members names are
meaningful.
While we're at it, this patch has several other resource table changes:
1. The resource table gains a simple header which contains the
number of entries in the table and their offsets within the table. This
makes the parsing code simpler and easier to read.
2. A version member is added to the resource table. Should we change the
format again, we'll bump up this version to prevent breakage with
existing firmware images.
3. The VRING and VIRTIO_DEV resource entries are combined to a single
VDEV entry. This paves the way to supporting multiple VDEV entries.
4. Since we don't really support 64-bit rprocs yet, convert two stray u64
members to u32.
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
Cc: Brian Swetland swetl...@google.com
Cc: Iliyan Malchev malc...@google.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Mark Grosen mgro...@ti.com
Cc: John Williams john.willi...@petalogix.com
Cc: Michal Simek mon...@monstr.eu
Cc: Loic PALLARDY loic.palla...@stericsson.com
Cc: Ludovic BARRE ludovic.ba...@stericsson.com
Cc: Omar Ramirez Luna omar.l...@linaro.org
Cc: Guzman Lugo Fernando fernando.l...@ti.com
Cc: Anna Suman s-a...@ti.com
Cc: Clark Rob r...@ti.com
Cc: Stephen Boyd sb...@codeaurora.org
Cc: Saravana Kannan skan...@codeaurora.org
Cc: David Brown dav...@codeaurora.org
Cc: Kieran Bingham kieranbing...@gmail.com
Cc: Tony Lindgren t...@atomide.com
---
Documentation/remoteproc.txt | 127 +++
drivers/remoteproc/remoteproc_core.c | 306 +++---
include/linux/remoteproc.h | 289 ++--
3 files changed, 505 insertions(+), 217 deletions(-)
diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
index 23ff734..07057ca 100644
--- a/Documentation/remoteproc.txt
+++ b/Documentation/remoteproc.txt
@@ -221,43 +221,52 @@ resource entries that publish the existence of supported
features
or configurations by the remote processor, such as trace buffers and
supported virtio devices (and their configurations).
-Currently the resource table is just an array of:
+The resource table begins with this header:
/**
- * struct fw_resource - describes an entry from the resource section
+ * struct resource_table - firmware resource table header
+ * @ver: version number
+ * @num: number of resource entries
+ * @reserved: reserved (must be zero)
+ * @offset: array of offsets pointing at the various resource entries
+ *
+ * The header of the resource table, as expressed by this structure,
+ * contains a version number (should we need to change this format in the
+ * future), the number of available resource entries, and their offsets
+ * in the table.
+ */
+struct resource_table {
+ u32 ver;
+ u32 num;
+ u32 reserved[2];
+ u32 offset[0];
+} __packed;
+
+Immediately following this header are the resource entries themselves,
+each of which begins with the following resource entry header:
+
+/**
+ * struct fw_rsc_hdr - firmware resource entry header
* @type: resource type
- * @id: index number of the resource
- * @da: device address of the resource
- * @pa: physical address of the resource
- * @len: size, in bytes, of the resource
- * @flags: properties of the resource, e.g. iommu protection required
- * @reserved: must be 0 atm
- * @name: name of resource
+ * @data: resource data
+ *
+ * Every resource entry begins with a 'struct fw_rsc_hdr' header providing
+ * its @type. The content of the entry itself will immediately follow
+ * this header, and it should be parsed according to the resource type.
*/
-struct fw_resource {
