On Wed 06 Apr 19:32 PDT 2016, Stefan Agner wrote:
> Start remote processor even if the ELF does not provide a resource
> table.
>
> Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
> ---
> Hi Bjorn,
>
> Nice meeting you at ELC!
You too, thanks for the good chat.
> I had this patch carrying in one of my tree
> which basically enables to boot a bare ELF file... Altough my goal
> is it to use firmwares with some kind of resource table, I think we
> should not enforce having a resource table...
>
I agree, we should support platforms without resource tables; and the
variants of no resource-table in the ELF and no loaded resource table
(transfered to the remote).
> --
> Stefan
>
> drivers/remoteproc/remoteproc_core.c | 44 +++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3d7d58a..5e16910 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -799,9 +799,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> struct resource_table *table, *loaded_table;
> int ret, tablesz;
>
> - if (!rproc->table_ptr)
> - return -ENOMEM;
> -
> ret = rproc_fw_sanity_check(rproc, fw);
> if (ret)
> return ret;
> @@ -824,21 +821,20 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> /* look for the resource table */
> table = rproc_find_rsc_table(rproc, fw, &tablesz);
> if (!table) {
We're fetching the resource table here just to verify that the firmware
was not changed since we set up the vrings and whatnot.
So I think it would be more consistent to check for rproc->table_ptr
being set here.
> - dev_err(dev, "Failed to find resource table\n");
> - goto clean_up;
> - }
> -
> - /* Verify that resource table in loaded fw is unchanged */
> - if (rproc->table_csum != crc32(0, table, tablesz)) {
> - dev_err(dev, "resource checksum failed, fw changed?\n");
> - goto clean_up;
> - }
> + dev_info(dev, "No resource table found, continuing...\n");
I think rather than just informing the user (if there even is someone
looking at the dmesg) we should push an "assert" down to the drivers
that require a resource table. But we can make that an incremental patch
(and just be silent for now).
> + } else {
> + /* Verify that resource table in loaded fw is unchanged */
> + if (rproc->table_csum != crc32(0, table, tablesz)) {
> + dev_err(dev, "resource checksum failed, fw changed?\n");
> + goto clean_up;
> + }
>
> - /* handle fw resources which are required to boot rproc */
> - ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
> - if (ret) {
> - dev_err(dev, "Failed to process resources: %d\n", ret);
> - goto clean_up;
> + /* handle fw resources which are required to boot rproc */
> + ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
> + if (ret) {
> + dev_err(dev, "Failed to process resources: %d\n", ret);
> + goto clean_up;
> + }
For the Qualcomm case I worked around this by returning a dummy resource
table, see qcom_mdt_find_rsc_table() in
https://patchwork.kernel.org/patch/8681951/
Please update this part and resend a patch and I'll give it some test. I
think we can merge this.
> }
>
> /* load the ELF segments to memory */
> @@ -855,13 +851,15 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> * In order to pass this information to the remote device we must
> * copy this information to device memory.
> */
> - loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> - if (!loaded_table) {
> - ret = -EINVAL;
> - goto clean_up;
> - }
> + if (table) {
> + loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> + if (!loaded_table) {
> + ret = -EINVAL;
> + goto clean_up;
> + }
>
> - memcpy(loaded_table, rproc->cached_table, tablesz);
> + memcpy(loaded_table, rproc->cached_table, tablesz);
> + }
I have a patch available for making the loadable table optional, please
have a look at:
https://patchwork.kernel.org/patch/8681971/
Comments/acks/tested-by appreciated.
Looking at it again I see that I should not assign the table_ptr if we
don't have a loaded table. The table_ptr will be set back to
cached_table upon shutdown, but this is not consistent so I will fix
that in a v3.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
