Re: [RFC 1/8] drivers: add generic remoteproc framework
On Tue, Jun 28, 2011 at 2:29 AM, Russell King - ARM Linux wrote: > (Don't have the original message to reply to...) Sorry about that. My recent emails to linux-arm-kernel were bounced with a "Message has a suspicious header" reason. not sure what am I doing wrong.. > Do we really want to end up with header being 5 bytes, header_len set > as 5, and having to load/store all this data using byte loads/stores ? As Grant said, we're trying to move to ELF now, but if we do end up sticking with this custom format eventually, we'll sure do take care of all the alignment issues. Thanks! -- 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: [RFC 1/8] drivers: add generic remoteproc framework
Hi Grant,
Thanks a lot for the exhaustive review and comments !
On Mon, Jun 27, 2011 at 11:49 PM, Grant Likely
wrote:
>> + my_rproc = rproc_get("ipu");
>
> I tend to be suspicious of apis whose primary interface is by-name
> lookup. It works fine when the system is small, but it can get
> unwieldy when the client driver doesn't have a direct relation to the
> setup code that chooses the name. At some point I suspect that there
> will need to be different lookup mechanism, such as which AMP
> processor is currently available (if there are multiple of the same
> type).
Yeah, this might be too limiting on some systems. I gave this a little
thought, but decided to wait until those systems show up first, so I
we can better understand them/their requirements/use-cases. For now,
I've just followed this simple name-based API model (which still seem
a bit popular in several SoC drivers I've looked at, probably due to
the general simplicity of it and its use cases).
> It also leaves no option for drivers to obtain a reference to the
> rproc instance, and bring it up/down as needed (without the name
> lookup every time).
..
> That said, it looks like only the rproc_get() api is using by-name
> lookup, and everything else is via the structure. Can (should) the
> by-name lookup part be factored out into a rproc_get_by_name()
> accessor?
I think you are looking for a different set of API here, probably
something that is better implemented using runtime PM.
When a driver calls rproc_get(), not only does it power on the remote
processor, but it also makes sure the underlying implementation cannot
go away (i.e. platform-specific remoteproc module cannot be removed,
and the rproc cannot be unregistered).
After it calls rproc_put(), it cannot rely anymore on the remote
processor to stick around (the rproc can be unregistered at this
point), so the next time it needs it, it must go through the full
get-by-name (or any other get API we will come up with eventually)
getter API.
If drivers need to hold onto the rproc instance, but still explicitly
allow it to power off at times, they should probably call something
like pm_runtime_put(rproc->dev).
(remoteproc runtime PM support is not implemented yet, but is
definitely planned, so we can suspend remote processors on
inactivity).
> Since rproc_register is allocating a struct rproc instance that
> represent the device, shouldn't the pointer to that device be returned
> to the caller?
Yes, it definitely should. We will have the underlying implementation
remember it, and then pass it to rproc_unregister when needed.
>> + int rproc_unregister(const char *name);
>
> I definitely would not do this by name. I think it is better to pass
> the actual instance pointer to rproc_unregister.
Much better, yeah.
> Naive question: Why is bootaddr an argument? Wouldn't rproc drivers
> keep track of the boot address in their driver private data?
Mark already got that one, but basically the boot address comes from
the firmware image: we need to let the implementation know the
physical address where the text section is mapped. This is ignored on
implementations where that address is fixed (e.g. OMAP's M3).
> Other have commented on the image format, so I'll skip this bit other
> than saying that I agree it would be great to have a common format.
We are evaluating now moving to ELF; let's see how it goes. Using a
standard format is an advantage (as long as it's not overly
complicated), but I wonder if achieving a common format is really
feasible and whether eventually different platforms will need
different binary formats anyway, and we'll have to abstract this out
of remoteproc (guess that as usual, we just need to start off with
something, and then evolve as requirements show up).
>> +Most likely this kind of static allocations of hardware resources for
>> +remote processors can also use DT, so it's interesting to see how
>> +this all work out when DT materializes.
>
> I imagine that it will be quite straight forward. There will probably
> be a node in the tree to represent each slave AMP processor, and other
> devices attached to it could be represented using 'phandle' links
> between the nodes. Any configuration of the AMP process can be
> handled with arbitrary device-specific properties in the AMP
> processor's node.
That sounds good. The dilemma is bigger, though.
The kind of stuff we need to synchronize about are not really
describing the hardware; it's more a runtime policy/configuration than
a hardware description.
As Brian mentioned in the other thread:
> The resource information is a description of
> what resources the firmware requires to work properly (it needs
> certain amounts of working memory, timers, peripheral interfaces like
> i2c to control camera hw, etc), which will be specific to a given
> firmware build.
Some of those resources will be allocated dynamically using an rpmsg
driver (developed by Fernando Guzman Lugo), but some must be supplied
RE: [RFC 1/8] drivers: add generic remoteproc framework
> From: Grant Likely > Sent: Monday, June 27, 2011 3:24 PM > > Our AMPs (remote processors) have a variety of boot mechanisms that vary > > across the different SoCs (yes, TI likes HW diversity). In some cases, the > > boot address is more like an entry point and that comes from the firmware, > > so it is not a static attribute of a driver. Correct me if I misunderstood > > your question. > > More to the point, I would expect the boot_address to be a property of > the rproc instance because it represents the configuration of the > remote processor. It seems odd that the caller of ->start would know > better than the rproc driver about the entry point of the processor. > > g. Yes, in many cases the boot_address will be defined by the HW. However, we have processors that are "soft" - the boot_address comes from the particular firmware being loaded and can (will) be different with each firmware image. We factored out the firmware loader to be device-independent (in remoteproc.c) so it's not repeated in each device-specific implementation like omap_remoteproc.c and davinci_remoteproc.c. In the cases where the HW dictates what happens, the start() method should just ignore the boot_address. Mark -- 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: [RFC 1/8] drivers: add generic remoteproc framework
On Mon, Jun 27, 2011 at 5:29 PM, Russell King - ARM Linux
wrote:
> On Mon, Jun 27, 2011 at 02:49:58PM -0600, Grant Likely wrote:
>> > +struct {
>> > + char magic[4] = { 'R', 'P', 'R', 'C' };
>> > + u32 version;
>> > + u32 header_len;
>> > + char header[...] = { header_len bytes of unformatted, textual
>> > header };
>> > + struct section {
>> > + u32 type;
>> > + u64 da;
>> > + u32 len;
>> > + u8 content[...] = { len bytes of binary data };
>> > + } [ no limit on number of sections ];
>> > +} __packed;
>>
>> Other have commented on the image format, so I'll skip this bit other
>> than saying that I agree it would be great to have a common format.
>
> (Don't have the original message to reply to...)
>
> Do we really want to end up with header being 5 bytes, header_len set
> as 5, and having to load/store all this data using byte loads/stores ?
>
> If we don't want that, then I suggest we get rid of the packed attribute,
> and require stuff to be naturally aligned.
>
> First issue is that struct section could be much better layed out:
>
> struct section {
> u32 type;
> u32 len;
> u64 da;
> u8 content[];
> };
>
> and require sizeof(struct section) % sizeof(u64) == 0 - iow, to find
> the next section, round len up to sizeof(u64). Ditto for the header.
Hopefully this will all be moot since it has been proposed to use elf
images directly.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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: [RFC 1/8] drivers: add generic remoteproc framework
On Mon, Jun 27, 2011 at 02:49:58PM -0600, Grant Likely wrote:
> > +struct {
> > + char magic[4] = { 'R', 'P', 'R', 'C' };
> > + u32 version;
> > + u32 header_len;
> > + char header[...] = { header_len bytes of unformatted, textual header
> > };
> > + struct section {
> > + u32 type;
> > + u64 da;
> > + u32 len;
> > + u8 content[...] = { len bytes of binary data };
> > + } [ no limit on number of sections ];
> > +} __packed;
>
> Other have commented on the image format, so I'll skip this bit other
> than saying that I agree it would be great to have a common format.
(Don't have the original message to reply to...)
Do we really want to end up with header being 5 bytes, header_len set
as 5, and having to load/store all this data using byte loads/stores ?
If we don't want that, then I suggest we get rid of the packed attribute,
and require stuff to be naturally aligned.
First issue is that struct section could be much better layed out:
struct section {
u32 type;
u32 len;
u64 da;
u8 content[];
};
and require sizeof(struct section) % sizeof(u64) == 0 - iow, to find
the next section, round len up to sizeof(u64). Ditto for the header.
--
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: [RFC 1/8] drivers: add generic remoteproc framework
On Mon, Jun 27, 2011 at 09:52:30PM +, Grosen, Mark wrote:
> > From: Grant Likely
> > Sent: Monday, June 27, 2011 1:50 PM
>
> Grant, thanks for the feedback. I'll try to answer one of your
> questions below and leave the rest for Ohad.
>
> Mark
>
> > > +Every remoteproc implementation must provide these handlers:
> > > +
> > > +struct rproc_ops {
> > > + int (*start)(struct rproc *rproc, u64 bootaddr);
> > > + int (*stop)(struct rproc *rproc);
> > > +};
> > > +
> > > +The ->start() handler takes a rproc handle and an optional bootaddr
> > argument,
> > > +and should power on the device and boot it (using the bootaddr
> > argument
> > > +if the hardware requires one).
> >
> > Naive question: Why is bootaddr an argument? Wouldn't rproc drivers
> > keep track of the boot address in their driver private data?
>
> Our AMPs (remote processors) have a variety of boot mechanisms that vary
> across the different SoCs (yes, TI likes HW diversity). In some cases, the
> boot address is more like an entry point and that comes from the firmware,
> so it is not a static attribute of a driver. Correct me if I misunderstood
> your question.
More to the point, I would expect the boot_address to be a property of
the rproc instance because it represents the configuration of the
remote processor. It seems odd that the caller of ->start would know
better than the rproc driver about the entry point of the processor.
g.
--
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: [RFC 1/8] drivers: add generic remoteproc framework
> From: Grant Likely
> Sent: Monday, June 27, 2011 1:50 PM
Grant, thanks for the feedback. I'll try to answer one of your
questions below and leave the rest for Ohad.
Mark
> > +Every remoteproc implementation must provide these handlers:
> > +
> > +struct rproc_ops {
> > + int (*start)(struct rproc *rproc, u64 bootaddr);
> > + int (*stop)(struct rproc *rproc);
> > +};
> > +
> > +The ->start() handler takes a rproc handle and an optional bootaddr
> argument,
> > +and should power on the device and boot it (using the bootaddr
> argument
> > +if the hardware requires one).
>
> Naive question: Why is bootaddr an argument? Wouldn't rproc drivers
> keep track of the boot address in their driver private data?
Our AMPs (remote processors) have a variety of boot mechanisms that vary
across the different SoCs (yes, TI likes HW diversity). In some cases, the
boot address is more like an entry point and that comes from the firmware,
so it is not a static attribute of a driver. Correct me if I misunderstood
your question.
Mark
--
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: [RFC 1/8] drivers: add generic remoteproc framework
On Tue, Jun 21, 2011 at 10:18:27AM +0300, Ohad Ben-Cohen wrote:
> Some systems have slave heterogeneous remote processor devices,
> that are usually used to offload cpu-intensive computations
> (e.g. multimedia codec tasks).
>
> Booting a remote processor typically involves:
> - Loading a firmware which contains the OS image (mainly text and data)
> - If needed, programming an IOMMU
> - Powering on the device
>
> This patch introduces a generic remoteproc framework that allows drivers
> to start and stop those remote processor devices, load up their firmware
> (which might not necessarily be Linux-based), and in the future also
> support power management and error recovery.
>
> It's still not clear how much this is really reusable for other
> platforms/architectures, especially the part that deals with the
> firmware.
>
> Moreover, it's not entirely clear whether this should really be an
> independent layer, or if it should just be squashed with the host-specific
> component of the rpmsg framework (there isn't really a remoteproc use case
> that doesn't require rpmsg).
>
> That said, it did prove useful for us on two completely different
> platforms: OMAP and Davinci, each with its different remote
> processor (Cortex-M3 and a C674x DSP, respectively). So to avoid
> egregious duplication of code, remoteproc must not be omap-only.
>
> Firmware loader is based on code by Mark Grosen .
>
> TODO:
> - drop rproc_da_to_pa(), use iommu_iova_to_phys() instead
> (requires completion of omap's iommu migration and some generic iommu
>API work)
> - instead of ioremapping reserved memory and handling IOMMUs, consider
> moving to the generic DMA mapping API (with a CMA backend)
>
> Signed-off-by: Ohad Ben-Cohen
Hi Ohad,
Overall, looks pretty nice to me. Comments below...
> ---
> Documentation/remoteproc.txt| 170 +
> drivers/Kconfig |2 +
> drivers/Makefile|1 +
> drivers/remoteproc/Kconfig |7 +
> drivers/remoteproc/Makefile |5 +
> drivers/remoteproc/remoteproc.c | 780
> +++
> include/linux/remoteproc.h | 273 ++
> 7 files changed, 1238 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/remoteproc.txt
> create mode 100644 drivers/remoteproc/Kconfig
> create mode 100644 drivers/remoteproc/Makefile
> create mode 100644 drivers/remoteproc/remoteproc.c
> create mode 100644 include/linux/remoteproc.h
>
> diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
> new file mode 100644
> index 000..3075813
> --- /dev/null
> +++ b/Documentation/remoteproc.txt
> @@ -0,0 +1,170 @@
> +Remote Processor Framework
> +
> +1. Introduction
> +
> +Modern SoCs typically have heterogeneous remote processor devices in
> asymmetric
> +multiprocessing (AMP) configurations, which may be running different
> instances
> +of operating system, whether it's Linux or any other flavor of real-time OS.
> +
> +OMAP4, for example, has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP.
> +In a typical configuration, the dual cortex-A9 is running Linux in a SMP
> +configuration, and each of the other three cores (two M3 cores and a DSP)
> +is running its own instance of RTOS in an AMP configuration.
> +
> +The generic remoteproc driver allows different platforms/architectures to
> +control (power on, load firmware, power off) those remote processors while
> +abstracting the hardware differences, so the entire driver doesn't need to be
> +duplicated.
> +
> +2. User API
> +
> + struct rproc *rproc_get(const char *name);
> + - power up the remote processor, identified by the 'name' argument,
> + and boot it. If the remote processor is already powered on, the
> + function immediately succeeds.
> + On success, returns the rproc handle. On failure, NULL is returned.
> +
> + void rproc_put(struct rproc *rproc);
> + - power off the remote processor, identified by the rproc handle.
> + Every call to rproc_get() must be (eventually) accompanied by a call
> + to rproc_put(). Calling rproc_put() redundantly is a bug.
> + Note: the remote processor will actually be powered off only when the
> + last user calls rproc_put().
> +
> +3. Typical usage
> +
> +#include
> +
> +int dummy_rproc_example(void)
> +{
> + struct rproc *my_rproc;
> +
> + /* let's power on and boot the image processing unit */
> + my_rproc = rproc_get("ipu");
I tend to be suspicious of apis whose primary interface is by-name
lookup. It works fine when the system is small, but it can get
unwieldy when the client driver doesn't have a direct relation to the
setup code that chooses the name. At some point I suspect that there
will need to be different lookup mechanism, such as which AMP
processor is currently available (if there are multiple of the same
type).
It also leaves no option for drivers to obtain a reference to the
rproc instance, and bring it up/down as need
Re: [RFC 1/8] drivers: add generic remoteproc framework
Hi Randy, On Wed, Jun 22, 2011 at 8:55 PM, Randy Dunlap wrote: > On Tue, 21 Jun 2011 10:18:27 +0300 Ohad Ben-Cohen wrote: > > Hi, > Just a few minor nits inline... 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: [RFC 1/8] drivers: add generic remoteproc framework
On Tue, 21 Jun 2011 10:18:27 +0300 Ohad Ben-Cohen wrote:
Hi,
Just a few minor nits inline...
> diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
> new file mode 100644
> index 000..3075813
> --- /dev/null
> +++ b/Documentation/remoteproc.txt
> @@ -0,0 +1,170 @@
> +Remote Processor Framework
> +
> +1. Introduction
> +
> +Modern SoCs typically have heterogeneous remote processor devices in
> asymmetric
> +multiprocessing (AMP) configurations, which may be running different
> instances
> +of operating system, whether it's Linux or any other flavor of real-time OS.
> +
> +OMAP4, for example, has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP.
> +In a typical configuration, the dual cortex-A9 is running Linux in a SMP
> +configuration, and each of the other three cores (two M3 cores and a DSP)
> +is running its own instance of RTOS in an AMP configuration.
> +
> +The generic remoteproc driver allows different platforms/architectures to
> +control (power on, load firmware, power off) those remote processors while
> +abstracting the hardware differences, so the entire driver doesn't need to be
> +duplicated.
> +
> +2. User API
> +
> + struct rproc *rproc_get(const char *name);
> + - power up the remote processor, identified by the 'name' argument,
> + and boot it. If the remote processor is already powered on, the
> + function immediately succeeds.
> + On success, returns the rproc handle. On failure, NULL is returned.
> +
> + void rproc_put(struct rproc *rproc);
> + - power off the remote processor, identified by the rproc handle.
> + Every call to rproc_get() must be (eventually) accompanied by a call
> + to rproc_put(). Calling rproc_put() redundantly is a bug.
> + Note: the remote processor will actually be powered off only when the
> + last user calls rproc_put().
> +
> +3. Typical usage
> +
> +#include
> +
> +int dummy_rproc_example(void)
> +{
> + struct rproc *my_rproc;
> +
> + /* let's power on and boot the image processing unit */
> + my_rproc = rproc_get("ipu");
> + if (!my_rproc) {
> + /*
> + * something went wrong. handle it and leave.
> + */
> + }
> +
> + /*
> + * the 'ipu' remote processor is now powered on... let it work !
> + */
> +
> + /* if we no longer need ipu's services, power it down */
> + rproc_put(my_rproc);
> +}
> +
> +4. API for implementors
> +
> + int rproc_register(struct device *dev, const char *name,
> + const struct rproc_ops *ops,
> + const char *firmware,
> + const struct rproc_mem_entry *memory_maps,
> + struct module *owner);
> + - should be called from the underlying platform-specific implementation,
> in
> + order to register a new remoteproc device. 'dev' is the underlying
> + device, 'name' is the name of the remote processor, which will be
> + specified by users calling rproc_get(), 'ops' is the platform-specific
> + start/stop handlers, 'firmware' is the name of the firmware file to
> + boot the processor with, 'memory_maps' is a table of da<->pa memory
> + mappings which should be used to configure the IOMMU (if not relevant,
> + just pass NULL here), 'owner' is the underlying module that should
> + not be removed while the remote processor is in use.
> +
> + Returns 0 on success, or an appropriate error code on failure.
> +
> + int rproc_unregister(const char *name);
> + - should be called from the underlying platform-specific implementation,
> in
> + order to unregister a remoteproc device that was previously registered
> + with rproc_register().
> +
> +5. Implementation callbacks
> +
> +Every remoteproc implementation must provide these handlers:
> +
> +struct rproc_ops {
> + int (*start)(struct rproc *rproc, u64 bootaddr);
> + int (*stop)(struct rproc *rproc);
> +};
> +
> +The ->start() handler takes a rproc handle and an optional bootaddr argument,
an rproc
> +and should power on the device and boot it (using the bootaddr argument
> +if the hardware requires one).
> +On success, 0 is returned, and on failure, an appropriate error code.
> +
> +The ->stop() handler takes a rproc handle and powers the device off.
an rproc
> +On success, 0 is returned, and on failure, an appropriate error code.
> +
> +6. Binary Firmware Structure
> +
> +The following enums and structures define the binary format of the images
> +remoteproc loads and boot the remote processors with.
boots
> +
> +The general binary format is as follows:
> +
> +struct {
> + char magic[4] = { 'R', 'P', 'R', 'C' };
> + u32 version;
> + u32 header_len;
> + char header[...] = { header_len bytes of unformatted, textual header };
> + struct section {
> + u32 type;
> +
