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

[MD Danish Anwar <danishanwar@ti.com>]

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