| Message ID | 1574940831-7433-1-git-send-email-loic.pallardy@st.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/1] remoteproc: add support for co-processor loaded and booted before kernel | expand |
On Thu 28 Nov 03:33 PST 2019, Loic Pallardy wrote: > 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. > It is platform driver responsibility to implement the right firmware > load ops according to HW specificities. > I was going to apply the patch, as I like what it actually does. But I'm concerned about how you're going to use it (which you fail to show in this single patch). Just two things below. > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > Change from v3: > - add comment about firmware NULL pointer > - add Mathieu Poirier Ack > Change from v2: > - rename property into skip_fw_load > - update rproc_boot and rproc_fw_boot description > - update commit message > Change from v1: > - Keep bool in struct rproc > --- > 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 > index 307df98347ba..367a7929b7a0 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > return ret; > } > > -/* > - * take a firmware and boot a remote processor with it. > +/** > + * rproc_fw_boot() - boot specified remote processor according to specified > + * firmware > + * @rproc: handle of a remote processor > + * @fw: pointer on firmware to handle > + * > + * Handle resources defined in resource table, load firmware and > + * start remote processor. > + * > + * If firmware pointer fw is NULL, firmware is not handled by remoteproc > + * core, but under the responsibility of platform driver. > + * > + * Returns 0 on success, and an appropriate error value otherwise. > */ > static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > { > @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > if (ret) > return ret; > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > + if (fw) > + dev_info(dev, "Booting fw image %s, size %zd\n", name, > + fw->size); > + else > + dev_info(dev, "Synchronizing with preloaded co-processor\n"); This log line implies that ops->start() doesn't actually start the remoteproc, but it sounds like a remote proc with skip_fw_load actually would boot the remote processor, but with some pre-existing firmware. As such it makes more sense, in this patch, to print "Booting\n" here. But I presume you have a platform driver with a nop start() implementation and no ability to reload the firmware on a crash? > > /* > * if enabling an IOMMU isn't relevant for this rproc, this is > @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work) > * rproc_boot() - boot a remote processor > * @rproc: handle of a remote processor > * > - * Boot a remote processor (i.e. load its firmware, power it on, ...). > + * Boot a remote processor (i.e. load its firmware, power it on, ...) from > + * different contexts: > + * - power off > + * - preloaded firmware > + * - started before kernel execution > + * The different operations are selected thanks to properties defined by > + * platform driver. > * > - * If the remote processor is already powered on, this function immediately > - * returns (successfully). > + * If the remote processor is already powered on at rproc level, this function > + * immediately returns (successfully). > * > * Returns 0 on success, and an appropriate error value otherwise. > */ > int rproc_boot(struct rproc *rproc) > { > - const struct firmware *firmware_p; > + const struct firmware *firmware_p = NULL; > struct device *dev; > int ret; > > @@ -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; Why do this on every boot? Why don't you change rproc_alloc() to never populate rproc->firmware? Regards, Bjorn > } > > 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 > + */ > + ret = rproc_boot(rproc); > + if (ret < 0) > + return ret; > + } else if (rproc->auto_boot) { > + /* if rproc is marked always-on, request it to boot */ > ret = rproc_trigger_auto_boot(rproc); > if (ret < 0) > return ret; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 16ad66683ad0..4fd5bedab4fa 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -479,6 +479,7 @@ struct rproc_dump_segment { > * @table_sz: size of @cached_table > * @has_iommu: flag to indicate if remote processor is behind an MMU > * @auto_boot: flag to indicate if remote processor should be auto-started > + * @skip_fw_load: remote processor has been preloaded before start sequence > * @dump_segments: list of segments in the firmware > * @nb_vdev: number of vdev currently handled by rproc > */ > @@ -512,6 +513,7 @@ struct rproc { > size_t table_sz; > bool has_iommu; > bool auto_boot; > + bool skip_fw_load; > struct list_head dump_segments; > int nb_vdev; > }; > -- > 2.7.4 >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 307df98347ba..367a7929b7a0 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) return ret; } -/* - * take a firmware and boot a remote processor with it. +/** + * rproc_fw_boot() - boot specified remote processor according to specified + * firmware + * @rproc: handle of a remote processor + * @fw: pointer on firmware to handle + * + * Handle resources defined in resource table, load firmware and + * start remote processor. + * + * If firmware pointer fw is NULL, firmware is not handled by remoteproc + * core, but under the responsibility of platform driver. + * + * Returns 0 on success, and an appropriate error value otherwise. */ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) { @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) if (ret) return ret; - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); + if (fw) + dev_info(dev, "Booting fw image %s, size %zd\n", name, + fw->size); + else + dev_info(dev, "Synchronizing with preloaded co-processor\n"); /* * if enabling an IOMMU isn't relevant for this rproc, this is @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work) * rproc_boot() - boot a remote processor * @rproc: handle of a remote processor * - * Boot a remote processor (i.e. load its firmware, power it on, ...). + * Boot a remote processor (i.e. load its firmware, power it on, ...) from + * different contexts: + * - power off + * - preloaded firmware + * - started before kernel execution + * The different operations are selected thanks to properties defined by + * platform driver. * - * If the remote processor is already powered on, this function immediately - * returns (successfully). + * If the remote processor is already powered on at rproc level, this function + * immediately returns (successfully). * * Returns 0 on success, and an appropriate error value otherwise. */ int rproc_boot(struct rproc *rproc) { - const struct firmware *firmware_p; + const struct firmware *firmware_p = NULL; struct device *dev; int ret; @@ -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; } 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 + */ + ret = rproc_boot(rproc); + if (ret < 0) + return ret; + } else if (rproc->auto_boot) { + /* if rproc is marked always-on, request it to boot */ ret = rproc_trigger_auto_boot(rproc); if (ret < 0) return ret; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 16ad66683ad0..4fd5bedab4fa 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -479,6 +479,7 @@ struct rproc_dump_segment { * @table_sz: size of @cached_table * @has_iommu: flag to indicate if remote processor is behind an MMU * @auto_boot: flag to indicate if remote processor should be auto-started + * @skip_fw_load: remote processor has been preloaded before start sequence * @dump_segments: list of segments in the firmware * @nb_vdev: number of vdev currently handled by rproc */ @@ -512,6 +513,7 @@ struct rproc { size_t table_sz; bool has_iommu; bool auto_boot; + bool skip_fw_load; struct list_head dump_segments; int nb_vdev; };