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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]