On 07/02/2024 09:15, MD Danish Anwar wrote:
> Hi Roger
>
> On 06/02/24 7:11 pm, Roger Quadros wrote:
>>
>>
>> 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-danishan...@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.
>>
>
> To use fs_size() we first need to call fs_set_blk_dev() to set the
> storage interface, device partition and fs_type. eg.
> fs_set_blk_dev("mmc, "1:2", FS_TYPE_ANY)
>
> Since we are setting the envs for storage_interface and partition I'll
> use the envs to call fs_set_blk_dev()
>
> This is how rproc_boot() will look now.
>
> int rproc_boot(struct udevice *rproc_dev)
> {
> struct dm_rproc_uclass_pdata *uc_pdata;
> char *storage_interface, *dev_part;
> struct udevice *fs_loader;
> int core_id, ret = 0;
> char *firmware;
> loff_t fw_size;
> 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 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;
> }
>
> storage_interface = env_get("fw_storage_interface");
> dev_part = env_get("fw_dev_part");
>
> if (storage_interface && dev_part) {
> ret = fs_set_blk_dev(storage_interface, dev_part,
> FS_TYPE_ANY);
> } else {
> debug("could not get env variables to load firmware\n");
> return -EINVAL;
> }
I'm not very sure about this as we are using firmware loader
specific environment variables outside the firmware loader driver.
I can see 2 solutions here:
1) ask firmware loader driver to tell us the firmware file size.
fw_get_filesystem_firmware() also deals with UBI filesystem.
2) use a large enough buffer whose size is set in Kconfig.
Tom, any insights?
>
> if (ret) {
> debug("fs_set_blk_dev failed %d\n", ret);
> return ret;
> }
>
> ret = fs_size(firmware, &fw_size);
> if (ret) {
> debug("could not get firmware size %s: %d\n", firmware, ret);
> return ret;
> }
>
> addr = malloc(fw_size);
> if (!addr)
> return -ENOMEM;
>
> ret = request_firmware_into_buf(fs_loader, firmware, addr, 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;
> }
>
> Please let me know if this looks ok. Without calling fs_set_blk_dev()
> first, fs_size() results in error.
>
>
>>>
>>>>>
>>>>>>>
>>>>>>>>> #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
