Messages in this thread Patch in this message | | | Subject | Re: [PATCH V7 XRT Alveo 00/20] XRT Alveo driver overview | From | Tom Rix <> | Date | Thu, 1 Jul 2021 13:32:29 -0700 |
| |
Lizhi,
Sorry for the delay in reviewing v7.
Is it too early to blame it on the July 4 holiday here ?!? :)
All the small stuff looks fine to me. In this pass I looked at issues that would need a refactoring. Since it would be a lot of work and I am not the final word on this, it would be good if some others to chime in. Also a couple of new spelling fixes at the end.
Tom
Having xrt/ dir ok with it or it will follow the subdir reorg of fpga/, afaik not a blocker
Location of xrt_bus_type ok, similar to dfl_bus_type
Non fpga subdevices should go to other subsystems. looking in drivers/fpga/xrt/lib/xleaf
clock clkfrq ucs these are clocks should move to drivers/clk/xilinx/
axigate, for fpga partitioning ok to stay
ddr_calibaration, a memory status checker should move drivers/memory dfl-emif is similar
devctl, a general purpose misc driver should move to drivers/mfd
icap, for fpga bitstream writing ok to stay
vsec, misc small drivers discovered via pci config vsec should move to drivers/mfd
For include/uapi/linux collapse include/uapi/linux/xrt/*.h into include/uapi/linux/fpga-xrt.h There are only 2 files, one really small. fpga-xrt.h follows fpga-dfl.h The comments are pretty messy, user should be able to scan them. Try cleaning them up.
Spelling mistakes
diff --git a/Documentation/fpga/xrt.rst b/Documentation/fpga/xrt.rst index 5a5b4d5a3bc6..84eb41be9ac1 100644 --- a/Documentation/fpga/xrt.rst +++ b/Documentation/fpga/xrt.rst @@ -275,7 +275,7 @@ fpga_bridge and fpga_region for the next region in the chain. fpga_bridge -----------
-Like the fpga_region, a fpga_bridge is created by walking the device tree +Like the fpga_region, an fpga_bridge is created by walking the device tree of the parent group. The bridge is used for isolation between a parent and its child.
@@ -416,7 +416,7 @@ xclbin is compiled by end user using `Vitis <https://www.xilinx.com/products/design-tools/vitis/vitis-platform.html>`_ tool set from Xilinx. The xclbin contains sections describing user compiled acceleration engines/kernels, memory subsystems, clocking information etc. It also -contains a FPGA bitstream for the user partition, UUIDs, platform name, etc. +contains an FPGA bitstream for the user partition, UUIDs, platform name, etc.
.. _xsabin_xclbin_container_format: diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h index c4df88262f8a..f48d6d42f5ef 100644 --- a/drivers/fpga/xrt/include/metadata.h +++ b/drivers/fpga/xrt/include/metadata.h @@ -194,7 +194,7 @@ int xrt_md_get_interface_uuids(struct device *dev, const char *blob, /* * The firmware provides a 128 bit hash string as a unique id to the * partition/interface. - * Existing hw does not yet use the cononical form, so it is necessary to + * Existing hw does not yet use the canonical form, so it is necessary to * use a translation function. */ static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr) diff --git a/drivers/fpga/xrt/lib/xroot.c b/drivers/fpga/xrt/lib/xroot.c index 7b3e540dd6c0..f324a25e1d4d 100644 --- a/drivers/fpga/xrt/lib/xroot.c +++ b/drivers/fpga/xrt/lib/xroot.c @@ -427,7 +427,7 @@ static void xroot_bringup_group_work(struct work_struct *work) r = xleaf_call(xdev, XRT_GROUP_INIT_CHILDREN, NULL); xroot_put_group(xr, xdev); if (r == -EEXIST) - continue; /* Already brough up, nothing to do. */ + continue; /* Already brought up, nothing to do. */ if (r) atomic_inc(&xr->groups.bringup_failed_cnt);
diff --git a/drivers/fpga/xrt/mgmt/xmgmt-main.c b/drivers/fpga/xrt/mgmt/xmgmt-main.c index 820c888e7918..9077254e0f8a 100644 --- a/drivers/fpga/xrt/mgmt/xmgmt-main.c +++ b/drivers/fpga/xrt/mgmt/xmgmt-main.c @@ -142,7 +142,7 @@ static ssize_t VBNV_show(struct device *dev, struct device_attribute *da, char * } static DEVICE_ATTR_RO(VBNV);
-/* logic uuid is the uuid uniquely identfy the partition */ +/* logic uuid is the uuid uniquely identify the partition */ static ssize_t logic_uuids_show(struct device *dev, struct device_attribute *da, char *buf) { struct xrt_device *xdev = to_xrt_dev(dev); diff --git a/drivers/fpga/xrt/mgmt/xrt-mgr.c b/drivers/fpga/xrt/mgmt/xrt-mgr.c index 41263a033d9d..ab253b516e8d 100644 --- a/drivers/fpga/xrt/mgmt/xrt-mgr.c +++ b/drivers/fpga/xrt/mgmt/xrt-mgr.c @@ -115,7 +115,7 @@ static int xmgmt_pr_write_init(struct fpga_manager *mgr, }
/* - * The implementation requries full xclbin image before we can start + * The implementation requires full xclbin image before we can start * programming the hardware via ICAP subsystem. The full image is required * for checking the validity of xclbin and walking the sections to * discover the bitstream. On 5/27/21 5:49 PM, Lizhi Hou wrote: > Hello, > > This is V7 of patch series which adds management physical function driver > for Xilinx Alveo PCIe accelerator cards. > https://www.xilinx.com/products/boards-and-kits/alveo.html > > This driver is part of Xilinx Runtime (XRT) open source stack. > > XILINX ALVEO PLATFORM ARCHITECTURE > > Alveo PCIe FPGA based platforms have a static *shell* partition and a > partial re-configurable *user* partition. The shell partition is > automatically loaded from flash when host is booted and PCIe is enumerated > by BIOS. Shell cannot be changed till the next cold reboot. The shell > exposes two PCIe physical functions: > > 1. management physical function > 2. user physical function > > The patch series includes Documentation/xrt.rst which describes Alveo > platform, XRT driver architecture and deployment model in more detail. > > Users compile their high level design in C/C++/OpenCL or RTL into FPGA > image using Vitis tools. > https://www.xilinx.com/products/design-tools/vitis/vitis-platform.html > > The compiled image is packaged as xclbin which contains partial bitstream > for the user partition and necessary metadata. Users can dynamically swap > the image running on the user partition in order to switch between > different workloads by loading different xclbins. > > XRT DRIVERS FOR XILINX ALVEO > > XRT Linux kernel driver *xrt-mgmt* binds to management physical function of > Alveo platform. The modular driver framework is organized into several > platform drivers which primarily handle the following functionality: > > 1. Loading firmware container also called xsabin at driver attach time > 2. Loading of user compiled xclbin with FPGA Manager integration > 3. Clock scaling of image running on user partition > 4. In-band sensors: temp, voltage, power, etc. > 5. Device reset and rescan > > The platform drivers are packaged into *xrt-lib* helper module with well > defined interfaces. The module provides a pseudo-bus implementation for the > platform drivers. More details on the driver model can be found in > Documentation/xrt.rst. > > User physical function driver is not included in this patch series. > > LIBFDT REQUIREMENT > > XRT driver infrastructure uses Device Tree as a metadata format to discover > HW subsystems in the Alveo PCIe device. The Device Tree schema used by XRT > is documented in Documentation/xrt.rst. > > TESTING AND VALIDATION > > xrt-mgmt driver can be tested with full XRT open source stack which > includes user space libraries, board utilities and (out of tree) first > generation user physical function driver xocl. XRT open source runtime > stack is available at https://github.com/Xilinx/XRT > > Complete documentation for XRT open source stack including sections on > Alveo/XRT security and platform architecture can be found here: > > https://xilinx.github.io/XRT/master/html/index.html > https://xilinx.github.io/XRT/master/html/security.html > https://xilinx.github.io/XRT/master/html/platforms_partitions.html > > Changes since v6: > - Resolved grammatical errors and cleaned up taxonomy in xrt.rst > documentation. > - Fixed clang warnings. > - Updated code base to include v6 code review comments. > > Changes since v5: > - Revert all changes 'mgnt/MGNT' back to 'mgmt/MGMT' > - Updated code base to include v5 code review comments. > xrt.rst: address grammar and taxonomy > subdev_id.h: defines XRT_SUBDEV_INVALID = 0 > xclbin.c: change shift operation to be_to_cpu > - Resolved kernel test robot errors. > > Changes since v4: > - Added xrt_bus_type and xrt_device. All sub devices were changed from > platform_bus_type/platform_device to xrt_bus_type/xrt_device. > - Renamed xrt-mgmt driver to xrt-mgnt driver. > - Replaced 'MGMT' with 'MGNT' and 'mgmt' with 'mgnt' in code and file names > - Moved pci function calls from infrastructure to xrt-mgnt driver. > - Renamed files: mgmt/main.c -> mgnt/xmgnt-main.c > mgmt/main-region.c -> mgnt/xmgnt-main-region.c > include/xmgmt-main.h -> include/xmgnt-main.h > mgmt/fmgr-drv.c -> mgnt/xrt-mgr.c > mgmt/fmgr.h -> mgnt/xrt-mgr.h > - Updated code base to include v4 code review comments. > > Changes since v3: > - Leaf drivers use regmap-mmio to access hardware registers. > - Renamed driver module: xmgmt.ko -> xrt-mgmt.ko > - Renamed files: calib.[c|h] -> ddr_calibration.[c|h], > lib/main.[c|h] -> lib/lib-drv.[c|h], > mgmt/main-impl.h - > mgmt/xmgnt.h > - Updated code base to include v3 code review comments. > > Changes since v2: > - Streamlined the driver framework into *xleaf*, *group* and *xroot* > - Updated documentation to show the driver model with examples > - Addressed kernel test robot errors > - Added a selftest for basic driver framework > - Documented device tree schema > - Removed need to export libfdt symbols > > Changes since v1: > - Updated the driver to use fpga_region and fpga_bridge for FPGA > programming > - Dropped platform drivers not related to PR programming to focus on XRT > core framework > - Updated Documentation/fpga/xrt.rst with information on XRT core framework > - Addressed checkpatch issues > - Dropped xrt- prefix from some header files > > For reference V6 version of patch series can be found here: > > https://lore.kernel.org/lkml/20210512015339.5649-1-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-2-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-3-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-4-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-5-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-6-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-7-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-8-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-9-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-10-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-11-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-12-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-13-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-14-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-15-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-16-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-17-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-18-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-19-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-20-lizhi.hou@xilinx.com/ > https://lore.kernel.org/lkml/20210512015339.5649-21-lizhi.hou@xilinx.com/ > > Lizhi Hou (20): > Documentation: fpga: Add a document describing XRT Alveo drivers > fpga: xrt: driver metadata helper functions > fpga: xrt: xclbin file helper functions > fpga: xrt: xrt-lib driver manager > fpga: xrt: group driver > fpga: xrt: char dev node helper functions > fpga: xrt: root driver infrastructure > fpga: xrt: driver infrastructure > fpga: xrt: management physical function driver (root) > fpga: xrt: main driver for management function device > fpga: xrt: fpga-mgr and region implementation for xclbin download > fpga: xrt: VSEC driver > fpga: xrt: User Clock Subsystem driver > fpga: xrt: ICAP driver > fpga: xrt: devctl xrt driver > fpga: xrt: clock driver > fpga: xrt: clock frequency counter driver > fpga: xrt: DDR calibration driver > fpga: xrt: partition isolation driver > fpga: xrt: Kconfig and Makefile updates for XRT drivers > > Documentation/fpga/index.rst | 1 + > Documentation/fpga/xrt.rst | 870 ++++++++++++++++++ > MAINTAINERS | 11 + > drivers/Makefile | 1 + > drivers/fpga/Kconfig | 2 + > drivers/fpga/Makefile | 5 + > drivers/fpga/xrt/Kconfig | 8 + > drivers/fpga/xrt/include/events.h | 45 + > drivers/fpga/xrt/include/group.h | 25 + > drivers/fpga/xrt/include/metadata.h | 236 +++++ > drivers/fpga/xrt/include/subdev_id.h | 39 + > drivers/fpga/xrt/include/xclbin-helper.h | 48 + > drivers/fpga/xrt/include/xdevice.h | 131 +++ > drivers/fpga/xrt/include/xleaf.h | 205 +++++ > drivers/fpga/xrt/include/xleaf/axigate.h | 23 + > drivers/fpga/xrt/include/xleaf/clkfreq.h | 21 + > drivers/fpga/xrt/include/xleaf/clock.h | 29 + > .../fpga/xrt/include/xleaf/ddr_calibration.h | 28 + > drivers/fpga/xrt/include/xleaf/devctl.h | 40 + > drivers/fpga/xrt/include/xleaf/icap.h | 27 + > drivers/fpga/xrt/include/xmgmt-main.h | 34 + > drivers/fpga/xrt/include/xroot.h | 117 +++ > drivers/fpga/xrt/lib/Kconfig | 17 + > drivers/fpga/xrt/lib/Makefile | 30 + > drivers/fpga/xrt/lib/cdev.c | 209 +++++ > drivers/fpga/xrt/lib/group.c | 278 ++++++ > drivers/fpga/xrt/lib/lib-drv.c | 328 +++++++ > drivers/fpga/xrt/lib/lib-drv.h | 21 + > drivers/fpga/xrt/lib/subdev.c | 859 +++++++++++++++++ > drivers/fpga/xrt/lib/subdev_pool.h | 53 ++ > drivers/fpga/xrt/lib/xclbin.c | 381 ++++++++ > drivers/fpga/xrt/lib/xleaf/axigate.c | 325 +++++++ > drivers/fpga/xrt/lib/xleaf/clkfreq.c | 223 +++++ > drivers/fpga/xrt/lib/xleaf/clock.c | 652 +++++++++++++ > drivers/fpga/xrt/lib/xleaf/ddr_calibration.c | 210 +++++ > drivers/fpga/xrt/lib/xleaf/devctl.c | 169 ++++ > drivers/fpga/xrt/lib/xleaf/icap.c | 328 +++++++ > drivers/fpga/xrt/lib/xleaf/ucs.c | 152 +++ > drivers/fpga/xrt/lib/xleaf/vsec.c | 372 ++++++++ > drivers/fpga/xrt/lib/xroot.c | 536 +++++++++++ > drivers/fpga/xrt/metadata/Kconfig | 12 + > drivers/fpga/xrt/metadata/Makefile | 16 + > drivers/fpga/xrt/metadata/metadata.c | 578 ++++++++++++ > drivers/fpga/xrt/mgmt/Kconfig | 15 + > drivers/fpga/xrt/mgmt/Makefile | 19 + > drivers/fpga/xrt/mgmt/root.c | 420 +++++++++ > drivers/fpga/xrt/mgmt/xmgmt-main-region.c | 483 ++++++++++ > drivers/fpga/xrt/mgmt/xmgmt-main.c | 662 +++++++++++++ > drivers/fpga/xrt/mgmt/xmgmt.h | 33 + > drivers/fpga/xrt/mgmt/xrt-mgr.c | 190 ++++ > drivers/fpga/xrt/mgmt/xrt-mgr.h | 16 + > include/uapi/linux/xrt/xclbin.h | 409 ++++++++ > include/uapi/linux/xrt/xmgmt-ioctl.h | 46 + > 53 files changed, 9988 insertions(+) > create mode 100644 Documentation/fpga/xrt.rst > create mode 100644 drivers/fpga/xrt/Kconfig > create mode 100644 drivers/fpga/xrt/include/events.h > create mode 100644 drivers/fpga/xrt/include/group.h > create mode 100644 drivers/fpga/xrt/include/metadata.h > create mode 100644 drivers/fpga/xrt/include/subdev_id.h > create mode 100644 drivers/fpga/xrt/include/xclbin-helper.h > create mode 100644 drivers/fpga/xrt/include/xdevice.h > create mode 100644 drivers/fpga/xrt/include/xleaf.h > create mode 100644 drivers/fpga/xrt/include/xleaf/axigate.h > create mode 100644 drivers/fpga/xrt/include/xleaf/clkfreq.h > create mode 100644 drivers/fpga/xrt/include/xleaf/clock.h > create mode 100644 drivers/fpga/xrt/include/xleaf/ddr_calibration.h > create mode 100644 drivers/fpga/xrt/include/xleaf/devctl.h > create mode 100644 drivers/fpga/xrt/include/xleaf/icap.h > create mode 100644 drivers/fpga/xrt/include/xmgmt-main.h > create mode 100644 drivers/fpga/xrt/include/xroot.h > create mode 100644 drivers/fpga/xrt/lib/Kconfig > create mode 100644 drivers/fpga/xrt/lib/Makefile > create mode 100644 drivers/fpga/xrt/lib/cdev.c > create mode 100644 drivers/fpga/xrt/lib/group.c > create mode 100644 drivers/fpga/xrt/lib/lib-drv.c > create mode 100644 drivers/fpga/xrt/lib/lib-drv.h > create mode 100644 drivers/fpga/xrt/lib/subdev.c > create mode 100644 drivers/fpga/xrt/lib/subdev_pool.h > create mode 100644 drivers/fpga/xrt/lib/xclbin.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/axigate.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/clkfreq.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/clock.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/ddr_calibration.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/devctl.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/icap.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/ucs.c > create mode 100644 drivers/fpga/xrt/lib/xleaf/vsec.c > create mode 100644 drivers/fpga/xrt/lib/xroot.c > create mode 100644 drivers/fpga/xrt/metadata/Kconfig > create mode 100644 drivers/fpga/xrt/metadata/Makefile > create mode 100644 drivers/fpga/xrt/metadata/metadata.c > create mode 100644 drivers/fpga/xrt/mgmt/Kconfig > create mode 100644 drivers/fpga/xrt/mgmt/Makefile > create mode 100644 drivers/fpga/xrt/mgmt/root.c > create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-main-region.c > create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-main.c > create mode 100644 drivers/fpga/xrt/mgmt/xmgmt.h > create mode 100644 drivers/fpga/xrt/mgmt/xrt-mgr.c > create mode 100644 drivers/fpga/xrt/mgmt/xrt-mgr.h > create mode 100644 include/uapi/linux/xrt/xclbin.h > create mode 100644 include/uapi/linux/xrt/xmgmt-ioctl.h >
| |