Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun <yilun.xu@linux.intel.com>]

> > > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > > +device
> > > + * enumeration/removal
> > > + * @region_status: returns the FPGA region status
> > > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> > > + * @region_remove: Remove all devices within the FPGA region
> > > + * (which are added as part of the enumeration).
> > > + */
> > > +struct fpga_region_ops {
> > > +	int (*region_status)(struct fpga_region *region);
> > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > +					 struct fpga_region_config_info *config_info);
> > 
> > My current concern is still about this combined API, it just offloads all work to low
> > level, but we have some common flows. That's why we introduce a common FPGA
> > reprograming API.
> > 
> > I didn't see issue about the vendor specific pre configuration. They are generally
> > needed to initialize the struct fpga_image_info, which is a common structure for
> > fpga_region_program_fpga().
> > 
> > For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be
> > implemented in this patchset). Previously DFL provides an uAPI for the whole
> > device, so it needs a port_id input to position which fpga_region within the device for
> > programming. But now, we are introducing a per fpga_region programming interface,
> > IIUC port_id should not be needed anymore.
> > 
> > The combined API is truly simple for leveraging the existing of-fpga-region overlay
> > apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to
> > adapt the generic configfs overlay interface, which comes to a dead end as you
> > mentioned.
> > 
> > My gut feeling for the generic programing flow should be:
> > 
> >  1. Program the image to HW.
> >  2. Enumerate the programmed image (apply the DT overlay)
> > 
> > Why we have to:
> > 
> >  1. Start enumeration.
> >  2. On pre enumeration, programe the image.
> >  3. Real enumeration.
> > 
> 
> I agree with the approach of leveraging vendor-specific callbacks to handle
> the distinct phases of the FPGA programming process. 
> Here's the proposed flow.
>  
> Pre-Configuration:
> A vendor-specific callback extracts the required pre-configuration details
> and initializes struct fpga_image_info. This ensures that all vendor-specific

Since we need to construct the fpga_image_info, initialize multiple
field as needed, I'm wondering if configfs could be a solution for the
uAPI?

> requirements are handled before proceeding to the programming phase.
>  
> Programming:
> The common API fpga_region_program_fpga() is used to program the image
> to hardware. This standardizes the programming logic, minimizing duplication
> and ensuring consistency across implementations.
>  
> Enumeration:
> A vendor-specific callback is used for real enumeration, enabling hardware
> specific customization while keeping the flow flexible and adaptable
> 
> This approach provides a clean separation of responsibilities, with
> vendor-specific logic confined to the pre-configuration and enumeration
> phases, while the programming phase leverages a common API.
> It simplifies maintenance and aligns well with the Program -> Enumerate flow.

Generally I'm good to this flow.

Thanks,
Yilun

>  
> Looking forward to your feedback.
> 
> 
> Regards,
> Navakishore.