[PATCH v5] remoteproc: uclass: Add methods to load firmware to rproc and boot rproc
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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 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. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.
+config REMOTEPROC_MAX_FW_SIZE + hex "Maximum size of firmware that needs to be loaded to rproc" + default 0x10000 + help + Maximum size of the firmware file (elf, binary) that needs to be + loaded to th rproc core. + endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..784361cef9 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,102 @@ 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); + if (!uc_pdata) + return -EINVAL; + + len = strcspn(fw_name, "\n"); + if (!len) { + debug("invalid firmware name\n"); + 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) +{ + struct dm_rproc_uclass_pdata *uc_pdata; + struct udevice *fs_loader; + int core_id, ret = 0; + char *firmware; + void *addr; + + if (!rproc_dev) + return -EINVAL; + + uc_pdata = dev_get_uclass_plat(rproc_dev); + if (!uc_pdata) + return -EINVAL; + + 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 */ + if (!rproc_is_initialized()) { + ret = rproc_init(); + if (ret) { + debug("rproc_init() failed: %d\n", ret); + return ret; + } + } + + /* 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; + } + + if (CONFIG_REMOTEPROC_MAX_FW_SIZE) { + addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE); + if (!addr) + return -ENOMEM; + } else { + debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n"); + return -EINVAL; + } + + ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE, + 0); + if (ret < 0) { + debug("could not request %s: %d\n", firmware, ret); + goto free_buffer; + } + + ret = rproc_load(core_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, (ulong)addr, ret); + goto free_buffer; + } + + ret = rproc_start(core_id); + if (ret) + debug("failed to start rproc core %d\n", core_id); + +free_buffer: + free(addr); + return ret; +} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 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,34 @@ 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 name + * @rproc_dev: device for which new firmware name is being assigned + * @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 + * + * 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); #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,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) +{ return -ENOSYS; } #endif
#endif /* _RPROC_H_ */
On 2/17/24 5:56 PM, 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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig
[...]
+free_buffer:
- free(addr);
Thanks for taking care of freeing the memory. Acked-by: Ravi Gunasekaran r-gunasekaran@ti.com
[...]
On 17/02/2024 14:26, 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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 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. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.
+config REMOTEPROC_MAX_FW_SIZE
- hex "Maximum size of firmware that needs to be loaded to rproc"
firmware file?
/rproc/the remote processor
- default 0x10000
- help
Maximum size of the firmware file (elf, binary) that needs to beloaded to th rproc core.
s/th/the s/rproc/remote processor
endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..784361cef9 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,102 @@ 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);
- if (!uc_pdata)
return -EINVAL;- len = strcspn(fw_name, "\n");
- if (!len) {
debug("invalid firmware name\n");return -EINVAL;- }
- p = strndup(fw_name, len);
- if (!p)
return -ENOMEM;
Aren't we leaking memory if rproc_set_firmware() is called multiple times? Can we check if uc_pdata->fw_name exists and free it before the strndup?
- uc_pdata->fw_name = p;
- return 0;
+}
+int rproc_boot(struct udevice *rproc_dev) +{
- struct dm_rproc_uclass_pdata *uc_pdata;
- struct udevice *fs_loader;
- int core_id, ret = 0;
- char *firmware;
- void *addr;
- if (!rproc_dev)
return -EINVAL;- uc_pdata = dev_get_uclass_plat(rproc_dev);
- if (!uc_pdata)
return -EINVAL;- core_id = dev_seq(rproc_dev);
- firmware = uc_pdata->fw_name;
unnecessary blank line.
- if (!firmware) {
debug("No firmware set for rproc core %d\n", core_id);
No firmware name...
return -EINVAL;- }
- /* Initialize all rproc cores */
- if (!rproc_is_initialized()) {
ret = rproc_init();if (ret) {debug("rproc_init() failed: %d\n", ret);return ret;}- }
- /* 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;- }
- if (CONFIG_REMOTEPROC_MAX_FW_SIZE) {
if (defined(CONFIG_REMOTEPROC_MAX_FW_SIZE)) {
addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE);if (!addr)return -ENOMEM;- } else {
debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n");return -EINVAL;- }
- ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE,
0);- if (ret < 0) {
debug("could not request %s: %d\n", firmware, ret);goto free_buffer;- }
- ret = rproc_load(core_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, (ulong)addr, ret);goto free_buffer;- }
- ret = rproc_start(core_id);
- if (ret)
debug("failed to start rproc core %d\n", core_id);+free_buffer:
- free(addr);
- return ret;
+} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 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,34 @@ 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 name
- @rproc_dev: device for which new firmware name is being assigned
- @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
- 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); #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,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) +{ return -ENOSYS; } #endif
#endif /* _RPROC_H_ */
On 28/02/24 3:30 pm, Roger Quadros wrote:
On 17/02/2024 14:26, 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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 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. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.
+config REMOTEPROC_MAX_FW_SIZE
- hex "Maximum size of firmware that needs to be loaded to rproc"
firmware file?
/rproc/the remote processor
- default 0x10000
- help
Maximum size of the firmware file (elf, binary) that needs to beloaded to th rproc core.s/th/the s/rproc/remote processor
Will fix these typos.
endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..784361cef9 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,102 @@ 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);
- if (!uc_pdata)
return -EINVAL;- len = strcspn(fw_name, "\n");
- if (!len) {
debug("invalid firmware name\n");return -EINVAL;- }
- p = strndup(fw_name, len);
- if (!p)
return -ENOMEM;Aren't we leaking memory if rproc_set_firmware() is called multiple times? Can we check if uc_pdata->fw_name exists and free it before the strndup?
Sure, How does the below diff looks to you?
diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 784361cef9..f373b64da4 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -982,6 +982,9 @@ int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) return -EINVAL; }
+ if (uc_pdata->fw_name) + free(uc_pdata->fw_name); + p = strndup(fw_name, len); if (!p) return -ENOMEM;
- uc_pdata->fw_name = p;
- return 0;
+}
+int rproc_boot(struct udevice *rproc_dev) +{
- struct dm_rproc_uclass_pdata *uc_pdata;
- struct udevice *fs_loader;
- int core_id, ret = 0;
- char *firmware;
- void *addr;
- if (!rproc_dev)
return -EINVAL;- uc_pdata = dev_get_uclass_plat(rproc_dev);
- if (!uc_pdata)
return -EINVAL;- core_id = dev_seq(rproc_dev);
- firmware = uc_pdata->fw_name;
unnecessary blank line.
- if (!firmware) {
debug("No firmware set for rproc core %d\n", core_id);No firmware name...
return -EINVAL;- }
- /* Initialize all rproc cores */
- if (!rproc_is_initialized()) {
ret = rproc_init();if (ret) {debug("rproc_init() failed: %d\n", ret);return ret;}- }
- /* 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;- }
- if (CONFIG_REMOTEPROC_MAX_FW_SIZE) {
if (defined(CONFIG_REMOTEPROC_MAX_FW_SIZE)) {
sure.
addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE);if (!addr)return -ENOMEM;- } else {
debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n");return -EINVAL;- }
- ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE,
0);- if (ret < 0) {
debug("could not request %s: %d\n", firmware, ret);goto free_buffer;- }
- ret = rproc_load(core_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, (ulong)addr, ret);goto free_buffer;- }
- ret = rproc_start(core_id);
- if (ret)
debug("failed to start rproc core %d\n", core_id);+free_buffer:
- free(addr);
- return ret;
+} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 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,34 @@ 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 name
- @rproc_dev: device for which new firmware name is being assigned
- @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
- 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); #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,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) +{ return -ENOSYS; } #endif
#endif /* _RPROC_H_ */
On 28/02/2024 12:35, MD Danish Anwar wrote:
On 28/02/24 3:30 pm, Roger Quadros wrote:
On 17/02/2024 14:26, 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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 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. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.
+config REMOTEPROC_MAX_FW_SIZE
- hex "Maximum size of firmware that needs to be loaded to rproc"
firmware file?
/rproc/the remote processor
- default 0x10000
- help
Maximum size of the firmware file (elf, binary) that needs to beloaded to th rproc core.s/th/the s/rproc/remote processor
Will fix these typos.
endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..784361cef9 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,102 @@ 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);
- if (!uc_pdata)
return -EINVAL;- len = strcspn(fw_name, "\n");
- if (!len) {
debug("invalid firmware name\n");return -EINVAL;- }
- p = strndup(fw_name, len);
- if (!p)
return -ENOMEM;Aren't we leaking memory if rproc_set_firmware() is called multiple times? Can we check if uc_pdata->fw_name exists and free it before the strndup?
Sure, How does the below diff looks to you?
diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 784361cef9..f373b64da4 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -982,6 +982,9 @@ int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) return -EINVAL; }
if (uc_pdata->fw_name)free(uc_pdata->fw_name);p = strndup(fw_name, len); if (!p) return -ENOMEM;
This is OK. Please see below.
- uc_pdata->fw_name = p;
- return 0;
+}
+int rproc_boot(struct udevice *rproc_dev) +{
- struct dm_rproc_uclass_pdata *uc_pdata;
- struct udevice *fs_loader;
- int core_id, ret = 0;
- char *firmware;
- void *addr;
- if (!rproc_dev)
return -EINVAL;- uc_pdata = dev_get_uclass_plat(rproc_dev);
- if (!uc_pdata)
return -EINVAL;- core_id = dev_seq(rproc_dev);
- firmware = uc_pdata->fw_name;
unnecessary blank line.
- if (!firmware) {
debug("No firmware set for rproc core %d\n", core_id);No firmware name...
return -EINVAL;- }
- /* Initialize all rproc cores */
- if (!rproc_is_initialized()) {
ret = rproc_init();if (ret) {debug("rproc_init() failed: %d\n", ret);return ret;}- }
- /* 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;- }
- if (CONFIG_REMOTEPROC_MAX_FW_SIZE) {
if (defined(CONFIG_REMOTEPROC_MAX_FW_SIZE)) {
sure.
This I'm not so sure. I think 'defined' can only be used with the pre-processor e.g. #if defined() But such usage is no longer encouraged.
e.g. Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
And I'm not sure if IS_ENABLED works for hex options.
If it does not then just keep it the way it was in this patch.
addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE);if (!addr)return -ENOMEM;- } else {
debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n");return -EINVAL;- }
- ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE,
0);- if (ret < 0) {
debug("could not request %s: %d\n", firmware, ret);goto free_buffer;- }
- ret = rproc_load(core_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, (ulong)addr, ret);goto free_buffer;- }
- ret = rproc_start(core_id);
- if (ret)
debug("failed to start rproc core %d\n", core_id);+free_buffer:
- free(addr);
- return ret;
+} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 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,34 @@ 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 name
- @rproc_dev: device for which new firmware name is being assigned
- @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
- 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);Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,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) +{ return -ENOSYS; } #endif
#endif /* _RPROC_H_ */
On 28/02/24 4:12 pm, Roger Quadros wrote:
On 28/02/2024 12:35, MD Danish Anwar wrote:
On 28/02/24 3:30 pm, Roger Quadros wrote:
On 17/02/2024 14:26, 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 v4 to v5: *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size that can be loaded to a rproc. *) Added freeing of address in rproc_boot() as pointed out by Ravi. *) Allocating the address at a later point in rproc_boot() *) Rebased on latest u-boot/master [commit 9e00b6993f724da9699ef12573307afea8c19284]
Changes from v3 to v4: *) No functional change. Splitted the patch out of the series as suggested by Nishant. *) Droppped the RFC tag.
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/ v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
drivers/remoteproc/Kconfig | 8 +++ drivers/remoteproc/rproc-uclass.c | 100 ++++++++++++++++++++++++++++++ include/remoteproc.h | 34 ++++++++++ 3 files changed, 142 insertions(+)
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 781de530af..759d21437a 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. @@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU help Say 'y' here to add support for TI' K3 remoteproc driver.
+config REMOTEPROC_MAX_FW_SIZE
- hex "Maximum size of firmware that needs to be loaded to rproc"
firmware file?
/rproc/the remote processor
- default 0x10000
- help
Maximum size of the firmware file (elf, binary) that needs to beloaded to th rproc core.s/th/the s/rproc/remote processor
Will fix these typos.
endmenu diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 28b362c887..784361cef9 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,102 @@ 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);
- if (!uc_pdata)
return -EINVAL;- len = strcspn(fw_name, "\n");
- if (!len) {
debug("invalid firmware name\n");return -EINVAL;- }
- p = strndup(fw_name, len);
- if (!p)
return -ENOMEM;Aren't we leaking memory if rproc_set_firmware() is called multiple times? Can we check if uc_pdata->fw_name exists and free it before the strndup?
Sure, How does the below diff looks to you?
diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c index 784361cef9..f373b64da4 100644 --- a/drivers/remoteproc/rproc-uclass.c +++ b/drivers/remoteproc/rproc-uclass.c @@ -982,6 +982,9 @@ int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name) return -EINVAL; }
if (uc_pdata->fw_name)free(uc_pdata->fw_name);p = strndup(fw_name, len); if (!p) return -ENOMEM;This is OK. Please see below.
- uc_pdata->fw_name = p;
- return 0;
+}
+int rproc_boot(struct udevice *rproc_dev) +{
- struct dm_rproc_uclass_pdata *uc_pdata;
- struct udevice *fs_loader;
- int core_id, ret = 0;
- char *firmware;
- void *addr;
- if (!rproc_dev)
return -EINVAL;- uc_pdata = dev_get_uclass_plat(rproc_dev);
- if (!uc_pdata)
return -EINVAL;- core_id = dev_seq(rproc_dev);
- firmware = uc_pdata->fw_name;
unnecessary blank line.
- if (!firmware) {
debug("No firmware set for rproc core %d\n", core_id);No firmware name...
return -EINVAL;- }
- /* Initialize all rproc cores */
- if (!rproc_is_initialized()) {
ret = rproc_init();if (ret) {debug("rproc_init() failed: %d\n", ret);return ret;}- }
- /* 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;- }
- if (CONFIG_REMOTEPROC_MAX_FW_SIZE) {
if (defined(CONFIG_REMOTEPROC_MAX_FW_SIZE)) {
sure.
This I'm not so sure. I think 'defined' can only be used with the pre-processor e.g. #if defined() But such usage is no longer encouraged.
My first approach was this as well. But this threw the checkpatch warning so I changed it to if (IS_ENABLED(CONFIG...)). But that was always returning 0 for hex value. As a result I kept the if condition as,
if (defined(CONFIG_REMOTEPROC_MAX_FW_SIZE))
e.g. Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
And I'm not sure if IS_ENABLED works for hex options.
No IS_ENABLED doesn't work for hex options. It only works for boolean. For hex value it will always return 0.
/* * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', * 0 otherwise. */ #define IS_ENABLED(option) config_enabled(option, 0)
If it does not then just keep it the way it was in this patch.
Sure. I will leave it as it is then.
addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE);if (!addr)return -ENOMEM;- } else {
debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n");return -EINVAL;- }
- ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE,
0);- if (ret < 0) {
debug("could not request %s: %d\n", firmware, ret);goto free_buffer;- }
- ret = rproc_load(core_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, (ulong)addr, ret);goto free_buffer;- }
- ret = rproc_start(core_id);
- if (ret)
debug("failed to start rproc core %d\n", core_id);+free_buffer:
- free(addr);
- return ret;
+} diff --git a/include/remoteproc.h b/include/remoteproc.h index 91a88791a4..6f8068e149 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,34 @@ 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 name
- @rproc_dev: device for which new firmware name is being assigned
- @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
- 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);Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible #else static inline int rproc_init(void) { return -ENOSYS; } static inline int rproc_dev_init(int id) { return -ENOSYS; } @@ -744,6 +774,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) +{ return -ENOSYS; } #endif
#endif /* _RPROC_H_ */
participants (3)
-
MD Danish Anwar -
Ravi Gunasekaran -
Roger Quadros