On Tue, Sep 17, 2024 at 11:16:08AM +0000, Manne, Nava kishore wrote:
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > Sent: Monday, August 5, 2024 11:51 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-fpga@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx
> > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA
> > configuration
> >
> > On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote:
> > > Hi Yilun,
> > >
> > > > -----Original Message-----
> > > > From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > > > Sent: Monday, July 29, 2024 9:27 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-fpga@xxxxxxxxxxxxxxx;
> > > > linux- kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support
> > > > for FPGA configuration
> > > >
> > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote:
> > > > > Adds sysfs interface as part of the of-fpga-region. This newly
> > > > > added sysfs interface uses Device Tree Overlay (DTO) files to
> > > > > configure/reprogram an FPGA while an operating system is
> > > > > running.This solution will not change the existing sequence When a
> > > > > DT overlay that targets an FPGA Region is applied.
> > > > > - Disable appropriate FPGA bridges.
> > > > > - Program the FPGA using the FPGA manager.
> > > > > - Enable the FPGA bridges.
> > > > > - The Device Tree overlay is accepted into the live tree.
> > > > > - Child devices are populated.
> > > > >
> > > > > When the overlay is removed, the child nodes will be removed, and
> > > > > the FPGA Region will disable the bridges.
> > > > >
> > > > > Usage:
> > > > > To configure/reprogram an FPGA region:
> > > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load
> > > >
> > > > IIRC, last time we are considering some generic interface for both
> > > > OF & non- OF FPGA region, but this is still OF specific.
> > > >
> > > At AMD, we exclusively use OF for FPGA configuration/reconfiguration, utilizing
> > overlay files as outlined in the fpga-region.txt documentation.
> > > However, some devices, like dfl.c those relying solely on the FPGA region, do not
> > use OF.
> > > For these non-OF devices, should we expect them to follow the fpga-region.txt
> > guidelines for FPGA configuration/reconfiguration?
> >
> > I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml.
> >
> > No, Non-OF devices don't have to follow the DT binding.
> >
> > > If so, it may be advantageous to develop a common interface for both OF and
> > non-OF.
> > > If not, it might be more appropriate to establish distinct interfaces to cater to their
> > specific requirements.
> >
> > I think each vendor may have specific way for device enumeration, but that doesn't
> > mean we need distinct user interfaces. For all FPGA devices, we should avoid the
> > situation that the HW is changed but system SW knows nothing. So the common
> > needs are:
> >
> > - Find out and remove all devices within the fpga region before
> > reprograming.
> > - Re-enumerate devices in fpga region after reprograming.
> >
> > I expect the fpga region class could generally enforce a flow for the reprograming
> > interface. And of-fpga-region could specifically implement it using DT overlay.
> >
>
> To address the vendor-specific nature(either of or non-of) of device enumeration
> in FPGA regions, As you suggested, we can develop a common programming
> interface that abstracts these vendor-specifc differences. This can be achieved
> by integrating vendor-specific callbacks(ex: of and non-of) for device configuration,
> enumeration and removal to fpga-region.
>
> I have outlined the top-level framework changes here:
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index b364a929425c..7d4b755dc8e0 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -213,6 +213,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> region->compat_id = info->compat_id;
> region->priv = info->priv;
> region->get_bridges = info->get_bridges;
> + region->region_ops = info->region_ops;
>
> mutex_init(®ion->mutex);
> INIT_LIST_HEAD(®ion->bridge_list);
> @@ -257,17 +258,46 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> */
> struct fpga_region *
> fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> + struct fpga_region_ops *region_ops,
> int (*get_bridges)(struct fpga_region *)) {
> struct fpga_region_info info = { 0 };
>
> info.mgr = mgr;
> info.get_bridges = get_bridges;
> + info.region_ops = region_ops;
>
> return fpga_region_register_full(parent, &info); } EXPORT_SYMBOL_GPL(fpga_region_register);
>
> +static int fpga_region_device_open(struct inode *inode, struct file *file) {
> + struct miscdevice *miscdev = file->private_data;
> + struct fpga_region *region = container_of(miscdev, struct fpga_region,
> +miscdev);
> +
> + file->private_data = region;
> +
> + return 0;
> +}
> +
> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) {
> + char __user *argp = (char __user *)arg;
> + struct fpga_region *region = (struct fpga_region *)(file->private_data);
> + int err;
> +
> + switch (cmd) {
> + case FPGA_REGION_IOCTL_LOAD:
> + err = region->region_ops->fpga_region_config_enumerate (region, argp);
Not sure "void *args" is a proposal or something yet to be decided.
I think we should try best not to give up parameter type and have a clear API
definition.
> + break;
> + case FPGA_REGION_IOCTL_REMOVE:
> + err = region->region_ops->fpga_region_remove(region, argp);
> + break;
> + case FPGA_REGION_IOCTL_STATUS:
> + err = region->region_ops->fpga_region_status(region, argp);
> + default:
> + err = -ENOTTY;
> +}
> +
> /**
> * fpga_region_unregister - unregister an FPGA region
> * @region: FPGA region
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 9d4d32909340..725fdcbab3d8 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -9,6 +9,20 @@
>
> struct fpga_region;
>
> +/**
> + * 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 config could be a common existing operation, fpga_region_program_fpga().
So maybe only enumeration is needed?
> + * @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 *bridge);
> + int (*region_config_enumeration)(struct fpga_region *region, void *args);
> + void (*region_remove)(struct fpga_region *region, void *args); };
> +
> /**
> * struct fpga_region_info - collection of parameters an FPGA Region
> * @mgr: fpga region manager
> @@ -26,6 +40,7 @@ struct fpga_region_info {
> struct fpga_compat_id *compat_id;
> void *priv;
> int (*get_bridges)(struct fpga_region *region);
> + struct fpga_region_ops *region_ops;
> };
>
> /**
> @@ -48,6 +63,7 @@ struct fpga_region {
> struct fpga_compat_id *compat_id;
> void *priv;
> int (*get_bridges)(struct fpga_region *region);
> + struct fpga_region_ops *region_ops;
> };
>
> #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
>
> In this approach, we utilized an IOCTL-based user interface, but it doesn't have
> to be confined to IOCTL. We can also use sysfs or configfs, or other appropriate
> options as we finalized on it.
>
> This call-backs approach works for both OF and non-OF devices.
> If this aligns with your expectations, I can do the necessary changes
There are still much to discuss, but yes this is a good start.
Thanks,
Yilun
> for one vendor specific interface (of-fpga-region.c) devices and submit
> the RFC patch shortly.
>
>
> Regards,
> Navakishore.
