[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 at ti.com>
>>>>> Acked-by: Ravi Gunasekaran <r-gunasekaran at ti.com>
>>>>> Reviewed-by: Roger Quadros <rogerq at 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