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; } 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 >>>>>> >>>>> >>>> >>> >> > -- Thanks and Regards, Danish