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 danishanwar@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-danishanwar@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...