On 07/02/24 6:27 pm, Anwar, Md Danish wrote: > On 2/7/2024 6:05 PM, Roger Quadros wrote: >> >> >> 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. >> > > Roger, I still think 2 is a better option. In ICSSG driver also I am > setting the fw_size to 64K. Instead of hard-coding, this can be set in > the config. The ICSSG firmware is usually 40KB so 64K should be OK. > Other clients, if they use rproc_boot() in future, can specify there max > firmware size in their config. And to rproc layer it will be abstract. > > We can also get the size from config (Setting somethinig like > CONFIG_RPROC_MAX_FW_SIZE=64K) directly inside rproc_boot() eliminating > the need for the client to pass the size. But in future when there are > many clients using rproc_boot() for different firmware files, It will > become difficult for remoteproc to hardcode a firmware size that can be > bigger than any firmware that any client is using. > > It's simpler if each client specify there max fw size instead of > remoteproc driver specifying max possible size. > >> Tom, any insights? >>
Hi Tom, any suggestions? Roger, If Tom doesn't have any comment I will respin the patch by defining REMOTEPROC_MAX_FW_SIZE in drivers/remoteproc/Kconfig. I will keep the default size as 0x10000 (64K) >>> >>> 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