Re: [PATCH v4] remoteproc: uclass: Add methods to load firmware to rproc and boot rproc

[Roger Quadros <rogerq@kernel.org>]

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

-- 
cheers,
-roger