Hi Roger,
On 2/2/2024 4:49 PM, Roger Quadros wrote:
>
>
> On 30/01/2024 08:33, MD Danish Anwar wrote:
>> Add APIs to set a firmware_name to a rproc and boot the rproc with the
>> same firmware.
>>
>> Clients can call rproc_set_firmware() API to set firmware_name for a rproc
>> whereas rproc_boot() will load the firmware set by rproc_set_firmware() to
>> a buffer by calling request_firmware_into_buf(). rproc_boot() will then
>> load the firmware file to the remote processor and start the remote
>> processor.
>>
>> Also include "fs-loader.h" and make remoteproc driver select FS_LOADER in
>> Kconfig so that we can call request_firmware_into_buf() from remoteproc
>> driver.
>>
>> Signed-off-by: MD Danish Anwar <danishan...@ti.com>
>> ---
>> Changes from v3 to v4:
>> *) No functional change. Splitted the patch out of the series as suggested
>> by Nishant.
>> *) Droppped the RFC tag.
>>
>> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishan...@ti.com/
>>
>> drivers/remoteproc/Kconfig | 1 +
>> drivers/remoteproc/rproc-uclass.c | 85 +++++++++++++++++++++++++++++++
>> include/remoteproc.h | 35 +++++++++++++
>> 3 files changed, 121 insertions(+)
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 781de530af..0fdf1b38ea 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -10,6 +10,7 @@ menu "Remote Processor drivers"
>> # All users should depend on DM
>> config REMOTEPROC
>> bool
>> + select FS_LOADER
>> depends on DM
>>
>> # Please keep the configuration alphabetically sorted.
>> diff --git a/drivers/remoteproc/rproc-uclass.c
>> b/drivers/remoteproc/rproc-uclass.c
>> index 28b362c887..76db4157f7 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -13,6 +13,7 @@
>> #include <log.h>
>> #include <malloc.h>
>> #include <virtio_ring.h>
>> +#include <fs_loader.h>
>> #include <remoteproc.h>
>> #include <asm/io.h>
>> #include <dm/device-internal.h>
>> @@ -961,3 +962,87 @@ unsigned long rproc_parse_resource_table(struct udevice
>> *dev, struct rproc *cfg)
>>
>> return 1;
>> }
>> +
>> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>> +{
>> + struct dm_rproc_uclass_pdata *uc_pdata;
>> + int len;
>> + char *p;
>> +
>> + if (!rproc_dev || !fw_name)
>> + return -EINVAL;
>> +
>> + uc_pdata = dev_get_uclass_plat(rproc_dev);
>
> This can return NULL and you shuould error out if it does.
>
Sure.
>> +
>> + len = strcspn(fw_name, "\n");
>> + if (!len) {
>> + debug("can't provide empty string for firmware name\n");
>
> how about "invalid filename" ?
>
Sure.
>> + return -EINVAL;
>> + }
>> +
>> + p = strndup(fw_name, len);
>> + if (!p)
>> + return -ENOMEM;
>> +
>> + uc_pdata->fw_name = p;
>> +
>> + return 0;
>> +}
>> +
>> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>> +{
>> + struct dm_rproc_uclass_pdata *uc_pdata;
>> + struct udevice *fs_loader;
>> + void *addr = malloc(fw_size);
>
> I will suggest to do malloc just before you need the buffer.
> You need to free up the buffer an all return paths after that.
>
That is correct. I will do malloc just before calling
request_firmware_into_buf() API.
>> + int core_id, ret = 0;
>> + char *firmware;
>> + ulong rproc_addr;
>
> do you really need rproc_addr? You could use addr itself.
>
Sure.
>> +
>> + if (!rproc_dev)
>> + return -EINVAL;
>> +
>> + if (!addr)
>> + return -ENOMEM;
>> +
>> + uc_pdata = dev_get_uclass_plat(rproc_dev);
>> + core_id = dev_seq(rproc_dev);
>> + firmware = uc_pdata->fw_name;
>> +
>> + if (!firmware) {
>> + debug("No firmware set for rproc core %d\n", core_id);
>> + return -EINVAL;
>> + }
>> +
>> + /* Initialize all rproc cores */
>> + rproc_init();
>
> if (!rproc_is_initialized()) {
> ret = rproc_init()
> if (ret) {
> debug("rproc_init() failed: %d\n", ret);
> return ret;
> }
> }
>
Sure.
>> +
>> + /* Loading firmware to a given address */
>> + ret = get_fs_loader(&fs_loader);
>> + if (ret) {
>> + debug("could not get fs loader: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = request_firmware_into_buf(fs_loader, firmware, addr, fw_size, 0);
>> + if (ret < 0) {
>> + debug("could not request %s: %d\n", firmware, ret);
>> + return ret;
>> + }
>> +
>> + rproc_addr = (ulong)addr;
>> +
>> + ret = rproc_load(core_id, rproc_addr, ret);
>
> ret = rproc_load(coare_id, (ulong)addr, ret);
>
>> + if (ret) {
>> + debug("failed to load %s to rproc core %d from addr 0x%08lX err
>> %d\n",
>> + uc_pdata->fw_name, core_id, rproc_addr, ret);
>> + return ret;
>> + }
>> +
>> + ret = rproc_start(core_id);
>> + if (ret) {
>> + debug("failed to start rproc core %d\n", core_id);
>> + return ret;
>> + }
>> +
>> + return ret;
>
> return 0;
>
>> +}
>> diff --git a/include/remoteproc.h b/include/remoteproc.h
>> index 91a88791a4..e53f85ba51 100644
>> --- a/include/remoteproc.h
>> +++ b/include/remoteproc.h
>> @@ -403,6 +403,7 @@ enum rproc_mem_type {
>> * @name: Platform-specific way of naming the Remote proc
>> * @mem_type: one of 'enum rproc_mem_type'
>> * @driver_plat_data: driver specific platform data that may be needed.
>> + * @fw_name: firmware name
>> *
>> * This can be accessed with dev_get_uclass_plat() for any UCLASS_REMOTEPROC
>> * device.
>> @@ -412,6 +413,7 @@ struct dm_rproc_uclass_pdata {
>> const char *name;
>> enum rproc_mem_type mem_type;
>> void *driver_plat_data;
>> + char *fw_name;
>> };
>>
>> /**
>> @@ -705,6 +707,35 @@ unsigned long rproc_parse_resource_table(struct udevice
>> *dev,
>> struct resource_table *rproc_find_resource_table(struct udevice *dev,
>> unsigned int addr,
>> int *tablesz);
>> +/**
>> + * rproc_set_firmware() - assign a new firmware
>
> firmware/firmware name.
>
>> + * @rproc_dev: device for wich new firmware is being assigned
>
> firmware/firmware name
> wich/which
>
>> + * @fw_name: new firmware name to be assigned
>> + *
>> + * This function allows remoteproc drivers or clients to configure a custom
>> + * firmware name. The function does not trigger a remote processor boot,
>> + * only sets the firmware name used for a subsequent boot.
>> + *
>> + * This function sets the fw_name field in uclass pdata of the Remote proc
>> + *
>> + * Return: 0 on success or a negative value upon failure
>> + */
>> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name);
>> +
>> +/**
>> + * rproc_boot() - boot a remote processor
>> + * @rproc_dev: rproc device to boot
>> + * @fw_size: Size of the memory to allocate for firmeware
>
> firmeware/firmware
>
I'll fix all these typos.
> How does caller know what firmware size to set to?
> This should already be private to the rproc as it knows
> how large is its program memory.
>
Caller is trying to boot the rproc with a firmware binary. Caller should
know the size of binary that it wants to load to rproc core. Caller will
specify the binary size to rproc_boot(). Based on the size provided by
caller, rproc_boot() will then allocate that much memory and call
request_firmware_into_buf() with the size and allocated buffer. If the
caller doesn't provide minimum size rproc_load() will fail.
rproc_load() calls respective driver ops, for example: pru_load().
pru_load() [1] API checks the required size of firmware to load by
casting the buffer into Elf32_Ehdr and Elf32_Phdr and returns error if
size provided by caller is less than this.
if (offset + filesz > size) {
dev_dbg(dev, "truncated fw: need 0x%x avail 0x%zx\n",
offset + filesz, size);
ret = -EINVAL;
break;
}
>> + *
>> + * Boot a remote processor (i.e. load its firmware, power it on, ...).
>> + *
>> + * This function first loads the firmware set in the uclass pdata of Remote
>> + * processor to a buffer and then loads firmware to the remote processor
>> + * using rproc_load().
>> + *
>> + * Return: 0 on success, and an appropriate error value otherwise
>> + */
>> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size);
>
> Was wondering if you need separate API for rproc_set_firmware or we can just
> pass firmware name as argument to rproc_boot()?
>
Technically we can. But when we discussed this approach first in v1, you
had asked to keep the APIs similar to upstream linux. Upstream linux has
these two APIs so I kept it that way. If you want I can drop the first
API. Please let me know.
>> #else
>> static inline int rproc_init(void) { return -ENOSYS; }
>> static inline int rproc_dev_init(int id) { return -ENOSYS; }
>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct
>> udevice *dev, ulong fw_addr,
>> ulong fw_size, ulong *rsc_addr,
>> ulong *rsc_size)
>> { return -ENOSYS; }
>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char
>> *fw_name)
>> +{ return -ENOSYS; }
>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>> +{ return -ENOSYS; }
>> #endif
>>
>> #endif /* _RPROC_H_ */
>
[1]
https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
--
Thanks and Regards,
Md Danish Anwar
