On Tue 11 Feb 09:42 PST 2020, Arnaud Pouliquen wrote:
> From: Loic Pallardy <loic.pallardy@xxxxxx>
>
> Remote processor could boot independently or be loaded/started before
> Linux kernel by bootloader or any firmware.
> This patch introduces a new property in rproc core, named skip_fw_load,
> to be able to allocate resources and sub-devices like vdev and to
> synchronize with current state without loading firmware from file system.
This sentence describes the provided patch.
As I expressed in the earlier version, in order to support remoteprocs
that doesn't need firmware loading, e.g. running from some ROM or
dedicated flash storage etc, this patch looks really good.
> It is platform driver responsibility to implement the right firmware
> load ops according to HW specificities.
But this last sentence describes a remoteproc that indeed needs
firmware and that the purpose of this patch is to work around the core's
handling of the firmware.
>
> Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> Acked-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
> ---
> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++------
> include/linux/remoteproc.h | 2 +
> 2 files changed, 55 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
[..]
> @@ -1758,11 +1779,20 @@ int rproc_boot(struct rproc *rproc)
>
> dev_info(dev, "powering up %s\n", rproc->name);
>
> - /* load firmware */
> - ret = request_firmware(&firmware_p, rproc->firmware, dev);
> - if (ret < 0) {
> - dev_err(dev, "request_firmware failed: %d\n", ret);
> - goto downref_rproc;
> + if (!rproc->skip_fw_load) {
> + /* load firmware */
> + ret = request_firmware(&firmware_p, rproc->firmware, dev);
> + if (ret < 0) {
> + dev_err(dev, "request_firmware failed: %d\n", ret);
> + goto downref_rproc;
> + }
> + } else {
> + /*
> + * Set firmware name pointer to null as remoteproc core is not
> + * in charge of firmware loading
> + */
> + kfree(rproc->firmware);
> + rproc->firmware = NULL;
As stated before, I think it would be more appropriate to allow a
remoteproc driver for hardware that shouldn't have firmware loaded to
never set rproc->firmware.
And I'm still curious how you're dealing with a crash or a restart on
this remoteproc. Don't you need to reload your firmware in these
circumstances? Do you perhaps have a remoteproc that's both
"already_booted" and "skip_fw_load"?
> }
>
> ret = rproc_fw_boot(rproc, firmware_p);
> @@ -1916,8 +1946,17 @@ int rproc_add(struct rproc *rproc)
> /* create debugfs entries */
> rproc_create_debug_dir(rproc);
>
> - /* if rproc is marked always-on, request it to boot */
> - if (rproc->auto_boot) {
> + if (rproc->skip_fw_load) {
> + /*
> + * If rproc is marked already booted, no need to wait
> + * for firmware.
> + * Just handle associated resources and start sub devices
> + */
Again, this describes a system where the remoteproc is already booted,
not a remoteproc that doesn't need firmware loading.
Regards,
Bjorn
