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

[Roger Quadros]

On 06/02/2024 07:31, MD Danish Anwar wrote:
> 
> 
> On 05/02/24 6:07 pm, Roger Quadros wrote:
>>
>>
>> On 05/02/2024 12:20, MD Danish Anwar wrote:
>>>
>>>
>>> On 05/02/24 3:36 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 02/02/2024 18:40, Anwar, Md Danish wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 2/2/2024 4:49 PM, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>>
> 
> <snip>
> 
>>>>>
>>>>>> 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.
>>>>>>
>>>>>
>>>>> Caller is trying to boot the rproc with a firmware binary. Caller should
>>>>> know the size of binary that it wants to load to rproc core. Caller will
>>>>> specify the binary size to rproc_boot(). Based on the size provided by
>>>>> caller, rproc_boot() will then allocate that much memory and call
>>>>> request_firmware_into_buf() with the size and allocated buffer. If the
>>>>> caller doesn't provide minimum size rproc_load() will fail.
>>>>
>>>> Caller only knows the filename. It need not know more details.
>>>
>>> Caller is trying to load a file of it's choice to a rproc. Caller should
>>> know the size of file it is trying to load or atleast the max size that
>>> the firmware file could be of.
>>>
>>>
>>>> Also see my comment below about rproc_boot() API.
>>>>
>>>>>
>>>>> rproc_load() calls respective driver ops, for example: pru_load().
>>>>> pru_load() [1] API checks the required size of firmware to load by
>>>>> casting the buffer into Elf32_Ehdr and Elf32_Phdr and returns error if
>>>>> size provided by caller is less than this.
>>>>>
>>>>>
>>>>> 	if (offset + filesz > size) {
>>>>> 		dev_dbg(dev, "truncated fw: need 0x%x avail 0x%zx\n",
>>>>> 			offset + filesz, size);
>>>>> 		ret = -EINVAL;
>>>>> 		break;
>>>>> 	}
>>>>>
>>>>>>> + *
>>>>>>> + * 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()?
>>>>>>
>>>>>
>>>>> Technically we can. But when we discussed this approach first in v1, you
>>>>> had asked to keep the APIs similar to upstream linux. Upstream linux has
>>>>> these two APIs so I kept it that way. If you want I can drop the first
>>>>> API. Please let me know.
>>>>
>>>> Sure you can keep it as it is in Linux, but there, rproc_boot doesn't
>>>> take fw_size argument. So wondering why you should have it in u-boot.
>>>>
>>>
>>> For loading firmware to a rproc core in u-boot, it's first neccassry to
>>> load the firmware into buffer and then load that buffer into rproc core
>>> using rproc_load() API. Now to load the firmware to a buffer ther is an
>>> API request_firmware_into_buf(). This API takes size of firmware as one
>>> of it's argument. So in order to call this API from rproc_boot() we need
>>> to pass fw_size to rproc_boot()
>>>
>>> Other u-boot drivers using request_firmware_into_buf() are also passing
>>> size of firmware from their driver.
>>
>> But in your driver you didn't use size of firmware but some 64K
>> https://lore.kernel.org/all/20240124064930.1787929-8-danishanwar@ti.com/
>>
> 
> Yes, in driver I am hardcoding the size to 64K. That's because I know
> the size of ICSSG firmwares are less than 64K. Instead of hardcoding I

What if you enable debugging symbols in the firmware file. Won't it exceed 64KB?
It is not a good idea to assume any firmware file size as it will eventually
break sometime in the future and will be a pain to debug.

> can also define macro or provide a config option where we set the size
> and the driver will read the size from the config and call rproc_boot()
> with size.
> 
> For example, fm.c driver reads the size from config option
> CONFIG_SYS_QE_FMAN_FW_LENGTH [1] and calls request_firmware_into_buf()
> 
> [1]
> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/fm/fm.c#L458
> 
>> So neither does the caller have a clue of firmware size?
>>
>>>
>>> If rproc_boot() doesn't take fw_size as argument then within
>>> rproc_boot() we need to figure out the fw_size before calling
>>> request_firmware_into_buf().
>>>
>>> If we don't know the size / maximum size of the firmware to load, how
>>> will we call request_firmware_into_buf(). Someone has to tell
>>> request_firmware_into_buf() the size of firmware. I am expecting that to
>>> be the caller. Do you have any other way of getting the firmware size
>>> before request_firmware_into_buf() is called?
>>
>> /**
>>  * request_firmware_into_buf - Load firmware into a previously allocated buffer.
>>  * @dev: An instance of a driver.
>>  * @name: Name of firmware file.
>>  * @buf: Address of buffer to load firmware into.
>>  * @size: Size of buffer.
>>  * @offset: Offset of a file for start reading into buffer.
>>
>> It needs size of pre-allocated buffer which can be smaller than file size.
>> It also has the option of offset. So you can load portions of the file limited
>> by buffer size.
>>
>> My suggestion is that Remoteproc layer should take care of how much buffer
>> to allocate and pass that buffer size to request_firmware_into_buf().
>> You are doing the malloc here itself anyways.
>>
> 
> But how would the remoteproc driver know how much buffer it needs to
> allocate before calling request_firmware_into_buf().

Only the filesystem driver knows what exactly is the firmware file size.
fs_size() API can be used for that.

> 
>>>
>>>>>
>>>>>>>  #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_ */
>>>>>>
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/u-boot/latest/source/drivers/remoteproc/pru_rproc.c#L324
>>>>>
>>>>
>>>
>>
> 

-- 
cheers,
-roger