Hi Yilun,
> -----Original Message-----
> From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Sent: Sunday, March 19, 2023 9:08 PM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx; hao.wu@xxxxxxxxx;
> yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx; robh@xxxxxxxxxx; saravanak@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
>
> On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote:
> > Hi Yilun,
> >
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > > Sent: Tuesday, December 10, 2024 2:34 PM
> > > To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> > > Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx;
> > > hao.wu@xxxxxxxxx; yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx;
> > > robh@xxxxxxxxxx; saravanak@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > linux-fpga@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface
> > > for runtime FPGA programming
> > >
> > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> > > > Hi Yilun,
> > > >
> > > > > -----Original Message-----
> > > > > From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > > > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > > > To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> > > > > Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx;
> > > > > hao.wu@xxxxxxxxx; yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx;
> > > > > robh@xxxxxxxxxx; saravanak@xxxxxxxxxx;
> > > > > linux-kernel@xxxxxxxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx;
> > > > > devicetree@xxxxxxxxxxxxxxx
> > > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL
> > > > > interface for runtime FPGA programming
> > > > >
> > > > > > > > + * 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?
> > > > >
> > > >
> > > > A configfs uAPI isn't necessary, we can manage this using the
> > > > proposed IOCTL
> > > flow.
> > > > The POC code looks as follows.
> > >
> > > I prefer more to configfs cause it provides standard FS way to
> > > create the fpga_image_info object, e.g. which attributes are visible
> > > for OF/non-OF region, which attributes come from image blob and can only be
> RO, etc.
> > >
> > > Of couse ioctl() could achieve the same goal but would add much more
> > > specific rules (maybe flags/types) for user to follow.
> > >
> >
> > Agreed. Using ConfigFS is preferable because it provides a
> > standardized filesystem interface for creating and managing the fpga_image_info
> object.
> >
> > The proposed new user interface is outlined as follows:
> >
> > # Mount ConfigFS filesystem
> > mount -t configfs none /sys/kernel/config
> >
> > # Upload Configuration and Load the Bitstream for the Targeted FPGA Region.
> >
> > Configuration File Upload:
> > Upload the configuration file containing the necessary metadata or
> > settings required for configuring the FPGA region. This file may vary
> > based on the vendor and includes important details specific to the vendor's
> requirements.
> >
> > Vendor-Specific Callback:
> > A vendor-specific callback function extracts the relevant configuration data from the
> file.
> > The format and contents of the configuration file can differ between
> > vendors. The callback then initializes the struct fpga_image_info,
> > ensuring all vendor-specific requirements are satisfied.
> >
> > Device-Specific Considerations:
> > For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config
> files.
> > These .dtbo files contain all necessary information to populate the
> fpga_image_info.
> > For non-OF devices, a vendor specific fpga.config files are used to
> > provide the required data for initializing the fpga_image_info.
>
> non-OF fpga images usually don't contain fpga_image_info data (e.g.
> enable/disable_timeout_us). I think we don't have to force users embed these data in
> fpga image, provide additional configfs attributes to input these data is possible. For
> some FPGA regions (e.g. OF), these attributes could be RO, some could be RW,
> depends on different FPGA region drivers.
>
> So I think we may have a Configuration File Upload interface, like:
>
> echo "config_file" > /sys/kernel/config/fpga/<region>/image
>
> Some additional parameter interfaces, like:
>
> echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout
> ...
>
> And a Configuration interface, like:
>
> # programming
> echo 1 > /sys/kernel/config/fpga/<region>/config
> # removing
> echo 0 > /sys/kernel/config/fpga/<region>/config
>
> How do you think?
>
I agree and am actively working on the POC changes.
I will submit the RFC patches at the earliest.
Regards,
Navakishore.
