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

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

[MD Danish Anwar]

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>
Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
---
Changes from v5 to v6:
*) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com>
*) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org>
*) Added if condition to check if uc_pdata->fw_name exists and free it
   before the strndup as suggested by Roger Quadros <rogerq@kernel.org>

Changes from v4 to v5:
*) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size
   that can be loaded to a rproc.
*) Added freeing of address in rproc_boot() as pointed out by Ravi.
*) Allocating the address at a later point in rproc_boot()
*) Rebased on latest u-boot/master [commit 
   9e00b6993f724da9699ef12573307afea8c19284]

Changes from v3 to v4:
*) No functional change. Splitted the patch out of the series as suggested
   by Nishant.
*) Droppped the RFC tag.

v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/
v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/
v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/

 drivers/remoteproc/Kconfig        |   8 +++
 drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++
 include/remoteproc.h              |  34 ++++++++++
 3 files changed, 144 insertions(+)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 781de530af..9f9877931c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -10,6 +10,7 @@ menu "Remote Processor drivers"
 # All users should depend on DM
 config REMOTEPROC
 	bool
+	select FS_LOADER
 	depends on DM
 
 # Please keep the configuration alphabetically sorted.
@@ -102,4 +103,11 @@ config REMOTEPROC_TI_IPU
 	help
 	  Say 'y' here to add support for TI' K3 remoteproc driver.
 
+config REMOTEPROC_MAX_FW_SIZE
+	hex "Maximum size of firmware file that needs to be loaded to the remote processor"
+	default 0x10000
+	help
+	  Maximum size of the firmware file (elf, binary) that needs to be
+	  loaded to the remote processor.
+
 endmenu
diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c
index 28b362c887..f4f22a3851 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,104 @@ unsigned long rproc_parse_resource_table(struct udevice *dev, struct rproc *cfg)
 
 	return 1;
 }
+
+int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
+{
+	struct dm_rproc_uclass_pdata *uc_pdata;
+	int len;
+	char *p;
+
+	if (!rproc_dev || !fw_name)
+		return -EINVAL;
+
+	uc_pdata = dev_get_uclass_plat(rproc_dev);
+	if (!uc_pdata)
+		return -EINVAL;
+
+	len = strcspn(fw_name, "\n");
+	if (!len) {
+		debug("invalid firmware name\n");
+		return -EINVAL;
+	}
+
+	if (uc_pdata->fw_name)
+		free(uc_pdata->fw_name);
+
+	p = strndup(fw_name, len);
+	if (!p)
+		return -ENOMEM;
+
+	uc_pdata->fw_name = p;
+
+	return 0;
+}
+
+int rproc_boot(struct udevice *rproc_dev)
+{
+	struct dm_rproc_uclass_pdata *uc_pdata;
+	struct udevice *fs_loader;
+	int core_id, ret = 0;
+	char *firmware;
+	void *addr;
+
+	if (!rproc_dev)
+		return -EINVAL;
+
+	uc_pdata = dev_get_uclass_plat(rproc_dev);
+	if (!uc_pdata)
+		return -EINVAL;
+
+	core_id = dev_seq(rproc_dev);
+	firmware = uc_pdata->fw_name;
+	if (!firmware) {
+		debug("No firmware name set for rproc core %d\n", core_id);
+		return -EINVAL;
+	}
+
+	/* Initialize all rproc cores */
+	if (!rproc_is_initialized()) {
+		ret = rproc_init();
+		if (ret) {
+			debug("rproc_init() failed: %d\n", ret);
+			return ret;
+		}
+	}
+
+	/* Loading firmware to a given address */
+	ret = get_fs_loader(&fs_loader);
+	if (ret) {
+		debug("could not get fs loader: %d\n", ret);
+		return ret;
+	}
+
+	if (CONFIG_REMOTEPROC_MAX_FW_SIZE) {
+		addr = malloc(CONFIG_REMOTEPROC_MAX_FW_SIZE);
+		if (!addr)
+			return -ENOMEM;
+	} else {
+		debug("CONFIG_REMOTEPROC_MAX_FW_SIZE not defined\n");
+		return -EINVAL;
+	}
+
+	ret = request_firmware_into_buf(fs_loader, firmware, addr, CONFIG_REMOTEPROC_MAX_FW_SIZE,
+					0);
+	if (ret < 0) {
+		debug("could not request %s: %d\n", firmware, ret);
+		goto free_buffer;
+	}
+
+	ret = rproc_load(core_id, (ulong)addr, ret);
+	if (ret) {
+		debug("failed to load %s to rproc core %d from addr 0x%08lX err %d\n",
+		      uc_pdata->fw_name, core_id, (ulong)addr, ret);
+		goto free_buffer;
+	}
+
+	ret = rproc_start(core_id);
+	if (ret)
+		debug("failed to start rproc core %d\n", core_id);
+
+free_buffer:
+	free(addr);
+	return ret;
+}
diff --git a/include/remoteproc.h b/include/remoteproc.h
index 91a88791a4..6f8068e149 100644
--- a/include/remoteproc.h
+++ b/include/remoteproc.h
@@ -403,6 +403,7 @@ enum rproc_mem_type {
  * @name: Platform-specific way of naming the Remote proc
  * @mem_type: one of 'enum rproc_mem_type'
  * @driver_plat_data: driver specific platform data that may be needed.
+ * @fw_name: firmware name
  *
  * This can be accessed with dev_get_uclass_plat() for any UCLASS_REMOTEPROC
  * device.
@@ -412,6 +413,7 @@ struct dm_rproc_uclass_pdata {
 	const char *name;
 	enum rproc_mem_type mem_type;
 	void *driver_plat_data;
+	char *fw_name;
 };
 
 /**
@@ -705,6 +707,34 @@ unsigned long rproc_parse_resource_table(struct udevice *dev,
 struct resource_table *rproc_find_resource_table(struct udevice *dev,
 						 unsigned int addr,
 						 int *tablesz);
+/**
+ * rproc_set_firmware() - assign a new firmware name
+ * @rproc_dev: device for which new firmware name is being assigned
+ * @fw_name: new firmware name to be assigned
+ *
+ * This function allows remoteproc drivers or clients to configure a custom
+ * firmware name. The function does not trigger a remote processor boot,
+ * only sets the firmware name used for a subsequent boot.
+ *
+ * This function sets the fw_name field in uclass pdata of the Remote proc
+ *
+ * Return: 0 on success or a negative value upon failure
+ */
+int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name);
+
+/**
+ * rproc_boot() - boot a remote processor
+ * @rproc_dev: rproc device to boot
+ *
+ * Boot a remote processor (i.e. load its firmware, power it on, ...).
+ *
+ * This function first loads the firmware set in the uclass pdata of Remote
+ * processor to a buffer and then loads firmware to the remote processor
+ * using rproc_load().
+ *
+ * Return: 0 on success, and an appropriate error value otherwise
+ */
+int rproc_boot(struct udevice *rproc_dev);
 #else
 static inline int rproc_init(void) { return -ENOSYS; }
 static inline int rproc_dev_init(int id) { return -ENOSYS; }
@@ -744,6 +774,10 @@ static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
 					   ulong fw_size, ulong *rsc_addr,
 					   ulong *rsc_size)
 { return -ENOSYS; }
+static inline int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
+{ return -ENOSYS; }
+static inline int rproc_boot(struct udevice *rproc_dev)
+{ return -ENOSYS; }
 #endif
 
 #endif	/* _RPROC_H_ */
-- 
2.34.1

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

[Roger Quadros]

On 28/02/2024 14:06, 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>
> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

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

[MD Danish Anwar]

On 29/02/24 3:43 pm, Roger Quadros wrote:
> 
> 
> On 28/02/2024 14:06, 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>
>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> 
> Reviewed-by: Roger Quadros <rogerq@kernel.org>

Hi Tom,

Can you please pick this patch. There are no active comments on this
patch and Roger and Ravi has reviewed / acked the patch.

-- 
Thanks and Regards,
Danish

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

On Tue, Mar 05, 2024 at 02:39:14PM +0530, MD Danish Anwar wrote:
> On 29/02/24 3:43 pm, Roger Quadros wrote:
> > 
> > 
> > On 28/02/2024 14:06, 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>
> >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> > 
> > Reviewed-by: Roger Quadros <rogerq@kernel.org>
> 
> Hi Tom,
> 
> Can you please pick this patch. There are no active comments on this
> patch and Roger and Ravi has reviewed / acked the patch.

Yes, I'm going to give people a little longer to comment / review and
will pull this to -next soon.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>

This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu
am65x_evm_r5_usbmsc in next currently, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Anwar, Md Danish]

On 3/7/2024 6:16 PM, Tom Rini wrote:
> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> 
> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu
> am65x_evm_r5_usbmsc in next currently, thanks.
> 
I will work on fixing this build error and re-spin the patch.

-- 
Thanks and Regards,
Md Danish Anwar

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

[MD Danish Anwar]

On 11/03/24 10:34 am, Anwar, Md Danish wrote:
> 
> 
> On 3/7/2024 6:16 PM, Tom Rini wrote:
>> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
>>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>>
>> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu
>> am65x_evm_r5_usbmsc in next currently, thanks.
>>
> I will work on fixing this build error and re-spin the patch.
> 

Hi Tom, Roger,

This patch adds "request_firmware_into_buf()" in the rproc driver. To
use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in
REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled,
FS_LOADER also gets enabled.

Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c
[2] has a "load_firmware()" API which calls fs-loader APIs and they have
below if condition before calling fs-loader APIs.

if (!IS_ENABLED(CONFIG_FS_LOADER))
		return 0;

Till now, CONFIG_FS_LOADER was not set as a result the load_firmware()
API in above mentioned files, was returning 0.

Now as this patch enables CONFIG_FS_LOADER, as a result the code after
the if check starts getting executed and it tries to look for
get_fs_loader() and other fs-loader APIs but this is done at SPL and at
this time FS_LOADER is not built yet as a result we see below error.
The if checks only checks for CONFIG_FS_LOADER but not for
CONFIG_SPL_FS_LOADER.

  AR      spl/boot/built-in.o
  LD      spl/u-boot-spl
arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function
`load_firmware':
/home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined
reference to `get_fs_loader'
arm-none-linux-gnueabihf-ld.bfd:
/home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined
reference to `request_firmware_into_buf'
make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527:
spl/u-boot-spl] Error 1
make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055:
spl/u-boot-spl] Error 2
make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5'
make: *** [Makefile:177: sub-make] Error 2

This bug has always been there but as CONFIG_FS_LOADER was never
enabled, this build error was never seen as the load_firmware() API will
return 0 without calling fs-loader APIs.

Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and
build error is seen.

My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER)
instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the
appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER)
based on the build stage.

I tested with the below diff and I don't see build errors with
am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs.

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index f411366778..6792ff7467 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char
*name_loadaddr, u32 *loadaddr)
        char *name = NULL;
        int size = 0;

-       if (!IS_ENABLED(CONFIG_FS_LOADER))
+       if (!CONFIG_IS_ENABLED(FS_LOADER))
                return 0;

        *loadaddr = 0;
diff --git a/arch/arm/mach-omap2/boot-common.c
b/arch/arm/mach-omap2/boot-common.c
index 57917da25c..aa0ab13d5f 100644
--- a/arch/arm/mach-omap2/boot-common.c
+++ b/arch/arm/mach-omap2/boot-common.c
@@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr)
        struct udevice *fsdev;
        int size = 0;

-       if (!IS_ENABLED(CONFIG_FS_LOADER))
+       if (!CONFIG_IS_ENABLED(FS_LOADER))
                return 0;

        if (!*loadaddr)


Tom, Roger, Please let me know if this looks ok.
If it's ok, I will post this diff as a separate patch and once that is
merged Tom can merge this patch or I can send a v7 if needed.

[1]
https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L159
[2]
https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-omap2/boot-common.c#L188

-- 
Thanks and Regards,
Danish

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4958 bytes --]

On Tue, Mar 12, 2024 at 02:02:08PM +0530, MD Danish Anwar wrote:
> 
> 
> On 11/03/24 10:34 am, Anwar, Md Danish wrote:
> > 
> > 
> > On 3/7/2024 6:16 PM, Tom Rini wrote:
> >> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
> >>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> >>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> >>
> >> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu
> >> am65x_evm_r5_usbmsc in next currently, thanks.
> >>
> > I will work on fixing this build error and re-spin the patch.
> > 
> 
> Hi Tom, Roger,
> 
> This patch adds "request_firmware_into_buf()" in the rproc driver. To
> use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in
> REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled,
> FS_LOADER also gets enabled.
> 
> Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c
> [2] has a "load_firmware()" API which calls fs-loader APIs and they have
> below if condition before calling fs-loader APIs.
> 
> if (!IS_ENABLED(CONFIG_FS_LOADER))
> 		return 0;
> 
> Till now, CONFIG_FS_LOADER was not set as a result the load_firmware()
> API in above mentioned files, was returning 0.
> 
> Now as this patch enables CONFIG_FS_LOADER, as a result the code after
> the if check starts getting executed and it tries to look for
> get_fs_loader() and other fs-loader APIs but this is done at SPL and at
> this time FS_LOADER is not built yet as a result we see below error.
> The if checks only checks for CONFIG_FS_LOADER but not for
> CONFIG_SPL_FS_LOADER.
> 
>   AR      spl/boot/built-in.o
>   LD      spl/u-boot-spl
> arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function
> `load_firmware':
> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined
> reference to `get_fs_loader'
> arm-none-linux-gnueabihf-ld.bfd:
> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined
> reference to `request_firmware_into_buf'
> make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527:
> spl/u-boot-spl] Error 1
> make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055:
> spl/u-boot-spl] Error 2
> make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5'
> make: *** [Makefile:177: sub-make] Error 2
> 
> This bug has always been there but as CONFIG_FS_LOADER was never
> enabled, this build error was never seen as the load_firmware() API will
> return 0 without calling fs-loader APIs.
> 
> Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and
> build error is seen.
> 
> My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER)
> instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the
> appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER)
> based on the build stage.
> 
> I tested with the below diff and I don't see build errors with
> am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs.
> 
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index f411366778..6792ff7467 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char
> *name_loadaddr, u32 *loadaddr)
>         char *name = NULL;
>         int size = 0;
> 
> -       if (!IS_ENABLED(CONFIG_FS_LOADER))
> +       if (!CONFIG_IS_ENABLED(FS_LOADER))
>                 return 0;
> 
>         *loadaddr = 0;
> diff --git a/arch/arm/mach-omap2/boot-common.c
> b/arch/arm/mach-omap2/boot-common.c
> index 57917da25c..aa0ab13d5f 100644
> --- a/arch/arm/mach-omap2/boot-common.c
> +++ b/arch/arm/mach-omap2/boot-common.c
> @@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr)
>         struct udevice *fsdev;
>         int size = 0;
> 
> -       if (!IS_ENABLED(CONFIG_FS_LOADER))
> +       if (!CONFIG_IS_ENABLED(FS_LOADER))
>                 return 0;
> 
>         if (!*loadaddr)
> 
> 
> Tom, Roger, Please let me know if this looks ok.
> If it's ok, I will post this diff as a separate patch and once that is
> merged Tom can merge this patch or I can send a v7 if needed.

Yes, this seems like the right path, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Anwar, Md Danish]

On 3/14/2024 6:16 PM, Tom Rini wrote:
> On Tue, Mar 12, 2024 at 02:02:08PM +0530, MD Danish Anwar wrote:
>>
>>
>> On 11/03/24 10:34 am, Anwar, Md Danish wrote:
>>>
>>>
>>> On 3/7/2024 6:16 PM, Tom Rini wrote:
>>>> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
>>>>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>>>>
>>>> This breaks building on am64x_evm_r5 am65x_evm_r5_usbdfu
>>>> am65x_evm_r5_usbmsc in next currently, thanks.
>>>>
>>> I will work on fixing this build error and re-spin the patch.
>>>
>>
>> Hi Tom, Roger,
>>
>> This patch adds "request_firmware_into_buf()" in the rproc driver. To
>> use this API, FS_LOADER is needed. So I am adding "select FS_LOADER" in
>> REMOTEPROC Kconfig option. As a result whenever REMOTEPROC is enabled,
>> FS_LOADER also gets enabled.
>>
>> Now arch/arm/mach-k3/common.c [1] and arch/arm/mach-omap2/boot-common.c
>> [2] has a "load_firmware()" API which calls fs-loader APIs and they have
>> below if condition before calling fs-loader APIs.
>>
>> if (!IS_ENABLED(CONFIG_FS_LOADER))
>> 		return 0;
>>
>> Till now, CONFIG_FS_LOADER was not set as a result the load_firmware()
>> API in above mentioned files, was returning 0.
>>
>> Now as this patch enables CONFIG_FS_LOADER, as a result the code after
>> the if check starts getting executed and it tries to look for
>> get_fs_loader() and other fs-loader APIs but this is done at SPL and at
>> this time FS_LOADER is not built yet as a result we see below error.
>> The if checks only checks for CONFIG_FS_LOADER but not for
>> CONFIG_SPL_FS_LOADER.
>>
>>   AR      spl/boot/built-in.o
>>   LD      spl/u-boot-spl
>> arm-none-linux-gnueabihf-ld.bfd: arch/arm/mach-k3/common.o: in function
>> `load_firmware':
>> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:184: undefined
>> reference to `get_fs_loader'
>> arm-none-linux-gnueabihf-ld.bfd:
>> /home/danish/workspace/u-boot/arch/arm/mach-k3/common.c:185: undefined
>> reference to `request_firmware_into_buf'
>> make[2]: *** [/home/danish/workspace/u-boot/scripts/Makefile.spl:527:
>> spl/u-boot-spl] Error 1
>> make[1]: *** [/home/danish/workspace/u-boot/Makefile:2055:
>> spl/u-boot-spl] Error 2
>> make[1]: Leaving directory '/home/danish/uboot_images/am64x/r5'
>> make: *** [Makefile:177: sub-make] Error 2
>>
>> This bug has always been there but as CONFIG_FS_LOADER was never
>> enabled, this build error was never seen as the load_firmware() API will
>> return 0 without calling fs-loader APIs.
>>
>> Now that this patch enables CONFIG_FS_LOADER, the bug gets exposed and
>> build error is seen.
>>
>> My opinion here would be, to check for CONFIG_IS_ENABLED(FS_LOADER)
>> instead of IS_ENABLED(CONFIG_FS_LOADER) as the former will check for the
>> appropriate config option (CONFIG_SPL_FS_LOADER / CONFIG_FS_LOADER)
>> based on the build stage.
>>
>> I tested with the below diff and I don't see build errors with
>> am64x_evm_r5, am65x_evm_r5_usbdfu, am65x_evm_r5_usbmsc configs.
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index f411366778..6792ff7467 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -162,7 +162,7 @@ int load_firmware(char *name_fw, char
>> *name_loadaddr, u32 *loadaddr)
>>         char *name = NULL;
>>         int size = 0;
>>
>> -       if (!IS_ENABLED(CONFIG_FS_LOADER))
>> +       if (!CONFIG_IS_ENABLED(FS_LOADER))
>>                 return 0;
>>
>>         *loadaddr = 0;
>> diff --git a/arch/arm/mach-omap2/boot-common.c
>> b/arch/arm/mach-omap2/boot-common.c
>> index 57917da25c..aa0ab13d5f 100644
>> --- a/arch/arm/mach-omap2/boot-common.c
>> +++ b/arch/arm/mach-omap2/boot-common.c
>> @@ -190,7 +190,7 @@ int load_firmware(char *name_fw, u32 *loadaddr)
>>         struct udevice *fsdev;
>>         int size = 0;
>>
>> -       if (!IS_ENABLED(CONFIG_FS_LOADER))
>> +       if (!CONFIG_IS_ENABLED(FS_LOADER))
>>                 return 0;
>>
>>         if (!*loadaddr)
>>
>>
>> Tom, Roger, Please let me know if this looks ok.
>> If it's ok, I will post this diff as a separate patch and once that is
>> merged Tom can merge this patch or I can send a v7 if needed.
> 
> Yes, this seems like the right path, thanks.
> 
Thanks Tom. Posted this diff as patch
https://lore.kernel.org/all/20240314143311.259568-1-danishanwar@ti.com/

-- 
Thanks and Regards,
Md Danish Anwar

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]

On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> ---
> Changes from v5 to v6:
> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com>
> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org>
> *) Added if condition to check if uc_pdata->fw_name exists and free it
>    before the strndup as suggested by Roger Quadros <rogerq@kernel.org>
> 
> Changes from v4 to v5:
> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size
>    that can be loaded to a rproc.
> *) Added freeing of address in rproc_boot() as pointed out by Ravi.
> *) Allocating the address at a later point in rproc_boot()
> *) Rebased on latest u-boot/master [commit 
>    9e00b6993f724da9699ef12573307afea8c19284]
> 
> Changes from v3 to v4:
> *) No functional change. Splitted the patch out of the series as suggested
>    by Nishant.
> *) Droppped the RFC tag.
> 
> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/
> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/
> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
> 
>  drivers/remoteproc/Kconfig        |   8 +++
>  drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++
>  include/remoteproc.h              |  34 ++++++++++
>  3 files changed, 144 insertions(+)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 781de530af..9f9877931c 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.

Can we not make the FS_LOADER portion optional? I didn't realize how
many non-TI platforms this impacted. And even then it's possible I
assume that custom designs will load the firmwares in other manners.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[MD Danish Anwar]

Hi Tom,

On 20/03/24 4:10 am, Tom Rini wrote:
> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>> ---
>> Changes from v5 to v6:
>> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com>
>> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org>
>> *) Added if condition to check if uc_pdata->fw_name exists and free it
>>    before the strndup as suggested by Roger Quadros <rogerq@kernel.org>
>>
>> Changes from v4 to v5:
>> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size
>>    that can be loaded to a rproc.
>> *) Added freeing of address in rproc_boot() as pointed out by Ravi.
>> *) Allocating the address at a later point in rproc_boot()
>> *) Rebased on latest u-boot/master [commit 
>>    9e00b6993f724da9699ef12573307afea8c19284]
>>
>> Changes from v3 to v4:
>> *) No functional change. Splitted the patch out of the series as suggested
>>    by Nishant.
>> *) Droppped the RFC tag.
>>
>> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/
>> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/
>> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
>>
>>  drivers/remoteproc/Kconfig        |   8 +++
>>  drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++
>>  include/remoteproc.h              |  34 ++++++++++
>>  3 files changed, 144 insertions(+)
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 781de530af..9f9877931c 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.
> 
> Can we not make the FS_LOADER portion optional? I didn't realize how
> many non-TI platforms this impacted. And even then it's possible I
> assume that custom designs will load the firmwares in other manners.
> 

Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef
CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER,
the clinet driver (ICSSG in this case) who is calling those remoteproc
APIs will select FS_LOADER and enable it.

This will make sure that other platforms (ti or non-ti) that doesn't
support ICSSG but enables Remoteproc, will not enable FS_LOADER. This
way we are not forcing other platforms using remoteproc to enable
FS_LOADER. In this case the APIs will not get built.

Now FS_LOADER will only be enabled when there is a client driver that
uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER

below is the diff,

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 9f9877931c..a49802c132 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -10,7 +10,6 @@ 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 f4f22a3851..a6a8be5009 100644
--- a/drivers/remoteproc/rproc-uclass.c
+++ b/drivers/remoteproc/rproc-uclass.c
@@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev,
const char *fw_name)
        return 0;
 }

+#ifdef CONFIG_FS_LOADER
 int rproc_boot(struct udevice *rproc_dev)
 {
        struct dm_rproc_uclass_pdata *uc_pdata;
@@ -1063,3 +1064,4 @@ free_buffer:
        free(addr);
        return ret;
 }
+#endif

Let me know if this looks ok. If it's ok I will post v7 with this change.

-- 
Thanks and Regards,
Danish

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4908 bytes --]

On Wed, Mar 20, 2024 at 11:19:01AM +0530, MD Danish Anwar wrote:
> Hi Tom,
> 
> On 20/03/24 4:10 am, Tom Rini wrote:
> > On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
> >> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> >> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >> Changes from v5 to v6:
> >> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com>
> >> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org>
> >> *) Added if condition to check if uc_pdata->fw_name exists and free it
> >>    before the strndup as suggested by Roger Quadros <rogerq@kernel.org>
> >>
> >> Changes from v4 to v5:
> >> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size
> >>    that can be loaded to a rproc.
> >> *) Added freeing of address in rproc_boot() as pointed out by Ravi.
> >> *) Allocating the address at a later point in rproc_boot()
> >> *) Rebased on latest u-boot/master [commit 
> >>    9e00b6993f724da9699ef12573307afea8c19284]
> >>
> >> Changes from v3 to v4:
> >> *) No functional change. Splitted the patch out of the series as suggested
> >>    by Nishant.
> >> *) Droppped the RFC tag.
> >>
> >> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/
> >> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/
> >> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
> >>
> >>  drivers/remoteproc/Kconfig        |   8 +++
> >>  drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++
> >>  include/remoteproc.h              |  34 ++++++++++
> >>  3 files changed, 144 insertions(+)
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index 781de530af..9f9877931c 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.
> > 
> > Can we not make the FS_LOADER portion optional? I didn't realize how
> > many non-TI platforms this impacted. And even then it's possible I
> > assume that custom designs will load the firmwares in other manners.
> > 
> 
> Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef
> CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER,
> the clinet driver (ICSSG in this case) who is calling those remoteproc
> APIs will select FS_LOADER and enable it.
> 
> This will make sure that other platforms (ti or non-ti) that doesn't
> support ICSSG but enables Remoteproc, will not enable FS_LOADER. This
> way we are not forcing other platforms using remoteproc to enable
> FS_LOADER. In this case the APIs will not get built.
> 
> Now FS_LOADER will only be enabled when there is a client driver that
> uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER
> 
> below is the diff,
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 9f9877931c..a49802c132 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -10,7 +10,6 @@ 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 f4f22a3851..a6a8be5009 100644
> --- a/drivers/remoteproc/rproc-uclass.c
> +++ b/drivers/remoteproc/rproc-uclass.c
> @@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev,
> const char *fw_name)
>         return 0;
>  }
> 
> +#ifdef CONFIG_FS_LOADER
>  int rproc_boot(struct udevice *rproc_dev)
>  {
>         struct dm_rproc_uclass_pdata *uc_pdata;
> @@ -1063,3 +1064,4 @@ free_buffer:
>         free(addr);
>         return ret;
>  }
> +#endif
> 
> Let me know if this looks ok. If it's ok I will post v7 with this change.

Yes please, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[MD Danish Anwar]

On 20/03/24 6:08 pm, Tom Rini wrote:
> On Wed, Mar 20, 2024 at 11:19:01AM +0530, MD Danish Anwar wrote:
>> Hi Tom,
>>
>> On 20/03/24 4:10 am, Tom Rini wrote:
>>> On Wed, Feb 28, 2024 at 05:36:45PM +0530, 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>
>>>> Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>> Changes from v5 to v6:
>>>> *) Collected Acked-by tag from Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>> *) Fixed few typos as pointed out by Roger Quadros <rogerq@kernel.org>
>>>> *) Added if condition to check if uc_pdata->fw_name exists and free it
>>>>    before the strndup as suggested by Roger Quadros <rogerq@kernel.org>
>>>>
>>>> Changes from v4 to v5:
>>>> *) Added Kconfig option REMOTEPROC_MAX_FW_SIZE to set max firmware size
>>>>    that can be loaded to a rproc.
>>>> *) Added freeing of address in rproc_boot() as pointed out by Ravi.
>>>> *) Allocating the address at a later point in rproc_boot()
>>>> *) Rebased on latest u-boot/master [commit 
>>>>    9e00b6993f724da9699ef12573307afea8c19284]
>>>>
>>>> Changes from v3 to v4:
>>>> *) No functional change. Splitted the patch out of the series as suggested
>>>>    by Nishant.
>>>> *) Droppped the RFC tag.
>>>>
>>>> v5: https://lore.kernel.org/all/20240217122602.3402774-1-danishanwar@ti.com/
>>>> v4: https://lore.kernel.org/all/20240130063322.2345057-1-danishanwar@ti.com/
>>>> v3: https://lore.kernel.org/all/20240124064930.1787929-4-danishanwar@ti.com/
>>>>
>>>>  drivers/remoteproc/Kconfig        |   8 +++
>>>>  drivers/remoteproc/rproc-uclass.c | 102 ++++++++++++++++++++++++++++++
>>>>  include/remoteproc.h              |  34 ++++++++++
>>>>  3 files changed, 144 insertions(+)
>>>>
>>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>>> index 781de530af..9f9877931c 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.
>>>
>>> Can we not make the FS_LOADER portion optional? I didn't realize how
>>> many non-TI platforms this impacted. And even then it's possible I
>>> assume that custom designs will load the firmwares in other manners.
>>>
>>
>> Yes we can. We can wrap the remoteproc APIs using FS_LOADER in #ifdef
>> CONFIG_FS_LOADER. And instead of REMOTEPROC driver selecting FS_LOADER,
>> the clinet driver (ICSSG in this case) who is calling those remoteproc
>> APIs will select FS_LOADER and enable it.
>>
>> This will make sure that other platforms (ti or non-ti) that doesn't
>> support ICSSG but enables Remoteproc, will not enable FS_LOADER. This
>> way we are not forcing other platforms using remoteproc to enable
>> FS_LOADER. In this case the APIs will not get built.
>>
>> Now FS_LOADER will only be enabled when there is a client driver that
>> uses rproc_boot() APIs. It's upto the client driver to enable FS_LOADER
>>
>> below is the diff,
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 9f9877931c..a49802c132 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -10,7 +10,6 @@ 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 f4f22a3851..a6a8be5009 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -994,6 +994,7 @@ int rproc_set_firmware(struct udevice *rproc_dev,
>> const char *fw_name)
>>         return 0;
>>  }
>>
>> +#ifdef CONFIG_FS_LOADER
>>  int rproc_boot(struct udevice *rproc_dev)
>>  {
>>         struct dm_rproc_uclass_pdata *uc_pdata;
>> @@ -1063,3 +1064,4 @@ free_buffer:
>>         free(addr);
>>         return ret;
>>  }
>> +#endif
>>
>> Let me know if this looks ok. If it's ok I will post v7 with this change.
> 
> Yes please, thanks.
> 

Posted v7 with the above changes
https://lore.kernel.org/all/20240321102819.1011011-1-danishanwar@ti.com/

Please check.

-- 
Thanks and Regards,
Danish