Re: [PATCH v3 2/2] remoteproc: add support to handle internal memories
Ohad, On 02/12/2015 11:20 PM, Ohad Ben-Cohen wrote: On Thu, Feb 12, 2015 at 10:54 PM, Suman Anna s-a...@ti.com wrote: My original motivation was that it would only need to be added on firmwares requiring support for loading into internal memories, otherwise, these are something left to be managed by the software running on the remote processor completely, and MPU will not even touch them. Sure. But even if you guys will use this interface correctly, this patch essentially exposes ioremap to user space, which is something we generally want to avoid. So, let me know if this is a NAK. If so, we have two options - one to go the sram node model where each of them have to be defined separately, and have a specific property in the rproc nodes to be able to get the gen_pool handles. The other one is simply to define these as reg and use devm_ioremap_resource() (so use DT for defining the regions instead of a resource table entry). Any approach where these regions are defined explicitly really sounds better. If you could look into these two alternatives that would be great. OK, will do. Meanwhile, can you pick up Patch 1, that is independent of this patch. regards Suman -- 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 v3 2/2] remoteproc: add support to handle internal memories
* Suman Anna s-a...@ti.com [150213 08:17]: Ohad, On 02/12/2015 11:20 PM, Ohad Ben-Cohen wrote: On Thu, Feb 12, 2015 at 10:54 PM, Suman Anna s-a...@ti.com wrote: My original motivation was that it would only need to be added on firmwares requiring support for loading into internal memories, otherwise, these are something left to be managed by the software running on the remote processor completely, and MPU will not even touch them. Sure. But even if you guys will use this interface correctly, this patch essentially exposes ioremap to user space, which is something we generally want to avoid. So, let me know if this is a NAK. If so, we have two options - one to go the sram node model where each of them have to be defined separately, and have a specific property in the rproc nodes to be able to get the gen_pool handles. The other one is simply to define these as reg and use devm_ioremap_resource() (so use DT for defining the regions instead of a resource table entry). Any approach where these regions are defined explicitly really sounds better. If you could look into these two alternatives that would be great. OK, will do. Meanwhile, can you pick up Patch 1, that is independent of this patch. If the memory are is hardware specific, then it should be specified in the dts file. If some further configuration depending on the firmware version is needed, then you can parse that from the firmware and make sure it's contained within the hardware specific memory area defined in the dts file. I guess in some cases module options may be also needed. Regards, Tony -- 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 v3 2/2] remoteproc: add support to handle internal memories
Hi Ohad,
On 02/12/2015 03:09 AM, Ohad Ben-Cohen wrote:
On Wed, Feb 11, 2015 at 10:57 PM, Tony Lindgren t...@atomide.com wrote:
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
Yes, the proposed interface essentially allows exactly this random
access, since the parameters to ioremap would be provided from the
user space (specifically from the resource table contained within the
firmware of the remote processor).
My original motivation was that it would only need to be added on
firmwares requiring support for loading into internal memories,
otherwise, these are something left to be managed by the software
running on the remote processor completely, and MPU will not even touch
them.
So, let me know if this is a NAK. If so, we have two options - one to go
the sram node model where each of them have to be defined separately,
and have a specific property in the rproc nodes to be able to get the
gen_pool handles. The other one is simply to define these as reg and
use devm_ioremap_resource() (so use DT for defining the regions instead
of a resource table entry).
regards
Suman
--
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 v3 2/2] remoteproc: add support to handle internal memories
On Wed, Feb 11, 2015 at 10:57 PM, Tony Lindgren t...@atomide.com wrote:
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
Yes, the proposed interface essentially allows exactly this random
access, since the parameters to ioremap would be provided from the
user space (specifically from the resource table contained within the
firmware of the remote processor).
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 v3 2/2] remoteproc: add support to handle internal memories
On Thu, Feb 12, 2015 at 10:54 PM, Suman Anna s-a...@ti.com wrote: My original motivation was that it would only need to be added on firmwares requiring support for loading into internal memories, otherwise, these are something left to be managed by the software running on the remote processor completely, and MPU will not even touch them. Sure. But even if you guys will use this interface correctly, this patch essentially exposes ioremap to user space, which is something we generally want to avoid. So, let me know if this is a NAK. If so, we have two options - one to go the sram node model where each of them have to be defined separately, and have a specific property in the rproc nodes to be able to get the gen_pool handles. The other one is simply to define these as reg and use devm_ioremap_resource() (so use DT for defining the regions instead of a resource table entry). Any approach where these regions are defined explicitly really sounds better. If you could look into these two alternatives that would be great. 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 v3 2/2] remoteproc: add support to handle internal memories
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
If it needs to be shared between the kernel and the accelerator,
then is the remoteproc or mailbox somehow needs to coordinating the
shared access to this memory.. I think those cases should be handled
separately and not with a single interface.
Regards,
Tony
--
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 v3 2/2] remoteproc: add support to handle internal memories
* Suman Anna s-a...@ti.com [150211 14:32]:
On 02/11/2015 02:57 PM, Tony Lindgren wrote:
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory
region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
The usage here is not really on regular memory, but on internal device
memory (eg: L2RAM within DSP which is accessible by MPU through L3 bus).
For the regular shared memory for vrings and vring buffers, the
remoteproc core does rely on CMA pools.
OK sounds like Linux needs to access it initially to load the DSP boot
code to L2RAM to get the DSP booted.
Maybe it can be done with the API provided by drivers/misc/sram.c?
You could set up the L2RAM as compatible = mmio-sram and then
parse the optional phandle for that in the remoteproc code, then
allocate some memory from it to load the DSP boot code and free
it.
Regards,
Tony
--
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 v3 2/2] remoteproc: add support to handle internal memories
* Suman Anna s-a...@ti.com [150211 16:05]:
On 02/11/2015 04:48 PM, Tony Lindgren wrote:
* Suman Anna s-a...@ti.com [150211 14:32]:
On 02/11/2015 02:57 PM, Tony Lindgren wrote:
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory
region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource
entry
+ * is primarily used for representing physical internal memories. If
the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries
in
+ * the firmware's resource table, as other firmware entries might
request
+ * placing other data objects inside these memory regions (e.g.
data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct
fw_rsc_intmem *rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
The usage here is not really on regular memory, but on internal device
memory (eg: L2RAM within DSP which is accessible by MPU through L3 bus).
For the regular shared memory for vrings and vring buffers, the
remoteproc core does rely on CMA pools.
OK sounds like Linux needs to access it initially to load the DSP boot
code to L2RAM to get the DSP booted.
Maybe it can be done with the API provided by drivers/misc/sram.c?
You could set up the L2RAM as compatible = mmio-sram and then
parse the optional phandle for that in the remoteproc code, then
allocate some memory from it to load the DSP boot code and free
it.
Not quite the same usage, there are no implicit assumptions on managing
this memory. Isn't the SRAM driver better suited for allocating memory
using the gen_pool API. It is just regular code that is being placed
into RAM, and the linker file on the remoteproc side dictates which
portion we are using. So, the section can be anywhere based on the ELF
parsing. Further, the same RAM space can be partitioned into Cache
and/or RAM, which is usually controlled from internal processor
subsystem register programming.
It still sounds like you need an API like gen_pool to allocate and
load the DSP code though? So from that point of view it's best to
use some Linux generic API.
Just guessing, but the process here is probably something like
request_firmware, configure hardware, allocate memory area,
copy firmware to memory, unallocate memory, boot m3 :)
Regards,
Tony
--
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 v3 2/2] remoteproc: add support to handle internal memories
On 02/11/2015 02:57 PM, Tony Lindgren wrote:
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
The usage here is not really on regular memory, but on internal device
memory (eg: L2RAM within DSP which is accessible by MPU through L3 bus).
For the regular shared memory for vrings and vring buffers, the
remoteproc core does rely on CMA pools.
regards
Suman
--
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 v3 2/2] remoteproc: add support to handle internal memories
On 02/11/2015 04:48 PM, Tony Lindgren wrote:
* Suman Anna s-a...@ti.com [150211 14:32]:
On 02/11/2015 02:57 PM, Tony Lindgren wrote:
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory
region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
The usage here is not really on regular memory, but on internal device
memory (eg: L2RAM within DSP which is accessible by MPU through L3 bus).
For the regular shared memory for vrings and vring buffers, the
remoteproc core does rely on CMA pools.
OK sounds like Linux needs to access it initially to load the DSP boot
code to L2RAM to get the DSP booted.
Maybe it can be done with the API provided by drivers/misc/sram.c?
You could set up the L2RAM as compatible = mmio-sram and then
parse the optional phandle for that in the remoteproc code, then
allocate some memory from it to load the DSP boot code and free
it.
Not quite the same usage, there are no implicit assumptions on managing
this memory. Isn't the SRAM driver better suited for allocating memory
using the gen_pool API. It is just regular code that is being placed
into RAM, and the linker file on the remoteproc side dictates which
portion we are using. So, the section can be anywhere based on the ELF
parsing. Further, the same RAM space can be partitioned into Cache
and/or RAM, which is usually controlled from internal processor
subsystem register programming.
regards
Suman
--
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 v3 2/2] remoteproc: add support to handle internal memories
On 02/11/2015 06:18 PM, Tony Lindgren wrote:
* Suman Anna s-a...@ti.com [150211 16:05]:
On 02/11/2015 04:48 PM, Tony Lindgren wrote:
* Suman Anna s-a...@ti.com [150211 14:32]:
On 02/11/2015 02:57 PM, Tony Lindgren wrote:
* Ohad Ben-Cohen o...@wizery.com [150210 02:14]:
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory
region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource
entry
+ * is primarily used for representing physical internal memories. If
the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries
in
+ * the firmware's resource table, as other firmware entries might
request
+ * placing other data objects inside these memory regions (e.g.
data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct
fw_rsc_intmem *rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
The use of ioremap in general is just fine for drivers as long
as they access a dedicated area to the specific device. Accessing
random registers and memory in the SoC is what I'm worried about.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Hmm sounds like this memory should be dedicated to the accelerator?
In that case it should use memblock to reserve that area early so
the kernel won't be accessing it at all.
The usage here is not really on regular memory, but on internal device
memory (eg: L2RAM within DSP which is accessible by MPU through L3 bus).
For the regular shared memory for vrings and vring buffers, the
remoteproc core does rely on CMA pools.
OK sounds like Linux needs to access it initially to load the DSP boot
code to L2RAM to get the DSP booted.
Maybe it can be done with the API provided by drivers/misc/sram.c?
You could set up the L2RAM as compatible = mmio-sram and then
parse the optional phandle for that in the remoteproc code, then
allocate some memory from it to load the DSP boot code and free
it.
Not quite the same usage, there are no implicit assumptions on managing
this memory. Isn't the SRAM driver better suited for allocating memory
using the gen_pool API. It is just regular code that is being placed
into RAM, and the linker file on the remoteproc side dictates which
portion we are using. So, the section can be anywhere based on the ELF
parsing. Further, the same RAM space can be partitioned into Cache
and/or RAM, which is usually controlled from internal processor
subsystem register programming.
It still sounds like you need an API like gen_pool to allocate and
load the DSP code though? So from that point of view it's best to
use some Linux generic API.
Just guessing, but the process here is probably something like
request_firmware, configure hardware, allocate memory area,
copy firmware to memory, unallocate memory, boot m3 :)
Yeah, atleast for the processors with MMUs, it's usually allocate
memory, program IOMMU, copy firmware, boot rproc. Memory is freed when
unloading the processor and loading a different firmware. For the cases
with internal memory, either I need an ioremap of the region for copying
the firmware sections, or as you said, allocate, copy and unallocate.
That almost always means, I have to allocate the entire region, since I
would need to usually copy the data to a specific location based on the
ELF pheader data. The sram driver also does an ioremap internally, so I
guess it can be done, and probably a bit more code for management within
Re: [PATCH v3 2/2] remoteproc: add support to handle internal memories
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna s-a...@ti.com wrote:
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
...
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
*rsc,
+ int offset, int avail)
+{
...
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
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
[PATCH v3 2/2] remoteproc: add support to handle internal memories
A remote processor may need to load certain firmware sections into
internal memories (eg: RAM at L1 or L2 levels) for performance or
other reasons. Introduce a new resource type (RSC_INTMEM) and add
an associated handler function to handle such memories. The handler
creates a kernel mapping for the resource's 'pa' (physical address).
Note that no iommu mapping is performed for this resource, as the
resource is primarily used to represent physical internal memories.
If the internal memory region can only be accessed through an iommu,
a devmem resource entry should be used instead.
Signed-off-by: Robert Tivy rt...@ti.com
Signed-off-by: Suman Anna s-a...@ti.com
---
v3:
- leverage memcpy_toio and memset_io for loading into internal memory
- rproc_da_to_va takes an additional argument to allow this distinction
drivers/remoteproc/remoteproc_core.c | 89 +-
drivers/remoteproc/remoteproc_elf_loader.c | 23 ++--
drivers/remoteproc/remoteproc_internal.h | 6 +-
include/linux/remoteproc.h | 43 ++-
4 files changed, 150 insertions(+), 11 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 11cdb119e4f3..e0ecc0f802c1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -153,7 +153,7 @@ static void rproc_disable_iommu(struct rproc *rproc)
* but only on kernel direct mapped RAM memory. Instead, we're just using
* here the output of the DMA API, which should be more correct.
*/
-void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+void *rproc_da_to_va(struct rproc *rproc, u64 da, int len, u32 *flags)
{
struct rproc_mem_entry *carveout;
void *ptr = NULL;
@@ -170,6 +170,8 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
continue;
ptr = carveout-va + offset;
+ if (flags carveout-priv)
+ *flags = RPROC_INTMEM;
break;
}
@@ -404,7 +406,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct
fw_rsc_trace *rsc,
}
/* what's the kernel address of this resource ? */
- ptr = rproc_da_to_va(rproc, rsc-da, rsc-len);
+ ptr = rproc_da_to_va(rproc, rsc-da, rsc-len, NULL);
if (!ptr) {
dev_err(dev, erroneous trace resource entry\n);
return -EINVAL;
@@ -664,6 +666,82 @@ free_carv:
return ret;
}
+/**
+ * rproc_handle_intmem() - handle internal memory resource entry
+ * @rproc: rproc handle
+ * @rsc: the intmem resource entry
+ * @offset: offset of the resource data in resource table
+ * @avail: size of available data (for image validation)
+ *
+ * This function will handle firmware requests for mapping a memory region
+ * internal to a remote processor into kernel. It neither allocates any
+ * physical pages, nor performs any iommu mapping, as this resource entry
+ * is primarily used for representing physical internal memories. If the
+ * internal memory region can only be accessed through an iommu, please
+ * use a devmem resource entry.
+ *
+ * These resource entries should be grouped near the carveout entries in
+ * the firmware's resource table, as other firmware entries might request
+ * placing other data objects inside these memory regions (e.g. data/code
+ * segments, trace resource entries, ...).
+ */
+static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem *rsc,
+ int offset, int avail)
+{
+ struct rproc_mem_entry *intmem;
+ struct device *dev = rproc-dev;
+ void *va;
+ int ret;
+
+ if (sizeof(*rsc) avail) {
+ dev_err(dev, intmem rsc is truncated\n);
+ return -EINVAL;
+ }
+
+ if (rsc-version != 1) {
+ dev_err(dev, intmem rsc version %d is not supported\n,
+ rsc-version);
+ return -EINVAL;
+ }
+
+ if (rsc-reserved) {
+ dev_err(dev, intmem rsc has non zero reserved bytes\n);
+ return -EINVAL;
+ }
+
+ dev_dbg(dev, intmem rsc: da 0x%x, pa 0x%x, len 0x%x\n,
+ rsc-da, rsc-pa, rsc-len);
+
+ intmem = kzalloc(sizeof(*intmem), GFP_KERNEL);
+ if (!intmem)
+ return -ENOMEM;
+
+ va = (__force void *)ioremap_nocache(rsc-pa, rsc-len);
+ if (!va) {
+ dev_err(dev, ioremap_nocache err: %d\n, rsc-len);
+ ret = -ENOMEM;
+ goto free_intmem;
+ }
+
+ dev_dbg(dev, intmem mapped pa 0x%x of len 0x%x into kernel va %p\n,
+ rsc-pa, rsc-len, va);
+
+ intmem-va = va;
+ intmem-len = rsc-len;
+ intmem-dma = rsc-pa;
+ intmem-da = rsc-da;
+ intmem-priv = (void *)RPROC_INTMEM;/* prevents freeing */
+
+ /* reuse the rproc-carveouts list, so that loading is automatic */
+
