[v4] 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 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(+)

On 1/30/2024 12:03 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 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);
> +
> +	len = strcspn(fw_name, "\n");
> +	if (!len) {
> +		debug("can't provide empty string for 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, size_t fw_size)
> +{
> +	struct dm_rproc_uclass_pdata *uc_pdata;
> +	struct udevice *fs_loader;
> +	void *addr = malloc(fw_size);
> +	int core_id, ret = 0;
> +	char *firmware;
> +	ulong rproc_addr;
> +
> +	if (!rproc_dev)
> +		return -EINVAL;
> +
> +	if (!addr)
> +		return -ENOMEM;
> +

    addr is not freed.

> +	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();
> +
> +	/* 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);
> +	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;
> +}
> 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
> + * @rproc_dev: device for wich new firmware 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
> + * @fw_size: Size of the memory to allocate for firmeware
> + *
> + * 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);
>  #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_ */

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.

> +
> +	len = strcspn(fw_name, "\n");
> +	if (!len) {
> +		debug("can't provide empty string for firmware name\n");

how about "invalid filename" ?

> +		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.

> +	int core_id, ret = 0;
> +	char *firmware;
> +	ulong rproc_addr;

do you really need rproc_addr? You could use addr itself.

> +
> +	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;
		}
	}

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

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.

> + *
> + * 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()?

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

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.c#L324

On 02/02/2024 18:40, Anwar, Md Danish wrote:
> 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.

Caller only knows the filename. It need not know more details.
Also see my comment below about rproc_boot() API.

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

Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
take fw_size argument. So wondering why you should have it in u-boot.

> 
>>>  #else
>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>  					   ulong fw_size, ulong *rsc_addr,
>>>  					   ulong *rsc_size)
>>>  { return -ENOSYS; }
>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>> +{ return -ENOSYS; }
>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>> +{ return -ENOSYS; }
>>>  #endif
>>>  
>>>  #endif	/* _RPROC_H_ */
>>
> 
> [1]
> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>

On 05/02/24 3:36 pm, Roger Quadros wrote:
> 
> 
> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>> 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.
> 
> Caller only knows the filename. It need not know more details.

Caller is trying to load a file of it's choice to a rproc. Caller should
know the size of file it is trying to load or atleast the max size that
the firmware file could be of.


> Also see my comment below about rproc_boot() API.
> 
>>
>> 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.
> 
> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
> take fw_size argument. So wondering why you should have it in u-boot.
> 

For loading firmware to a rproc core in u-boot, it's first neccassry to
load the firmware into buffer and then load that buffer into rproc core
using rproc_load() API. Now to load the firmware to a buffer ther is an
API request_firmware_into_buf(). This API takes size of firmware as one
of it's argument. So in order to call this API from rproc_boot() we need
to pass fw_size to rproc_boot()

Other u-boot drivers using request_firmware_into_buf() are also passing
size of firmware from their driver.

If rproc_boot() doesn't take fw_size as argument then within
rproc_boot() we need to figure out the fw_size before calling
request_firmware_into_buf().

If we don't know the size / maximum size of the firmware to load, how
will we call request_firmware_into_buf(). Someone has to tell
request_firmware_into_buf() the size of firmware. I am expecting that to
be the caller. Do you have any other way of getting the firmware size
before request_firmware_into_buf() is called?

>>
>>>>  #else
>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>  					   ulong *rsc_size)
>>>>  { return -ENOSYS; }
>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>> +{ return -ENOSYS; }
>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>> +{ return -ENOSYS; }
>>>>  #endif
>>>>  
>>>>  #endif	/* _RPROC_H_ */
>>>
>>
>> [1]
>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>
>

On 05/02/2024 12:20, MD Danish Anwar wrote:
> 
> 
> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>
>>
>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>> 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.
>>
>> Caller only knows the filename. It need not know more details.
> 
> Caller is trying to load a file of it's choice to a rproc. Caller should
> know the size of file it is trying to load or atleast the max size that
> the firmware file could be of.
> 
> 
>> Also see my comment below about rproc_boot() API.
>>
>>>
>>> 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.
>>
>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>> take fw_size argument. So wondering why you should have it in u-boot.
>>
> 
> For loading firmware to a rproc core in u-boot, it's first neccassry to
> load the firmware into buffer and then load that buffer into rproc core
> using rproc_load() API. Now to load the firmware to a buffer ther is an
> API request_firmware_into_buf(). This API takes size of firmware as one
> of it's argument. So in order to call this API from rproc_boot() we need
> to pass fw_size to rproc_boot()
> 
> Other u-boot drivers using request_firmware_into_buf() are also passing
> size of firmware from their driver.

But in your driver you didn't use size of firmware but some 64K
https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/

So neither does the caller have a clue of firmware size?

> 
> If rproc_boot() doesn't take fw_size as argument then within
> rproc_boot() we need to figure out the fw_size before calling
> request_firmware_into_buf().
> 
> If we don't know the size / maximum size of the firmware to load, how
> will we call request_firmware_into_buf(). Someone has to tell
> request_firmware_into_buf() the size of firmware. I am expecting that to
> be the caller. Do you have any other way of getting the firmware size
> before request_firmware_into_buf() is called?

/**
 * request_firmware_into_buf - Load firmware into a previously allocated buffer.
 * @dev: An instance of a driver.
 * @name: Name of firmware file.
 * @buf: Address of buffer to load firmware into.
 * @size: Size of buffer.
 * @offset: Offset of a file for start reading into buffer.

It needs size of pre-allocated buffer which can be smaller than file size.
It also has the option of offset. So you can load portions of the file limited
by buffer size.

My suggestion is that Remoteproc layer should take care of how much buffer
to allocate and pass that buffer size to request_firmware_into_buf().
You are doing the malloc here itself anyways.

> 
>>>
>>>>>  #else
>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>  					   ulong *rsc_size)
>>>>>  { return -ENOSYS; }
>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>> +{ return -ENOSYS; }
>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>> +{ return -ENOSYS; }
>>>>>  #endif
>>>>>  
>>>>>  #endif	/* _RPROC_H_ */
>>>>
>>>
>>> [1]
>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>
>>
>

On 05/02/24 6:07 pm, Roger Quadros wrote:
> 
> 
> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>
>>
>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>
>>>
>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>> 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.
>>>>>>

<snip>

>>>>
>>>>> 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.
>>>
>>> Caller only knows the filename. It need not know more details.
>>
>> Caller is trying to load a file of it's choice to a rproc. Caller should
>> know the size of file it is trying to load or atleast the max size that
>> the firmware file could be of.
>>
>>
>>> Also see my comment below about rproc_boot() API.
>>>
>>>>
>>>> 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.
>>>
>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>
>>
>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>> load the firmware into buffer and then load that buffer into rproc core
>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>> API request_firmware_into_buf(). This API takes size of firmware as one
>> of it's argument. So in order to call this API from rproc_boot() we need
>> to pass fw_size to rproc_boot()
>>
>> Other u-boot drivers using request_firmware_into_buf() are also passing
>> size of firmware from their driver.
> 
> But in your driver you didn't use size of firmware but some 64K
> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
> 

Yes, in driver I am hardcoding the size to 64K. That's because I know
the size of ICSSG firmwares are less than 64K. Instead of hardcoding I
can also define macro or provide a config option where we set the size
and the driver will read the size from the config and call rproc_boot()
with size.

For example, fm.c driver reads the size from config option
CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()

[1]
https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458

> So neither does the caller have a clue of firmware size?
> 
>>
>> If rproc_boot() doesn't take fw_size as argument then within
>> rproc_boot() we need to figure out the fw_size before calling
>> request_firmware_into_buf().
>>
>> If we don't know the size / maximum size of the firmware to load, how
>> will we call request_firmware_into_buf(). Someone has to tell
>> request_firmware_into_buf() the size of firmware. I am expecting that to
>> be the caller. Do you have any other way of getting the firmware size
>> before request_firmware_into_buf() is called?
> 
> /**
>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>  * @dev: An instance of a driver.
>  * @name: Name of firmware file.
>  * @buf: Address of buffer to load firmware into.
>  * @size: Size of buffer.
>  * @offset: Offset of a file for start reading into buffer.
> 
> It needs size of pre-allocated buffer which can be smaller than file size.
> It also has the option of offset. So you can load portions of the file limited
> by buffer size.
> 
> My suggestion is that Remoteproc layer should take care of how much buffer
> to allocate and pass that buffer size to request_firmware_into_buf().
> You are doing the malloc here itself anyways.
> 

But how would the remoteproc driver know how much buffer it needs to
allocate before calling request_firmware_into_buf().

>>
>>>>
>>>>>>  #else
>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>  					   ulong *rsc_size)
>>>>>>  { return -ENOSYS; }
>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>> +{ return -ENOSYS; }
>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>> +{ return -ENOSYS; }
>>>>>>  #endif
>>>>>>  
>>>>>>  #endif	/* _RPROC_H_ */
>>>>>
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>
>>>
>>
>

On 06/02/2024 07:31, MD Danish Anwar wrote:
> 
> 
> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>
>>
>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>
>>>
>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>> 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.
>>>>>>>
> 
> <snip>
> 
>>>>>
>>>>>> 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.
>>>>
>>>> Caller only knows the filename. It need not know more details.
>>>
>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>> know the size of file it is trying to load or atleast the max size that
>>> the firmware file could be of.
>>>
>>>
>>>> Also see my comment below about rproc_boot() API.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>
>>>
>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>> load the firmware into buffer and then load that buffer into rproc core
>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>> of it's argument. So in order to call this API from rproc_boot() we need
>>> to pass fw_size to rproc_boot()
>>>
>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>> size of firmware from their driver.
>>
>> But in your driver you didn't use size of firmware but some 64K
>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>
> 
> Yes, in driver I am hardcoding the size to 64K. That's because I know
> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I

What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
It is not a good idea to assume any firmware file size as it will eventually
break sometime in the future and will be a pain to debug.

> can also define macro or provide a config option where we set the size
> and the driver will read the size from the config and call rproc_boot()
> with size.
> 
> For example, fm.c driver reads the size from config option
> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
> 
> [1]
> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
> 
>> So neither does the caller have a clue of firmware size?
>>
>>>
>>> If rproc_boot() doesn't take fw_size as argument then within
>>> rproc_boot() we need to figure out the fw_size before calling
>>> request_firmware_into_buf().
>>>
>>> If we don't know the size / maximum size of the firmware to load, how
>>> will we call request_firmware_into_buf(). Someone has to tell
>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>> be the caller. Do you have any other way of getting the firmware size
>>> before request_firmware_into_buf() is called?
>>
>> /**
>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>  * @dev: An instance of a driver.
>>  * @name: Name of firmware file.
>>  * @buf: Address of buffer to load firmware into.
>>  * @size: Size of buffer.
>>  * @offset: Offset of a file for start reading into buffer.
>>
>> It needs size of pre-allocated buffer which can be smaller than file size.
>> It also has the option of offset. So you can load portions of the file limited
>> by buffer size.
>>
>> My suggestion is that Remoteproc layer should take care of how much buffer
>> to allocate and pass that buffer size to request_firmware_into_buf().
>> You are doing the malloc here itself anyways.
>>
> 
> But how would the remoteproc driver know how much buffer it needs to
> allocate before calling request_firmware_into_buf().

Only the filesystem driver knows what exactly is the firmware file size.
fs_size() API can be used for that.

> 
>>>
>>>>>
>>>>>>>  #else
>>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>>  					   ulong *rsc_size)
>>>>>>>  { return -ENOSYS; }
>>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>>> +{ return -ENOSYS; }
>>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>>> +{ return -ENOSYS; }
>>>>>>>  #endif
>>>>>>>  
>>>>>>>  #endif	/* _RPROC_H_ */
>>>>>>
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>
>>>>
>>>
>>
>

Hi Roger

On 06/02/24 7:11 pm, Roger Quadros wrote:
> 
> 
> On 06/02/2024 07:31, MD Danish Anwar wrote:
>>
>>
>> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>>
>>>
>>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>>
>>>>
>>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>>> 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.
>>>>>>>>
>>
>> <snip>
>>
>>>>>>
>>>>>>> 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.
>>>>>
>>>>> Caller only knows the filename. It need not know more details.
>>>>
>>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>>> know the size of file it is trying to load or atleast the max size that
>>>> the firmware file could be of.
>>>>
>>>>
>>>>> Also see my comment below about rproc_boot() API.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>>
>>>>
>>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>>> load the firmware into buffer and then load that buffer into rproc core
>>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>>> of it's argument. So in order to call this API from rproc_boot() we need
>>>> to pass fw_size to rproc_boot()
>>>>
>>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>>> size of firmware from their driver.
>>>
>>> But in your driver you didn't use size of firmware but some 64K
>>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>>
>>
>> Yes, in driver I am hardcoding the size to 64K. That's because I know
>> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I
> 
> What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
> It is not a good idea to assume any firmware file size as it will eventually
> break sometime in the future and will be a pain to debug.
> 
>> can also define macro or provide a config option where we set the size
>> and the driver will read the size from the config and call rproc_boot()
>> with size.
>>
>> For example, fm.c driver reads the size from config option
>> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
>>
>> [1]
>> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
>>
>>> So neither does the caller have a clue of firmware size?
>>>
>>>>
>>>> If rproc_boot() doesn't take fw_size as argument then within
>>>> rproc_boot() we need to figure out the fw_size before calling
>>>> request_firmware_into_buf().
>>>>
>>>> If we don't know the size / maximum size of the firmware to load, how
>>>> will we call request_firmware_into_buf(). Someone has to tell
>>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>>> be the caller. Do you have any other way of getting the firmware size
>>>> before request_firmware_into_buf() is called?
>>>
>>> /**
>>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>>  * @dev: An instance of a driver.
>>>  * @name: Name of firmware file.
>>>  * @buf: Address of buffer to load firmware into.
>>>  * @size: Size of buffer.
>>>  * @offset: Offset of a file for start reading into buffer.
>>>
>>> It needs size of pre-allocated buffer which can be smaller than file size.
>>> It also has the option of offset. So you can load portions of the file limited
>>> by buffer size.
>>>
>>> My suggestion is that Remoteproc layer should take care of how much buffer
>>> to allocate and pass that buffer size to request_firmware_into_buf().
>>> You are doing the malloc here itself anyways.
>>>
>>
>> But how would the remoteproc driver know how much buffer it needs to
>> allocate before calling request_firmware_into_buf().
> 
> Only the filesystem driver knows what exactly is the firmware file size.
> fs_size() API can be used for that.
> 

To use fs_size() we first need to call fs_set_blk_dev() to set the
storage interface, device partition and fs_type. eg.
fs_set_blk_dev("mmc, "1:2", FS_TYPE_ANY)

Since we are setting the envs for storage_interface and partition I'll
use the envs to call fs_set_blk_dev()

This is how rproc_boot() will look now.

int rproc_boot(struct udevice *rproc_dev)
{
	struct dm_rproc_uclass_pdata *uc_pdata;
	char *storage_interface, *dev_part;
	struct udevice *fs_loader;
	int core_id, ret = 0;
	char *firmware;
	loff_t fw_size;
	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;
	}

	storage_interface = env_get("fw_storage_interface");
	dev_part = env_get("fw_dev_part");

	if (storage_interface && dev_part) {
		ret = fs_set_blk_dev(storage_interface, dev_part, 	FS_TYPE_ANY);
	} else {
		debug("could not get env variables to load firmware\n");
		return -EINVAL;
	}

	if (ret) {
		debug("fs_set_blk_dev failed %d\n", ret);
		return ret;
	}

	ret = fs_size(firmware, &fw_size);
	if (ret) {
		debug("could not get firmware size %s: %d\n", firmware, ret);
		return ret;
	}

	addr = malloc(fw_size);
	if (!addr)
		return -ENOMEM;

	ret = request_firmware_into_buf(fs_loader, firmware, addr, 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;
}

Please let me know if this looks ok. Without calling fs_set_blk_dev()
first, fs_size() results in error.


>>
>>>>
>>>>>>
>>>>>>>>  #else
>>>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>>>  					   ulong *rsc_size)
>>>>>>>>  { return -ENOSYS; }
>>>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>>>> +{ return -ENOSYS; }
>>>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>  #endif
>>>>>>>>  
>>>>>>>>  #endif	/* _RPROC_H_ */
>>>>>>>
>>>>>>
>>>>>> [1]
>>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>>
>>>>>
>>>>
>>>
>>
>

On 07/02/2024 09:15, MD Danish Anwar wrote:
> Hi Roger
> 
> On 06/02/24 7:11 pm, Roger Quadros wrote:
>>
>>
>> On 06/02/2024 07:31, MD Danish Anwar wrote:
>>>
>>>
>>> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>>>
>>>>>
>>>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>>>> 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.
>>>>>>>>>
>>>
>>> <snip>
>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Caller only knows the filename. It need not know more details.
>>>>>
>>>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>>>> know the size of file it is trying to load or atleast the max size that
>>>>> the firmware file could be of.
>>>>>
>>>>>
>>>>>> Also see my comment below about rproc_boot() API.
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>>>
>>>>>
>>>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>>>> load the firmware into buffer and then load that buffer into rproc core
>>>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>>>> of it's argument. So in order to call this API from rproc_boot() we need
>>>>> to pass fw_size to rproc_boot()
>>>>>
>>>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>>>> size of firmware from their driver.
>>>>
>>>> But in your driver you didn't use size of firmware but some 64K
>>>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>>>
>>>
>>> Yes, in driver I am hardcoding the size to 64K. That's because I know
>>> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I
>>
>> What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
>> It is not a good idea to assume any firmware file size as it will eventually
>> break sometime in the future and will be a pain to debug.
>>
>>> can also define macro or provide a config option where we set the size
>>> and the driver will read the size from the config and call rproc_boot()
>>> with size.
>>>
>>> For example, fm.c driver reads the size from config option
>>> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
>>>
>>> [1]
>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
>>>
>>>> So neither does the caller have a clue of firmware size?
>>>>
>>>>>
>>>>> If rproc_boot() doesn't take fw_size as argument then within
>>>>> rproc_boot() we need to figure out the fw_size before calling
>>>>> request_firmware_into_buf().
>>>>>
>>>>> If we don't know the size / maximum size of the firmware to load, how
>>>>> will we call request_firmware_into_buf(). Someone has to tell
>>>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>>>> be the caller. Do you have any other way of getting the firmware size
>>>>> before request_firmware_into_buf() is called?
>>>>
>>>> /**
>>>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>>>  * @dev: An instance of a driver.
>>>>  * @name: Name of firmware file.
>>>>  * @buf: Address of buffer to load firmware into.
>>>>  * @size: Size of buffer.
>>>>  * @offset: Offset of a file for start reading into buffer.
>>>>
>>>> It needs size of pre-allocated buffer which can be smaller than file size.
>>>> It also has the option of offset. So you can load portions of the file limited
>>>> by buffer size.
>>>>
>>>> My suggestion is that Remoteproc layer should take care of how much buffer
>>>> to allocate and pass that buffer size to request_firmware_into_buf().
>>>> You are doing the malloc here itself anyways.
>>>>
>>>
>>> But how would the remoteproc driver know how much buffer it needs to
>>> allocate before calling request_firmware_into_buf().
>>
>> Only the filesystem driver knows what exactly is the firmware file size.
>> fs_size() API can be used for that.
>>
> 
> To use fs_size() we first need to call fs_set_blk_dev() to set the
> storage interface, device partition and fs_type. eg.
> fs_set_blk_dev("mmc, "1:2", FS_TYPE_ANY)
> 
> Since we are setting the envs for storage_interface and partition I'll
> use the envs to call fs_set_blk_dev()
> 
> This is how rproc_boot() will look now.
> 
> int rproc_boot(struct udevice *rproc_dev)
> {
> 	struct dm_rproc_uclass_pdata *uc_pdata;
> 	char *storage_interface, *dev_part;
> 	struct udevice *fs_loader;
> 	int core_id, ret = 0;
> 	char *firmware;
> 	loff_t fw_size;
> 	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;
> 	}
> 
> 	storage_interface = env_get("fw_storage_interface");
> 	dev_part = env_get("fw_dev_part");
> 
> 	if (storage_interface && dev_part) {
> 		ret = fs_set_blk_dev(storage_interface, dev_part, 	FS_TYPE_ANY);
> 	} else {
> 		debug("could not get env variables to load firmware\n");
> 		return -EINVAL;
> 	}

I'm not very sure about this as we are using firmware loader
specific environment variables outside the firmware loader driver.

I can see 2 solutions here:

1) ask firmware loader driver to tell us the firmware file size.
fw_get_filesystem_firmware() also deals with UBI filesystem.

2) use a large enough buffer whose size is set in Kconfig.

Tom, any insights?

> 
> 	if (ret) {
> 		debug("fs_set_blk_dev failed %d\n", ret);
> 		return ret;
> 	}
> 
> 	ret = fs_size(firmware, &fw_size);
> 	if (ret) {
> 		debug("could not get firmware size %s: %d\n", firmware, ret);
> 		return ret;
> 	}
> 
> 	addr = malloc(fw_size);
> 	if (!addr)
> 		return -ENOMEM;
> 
> 	ret = request_firmware_into_buf(fs_loader, firmware, addr, 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;
> }
> 
> Please let me know if this looks ok. Without calling fs_set_blk_dev()
> first, fs_size() results in error.
> 
> 
>>>
>>>>>
>>>>>>>
>>>>>>>>>  #else
>>>>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>>>>  					   ulong *rsc_size)
>>>>>>>>>  { return -ENOSYS; }
>>>>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>>  #endif
>>>>>>>>>  
>>>>>>>>>  #endif	/* _RPROC_H_ */
>>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

On 2/7/2024 6:05 PM, Roger Quadros wrote:
> 
> 
> On 07/02/2024 09:15, MD Danish Anwar wrote:
>> Hi Roger
>>
>> On 06/02/24 7:11 pm, Roger Quadros wrote:
>>>
>>>
>>> On 06/02/2024 07:31, MD Danish Anwar wrote:
>>>>
>>>>
>>>> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>>>>
>>>>>>
>>>>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>>>>> 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.
>>>>>>>>>>
>>>>
>>>> <snip>
>>>>
>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> Caller only knows the filename. It need not know more details.
>>>>>>
>>>>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>>>>> know the size of file it is trying to load or atleast the max size that
>>>>>> the firmware file could be of.
>>>>>>
>>>>>>
>>>>>>> Also see my comment below about rproc_boot() API.
>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>>>>
>>>>>>
>>>>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>>>>> load the firmware into buffer and then load that buffer into rproc core
>>>>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>>>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>>>>> of it's argument. So in order to call this API from rproc_boot() we need
>>>>>> to pass fw_size to rproc_boot()
>>>>>>
>>>>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>>>>> size of firmware from their driver.
>>>>>
>>>>> But in your driver you didn't use size of firmware but some 64K
>>>>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>>>>
>>>>
>>>> Yes, in driver I am hardcoding the size to 64K. That's because I know
>>>> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I
>>>
>>> What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
>>> It is not a good idea to assume any firmware file size as it will eventually
>>> break sometime in the future and will be a pain to debug.
>>>
>>>> can also define macro or provide a config option where we set the size
>>>> and the driver will read the size from the config and call rproc_boot()
>>>> with size.
>>>>
>>>> For example, fm.c driver reads the size from config option
>>>> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
>>>>
>>>>> So neither does the caller have a clue of firmware size?
>>>>>
>>>>>>
>>>>>> If rproc_boot() doesn't take fw_size as argument then within
>>>>>> rproc_boot() we need to figure out the fw_size before calling
>>>>>> request_firmware_into_buf().
>>>>>>
>>>>>> If we don't know the size / maximum size of the firmware to load, how
>>>>>> will we call request_firmware_into_buf(). Someone has to tell
>>>>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>>>>> be the caller. Do you have any other way of getting the firmware size
>>>>>> before request_firmware_into_buf() is called?
>>>>>
>>>>> /**
>>>>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>>>>  * @dev: An instance of a driver.
>>>>>  * @name: Name of firmware file.
>>>>>  * @buf: Address of buffer to load firmware into.
>>>>>  * @size: Size of buffer.
>>>>>  * @offset: Offset of a file for start reading into buffer.
>>>>>
>>>>> It needs size of pre-allocated buffer which can be smaller than file size.
>>>>> It also has the option of offset. So you can load portions of the file limited
>>>>> by buffer size.
>>>>>
>>>>> My suggestion is that Remoteproc layer should take care of how much buffer
>>>>> to allocate and pass that buffer size to request_firmware_into_buf().
>>>>> You are doing the malloc here itself anyways.
>>>>>
>>>>
>>>> But how would the remoteproc driver know how much buffer it needs to
>>>> allocate before calling request_firmware_into_buf().
>>>
>>> Only the filesystem driver knows what exactly is the firmware file size.
>>> fs_size() API can be used for that.
>>>
>>
>> To use fs_size() we first need to call fs_set_blk_dev() to set the
>> storage interface, device partition and fs_type. eg.
>> fs_set_blk_dev("mmc, "1:2", FS_TYPE_ANY)
>>
>> Since we are setting the envs for storage_interface and partition I'll
>> use the envs to call fs_set_blk_dev()
>>
>> This is how rproc_boot() will look now.
>>
>> int rproc_boot(struct udevice *rproc_dev)
>> {
>> 	struct dm_rproc_uclass_pdata *uc_pdata;
>> 	char *storage_interface, *dev_part;
>> 	struct udevice *fs_loader;
>> 	int core_id, ret = 0;
>> 	char *firmware;
>> 	loff_t fw_size;
>> 	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;
>> 	}
>>
>> 	storage_interface = env_get("fw_storage_interface");
>> 	dev_part = env_get("fw_dev_part");
>>
>> 	if (storage_interface && dev_part) {
>> 		ret = fs_set_blk_dev(storage_interface, dev_part, 	FS_TYPE_ANY);
>> 	} else {
>> 		debug("could not get env variables to load firmware\n");
>> 		return -EINVAL;
>> 	}
> 
> I'm not very sure about this as we are using firmware loader
> specific environment variables outside the firmware loader driver.
> 
> I can see 2 solutions here:
> 
> 1) ask firmware loader driver to tell us the firmware file size.
> fw_get_filesystem_firmware() also deals with UBI filesystem.
> 
> 2) use a large enough buffer whose size is set in Kconfig.
> 

Roger, I still think 2 is a better option. In ICSSG driver also I am
setting the fw_size to 64K. Instead of hard-coding, this can be set in
the config. The ICSSG firmware is usually 40KB so 64K should be OK.
Other clients, if they use rproc_boot() in future, can specify there max
firmware size in their config. And to rproc layer it will be abstract.

We can also get the size from config (Setting somethinig like
CONFIG_RPROC_MAX_FW_SIZE=64K) directly inside rproc_boot() eliminating
the need for the client to pass the size. But in future when there are
many clients using rproc_boot() for different firmware files, It will
become difficult for remoteproc to hardcode a firmware size that can be
bigger than any firmware that any client is using.

It's simpler if each client specify there max fw size instead of
remoteproc driver specifying max possible size.

> Tom, any insights?
> 
>>
>> 	if (ret) {
>> 		debug("fs_set_blk_dev failed %d\n", ret);
>> 		return ret;
>> 	}
>>
>> 	ret = fs_size(firmware, &fw_size);
>> 	if (ret) {
>> 		debug("could not get firmware size %s: %d\n", firmware, ret);
>> 		return ret;
>> 	}
>>
>> 	addr = malloc(fw_size);
>> 	if (!addr)
>> 		return -ENOMEM;
>>
>> 	ret = request_firmware_into_buf(fs_loader, firmware, addr, 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;
>> }
>>
>> Please let me know if this looks ok. Without calling fs_set_blk_dev()
>> first, fs_size() results in error.
>>
>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>  #else
>>>>>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>>>>>  					   ulong *rsc_size)
>>>>>>>>>>  { return -ENOSYS; }
>>>>>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>>>  #endif
>>>>>>>>>>  
>>>>>>>>>>  #endif	/* _RPROC_H_ */
>>>>>>>>>
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

On 07/02/24 6:27 pm, Anwar, Md Danish wrote:
> On 2/7/2024 6:05 PM, Roger Quadros wrote:
>>
>>
>> On 07/02/2024 09:15, MD Danish Anwar wrote:
>>> Hi Roger
>>>
>>> On 06/02/24 7:11 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 06/02/2024 07:31, MD Danish Anwar wrote:
>>>>>
>>>>>
>>>>> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Caller only knows the filename. It need not know more details.
>>>>>>>
>>>>>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>>>>>> know the size of file it is trying to load or atleast the max size that
>>>>>>> the firmware file could be of.
>>>>>>>
>>>>>>>
>>>>>>>> Also see my comment below about rproc_boot() API.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>>>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>>>>>
>>>>>>>
>>>>>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>>>>>> load the firmware into buffer and then load that buffer into rproc core
>>>>>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>>>>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>>>>>> of it's argument. So in order to call this API from rproc_boot() we need
>>>>>>> to pass fw_size to rproc_boot()
>>>>>>>
>>>>>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>>>>>> size of firmware from their driver.
>>>>>>
>>>>>> But in your driver you didn't use size of firmware but some 64K
>>>>>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>>>>>
>>>>>
>>>>> Yes, in driver I am hardcoding the size to 64K. That's because I know
>>>>> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I
>>>>
>>>> What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
>>>> It is not a good idea to assume any firmware file size as it will eventually
>>>> break sometime in the future and will be a pain to debug.
>>>>
>>>>> can also define macro or provide a config option where we set the size
>>>>> and the driver will read the size from the config and call rproc_boot()
>>>>> with size.
>>>>>
>>>>> For example, fm.c driver reads the size from config option
>>>>> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
>>>>>
>>>>>> So neither does the caller have a clue of firmware size?
>>>>>>
>>>>>>>
>>>>>>> If rproc_boot() doesn't take fw_size as argument then within
>>>>>>> rproc_boot() we need to figure out the fw_size before calling
>>>>>>> request_firmware_into_buf().
>>>>>>>
>>>>>>> If we don't know the size / maximum size of the firmware to load, how
>>>>>>> will we call request_firmware_into_buf(). Someone has to tell
>>>>>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>>>>>> be the caller. Do you have any other way of getting the firmware size
>>>>>>> before request_firmware_into_buf() is called?
>>>>>>
>>>>>> /**
>>>>>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>>>>>  * @dev: An instance of a driver.
>>>>>>  * @name: Name of firmware file.
>>>>>>  * @buf: Address of buffer to load firmware into.
>>>>>>  * @size: Size of buffer.
>>>>>>  * @offset: Offset of a file for start reading into buffer.
>>>>>>
>>>>>> It needs size of pre-allocated buffer which can be smaller than file size.
>>>>>> It also has the option of offset. So you can load portions of the file limited
>>>>>> by buffer size.
>>>>>>
>>>>>> My suggestion is that Remoteproc layer should take care of how much buffer
>>>>>> to allocate and pass that buffer size to request_firmware_into_buf().
>>>>>> You are doing the malloc here itself anyways.
>>>>>>
>>>>>
>>>>> But how would the remoteproc driver know how much buffer it needs to
>>>>> allocate before calling request_firmware_into_buf().
>>>>
>>>> Only the filesystem driver knows what exactly is the firmware file size.
>>>> fs_size() API can be used for that.
>>>>
>>>
>>> To use fs_size() we first need to call fs_set_blk_dev() to set the
>>> storage interface, device partition and fs_type. eg.
>>> fs_set_blk_dev("mmc, "1:2", FS_TYPE_ANY)
>>>
>>> Since we are setting the envs for storage_interface and partition I'll
>>> use the envs to call fs_set_blk_dev()
>>>
>>> This is how rproc_boot() will look now.
>>>
>>> int rproc_boot(struct udevice *rproc_dev)
>>> {
>>> 	struct dm_rproc_uclass_pdata *uc_pdata;
>>> 	char *storage_interface, *dev_part;
>>> 	struct udevice *fs_loader;
>>> 	int core_id, ret = 0;
>>> 	char *firmware;
>>> 	loff_t fw_size;
>>> 	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;
>>> 	}
>>>
>>> 	storage_interface = env_get("fw_storage_interface");
>>> 	dev_part = env_get("fw_dev_part");
>>>
>>> 	if (storage_interface && dev_part) {
>>> 		ret = fs_set_blk_dev(storage_interface, dev_part, 	FS_TYPE_ANY);
>>> 	} else {
>>> 		debug("could not get env variables to load firmware\n");
>>> 		return -EINVAL;
>>> 	}
>>
>> I'm not very sure about this as we are using firmware loader
>> specific environment variables outside the firmware loader driver.
>>
>> I can see 2 solutions here:
>>
>> 1) ask firmware loader driver to tell us the firmware file size.
>> fw_get_filesystem_firmware() also deals with UBI filesystem.
>>
>> 2) use a large enough buffer whose size is set in Kconfig.
>>
> 
> Roger, I still think 2 is a better option. In ICSSG driver also I am
> setting the fw_size to 64K. Instead of hard-coding, this can be set in
> the config. The ICSSG firmware is usually 40KB so 64K should be OK.
> Other clients, if they use rproc_boot() in future, can specify there max
> firmware size in their config. And to rproc layer it will be abstract.
> 
> We can also get the size from config (Setting somethinig like
> CONFIG_RPROC_MAX_FW_SIZE=64K) directly inside rproc_boot() eliminating
> the need for the client to pass the size. But in future when there are
> many clients using rproc_boot() for different firmware files, It will
> become difficult for remoteproc to hardcode a firmware size that can be
> bigger than any firmware that any client is using.
> 
> It's simpler if each client specify there max fw size instead of
> remoteproc driver specifying max possible size.
> 
>> Tom, any insights?
>>

Hi Tom, any suggestions?

Roger, If Tom doesn't have any comment I will respin the patch by
defining REMOTEPROC_MAX_FW_SIZE in drivers/remoteproc/Kconfig. I will
keep the default size as 0x10000 (64K)

>>>
>>> 	if (ret) {
>>> 		debug("fs_set_blk_dev failed %d\n", ret);
>>> 		return ret;
>>> 	}
>>>
>>> 	ret = fs_size(firmware, &fw_size);
>>> 	if (ret) {
>>> 		debug("could not get firmware size %s: %d\n", firmware, ret);
>>> 		return ret;
>>> 	}
>>>
>>> 	addr = malloc(fw_size);
>>> 	if (!addr)
>>> 		return -ENOMEM;
>>>
>>> 	ret = request_firmware_into_buf(fs_loader, firmware, addr, 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;
>>> }
>>>
>>> Please let me know if this looks ok. Without calling fs_set_blk_dev()
>>> first, fs_size() results in error.
>>>
>>>
>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>  #else
>>>>>>>>>>>  static inline int rproc_init(void) { return -ENOSYS; }
>>>>>>>>>>>  static inline int rproc_dev_init(int id) { return -ENOSYS; }
>>>>>>>>>>> @@ -744,6 +775,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
>>>>>>>>>>>  					   ulong fw_size, ulong *rsc_addr,
>>>>>>>>>>>  					   ulong *rsc_size)
>>>>>>>>>>>  { return -ENOSYS; }
>>>>>>>>>>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>>>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>>>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>>>>>>>>>>> +{ return -ENOSYS; }
>>>>>>>>>>>  #endif
>>>>>>>>>>>  
>>>>>>>>>>>  #endif	/* _RPROC_H_ */
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>