diff mbox series

[v5] remoteproc: uclass: Add methods to load firmware to rproc and boot rproc

Message ID 20240217122602.3402774-1-danishanwar@ti.com
State Superseded
Delegated to: Tom Rini
Headers show
Series [v5] remoteproc: uclass: Add methods to load firmware to rproc and boot rproc | expand

Commit Message

MD Danish Anwar Feb. 17, 2024, 12:26 p.m. UTC
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(+)

Comments

Ravi Gunasekaran Feb. 27, 2024, 11:17 a.m. UTC | #1
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>

[...]
Roger Quadros Feb. 28, 2024, 10 a.m. UTC | #2
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 be
> +	  loaded 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_ */
MD Danish Anwar Feb. 28, 2024, 10:35 a.m. UTC | #3
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 be
>> +	  loaded 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_ */
>
Roger Quadros Feb. 28, 2024, 10:42 a.m. UTC | #4
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 be
>>> +	  loaded 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_ */
>>
>
MD Danish Anwar Feb. 28, 2024, 11:45 a.m. UTC | #5
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 be
>>>> +	  loaded 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_ */
>>>
>>
>
diff mbox series

Patch

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_ */