All of lore.kernel.org
 help / color / mirror / Atom feed
* Ongoing remoteproc discussions
@ 2016-07-18 23:10 Bjorn Andersson
  2016-07-25 12:26 ` Peter Griffin
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Bjorn Andersson @ 2016-07-18 23:10 UTC (permalink / raw)
  To: linux-remoteproc
  Cc: linux-kernel, Lee Jones, Sarangdhar Joshi, Loic PALLARDY,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen, Suman Anna

During discussions with various people interested in moving their
remoteproc-related out-of-tree patches towards mainline I have come
across a set of topics common among various teams. The purpose of this
email is to share some insight into these discussions and the current
ideas for moving forward.

== Auto-boot or always-on:
There are cases where we want to achieve the current auto-boot mechanism
without rpmsg and there are cases where we don't want to auto-boot a
remoteproc just because its resource table contains rpmsg entries. So we
need to decouple this logic from the vdev. I suggest that:

* We introduce a property in the rproc struct where drivers can toggle
  if they want always-on or not. We default this to true, as an estimate
  of backwards compatibility.

* We move the allocation of vrings to be done at the time of the other
  allocations and follow that with the registration of virtio devices,
  before actually booting the rproc. And we tear down the virtio devices
  as part of shutdown.

* We remove the rproc_boot()/shutdown() calls from the
  remoteproc_virtio.c and based on rproc->always_on we call these in the
  async firmware loader callback, in rproc_trigger_recovery() and
  rproc_del().


A side effect of this is that the async firmware loading only purpose is
to trigger the boot as the firmware becomes available (something we need
to tweak further). I therefor see no reason to mandate the firmware is
unchanged between boots, which simplifies the posed rproc_set_firmware()
function - as long as it's done on a remoteproc that's not always_on.


== Amended resources:
We need a way for the driver to amend resource definitions of the
firmware with e.g. physical addresses and size constraints from
devicetree, so I suggest that:

* We split the resource table parsing and allocation of resources in two
  steps; where the parse step updates the lists of resources and then at
  the time of boot we run through these and allocate the actual
  resources.

* We expose an API to the drivers so that they can register resources,
  as if they came from the table parser. We match each resource against
  previously registered resources based on name and merge or reject the
  updates. E.g. we merge a reference to the resource table offset and we
  reject incompatible size changes.


== Resource-less firmware:
To be able to use remoteproc with firmware either without a resource
table or resource data in other forms we today provide a resource table
stub in each driver, instead we could refactor the resource table
parsing code.

* We replace the find_rsc_table operation in rproc_fw_ops with a parse
  operation, that uses the newly created API (above) to register the
  resources with the core; largely decoupling the resource table format
  from the remoteproc core.

* We make the parse() function in rproc_fw_ops optional, allowing
  remoteproc drivers to specify that there's no resource parsing to be
  done (they can still provide resources programmatically between
  rproc_alloc() and rproc_add()).

This setup allows custom resource building functions to be implemented,
one such example is the Qualcomm firmware files where most resource data
is a combination of static information (DT) and data from the ELF
header.


== Resource-less firmware with installed resource table:
The now available list of resources that have been registered with the
core can serve as input for a function that generates a resource table
for the remote.  This gives us a mechanism for providing a remoteproc
with information about resources in cases where the firmware lacks a
resource table.

* We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
  tasked with installing the resource table and update rproc->table_ptr.
  This op is made optional, for the remoteprocs with no installed
  resource table.

* We provide a helper function in the core that can be used in the
  fw_op, that builds a resource table in memory and copy this into the
  appropriate address of the remote, and  update rproc->table_ptr to
  this.

The install function will be tasked with making sure table_ptr is valid
and we make sure that error paths out of there and upon shutdown the
core will make table_ptr reference the core's version - which might be
NULL if no resource table exist.


== Supporting rpmsg alternatives:
For systems with other communication mechanisms than rpmsg we still want
to be able to instantiate and tear down devices representing these
communication channels, in a way that follows the life cycle of the
remoteproc. To do this I suggest that:

* Like other resources we split virtio device handling in the remoteproc
  core into adding rproc_vdevs to rproc->rvdevs and actually calling
  rproc_add_virtio_dev().

* We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
  that are registered with the remoteproc; each providing callbacks for
  registering and unregistering themselves as we bring the remoteproc up
  and down.

I made a quick hack of this with the Qualcomm SMD code and it turned out
pretty good, but I believe it's too ugly to post until we get the
cleanups from above tasks sorted out.


== Ramdump:
Being able to acquire core dumps from a miss-behaving remoteproc is an
important feature in product development. I believe this snapshot should
be taken between the shutdown of the remote and freeing of resources. As
such I think it would be appropriate to:

* Split the inner working of rproc_shutdown() into the two steps of
  shutting down the remote and cleaning up its resources. Giving us a
  window of opportunity for snapshotting any resources.

In the generic case we will have to repopulate the resources with data
from the firmware file (in case of corruptions), but we're expecting to
load the same firmware again and as such I see no meaning in releasing
and reacquiring carveouts etc. As such if we split the inner working of
rproc_boot() into resource allocation, virtio device allocation and
booting we can make rproc_trigger_recovery() do:

    * Shutdown the remoteproc
    * Shutdown virtio devices
    * Take snapshot
    * Register virtio devices
    * Start the remoteproc

(The order of the top two is opposite of todays implementation, but
probably more appropriate for the case of getting an accurate snapshot
of the device).

We need to discuss the requirements for the format of what comes out of
this.  I've seen raw memory dumps of a fixed memory segments and I've
seen ELF generators with segments matching those of the loaded ELF in
the past.


== Firmware from late mounted file systems (needs discussion):
Typical for most remoteproc systems seems to be that the firmware files
tend to be way to large for being stored in a initramfs. The two
available mechanisms for dealing with this is to build remoteproc
drivers as modules and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
the latter is being frowned upon by everyone but works fairly well for
our purpose.

Forcing systems to compile remoteproc drivers as modules to achieve the
delayed firmware load is not acceptable in my view.

So we need to come up with some mechanism for triggering auto-booting
when firmware files are stored in a lately mounted file system.



Interwoven in these discussions are of course topics related to rpmsg
and I will try to get back to these as things materialize.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-07-18 23:10 Ongoing remoteproc discussions Bjorn Andersson
@ 2016-07-25 12:26 ` Peter Griffin
  2016-07-28 17:32   ` Bjorn Andersson
  2016-08-03 14:52   ` Loic PALLARDY
  2016-08-11  0:19   ` Suman Anna
  2 siblings, 1 reply; 13+ messages in thread
From: Peter Griffin @ 2016-07-25 12:26 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Loic PALLARDY, Eric FINCO, Russell Wayman, Matthew Locke,
	Kumar Gala, Bill Fletcher, Puja Gupta, Ohad Ben-Cohen,
	Suman Anna

Hi Bjorn,

On Mon, 18 Jul 2016, Bjorn Andersson wrote:

> During discussions with various people interested in moving their
> remoteproc-related out-of-tree patches towards mainline I have come
> across a set of topics common among various teams. The purpose of this
> email is to share some insight into these discussions and the current
> ideas for moving forward.
> 
> == Auto-boot or always-on:
> There are cases where we want to achieve the current auto-boot mechanism
> without rpmsg and there are cases where we don't want to auto-boot a
> remoteproc just because its resource table contains rpmsg entries. So we
> need to decouple this logic from the vdev. I suggest that:
> 
> * We introduce a property in the rproc struct where drivers can toggle
>   if they want always-on or not. We default this to true, as an estimate
>   of backwards compatibility.
> 
> * We move the allocation of vrings to be done at the time of the other
>   allocations and follow that with the registration of virtio devices,
>   before actually booting the rproc. And we tear down the virtio devices
>   as part of shutdown.
> 
> * We remove the rproc_boot()/shutdown() calls from the
>   remoteproc_virtio.c and based on rproc->always_on we call these in the
>   async firmware loader callback, in rproc_trigger_recovery() and
>   rproc_del().

What happens in the built-in case where firnmware isn't available until rootfs
is mounted? In this case async fw load will have failed. 

> 
> 
> A side effect of this is that the async firmware loading only purpose is
> to trigger the boot as the firmware becomes available (something we need
> to tweak further). I therefor see no reason to mandate the firmware is
> unchanged between boots, which simplifies the posed rproc_set_firmware()
> function - as long as it's done on a remoteproc that's not always_on.
> 
> 
> == Amended resources:
> We need a way for the driver to amend resource definitions of the
> firmware with e.g. physical addresses and size constraints from
> devicetree, so I suggest that:
> 
> * We split the resource table parsing and allocation of resources in two
>   steps; where the parse step updates the lists of resources and then at
>   the time of boot we run through these and allocate the actual
>   resources.
> 
> * We expose an API to the drivers so that they can register resources,
>   as if they came from the table parser. We match each resource against
>   previously registered resources based on name and merge or reject the
>   updates. E.g. we merge a reference to the resource table offset and we
>   reject incompatible size changes.
> 
> 
> == Resource-less firmware:
> To be able to use remoteproc with firmware either without a resource
> table or resource data in other forms we today provide a resource table
> stub in each driver, instead we could refactor the resource table
> parsing code.
> 
> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>   operation, that uses the newly created API (above) to register the
>   resources with the core; largely decoupling the resource table format
>   from the remoteproc core.
> 
> * We make the parse() function in rproc_fw_ops optional, allowing
>   remoteproc drivers to specify that there's no resource parsing to be
>   done (they can still provide resources programmatically between
>   rproc_alloc() and rproc_add()).
> 
> This setup allows custom resource building functions to be implemented,
> one such example is the Qualcomm firmware files where most resource data
> is a combination of static information (DT) and data from the ELF
> header.
> 
> 
> == Resource-less firmware with installed resource table:
> The now available list of resources that have been registered with the
> core can serve as input for a function that generates a resource table
> for the remote.  This gives us a mechanism for providing a remoteproc
> with information about resources in cases where the firmware lacks a
> resource table.
> 
> * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
>   tasked with installing the resource table and update rproc->table_ptr.
>   This op is made optional, for the remoteprocs with no installed
>   resource table.
> 
> * We provide a helper function in the core that can be used in the
>   fw_op, that builds a resource table in memory and copy this into the
>   appropriate address of the remote, and  update rproc->table_ptr to
>   this.
> 
> The install function will be tasked with making sure table_ptr is valid
> and we make sure that error paths out of there and upon shutdown the
> core will make table_ptr reference the core's version - which might be
> NULL if no resource table exist.
> 
> 
> == Supporting rpmsg alternatives:
> For systems with other communication mechanisms than rpmsg we still want
> to be able to instantiate and tear down devices representing these
> communication channels, in a way that follows the life cycle of the
> remoteproc. To do this I suggest that:
> 
> * Like other resources we split virtio device handling in the remoteproc
>   core into adding rproc_vdevs to rproc->rvdevs and actually calling
>   rproc_add_virtio_dev().
> 
> * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
>   that are registered with the remoteproc; each providing callbacks for
>   registering and unregistering themselves as we bring the remoteproc up
>   and down.
> 
> I made a quick hack of this with the Qualcomm SMD code and it turned out
> pretty good, but I believe it's too ugly to post until we get the
> cleanups from above tasks sorted out.
> 
> 
> == Ramdump:
> Being able to acquire core dumps from a miss-behaving remoteproc is an
> important feature in product development. I believe this snapshot should
> be taken between the shutdown of the remote and freeing of resources. As
> such I think it would be appropriate to:
> 
> * Split the inner working of rproc_shutdown() into the two steps of
>   shutting down the remote and cleaning up its resources. Giving us a
>   window of opportunity for snapshotting any resources.
> 
> In the generic case we will have to repopulate the resources with data
> from the firmware file (in case of corruptions), but we're expecting to
> load the same firmware again and as such I see no meaning in releasing
> and reacquiring carveouts etc. As such if we split the inner working of
> rproc_boot() into resource allocation, virtio device allocation and
> booting we can make rproc_trigger_recovery() do:
> 
>     * Shutdown the remoteproc
>     * Shutdown virtio devices
>     * Take snapshot
>     * Register virtio devices
>     * Start the remoteproc
> 
> (The order of the top two is opposite of todays implementation, but
> probably more appropriate for the case of getting an accurate snapshot
> of the device).
> 
> We need to discuss the requirements for the format of what comes out of
> this.  I've seen raw memory dumps of a fixed memory segments and I've
> seen ELF generators with segments matching those of the loaded ELF in
> the past.
> 
> 
> == Firmware from late mounted file systems (needs discussion):
> Typical for most remoteproc systems seems to be that the firmware files
> tend to be way to large for being stored in a initramfs.

In the case of fdma the firmware is actually very small. I think for my usecase
I would be best off putting the fdma firmware in the initramfs and then all
the current issues with async fw, deferred probe  and late rootfs will go away.

If some better solution in remoteproc comes along in the future fdma can migrate
over to using it.

> The two
> available mechanisms for dealing with this is to build remoteproc
> drivers as modules and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> the latter is being frowned upon by everyone but works fairly well for
> our purpose.

I agree has worked well for me in the past, but is frowned upon.
> 
> Forcing systems to compile remoteproc drivers as modules to achieve the
> delayed firmware load is not acceptable in my view.

Agree again.

> 
> So we need to come up with some mechanism for triggering auto-booting
> when firmware files are stored in a lately mounted file system.

One approach I've looked at is using -EPROBE_DEFER in the client driver (in my
case dmaengine) if rproc_boot() can't find the firmware.

This does require a couple of changes to get working with mainline though: -

1) rproc async fw mechanism in rproc_add_virtio_devices() will have failed,
   so resource table parsing needs to be re-tried in rproc_boot(). See here
    http://thread.gmane.org/gmane.linux.ports.arm.kernel/512079

2) It requires a change in drivers-core to re-run deferred probes after
rootfs has been mounted. See here https://lkml.org/lkml/2016/7/12/503.

With those two changes the deferred approach works well. It also has the benefit
of not allowing devices to be registered which can never work due to missing
firmware. In my case ALSA ASoC will be deffered until dma which it relies upon
to work has its firmware in place.

One other quick question, you haven't mentioned subdev carveout here? Are you ok
with me re-submitting fdma series as it is currently, and we can migrate over to
using subdev careout if / when it gets merged in the future?

regards,

Peter.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-07-25 12:26 ` Peter Griffin
@ 2016-07-28 17:32   ` Bjorn Andersson
  0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2016-07-28 17:32 UTC (permalink / raw)
  To: Peter Griffin
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Loic PALLARDY, Eric FINCO, Russell Wayman, Matthew Locke,
	Kumar Gala, Bill Fletcher, Puja Gupta, Ohad Ben-Cohen,
	Suman Anna

On Mon 25 Jul 05:26 PDT 2016, Peter Griffin wrote:

> Hi Bjorn,
> 
> On Mon, 18 Jul 2016, Bjorn Andersson wrote:
> 
> > During discussions with various people interested in moving their
> > remoteproc-related out-of-tree patches towards mainline I have come
> > across a set of topics common among various teams. The purpose of this
> > email is to share some insight into these discussions and the current
> > ideas for moving forward.
> > 
> > == Auto-boot or always-on:
> > There are cases where we want to achieve the current auto-boot mechanism
> > without rpmsg and there are cases where we don't want to auto-boot a
> > remoteproc just because its resource table contains rpmsg entries. So we
> > need to decouple this logic from the vdev. I suggest that:
> > 
> > * We introduce a property in the rproc struct where drivers can toggle
> >   if they want always-on or not. We default this to true, as an estimate
> >   of backwards compatibility.
> > 
> > * We move the allocation of vrings to be done at the time of the other
> >   allocations and follow that with the registration of virtio devices,
> >   before actually booting the rproc. And we tear down the virtio devices
> >   as part of shutdown.
> > 
> > * We remove the rproc_boot()/shutdown() calls from the
> >   remoteproc_virtio.c and based on rproc->always_on we call these in the
> >   async firmware loader callback, in rproc_trigger_recovery() and
> >   rproc_del().
> 
> What happens in the built-in case where firnmware isn't available until rootfs
> is mounted? In this case async fw load will have failed. 
> 

As the only purpose of the async callback becomes triggering the boot I
believe the result will be that the processor didn't boot. Beyond that I
think we're into the question below of late firmware, something we need
to discuss more.


With the current implementation one difference that we would have is
that we must dereference the rproc according to this in recover and
rproc_del(). But I think those needs some TLC anyways, to be more robust.

> > 
> > 
> > A side effect of this is that the async firmware loading only purpose is
> > to trigger the boot as the firmware becomes available (something we need
> > to tweak further). I therefor see no reason to mandate the firmware is
> > unchanged between boots, which simplifies the posed rproc_set_firmware()
> > function - as long as it's done on a remoteproc that's not always_on.
> > 
> > 
[..]
> > == Firmware from late mounted file systems (needs discussion):
> > Typical for most remoteproc systems seems to be that the firmware files
> > tend to be way to large for being stored in a initramfs.
> 
> In the case of fdma the firmware is actually very small. I think for my usecase
> I would be best off putting the fdma firmware in the initramfs and then all
> the current issues with async fw, deferred probe  and late rootfs will go away.
> 
> If some better solution in remoteproc comes along in the future fdma can migrate
> over to using it.
> 

There are cases where this can be done, but it's something we must deal
with for most of them.

> > The two
> > available mechanisms for dealing with this is to build remoteproc
> > drivers as modules and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> > the latter is being frowned upon by everyone but works fairly well for
> > our purpose.
> 
> I agree has worked well for me in the past, but is frowned upon.
> > 
> > Forcing systems to compile remoteproc drivers as modules to achieve the
> > delayed firmware load is not acceptable in my view.
> 
> Agree again.
> 
> > 
> > So we need to come up with some mechanism for triggering auto-booting
> > when firmware files are stored in a lately mounted file system.
> 
> One approach I've looked at is using -EPROBE_DEFER in the client driver (in my
> case dmaengine) if rproc_boot() can't find the firmware.
> 
> This does require a couple of changes to get working with mainline though: -
> 
> 1) rproc async fw mechanism in rproc_add_virtio_devices() will have failed,
>    so resource table parsing needs to be re-tried in rproc_boot(). See here
>     http://thread.gmane.org/gmane.linux.ports.arm.kernel/512079
> 
> 2) It requires a change in drivers-core to re-run deferred probes after
> rootfs has been mounted. See here https://lkml.org/lkml/2016/7/12/503.
> 
> With those two changes the deferred approach works well. It also has the benefit
> of not allowing devices to be registered which can never work due to missing
> firmware. In my case ALSA ASoC will be deffered until dma which it relies upon
> to work has its firmware in place.
> 

I'm not much in favor of using EPROBE_DEFER as an indicator of the
firmware might be available and I'm not sure we can make it work in a
sane way for the always-on/auto-boot cases.

> One other quick question, you haven't mentioned subdev carveout here? Are you ok
> with me re-submitting fdma series as it is currently, and we can migrate over to
> using subdev careout if / when it gets merged in the future?
> 

I don't remember the details of your resources, but I would prefer if
we can get the driver merged separate of any remoteproc improvements.

Thanks for your feedback!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Ongoing remoteproc discussions
  2016-07-18 23:10 Ongoing remoteproc discussions Bjorn Andersson
@ 2016-08-03 14:52   ` Loic PALLARDY
  2016-08-03 14:52   ` Loic PALLARDY
  2016-08-11  0:19   ` Suman Anna
  2 siblings, 0 replies; 13+ messages in thread
From: Loic PALLARDY @ 2016-08-03 14:52 UTC (permalink / raw)
  To: Bjorn Andersson, linux-remoteproc
  Cc: linux-kernel, Lee Jones, Sarangdhar Joshi, Eric FINCO,
	Russell Wayman, Matthew Locke  <matthew.locke@linaro.org>,
	Kumar Gala, Bill Fletcher  <bill.fletcher@linaro.org>,
	Puja Gupta, Ohad Ben-Cohen, Suman Anna

Hi Bjorn,

Please find below some comments/questions.

Regards,
Loic

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Tuesday, July 19, 2016 1:10 AM
> To: linux-remoteproc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Lee Jones <lee.jones@linaro.org>;
> Sarangdhar Joshi <spjoshi@codeaurora.org>; Loic PALLARDY
> <loic.pallardy@st.com>; Eric FINCO <eric.finco@st.com>; Russell Wayman
> <russell.wayman@linaro.org>; Matthew Locke <matthew.locke@linaro.org>;
> Kumar Gala <kumar.gala@linaro.org>; Bill Fletcher <bill.fletcher@linaro.org>;
> Puja Gupta <pujag@codeaurora.org>; Ohad Ben-Cohen
> <ohad@wizery.com>; Suman Anna <s-anna@ti.com>
> Subject: Ongoing remoteproc discussions
> 
> During discussions with various people interested in moving their
> remoteproc-related out-of-tree patches towards mainline I have come
> across a set of topics common among various teams. The purpose of this
> email is to share some insight into these discussions and the current ideas for
> moving forward.
> 
> == Auto-boot or always-on:
> There are cases where we want to achieve the current auto-boot mechanism
> without rpmsg and there are cases where we don't want to auto-boot a
> remoteproc just because its resource table contains rpmsg entries. So we
> need to decouple this logic from the vdev. I suggest that:
> 
> * We introduce a property in the rproc struct where drivers can toggle
>   if they want always-on or not. We default this to true, as an estimate
>   of backwards compatibility.
> 
> * We move the allocation of vrings to be done at the time of the other
>   allocations and follow that with the registration of virtio devices,
>   before actually booting the rproc. And we tear down the virtio devices
>   as part of shutdown.
> 
> * We remove the rproc_boot()/shutdown() calls from the
>   remoteproc_virtio.c and based on rproc->always_on we call these in the
>   async firmware loader callback, in rproc_trigger_recovery() and
>   rproc_del().
> 
> 
> A side effect of this is that the async firmware loading only purpose is to
> trigger the boot as the firmware becomes available (something we need to
> tweak further). I therefor see no reason to mandate the firmware is
> unchanged between boots, which simplifies the posed rproc_set_firmware()
> function - as long as it's done on a remoteproc that's not always_on.
> 
[LPA] As already mentioned in patch review, I would prefer auto-boot name rather than always-on for this feature.
What about coprocessor already loaded and started at boot stage? It may be the case of coprocessor used by bootloader and can't reset without breaking use case or coprocessor with security constraints.
In that case, remoteproc should allocate rproc resource at linux level and sync on current rproc state.
> 
> == Amended resources:
> We need a way for the driver to amend resource definitions of the firmware
> with e.g. physical addresses and size constraints from devicetree, so I
> suggest that:
> 
> * We split the resource table parsing and allocation of resources in two
>   steps; where the parse step updates the lists of resources and then at
>   the time of boot we run through these and allocate the actual
>   resources.
> 
> * We expose an API to the drivers so that they can register resources,
>   as if they came from the table parser. We match each resource against
>   previously registered resources based on name and merge or reject the
>   updates. E.g. we merge a reference to the resource table offset and we
>   reject incompatible size changes.
> 
> 
> == Resource-less firmware:
> To be able to use remoteproc with firmware either without a resource table
> or resource data in other forms we today provide a resource table stub in
> each driver, instead we could refactor the resource table parsing code.
> 
> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>   operation, that uses the newly created API (above) to register the
>   resources with the core; largely decoupling the resource table format
>   from the remoteproc core.
> 
> * We make the parse() function in rproc_fw_ops optional, allowing
>   remoteproc drivers to specify that there's no resource parsing to be
>   done (they can still provide resources programmatically between
>   rproc_alloc() and rproc_add()).
> 
> This setup allows custom resource building functions to be implemented,
> one such example is the Qualcomm firmware files where most resource data
> is a combination of static information (DT) and data from the ELF header.
[LPA] Do you have a list of resources you would like to support here?
In ST we plan to have DT for rproc resource description (PIO, peripheral bus...). Today coprocessor resources are managed dynamically using resource manager developed by TI on omap.
But this solution is consuming time and code size.
We would like to implement rproc resource allocation at rproc_boot time, parsing associated DT section and getting the different requested resources...
Is it aligned with your view?

> 
> 
> == Resource-less firmware with installed resource table:
> The now available list of resources that have been registered with the core
> can serve as input for a function that generates a resource table for the
> remote.  This gives us a mechanism for providing a remoteproc with
> information about resources in cases where the firmware lacks a resource
> table.
> 
> * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
>   tasked with installing the resource table and update rproc->table_ptr.
>   This op is made optional, for the remoteprocs with no installed
>   resource table.
> 
> * We provide a helper function in the core that can be used in the
>   fw_op, that builds a resource table in memory and copy this into the
>   appropriate address of the remote, and  update rproc->table_ptr to
>   this.
> 
> The install function will be tasked with making sure table_ptr is valid and we
> make sure that error paths out of there and upon shutdown the core will
> make table_ptr reference the core's version - which might be NULL if no
> resource table exist.
> 
> 
> == Supporting rpmsg alternatives:
> For systems with other communication mechanisms than rpmsg we still want
> to be able to instantiate and tear down devices representing these
> communication channels, in a way that follows the life cycle of the
> remoteproc. To do this I suggest that:
> 
> * Like other resources we split virtio device handling in the remoteproc
>   core into adding rproc_vdevs to rproc->rvdevs and actually calling
>   rproc_add_virtio_dev().
> 
> * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
>   that are registered with the remoteproc; each providing callbacks for
>   registering and unregistering themselves as we bring the remoteproc up
>   and down.
> 
> I made a quick hack of this with the Qualcomm SMD code and it turned out
> pretty good, but I believe it's too ugly to post until we get the cleanups from
> above tasks sorted out.
> 
> 
> == Ramdump:
> Being able to acquire core dumps from a miss-behaving remoteproc is an
> important feature in product development. I believe this snapshot should be
> taken between the shutdown of the remote and freeing of resources. As
> such I think it would be appropriate to:
> 
> * Split the inner working of rproc_shutdown() into the two steps of
>   shutting down the remote and cleaning up its resources. Giving us a
>   window of opportunity for snapshotting any resources.
> 
> In the generic case we will have to repopulate the resources with data from
> the firmware file (in case of corruptions), but we're expecting to load the
> same firmware again and as such I see no meaning in releasing and
> reacquiring carveouts etc. As such if we split the inner working of
> rproc_boot() into resource allocation, virtio device allocation and booting we
> can make rproc_trigger_recovery() do:
> 
>     * Shutdown the remoteproc
[LPA] just a technical remark, shutting down the remoteproc may switch off some power domain and disable access to remoteproc memories or registers.
An intermediate state is need to allow snapshot generation.

>     * Shutdown virtio devices
>     * Take snapshot
>     * Register virtio devices
>     * Start the remoteproc
> 
> (The order of the top two is opposite of todays implementation, but
> probably more appropriate for the case of getting an accurate snapshot of
> the device).
> 
> We need to discuss the requirements for the format of what comes out of
> this.  I've seen raw memory dumps of a fixed memory segments and I've
> seen ELF generators with segments matching those of the loaded ELF in the
> past.
[LPA] +1 for ELF format
> 
> 
> == Firmware from late mounted file systems (needs discussion):
> Typical for most remoteproc systems seems to be that the firmware files
> tend to be way to large for being stored in a initramfs. The two available
> mechanisms for dealing with this is to build remoteproc drivers as modules
> and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> the latter is being frowned upon by everyone but works fairly well for our
> purpose.
> 
> Forcing systems to compile remoteproc drivers as modules to achieve the
> delayed firmware load is not acceptable in my view.
> 
> So we need to come up with some mechanism for triggering auto-booting
> when firmware files are stored in a lately mounted file system.
> 
> 
> 
> Interwoven in these discussions are of course topics related to rpmsg and I
> will try to get back to these as things materialize.
> 
> Regards,
> Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Ongoing remoteproc discussions
@ 2016-08-03 14:52   ` Loic PALLARDY
  0 siblings, 0 replies; 13+ messages in thread
From: Loic PALLARDY @ 2016-08-03 14:52 UTC (permalink / raw)
  To: Bjorn Andersson, linux-remoteproc
  Cc: linux-kernel, Lee Jones, Sarangdhar Joshi, Eric FINCO,
	Russell Wayman, Matthew Locke, Kumar Gala, Bill Fletcher,
	Puja Gupta, Ohad Ben-Cohen, Suman Anna

Hi Bjorn,

Please find below some comments/questions.

Regards,
Loic

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Tuesday, July 19, 2016 1:10 AM
> To: linux-remoteproc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Lee Jones <lee.jones@linaro.org>;
> Sarangdhar Joshi <spjoshi@codeaurora.org>; Loic PALLARDY
> <loic.pallardy@st.com>; Eric FINCO <eric.finco@st.com>; Russell Wayman
> <russell.wayman@linaro.org>; Matthew Locke <matthew.locke@linaro.org>;
> Kumar Gala <kumar.gala@linaro.org>; Bill Fletcher <bill.fletcher@linaro.org>;
> Puja Gupta <pujag@codeaurora.org>; Ohad Ben-Cohen
> <ohad@wizery.com>; Suman Anna <s-anna@ti.com>
> Subject: Ongoing remoteproc discussions
> 
> During discussions with various people interested in moving their
> remoteproc-related out-of-tree patches towards mainline I have come
> across a set of topics common among various teams. The purpose of this
> email is to share some insight into these discussions and the current ideas for
> moving forward.
> 
> == Auto-boot or always-on:
> There are cases where we want to achieve the current auto-boot mechanism
> without rpmsg and there are cases where we don't want to auto-boot a
> remoteproc just because its resource table contains rpmsg entries. So we
> need to decouple this logic from the vdev. I suggest that:
> 
> * We introduce a property in the rproc struct where drivers can toggle
>   if they want always-on or not. We default this to true, as an estimate
>   of backwards compatibility.
> 
> * We move the allocation of vrings to be done at the time of the other
>   allocations and follow that with the registration of virtio devices,
>   before actually booting the rproc. And we tear down the virtio devices
>   as part of shutdown.
> 
> * We remove the rproc_boot()/shutdown() calls from the
>   remoteproc_virtio.c and based on rproc->always_on we call these in the
>   async firmware loader callback, in rproc_trigger_recovery() and
>   rproc_del().
> 
> 
> A side effect of this is that the async firmware loading only purpose is to
> trigger the boot as the firmware becomes available (something we need to
> tweak further). I therefor see no reason to mandate the firmware is
> unchanged between boots, which simplifies the posed rproc_set_firmware()
> function - as long as it's done on a remoteproc that's not always_on.
> 
[LPA] As already mentioned in patch review, I would prefer auto-boot name rather than always-on for this feature.
What about coprocessor already loaded and started at boot stage? It may be the case of coprocessor used by bootloader and can't reset without breaking use case or coprocessor with security constraints.
In that case, remoteproc should allocate rproc resource at linux level and sync on current rproc state.
> 
> == Amended resources:
> We need a way for the driver to amend resource definitions of the firmware
> with e.g. physical addresses and size constraints from devicetree, so I
> suggest that:
> 
> * We split the resource table parsing and allocation of resources in two
>   steps; where the parse step updates the lists of resources and then at
>   the time of boot we run through these and allocate the actual
>   resources.
> 
> * We expose an API to the drivers so that they can register resources,
>   as if they came from the table parser. We match each resource against
>   previously registered resources based on name and merge or reject the
>   updates. E.g. we merge a reference to the resource table offset and we
>   reject incompatible size changes.
> 
> 
> == Resource-less firmware:
> To be able to use remoteproc with firmware either without a resource table
> or resource data in other forms we today provide a resource table stub in
> each driver, instead we could refactor the resource table parsing code.
> 
> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>   operation, that uses the newly created API (above) to register the
>   resources with the core; largely decoupling the resource table format
>   from the remoteproc core.
> 
> * We make the parse() function in rproc_fw_ops optional, allowing
>   remoteproc drivers to specify that there's no resource parsing to be
>   done (they can still provide resources programmatically between
>   rproc_alloc() and rproc_add()).
> 
> This setup allows custom resource building functions to be implemented,
> one such example is the Qualcomm firmware files where most resource data
> is a combination of static information (DT) and data from the ELF header.
[LPA] Do you have a list of resources you would like to support here?
In ST we plan to have DT for rproc resource description (PIO, peripheral bus...). Today coprocessor resources are managed dynamically using resource manager developed by TI on omap.
But this solution is consuming time and code size.
We would like to implement rproc resource allocation at rproc_boot time, parsing associated DT section and getting the different requested resources...
Is it aligned with your view?

> 
> 
> == Resource-less firmware with installed resource table:
> The now available list of resources that have been registered with the core
> can serve as input for a function that generates a resource table for the
> remote.  This gives us a mechanism for providing a remoteproc with
> information about resources in cases where the firmware lacks a resource
> table.
> 
> * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
>   tasked with installing the resource table and update rproc->table_ptr.
>   This op is made optional, for the remoteprocs with no installed
>   resource table.
> 
> * We provide a helper function in the core that can be used in the
>   fw_op, that builds a resource table in memory and copy this into the
>   appropriate address of the remote, and  update rproc->table_ptr to
>   this.
> 
> The install function will be tasked with making sure table_ptr is valid and we
> make sure that error paths out of there and upon shutdown the core will
> make table_ptr reference the core's version - which might be NULL if no
> resource table exist.
> 
> 
> == Supporting rpmsg alternatives:
> For systems with other communication mechanisms than rpmsg we still want
> to be able to instantiate and tear down devices representing these
> communication channels, in a way that follows the life cycle of the
> remoteproc. To do this I suggest that:
> 
> * Like other resources we split virtio device handling in the remoteproc
>   core into adding rproc_vdevs to rproc->rvdevs and actually calling
>   rproc_add_virtio_dev().
> 
> * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
>   that are registered with the remoteproc; each providing callbacks for
>   registering and unregistering themselves as we bring the remoteproc up
>   and down.
> 
> I made a quick hack of this with the Qualcomm SMD code and it turned out
> pretty good, but I believe it's too ugly to post until we get the cleanups from
> above tasks sorted out.
> 
> 
> == Ramdump:
> Being able to acquire core dumps from a miss-behaving remoteproc is an
> important feature in product development. I believe this snapshot should be
> taken between the shutdown of the remote and freeing of resources. As
> such I think it would be appropriate to:
> 
> * Split the inner working of rproc_shutdown() into the two steps of
>   shutting down the remote and cleaning up its resources. Giving us a
>   window of opportunity for snapshotting any resources.
> 
> In the generic case we will have to repopulate the resources with data from
> the firmware file (in case of corruptions), but we're expecting to load the
> same firmware again and as such I see no meaning in releasing and
> reacquiring carveouts etc. As such if we split the inner working of
> rproc_boot() into resource allocation, virtio device allocation and booting we
> can make rproc_trigger_recovery() do:
> 
>     * Shutdown the remoteproc
[LPA] just a technical remark, shutting down the remoteproc may switch off some power domain and disable access to remoteproc memories or registers.
An intermediate state is need to allow snapshot generation.

>     * Shutdown virtio devices
>     * Take snapshot
>     * Register virtio devices
>     * Start the remoteproc
> 
> (The order of the top two is opposite of todays implementation, but
> probably more appropriate for the case of getting an accurate snapshot of
> the device).
> 
> We need to discuss the requirements for the format of what comes out of
> this.  I've seen raw memory dumps of a fixed memory segments and I've
> seen ELF generators with segments matching those of the loaded ELF in the
> past.
[LPA] +1 for ELF format
> 
> 
> == Firmware from late mounted file systems (needs discussion):
> Typical for most remoteproc systems seems to be that the firmware files
> tend to be way to large for being stored in a initramfs. The two available
> mechanisms for dealing with this is to build remoteproc drivers as modules
> and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> the latter is being frowned upon by everyone but works fairly well for our
> purpose.
> 
> Forcing systems to compile remoteproc drivers as modules to achieve the
> delayed firmware load is not acceptable in my view.
> 
> So we need to come up with some mechanism for triggering auto-booting
> when firmware files are stored in a lately mounted file system.
> 
> 
> 
> Interwoven in these discussions are of course topics related to rpmsg and I
> will try to get back to these as things materialize.
> 
> Regards,
> Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-08-03 14:52   ` Loic PALLARDY
  (?)
@ 2016-08-10 20:22   ` Bjorn Andersson
  2016-08-11 16:48     ` Suman Anna
  -1 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2016-08-10 20:22 UTC (permalink / raw)
  To: Loic PALLARDY
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen, Suman Anna

On Wed 03 Aug 07:52 PDT 2016, Loic PALLARDY wrote:

> > == Auto-boot or always-on:
[..]
> > 
> [LPA] As already mentioned in patch review, I would prefer auto-boot
> name rather than always-on for this feature.

Agreed.

> What about coprocessor already loaded and started at boot stage? It
> may be the case of coprocessor used by bootloader and can't reset
> without breaking use case or coprocessor with security constraints.

For the cases I've dealt with we just didn't represent the remote
processor in the kernel, we just reserved the carveouts and communicated
with it.

> In that case, remoteproc should allocate rproc resource at linux level
> and sync on current rproc state.

Sure.

> > 
[..]
> > == Resource-less firmware:
> > To be able to use remoteproc with firmware either without a resource table
> > or resource data in other forms we today provide a resource table stub in
> > each driver, instead we could refactor the resource table parsing code.
> > 
> > * We replace the find_rsc_table operation in rproc_fw_ops with a parse
> >   operation, that uses the newly created API (above) to register the
> >   resources with the core; largely decoupling the resource table format
> >   from the remoteproc core.
> > 
> > * We make the parse() function in rproc_fw_ops optional, allowing
> >   remoteproc drivers to specify that there's no resource parsing to be
> >   done (they can still provide resources programmatically between
> >   rproc_alloc() and rproc_add()).
> > 
> > This setup allows custom resource building functions to be implemented,
> > one such example is the Qualcomm firmware files where most resource data
> > is a combination of static information (DT) and data from the ELF header.
> [LPA] Do you have a list of resources you would like to support here?

With resources here I meant the existing remoteproc resources, i.e.
carveouts, devmem, trace and vdev/vrings.

> In ST we plan to have DT for rproc resource description (PIO,
> peripheral bus...). Today coprocessor resources are managed
> dynamically using resource manager developed by TI on omap.
> But this solution is consuming time and code size.
> We would like to implement rproc resource allocation at rproc_boot
> time, parsing associated DT section and getting the different
> requested resources...

Are you talking about the resmgr found in downstream TI trees? What
kinds of resources and how would this look like?

> Is it aligned with your view?
> 

I'm generally considering these resources (e.g. regulators exposed by
resmgr) not being part of the life cycle management of the remote
processor, but rather related to the application running on the remote
processor; as such I don't think they should reside in the remoteproc
core.

That said, for resmgr to move upstream I think it needs to be
generalized.

> > 
> > 
[..]
> > == Ramdump:
[..]
> > 
> >     * Shutdown the remoteproc

> [LPA] just a technical remark, shutting down the remoteproc may switch
> off some power domain and disable access to remoteproc memories or
> registers.
> An intermediate state is need to allow snapshot generation.

Ahh, you're right. Then we need to complicate this dance a little bit,
and probably make it possible for drivers to take part in it.

> 
> >     * Shutdown virtio devices
> >     * Take snapshot
> >     * Register virtio devices
> >     * Start the remoteproc
> > 

Thanks for your reply Loic.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-07-18 23:10 Ongoing remoteproc discussions Bjorn Andersson
@ 2016-08-11  0:19   ` Suman Anna
  2016-08-03 14:52   ` Loic PALLARDY
  2016-08-11  0:19   ` Suman Anna
  2 siblings, 0 replies; 13+ messages in thread
From: Suman Anna @ 2016-08-11  0:19 UTC (permalink / raw)
  To: Bjorn Andersson, linux-remoteproc
  Cc: linux-kernel, Lee Jones, Sarangdhar Joshi, Loic PALLARDY,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen

Hi Bjorn,

On 07/18/2016 06:10 PM, Bjorn Andersson wrote:
> During discussions with various people interested in moving their
> remoteproc-related out-of-tree patches towards mainline I have come
> across a set of topics common among various teams. The purpose of this
> email is to share some insight into these discussions and the current
> ideas for moving forward.

Thanks for putting this together - a very good summary. I have a few
comments/questions below.

> 
> == Auto-boot or always-on:
> There are cases where we want to achieve the current auto-boot mechanism
> without rpmsg and there are cases where we don't want to auto-boot a
> remoteproc just because its resource table contains rpmsg entries. So we
> need to decouple this logic from the vdev. I suggest that:

I am trying to understand the usecase where one doesn't want to
auto-boot with rpmsg entries, did you come across such an usecase?

> 
> * We introduce a property in the rproc struct where drivers can toggle
>   if they want always-on or not. We default this to true, as an estimate
>   of backwards compatibility.

Can this be made into a flags field rather than a boolean, especially to
dictate the behavior between firmwares with vdevs and without vdevs. I
do have some usecases that can have vdevs (so auto-boot) or no vdevs
(with dummy resource tables) based on firmware being loaded (rproc_boot
will be called by a client driver in that case).

Your current patch-set does break the wkup_m3_rproc driver as it now
auto-boots.

> 
> * We move the allocation of vrings to be done at the time of the other
>   allocations and follow that with the registration of virtio devices,
>   before actually booting the rproc. And we tear down the virtio devices
>   as part of shutdown.
> 
> * We remove the rproc_boot()/shutdown() calls from the
>   remoteproc_virtio.c and based on rproc->always_on we call these in the
>   async firmware loader callback, in rproc_trigger_recovery() and
>   rproc_del().
> 
> 
> A side effect of this is that the async firmware loading only purpose is
> to trigger the boot as the firmware becomes available (something we need
> to tweak further). I therefor see no reason to mandate the firmware is
> unchanged between boots, which simplifies the posed rproc_set_firmware()
> function - as long as it's done on a remoteproc that's not always_on.

I take it that you are talking about adding a new API down the road
which should allow client drivers to set firmwares.

+1 for that.

> 
> 
> == Amended resources:
> We need a way for the driver to amend resource definitions of the
> firmware with e.g. physical addresses and size constraints from
> devicetree, so I suggest that:
> 
> * We split the resource table parsing and allocation of resources in two
>   steps; where the parse step updates the lists of resources and then at
>   the time of boot we run through these and allocate the actual
>   resources.
> 
> * We expose an API to the drivers so that they can register resources,
>   as if they came from the table parser. We match each resource against
>   previously registered resources based on name and merge or reject the
>   updates. E.g. we merge a reference to the resource table offset and we
>   reject incompatible size changes.

So, all this to be invoked by a platform driver before rproc_add() even
before requesting and processing a firmware? Is this also only limited
for remoteproc platform drivers, or gonna be extended to client drivers
(in the case where they are setting the firmwares).

One question w.r.t Lee's series is also adding new resources - how does
that work with loaded resource tables given that you are dynamically
increasing the table size, and the firmware image is already pre-linked.
Or is the assumption here that there will not be a loaded resource table
in such a scenario, we will have a loaded resource table for firmwares
with vdevs for sure.

> 
> 
> == Resource-less firmware:
> To be able to use remoteproc with firmware either without a resource
> table or resource data in other forms we today provide a resource table
> stub in each driver, instead we could refactor the resource table
> parsing code.
> 
> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>   operation, that uses the newly created API (above) to register the
>   resources with the core; largely decoupling the resource table format
>   from the remoteproc core.
> 
> * We make the parse() function in rproc_fw_ops optional, allowing
>   remoteproc drivers to specify that there's no resource parsing to be
>   done (they can still provide resources programmatically between
>   rproc_alloc() and rproc_add()).
> 
> This setup allows custom resource building functions to be implemented,
> one such example is the Qualcomm firmware files where most resource data
> is a combination of static information (DT) and data from the ELF
> header.
> 
> 
> == Resource-less firmware with installed resource table:
> The now available list of resources that have been registered with the
> core can serve as input for a function that generates a resource table
> for the remote.  This gives us a mechanism for providing a remoteproc
> with information about resources in cases where the firmware lacks a
> resource table.
> 
> * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
>   tasked with installing the resource table and update rproc->table_ptr.
>   This op is made optional, for the remoteprocs with no installed
>   resource table.

So, kinda similar to above question, how do you find a suitable location
for this in the case of installed resource table?

> 
> * We provide a helper function in the core that can be used in the
>   fw_op, that builds a resource table in memory and copy this into the
>   appropriate address of the remote, and  update rproc->table_ptr to
>   this.
> 
> The install function will be tasked with making sure table_ptr is valid
> and we make sure that error paths out of there and upon shutdown the
> core will make table_ptr reference the core's version - which might be
> NULL if no resource table exist.
> 
> 
> == Supporting rpmsg alternatives:
> For systems with other communication mechanisms than rpmsg we still want
> to be able to instantiate and tear down devices representing these
> communication channels, in a way that follows the life cycle of the
> remoteproc. To do this I suggest that:
> 
> * Like other resources we split virtio device handling in the remoteproc
>   core into adding rproc_vdevs to rproc->rvdevs and actually calling
>   rproc_add_virtio_dev().
> 
> * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
>   that are registered with the remoteproc; each providing callbacks for
>   registering and unregistering themselves as we bring the remoteproc up
>   and down.

+1, this is a good direction.

regards
Suman

> 
> I made a quick hack of this with the Qualcomm SMD code and it turned out
> pretty good, but I believe it's too ugly to post until we get the
> cleanups from above tasks sorted out.
> 
> 
> == Ramdump:
> Being able to acquire core dumps from a miss-behaving remoteproc is an
> important feature in product development. I believe this snapshot should
> be taken between the shutdown of the remote and freeing of resources. As
> such I think it would be appropriate to:
> 
> * Split the inner working of rproc_shutdown() into the two steps of
>   shutting down the remote and cleaning up its resources. Giving us a
>   window of opportunity for snapshotting any resources.
> 
> In the generic case we will have to repopulate the resources with data
> from the firmware file (in case of corruptions), but we're expecting to
> load the same firmware again and as such I see no meaning in releasing
> and reacquiring carveouts etc. As such if we split the inner working of
> rproc_boot() into resource allocation, virtio device allocation and
> booting we can make rproc_trigger_recovery() do:
> 
>     * Shutdown the remoteproc
>     * Shutdown virtio devices
>     * Take snapshot
>     * Register virtio devices
>     * Start the remoteproc
> 
> (The order of the top two is opposite of todays implementation, but
> probably more appropriate for the case of getting an accurate snapshot
> of the device).
> 
> We need to discuss the requirements for the format of what comes out of
> this.  I've seen raw memory dumps of a fixed memory segments and I've
> seen ELF generators with segments matching those of the loaded ELF in
> the past.
> 
> 
> == Firmware from late mounted file systems (needs discussion):
> Typical for most remoteproc systems seems to be that the firmware files
> tend to be way to large for being stored in a initramfs. The two
> available mechanisms for dealing with this is to build remoteproc
> drivers as modules and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> the latter is being frowned upon by everyone but works fairly well for
> our purpose.
> 
> Forcing systems to compile remoteproc drivers as modules to achieve the
> delayed firmware load is not acceptable in my view.
> 
> So we need to come up with some mechanism for triggering auto-booting
> when firmware files are stored in a lately mounted file system.
> 
> 
> 
> Interwoven in these discussions are of course topics related to rpmsg
> and I will try to get back to these as things materialize.
> 
> Regards,
> Bjorn
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
@ 2016-08-11  0:19   ` Suman Anna
  0 siblings, 0 replies; 13+ messages in thread
From: Suman Anna @ 2016-08-11  0:19 UTC (permalink / raw)
  To: Bjorn Andersson, linux-remoteproc
  Cc: linux-kernel, Lee Jones, Sarangdhar Joshi, Loic PALLARDY,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen, Lee Jones

Hi Bjorn,

On 07/18/2016 06:10 PM, Bjorn Andersson wrote:
> During discussions with various people interested in moving their
> remoteproc-related out-of-tree patches towards mainline I have come
> across a set of topics common among various teams. The purpose of this
> email is to share some insight into these discussions and the current
> ideas for moving forward.

Thanks for putting this together - a very good summary. I have a few
comments/questions below.

> 
> == Auto-boot or always-on:
> There are cases where we want to achieve the current auto-boot mechanism
> without rpmsg and there are cases where we don't want to auto-boot a
> remoteproc just because its resource table contains rpmsg entries. So we
> need to decouple this logic from the vdev. I suggest that:

I am trying to understand the usecase where one doesn't want to
auto-boot with rpmsg entries, did you come across such an usecase?

> 
> * We introduce a property in the rproc struct where drivers can toggle
>   if they want always-on or not. We default this to true, as an estimate
>   of backwards compatibility.

Can this be made into a flags field rather than a boolean, especially to
dictate the behavior between firmwares with vdevs and without vdevs. I
do have some usecases that can have vdevs (so auto-boot) or no vdevs
(with dummy resource tables) based on firmware being loaded (rproc_boot
will be called by a client driver in that case).

Your current patch-set does break the wkup_m3_rproc driver as it now
auto-boots.

> 
> * We move the allocation of vrings to be done at the time of the other
>   allocations and follow that with the registration of virtio devices,
>   before actually booting the rproc. And we tear down the virtio devices
>   as part of shutdown.
> 
> * We remove the rproc_boot()/shutdown() calls from the
>   remoteproc_virtio.c and based on rproc->always_on we call these in the
>   async firmware loader callback, in rproc_trigger_recovery() and
>   rproc_del().
> 
> 
> A side effect of this is that the async firmware loading only purpose is
> to trigger the boot as the firmware becomes available (something we need
> to tweak further). I therefor see no reason to mandate the firmware is
> unchanged between boots, which simplifies the posed rproc_set_firmware()
> function - as long as it's done on a remoteproc that's not always_on.

I take it that you are talking about adding a new API down the road
which should allow client drivers to set firmwares.

+1 for that.

> 
> 
> == Amended resources:
> We need a way for the driver to amend resource definitions of the
> firmware with e.g. physical addresses and size constraints from
> devicetree, so I suggest that:
> 
> * We split the resource table parsing and allocation of resources in two
>   steps; where the parse step updates the lists of resources and then at
>   the time of boot we run through these and allocate the actual
>   resources.
> 
> * We expose an API to the drivers so that they can register resources,
>   as if they came from the table parser. We match each resource against
>   previously registered resources based on name and merge or reject the
>   updates. E.g. we merge a reference to the resource table offset and we
>   reject incompatible size changes.

So, all this to be invoked by a platform driver before rproc_add() even
before requesting and processing a firmware? Is this also only limited
for remoteproc platform drivers, or gonna be extended to client drivers
(in the case where they are setting the firmwares).

One question w.r.t Lee's series is also adding new resources - how does
that work with loaded resource tables given that you are dynamically
increasing the table size, and the firmware image is already pre-linked.
Or is the assumption here that there will not be a loaded resource table
in such a scenario, we will have a loaded resource table for firmwares
with vdevs for sure.

> 
> 
> == Resource-less firmware:
> To be able to use remoteproc with firmware either without a resource
> table or resource data in other forms we today provide a resource table
> stub in each driver, instead we could refactor the resource table
> parsing code.
> 
> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>   operation, that uses the newly created API (above) to register the
>   resources with the core; largely decoupling the resource table format
>   from the remoteproc core.
> 
> * We make the parse() function in rproc_fw_ops optional, allowing
>   remoteproc drivers to specify that there's no resource parsing to be
>   done (they can still provide resources programmatically between
>   rproc_alloc() and rproc_add()).
> 
> This setup allows custom resource building functions to be implemented,
> one such example is the Qualcomm firmware files where most resource data
> is a combination of static information (DT) and data from the ELF
> header.
> 
> 
> == Resource-less firmware with installed resource table:
> The now available list of resources that have been registered with the
> core can serve as input for a function that generates a resource table
> for the remote.  This gives us a mechanism for providing a remoteproc
> with information about resources in cases where the firmware lacks a
> resource table.
> 
> * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
>   tasked with installing the resource table and update rproc->table_ptr.
>   This op is made optional, for the remoteprocs with no installed
>   resource table.

So, kinda similar to above question, how do you find a suitable location
for this in the case of installed resource table?

> 
> * We provide a helper function in the core that can be used in the
>   fw_op, that builds a resource table in memory and copy this into the
>   appropriate address of the remote, and  update rproc->table_ptr to
>   this.
> 
> The install function will be tasked with making sure table_ptr is valid
> and we make sure that error paths out of there and upon shutdown the
> core will make table_ptr reference the core's version - which might be
> NULL if no resource table exist.
> 
> 
> == Supporting rpmsg alternatives:
> For systems with other communication mechanisms than rpmsg we still want
> to be able to instantiate and tear down devices representing these
> communication channels, in a way that follows the life cycle of the
> remoteproc. To do this I suggest that:
> 
> * Like other resources we split virtio device handling in the remoteproc
>   core into adding rproc_vdevs to rproc->rvdevs and actually calling
>   rproc_add_virtio_dev().
> 
> * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
>   that are registered with the remoteproc; each providing callbacks for
>   registering and unregistering themselves as we bring the remoteproc up
>   and down.

+1, this is a good direction.

regards
Suman

> 
> I made a quick hack of this with the Qualcomm SMD code and it turned out
> pretty good, but I believe it's too ugly to post until we get the
> cleanups from above tasks sorted out.
> 
> 
> == Ramdump:
> Being able to acquire core dumps from a miss-behaving remoteproc is an
> important feature in product development. I believe this snapshot should
> be taken between the shutdown of the remote and freeing of resources. As
> such I think it would be appropriate to:
> 
> * Split the inner working of rproc_shutdown() into the two steps of
>   shutting down the remote and cleaning up its resources. Giving us a
>   window of opportunity for snapshotting any resources.
> 
> In the generic case we will have to repopulate the resources with data
> from the firmware file (in case of corruptions), but we're expecting to
> load the same firmware again and as such I see no meaning in releasing
> and reacquiring carveouts etc. As such if we split the inner working of
> rproc_boot() into resource allocation, virtio device allocation and
> booting we can make rproc_trigger_recovery() do:
> 
>     * Shutdown the remoteproc
>     * Shutdown virtio devices
>     * Take snapshot
>     * Register virtio devices
>     * Start the remoteproc
> 
> (The order of the top two is opposite of todays implementation, but
> probably more appropriate for the case of getting an accurate snapshot
> of the device).
> 
> We need to discuss the requirements for the format of what comes out of
> this.  I've seen raw memory dumps of a fixed memory segments and I've
> seen ELF generators with segments matching those of the loaded ELF in
> the past.
> 
> 
> == Firmware from late mounted file systems (needs discussion):
> Typical for most remoteproc systems seems to be that the firmware files
> tend to be way to large for being stored in a initramfs. The two
> available mechanisms for dealing with this is to build remoteproc
> drivers as modules and relying on CONFIG_FW_LOADER_USER_HELPER_FALLBACK;
> the latter is being frowned upon by everyone but works fairly well for
> our purpose.
> 
> Forcing systems to compile remoteproc drivers as modules to achieve the
> delayed firmware load is not acceptable in my view.
> 
> So we need to come up with some mechanism for triggering auto-booting
> when firmware files are stored in a lately mounted file system.
> 
> 
> 
> Interwoven in these discussions are of course topics related to rpmsg
> and I will try to get back to these as things materialize.
> 
> Regards,
> Bjorn
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-08-10 20:22   ` Bjorn Andersson
@ 2016-08-11 16:48     ` Suman Anna
  2016-08-11 19:05       ` Bjorn Andersson
  0 siblings, 1 reply; 13+ messages in thread
From: Suman Anna @ 2016-08-11 16:48 UTC (permalink / raw)
  To: Bjorn Andersson, Loic PALLARDY
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen

On 08/10/2016 03:22 PM, Bjorn Andersson wrote:
> On Wed 03 Aug 07:52 PDT 2016, Loic PALLARDY wrote:
> 
>>> == Auto-boot or always-on:
> [..]
>>>
>> [LPA] As already mentioned in patch review, I would prefer auto-boot
>> name rather than always-on for this feature.
> 
> Agreed.
> 
>> What about coprocessor already loaded and started at boot stage? It
>> may be the case of coprocessor used by bootloader and can't reset
>> without breaking use case or coprocessor with security constraints.
> 
> For the cases I've dealt with we just didn't represent the remote
> processor in the kernel, we just reserved the carveouts and communicated
> with it.

Yeah, we have a similar usecase as well, and we do want the remoteproc
to behave as in the normal case once the kernel has booted up and the
corresponding driver has been probed. We have had to do some magic (not
zeroing memory) for presenting the remoteproc still to Linux-side
applications and client drivers.

This indeed brings me one of the list of enhancements I have in mind -
to add an ops for individual driver control for allocating memory on
carveouts, vrings etc with a fall-back to the dma_alloc API in the
remoteproc core.

Bjorn, I take it that you are not using rpmsg here if that lifecycle is
managed separately from remoteproc.

> 
>> In that case, remoteproc should allocate rproc resource at linux level
>> and sync on current rproc state.
> 
> Sure.
> 
>>>
> [..]
>>> == Resource-less firmware:
>>> To be able to use remoteproc with firmware either without a resource table
>>> or resource data in other forms we today provide a resource table stub in
>>> each driver, instead we could refactor the resource table parsing code.
>>>
>>> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
>>>   operation, that uses the newly created API (above) to register the
>>>   resources with the core; largely decoupling the resource table format
>>>   from the remoteproc core.
>>>
>>> * We make the parse() function in rproc_fw_ops optional, allowing
>>>   remoteproc drivers to specify that there's no resource parsing to be
>>>   done (they can still provide resources programmatically between
>>>   rproc_alloc() and rproc_add()).
>>>
>>> This setup allows custom resource building functions to be implemented,
>>> one such example is the Qualcomm firmware files where most resource data
>>> is a combination of static information (DT) and data from the ELF header.
>> [LPA] Do you have a list of resources you would like to support here?
> 
> With resources here I meant the existing remoteproc resources, i.e.
> carveouts, devmem, trace and vdev/vrings.
> 
>> In ST we plan to have DT for rproc resource description (PIO,
>> peripheral bus...). Today coprocessor resources are managed
>> dynamically using resource manager developed by TI on omap.
>> But this solution is consuming time and code size.
>> We would like to implement rproc resource allocation at rproc_boot
>> time, parsing associated DT section and getting the different
>> requested resources...

Yeah, this becomes somewhat complicated when we are talking about
peripherals, because it depends on they get used. I see the following
usage patterns:
1. do not instantiate the devices on Linux, and leave them to be managed
completely by s/w running on remoteproc.
2. resources that can be managed alongside the remoteproc state (request
them up before rproc_boot, and release them after rproc_shutdown). This
can always be done within the respective remoteproc driver as the
peripherals used are specific to each platform.
3. resources that only need to be managed at runtime, especially if the
PM around them in controlled on the Linux-side.

> 
> Are you talking about the resmgr found in downstream TI trees? What
> kinds of resources and how would this look like?
> 
>> Is it aligned with your view?
>>
> 
> I'm generally considering these resources (e.g. regulators exposed by
> resmgr) not being part of the life cycle management of the remote
> processor, but rather related to the application running on the remote
> processor; as such I don't think they should reside in the remoteproc
> core.

Agreed, we did use resmgr specifically for #3. It also allowed us to
recover these resources in case of a remoteproc crash while holding them.

> 
> That said, for resmgr to move upstream I think it needs to be
> generalized.

Indeed, the TI resmgr was written before DT, and it would need rework if
we were to go down that path.

That said, if the management is moving towards the System Control
Processor like frameworks, this won't be needed.

regards
Suman

> 
>>>
>>>
> [..]
>>> == Ramdump:
> [..]
>>>
>>>     * Shutdown the remoteproc
> 
>> [LPA] just a technical remark, shutting down the remoteproc may switch
>> off some power domain and disable access to remoteproc memories or
>> registers.
>> An intermediate state is need to allow snapshot generation.
> 
> Ahh, you're right. Then we need to complicate this dance a little bit,
> and probably make it possible for drivers to take part in it.
> 
>>
>>>     * Shutdown virtio devices
>>>     * Take snapshot
>>>     * Register virtio devices
>>>     * Start the remoteproc
>>>
> 
> Thanks for your reply Loic.
> 
> Regards,
> Bjorn
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-08-11  0:19   ` Suman Anna
  (?)
@ 2016-08-11 18:13   ` Bjorn Andersson
  2016-08-16 12:24       ` Loic PALLARDY
  -1 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2016-08-11 18:13 UTC (permalink / raw)
  To: Suman Anna
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Loic PALLARDY, Eric FINCO, Russell Wayman, Matthew Locke,
	Kumar Gala, Bill Fletcher, Puja Gupta, Ohad Ben-Cohen

On Wed 10 Aug 17:19 PDT 2016, Suman Anna wrote:

> Hi Bjorn,
> 

Hi,

> On 07/18/2016 06:10 PM, Bjorn Andersson wrote:
[..]
> > == Auto-boot or always-on:
> > There are cases where we want to achieve the current auto-boot mechanism
> > without rpmsg and there are cases where we don't want to auto-boot a
> > remoteproc just because its resource table contains rpmsg entries. So we
> > need to decouple this logic from the vdev. I suggest that:
> 
> I am trying to understand the usecase where one doesn't want to
> auto-boot with rpmsg entries, did you come across such an usecase?
> 

This is a request that comes from Loic (ST). I'm unaware of the details,
but I can think of scenarios where rpmsg channels serves as auxiliary
functionality to the main purpose of a co-processor (e.g. debug
functionality).

> > 
> > * We introduce a property in the rproc struct where drivers can toggle
> >   if they want always-on or not. We default this to true, as an estimate
> >   of backwards compatibility.
> 
> Can this be made into a flags field rather than a boolean, especially to
> dictate the behavior between firmwares with vdevs and without vdevs. I
> do have some usecases that can have vdevs (so auto-boot) or no vdevs
> (with dummy resource tables) based on firmware being loaded (rproc_boot
> will be called by a client driver in that case).
> 

I would prefer that we don't have to scan the resource table and make
"advanced" decisions based on its content - and as you say it has to be
at least tristate to work based on above.

I'm hoping that we can approximate this with a boolean for now and then
I would like to reverse the resource table parsing mechanism, so that
instead of calling "give me a resource table" we would have a fw_ops
that we call to "parse" the firmware and with below APIs we can build up
the resource table from that.

This parse could then work on other structures than the resource table
and it could be made processor specific to set auto-boot based on
properties of the firmware.

> Your current patch-set does break the wkup_m3_rproc driver as it now
> auto-boots.
> 

Ahh right, thanks for pointing this out.

I'll look through the tree to figure out which drivers seems to have
clients calling rproc_boot() and make them auto_boot = false;

> > 
> > * We move the allocation of vrings to be done at the time of the other
> >   allocations and follow that with the registration of virtio devices,
> >   before actually booting the rproc. And we tear down the virtio devices
> >   as part of shutdown.
> > 
> > * We remove the rproc_boot()/shutdown() calls from the
> >   remoteproc_virtio.c and based on rproc->always_on we call these in the
> >   async firmware loader callback, in rproc_trigger_recovery() and
> >   rproc_del().
> > 
> > 
> > A side effect of this is that the async firmware loading only purpose is
> > to trigger the boot as the firmware becomes available (something we need
> > to tweak further). I therefor see no reason to mandate the firmware is
> > unchanged between boots, which simplifies the posed rproc_set_firmware()
> > function - as long as it's done on a remoteproc that's not always_on.
> 
> I take it that you are talking about adding a new API down the road
> which should allow client drivers to set firmwares.
> 
> +1 for that.

A while back Lee posted a patch for this. With the auto-boot rework the
resource table resources gets a much cleaner life cycle and that patch
can be simplified.

So I'm hoping we can introduce that shortly.


What I was referring to is the fact that you today can't modify your
resource table calling rproc_add(), so you can replace your firmware,
but only as long as it's sharing the same resource table - unless you
start the old one, replace the files and then trigger a crash...

> 
> > 
> > 
> > == Amended resources:
> > We need a way for the driver to amend resource definitions of the
> > firmware with e.g. physical addresses and size constraints from
> > devicetree, so I suggest that:
> > 
> > * We split the resource table parsing and allocation of resources in two
> >   steps; where the parse step updates the lists of resources and then at
> >   the time of boot we run through these and allocate the actual
> >   resources.
> > 
> > * We expose an API to the drivers so that they can register resources,
> >   as if they came from the table parser. We match each resource against
> >   previously registered resources based on name and merge or reject the
> >   updates. E.g. we merge a reference to the resource table offset and we
> >   reject incompatible size changes.
> 
> So, all this to be invoked by a platform driver before rproc_add() even
> before requesting and processing a firmware? Is this also only limited
> for remoteproc platform drivers, or gonna be extended to client drivers
> (in the case where they are setting the firmwares).
> 

It provides the mechanism for remoteproc drivers to provide the core
with resources specified in e.g. devicetree. Allowing e.g. me to reuse
the carveout handling on Qualcomm platforms, where we don't have a
resource table.

> One question w.r.t Lee's series is also adding new resources - how does
> that work with loaded resource tables given that you are dynamically
> increasing the table size, and the firmware image is already pre-linked.
> Or is the assumption here that there will not be a loaded resource table
> in such a scenario, we will have a loaded resource table for firmwares
> with vdevs for sure.
> 

That's a very valid concern, I would hope that we could look at the size
of the loaded-resource segment in the ".resource_table" segment. Perhaps
as we validate the resource table?

Can you please bring this issue up on the affected patch?

> > 
> > 
> > == Resource-less firmware:
> > To be able to use remoteproc with firmware either without a resource
> > table or resource data in other forms we today provide a resource table
> > stub in each driver, instead we could refactor the resource table
> > parsing code.
> > 
> > * We replace the find_rsc_table operation in rproc_fw_ops with a parse
> >   operation, that uses the newly created API (above) to register the
> >   resources with the core; largely decoupling the resource table format
> >   from the remoteproc core.
> > 
> > * We make the parse() function in rproc_fw_ops optional, allowing
> >   remoteproc drivers to specify that there's no resource parsing to be
> >   done (they can still provide resources programmatically between
> >   rproc_alloc() and rproc_add()).
> > 
> > This setup allows custom resource building functions to be implemented,
> > one such example is the Qualcomm firmware files where most resource data
> > is a combination of static information (DT) and data from the ELF
> > header.
> > 
> > 
> > == Resource-less firmware with installed resource table:
> > The now available list of resources that have been registered with the
> > core can serve as input for a function that generates a resource table
> > for the remote.  This gives us a mechanism for providing a remoteproc
> > with information about resources in cases where the firmware lacks a
> > resource table.
> > 
> > * We replace the rproc_find_loaded_rsc_table() with an new fw_op that is
> >   tasked with installing the resource table and update rproc->table_ptr.
> >   This op is made optional, for the remoteprocs with no installed
> >   resource table.
> 
> So, kinda similar to above question, how do you find a suitable location
> for this in the case of installed resource table?
> 

Dito.

But here it would be more natural to return -ENOSPC or similar if there
is a lack of space.

> > 
> > * We provide a helper function in the core that can be used in the
> >   fw_op, that builds a resource table in memory and copy this into the
> >   appropriate address of the remote, and  update rproc->table_ptr to
> >   this.
> > 
> > The install function will be tasked with making sure table_ptr is valid
> > and we make sure that error paths out of there and upon shutdown the
> > core will make table_ptr reference the core's version - which might be
> > NULL if no resource table exist.
> > 
> > 
> > == Supporting rpmsg alternatives:
> > For systems with other communication mechanisms than rpmsg we still want
> > to be able to instantiate and tear down devices representing these
> > communication channels, in a way that follows the life cycle of the
> > remoteproc. To do this I suggest that:
> > 
> > * Like other resources we split virtio device handling in the remoteproc
> >   core into adding rproc_vdevs to rproc->rvdevs and actually calling
> >   rproc_add_virtio_dev().
> > 
> > * We generalise the rproc->rvdev somewhat, to be a list of "subdevices"
> >   that are registered with the remoteproc; each providing callbacks for
> >   registering and unregistering themselves as we bring the remoteproc up
> >   and down.
> 
> +1, this is a good direction.
> 

Thanks for your input!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Ongoing remoteproc discussions
  2016-08-11 16:48     ` Suman Anna
@ 2016-08-11 19:05       ` Bjorn Andersson
  0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2016-08-11 19:05 UTC (permalink / raw)
  To: Suman Anna
  Cc: Loic PALLARDY, linux-remoteproc, linux-kernel, Lee Jones,
	Sarangdhar Joshi, Eric FINCO, Russell Wayman, Matthew Locke,
	Kumar Gala, Bill Fletcher, Puja Gupta, Ohad Ben-Cohen

On Thu 11 Aug 09:48 PDT 2016, Suman Anna wrote:

> On 08/10/2016 03:22 PM, Bjorn Andersson wrote:
> > On Wed 03 Aug 07:52 PDT 2016, Loic PALLARDY wrote:
> > 
> >>> == Auto-boot or always-on:
> > [..]
> >>>
> >> [LPA] As already mentioned in patch review, I would prefer auto-boot
> >> name rather than always-on for this feature.
> > 
> > Agreed.
> > 
> >> What about coprocessor already loaded and started at boot stage? It
> >> may be the case of coprocessor used by bootloader and can't reset
> >> without breaking use case or coprocessor with security constraints.
> > 
> > For the cases I've dealt with we just didn't represent the remote
> > processor in the kernel, we just reserved the carveouts and communicated
> > with it.
> 
> Yeah, we have a similar usecase as well, and we do want the remoteproc
> to behave as in the normal case once the kernel has booted up and the
> corresponding driver has been probed. We have had to do some magic (not
> zeroing memory) for presenting the remoteproc still to Linux-side
> applications and client drivers.
> 
> This indeed brings me one of the list of enhancements I have in mind -
> to add an ops for individual driver control for allocating memory on
> carveouts, vrings etc with a fall-back to the dma_alloc API in the
> remoteproc core.
> 

For this case you would provide the carveout resource a fixed location,
Loic is currently looking at one of my suggestions of using memremap()
instead of dma_alloc_coherent() for the resources that has a specified
"pa".

FYI, the alternative suggestion for handling regions with fixed location
is to create "surrogate" devices, that gets assigned the memory range
and use this for dma_alloc_coherent() - which would not solve this
problem.

> Bjorn, I take it that you are not using rpmsg here if that lifecycle is
> managed separately from remoteproc.
> 

On this platform it's the Qualcomm equivalent "SMD", and we do. During
boot we detect the presence of the vdev-equivalent and the link to this
processor comes up and stays up until you power off the board.

> > 
> >> In that case, remoteproc should allocate rproc resource at linux level
> >> and sync on current rproc state.
> > 
> > Sure.
> > 
> >>>
> > [..]
> >>> == Resource-less firmware:
> >>> To be able to use remoteproc with firmware either without a resource table
> >>> or resource data in other forms we today provide a resource table stub in
> >>> each driver, instead we could refactor the resource table parsing code.
> >>>
> >>> * We replace the find_rsc_table operation in rproc_fw_ops with a parse
> >>>   operation, that uses the newly created API (above) to register the
> >>>   resources with the core; largely decoupling the resource table format
> >>>   from the remoteproc core.
> >>>
> >>> * We make the parse() function in rproc_fw_ops optional, allowing
> >>>   remoteproc drivers to specify that there's no resource parsing to be
> >>>   done (they can still provide resources programmatically between
> >>>   rproc_alloc() and rproc_add()).
> >>>
> >>> This setup allows custom resource building functions to be implemented,
> >>> one such example is the Qualcomm firmware files where most resource data
> >>> is a combination of static information (DT) and data from the ELF header.
> >> [LPA] Do you have a list of resources you would like to support here?
> > 
> > With resources here I meant the existing remoteproc resources, i.e.
> > carveouts, devmem, trace and vdev/vrings.
> > 
> >> In ST we plan to have DT for rproc resource description (PIO,
> >> peripheral bus...). Today coprocessor resources are managed
> >> dynamically using resource manager developed by TI on omap.
> >> But this solution is consuming time and code size.
> >> We would like to implement rproc resource allocation at rproc_boot
> >> time, parsing associated DT section and getting the different
> >> requested resources...
> 
> Yeah, this becomes somewhat complicated when we are talking about
> peripherals, because it depends on they get used. I see the following
> usage patterns:
> 1. do not instantiate the devices on Linux, and leave them to be managed
> completely by s/w running on remoteproc.

This is the easy case, if you ship a product you know which resources
belong to the remote and can make sure that they are not referenced by
the Linux system.

> 2. resources that can be managed alongside the remoteproc state (request
> them up before rproc_boot, and release them after rproc_shutdown). This
> can always be done within the respective remoteproc driver as the
> peripherals used are specific to each platform.
> 3. resources that only need to be managed at runtime, especially if the
> PM around them in controlled on the Linux-side.

If its resources that are related to the life cycle of the remoteproc I
think they belong in the remoteproc driver itself, if it's dynamic,
application level resources I think they should be handled by some sort
of (e.g. rpmsg) client driver.

> 
> > 
> > Are you talking about the resmgr found in downstream TI trees? What
> > kinds of resources and how would this look like?
> > 
> >> Is it aligned with your view?
> >>
> > 
> > I'm generally considering these resources (e.g. regulators exposed by
> > resmgr) not being part of the life cycle management of the remote
> > processor, but rather related to the application running on the remote
> > processor; as such I don't think they should reside in the remoteproc
> > core.
> 
> Agreed, we did use resmgr specifically for #3. It also allowed us to
> recover these resources in case of a remoteproc crash while holding them.
> 
> > 
> > That said, for resmgr to move upstream I think it needs to be
> > generalized.
> 
> Indeed, the TI resmgr was written before DT, and it would need rework if
> we were to go down that path.
> 
> That said, if the management is moving towards the System Control
> Processor like frameworks, this won't be needed.
> 

I'm looking forward to learn the details of these requirements, so that
we can figure out how to best support them upstream.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Ongoing remoteproc discussions
  2016-08-11 18:13   ` Bjorn Andersson
@ 2016-08-16 12:24       ` Loic PALLARDY
  0 siblings, 0 replies; 13+ messages in thread
From: Loic PALLARDY @ 2016-08-16 12:24 UTC (permalink / raw)
  To: Bjorn Andersson, Suman Anna
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Eric FINCO, Russell Wayman,
	Matthew Locke  <matthew.locke@linaro.org>,
	Kumar Gala, Bill Fletcher  <bill.fletcher@linaro.org>,
	Puja Gupta, Ohad Ben-Cohen



> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, August 11, 2016 8:13 PM
> To: Suman Anna <s-anna@ti.com>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Lee
> Jones <lee.jones@linaro.org>; Sarangdhar Joshi <spjoshi@codeaurora.org>;
> Loic PALLARDY <loic.pallardy@st.com>; Eric FINCO <eric.finco@st.com>;
> Russell Wayman <russell.wayman@linaro.org>; Matthew Locke
> <matthew.locke@linaro.org>; Kumar Gala <kumar.gala@linaro.org>; Bill
> Fletcher <bill.fletcher@linaro.org>; Puja Gupta <pujag@codeaurora.org>;
> Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: Ongoing remoteproc discussions
> 
> On Wed 10 Aug 17:19 PDT 2016, Suman Anna wrote:
> 
> > Hi Bjorn,
> >
> 
> Hi,
> 
> > On 07/18/2016 06:10 PM, Bjorn Andersson wrote:
> [..]
> > > == Auto-boot or always-on:
> > > There are cases where we want to achieve the current auto-boot
> > > mechanism without rpmsg and there are cases where we don't want to
> > > auto-boot a remoteproc just because its resource table contains
> > > rpmsg entries. So we need to decouple this logic from the vdev. I suggest
> that:
> >
> > I am trying to understand the usecase where one doesn't want to
> > auto-boot with rpmsg entries, did you come across such an usecase?
> >
> 
> This is a request that comes from Loic (ST). I'm unaware of the details, but I
> can think of scenarios where rpmsg channels serves as auxiliary functionality
> to the main purpose of a co-processor (e.g. debug functionality).
> 
Hi Bjorn, Suman,

In ST, in general we have a driver on the top of remoteproc to control rproc activities (set fw name, boot, shutdown...).
The request was to have the same behavior between rproc with and without rpmsg.
With rpmsg, a rproc is automatically boot and so if on the top driver wants to apply same boot sequence (rproc_set_fw_name, rproc_boot, rproc_shutdown), rproc_shutdown will never happened as rproc_boot called twice (one by on the top driver, one by "rpmsg").

So the proposal was to have autoboot capability linked to the rproc itself, not to the communication link.

Regards,
Loic
[...]
> 
> Thanks for your input!
> 
> Regards,
> Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Ongoing remoteproc discussions
@ 2016-08-16 12:24       ` Loic PALLARDY
  0 siblings, 0 replies; 13+ messages in thread
From: Loic PALLARDY @ 2016-08-16 12:24 UTC (permalink / raw)
  To: Bjorn Andersson, Suman Anna
  Cc: linux-remoteproc, linux-kernel, Lee Jones, Sarangdhar Joshi,
	Eric FINCO, Russell Wayman, Matthew Locke, Kumar Gala,
	Bill Fletcher, Puja Gupta, Ohad Ben-Cohen



> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, August 11, 2016 8:13 PM
> To: Suman Anna <s-anna@ti.com>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Lee
> Jones <lee.jones@linaro.org>; Sarangdhar Joshi <spjoshi@codeaurora.org>;
> Loic PALLARDY <loic.pallardy@st.com>; Eric FINCO <eric.finco@st.com>;
> Russell Wayman <russell.wayman@linaro.org>; Matthew Locke
> <matthew.locke@linaro.org>; Kumar Gala <kumar.gala@linaro.org>; Bill
> Fletcher <bill.fletcher@linaro.org>; Puja Gupta <pujag@codeaurora.org>;
> Ohad Ben-Cohen <ohad@wizery.com>
> Subject: Re: Ongoing remoteproc discussions
> 
> On Wed 10 Aug 17:19 PDT 2016, Suman Anna wrote:
> 
> > Hi Bjorn,
> >
> 
> Hi,
> 
> > On 07/18/2016 06:10 PM, Bjorn Andersson wrote:
> [..]
> > > == Auto-boot or always-on:
> > > There are cases where we want to achieve the current auto-boot
> > > mechanism without rpmsg and there are cases where we don't want to
> > > auto-boot a remoteproc just because its resource table contains
> > > rpmsg entries. So we need to decouple this logic from the vdev. I suggest
> that:
> >
> > I am trying to understand the usecase where one doesn't want to
> > auto-boot with rpmsg entries, did you come across such an usecase?
> >
> 
> This is a request that comes from Loic (ST). I'm unaware of the details, but I
> can think of scenarios where rpmsg channels serves as auxiliary functionality
> to the main purpose of a co-processor (e.g. debug functionality).
> 
Hi Bjorn, Suman,

In ST, in general we have a driver on the top of remoteproc to control rproc activities (set fw name, boot, shutdown...).
The request was to have the same behavior between rproc with and without rpmsg.
With rpmsg, a rproc is automatically boot and so if on the top driver wants to apply same boot sequence (rproc_set_fw_name, rproc_boot, rproc_shutdown), rproc_shutdown will never happened as rproc_boot called twice (one by on the top driver, one by "rpmsg").

So the proposal was to have autoboot capability linked to the rproc itself, not to the communication link.

Regards,
Loic
[...]
> 
> Thanks for your input!
> 
> Regards,
> Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-08-16 12:24 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-18 23:10 Ongoing remoteproc discussions Bjorn Andersson
2016-07-25 12:26 ` Peter Griffin
2016-07-28 17:32   ` Bjorn Andersson
2016-08-03 14:52 ` Loic PALLARDY
2016-08-03 14:52   ` Loic PALLARDY
2016-08-10 20:22   ` Bjorn Andersson
2016-08-11 16:48     ` Suman Anna
2016-08-11 19:05       ` Bjorn Andersson
2016-08-11  0:19 ` Suman Anna
2016-08-11  0:19   ` Suman Anna
2016-08-11 18:13   ` Bjorn Andersson
2016-08-16 12:24     ` Loic PALLARDY
2016-08-16 12:24       ` Loic PALLARDY

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.