On Tue, Oct 12, 2021 at 03:47:52PM +0800, Xu Yilun wrote: > On Mon, Oct 11, 2021 at 06:00:16PM -0700, Russ Weight wrote: > > > > > > On 10/11/21 5:35 AM, Tom Rix wrote: > > > > > > On 10/10/21 6:41 PM, Xu Yilun wrote: > > >> On Sat, Oct 09, 2021 at 05:11:20AM -0700, Tom Rix wrote: > > >>> On 10/9/21 1:08 AM, Xu Yilun wrote: > > >>>> On Wed, Sep 29, 2021 at 04:00:20PM -0700, Russ Weight wrote: > > >>>>> The FPGA Image Load framework provides an API to upload image > > >>>>> files to an FPGA device. Image files are self-describing. They could > > >>>>> contain FPGA images, BMC images, Root Entry Hashes, or other device > > >>>>> specific files. It is up to the lower-level device driver and the > > >>>>> target device to authenticate and disposition the file data. > > >>>> I've reconsider the FPGA persistent image update again, and think we > > >>>> may include it in FPGA manager framework. > > >>>> > > >>>> Sorry I raised this topic again when it is already at patch v17, but now > > >>>> I need to consider more seriously than before. > > >>>> > > >>>> We have consensus the FPGA persistent image update is just like a normal > > >>>> firmware update which finally writes the nvmem like flash or eeprom, > > >>>> while the current FPGA manager deals with the active FPGA region update > > >>>> and re-activation. Could we just expand the FPGA manager and let it handle > > >>>> the nvmem update as well? Many FPGA cards have nvmem and downloaders > > >>>> supports updating both FPGA region and nvmem. > > The fpga-image-load driver is actually just a data transfer. The class > > IMHO, The fpga-mgr dev is also a data transfer. The fpga-region dev is > acting as the FPGA region admin responsible for gating, transfering and > re-enumerating. > > So my opinion is to add a new data transfer type and keep a unified process. > > > driver has no knowledge about what the data is or where/if the data will > > be stored. > > The fpga-image-load driver knows the data will be stored in nvmem, while > the fpga-mgr knows the data will be stored in FPGA cells. They may need > to know the exact physical position to store, may not, depends on what the > HW engines are. > > > > > This functionality could, of course, be merged into the fpga-mgr. I did > > a proof of concept of this a while back and we discussed the pros and cons. > > See this email for a recap: > > > > https://marc.info/?l=linux-fpga&m=161998085507374&w=2 > > > > Things have changed some with the evolution of the driver. The IOCTL > > approach probably fits better than the sysfs implementation. At the time > > it seemed that a merge would add unnecessary complexity without adding value. > > I think at least developers don't have to go through 2 sets of software > stacks which are of the same concept. And adding some new features like > optionally threading or canceling data transfer are also good to FPGA > region update. And the nvmem update could also be benifit from exsiting > implementations like scatter-gather buffers, in-kernel firmware loading. > > I try to explain myself according to each of your concern from that mail > thread: > > Purpose of the 2 updates > ======================== > > As I said before, I think they are both data transfer devices. FPGA > region update gets extra support from fpga-region & fpga-bridge, and > FPGA nvmem update could be a standalone fpga-mgr. > > Extra APIs that are unique to nvmem update > ========================================== > > cdev APIs for nvmem update: > Yes we need to expand the functionality so we need them. > > available_images, image_load APIs for loading nvmem content to FPGA > region: > These are features in later patchsets which are not submitted, but we > could talk about it in advance. I think this is actually a FPGA region > update from nvmem, it also requires gating, data loading (no SW transfer) > and re-enumeration, or a single command to image_load HW may result system > disordered. The FPGA framework now only supports update from in-kernel > user data, maybe we add support for update from nvmem later. > > fpga-mgr state extend: > I think it could be extended, The current states are not perfect, > adding something like IDLE or READY is just fine. > > fpga-mgr status extend: > Add general error definitions as needed. If there is some status > that is quite vendor specific, expose it in low-level driver. > > remaining-size: > Nice to have for all. > > Threading the update > ==================== > > Also a good option for FPGA region update, maybe we also have a slow FPGA > reprogrammer? Another thought is, could we implement the non-threading version firstly, so there would be less change and faster to have the basic functionality. But either is OK for me. Thanks, Yilun > > Cancelling the update > ==================== > > Also a good option for FPGA region update if HW engine supports. > > Thanks, > Yilun > > > > > >>>> > > >>>> According to the patchset, the basic workflow of the 2 update types are > > >>>> quite similar, get the data, prepare for the HW, write and complete. > > >>>> They are already implemented in FPGA manager. We've discussed some > > >>>> differences like threading or canceling the update, which are > > >>>> not provided by FPGA manager but they may also nice to have for FPGA > > >>>> region update. An FPGA region update may also last for a long time?? > > >>>> So I think having 2 sets of similar frameworks in FPGA is unnecessary. > > >>>> > > >>>> My quick mind is that we add some flags in struct fpga_mgr & struct > > >>>> fpga_image_info to indicate the HW capability (support FPGA region > > >>>> update or nvmem update or both) of the download engine and the provided > > >>>> image type. Then the low-level driver knows how to download if it > > >>>> supports both image types.An char device could be added for each fpga manager dev, providing the > > >>>> user APIs for nvmem update. We may not use the char dev for FPGA region > > >>>> update cause it changes the system HW devices and needs device hotplug > > >>>> in FPGA region. We'd better leave it to FPGA region class, this is > > >>>> another topic. > > I'll give this some more thought and see if I can come up with some RFC > > patches. > > > > - Russ > > >>>> > > >>>> More discussion is appreciated. > > >>> I also think fpga_mgr could be extended. > > >>> > > >>> In this patchset, > > >>> > > >>> https://lore.kernel.org/linux-fpga/20210625195849.837976-1-trix@xxxxxxxxxx/ > > >>> > > >>> A second, similar set of write ops was added to fpga_manger_ops, > > >>> > > >>> new bit/flag was added to fpga_image_info > > >>> > > >>> The intent was for dfl to add their specific ops to cover what is done in > > >>> this patchset. > > >> I think we don't have to add 2 ops for reconfig & reimage in framework, > > >> the 2 processes are almost the same. > > >> > > >> Just add the _REIMAGE (or something else, NVMEM?) flag for > > >> fpga_image_info, and low level drivers handle it as they do for other > > >> flags. > > >> > > >> How do you think? > > > > > > A single set is fine. > > > > > > A difficult part of is the length of time to do the write. The existing write should be improved to use a worker thread. > > > > > > Tom > > > > > >> > > >> Thanks, > > >> Yilun > > >> > > >>> Any other driver would do similar. > > >>> > > >>> Is this close to what you are thinking ? > > >>> > > >>> Tom > > >>> > > >>>> Thanks, > > >>>> Yilun > > >>>> > > >
