Re: [PATCH V7 XRT Alveo 00/20] XRT Alveo driver overview

From: Tom Rix
Date: Thu Jul 01 2021 - 16:32:40 EST


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@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-2-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-3-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-4-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-5-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-6-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-7-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-8-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-9-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-10-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-11-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-12-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-13-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-14-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-15-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-16-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-17-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-18-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-19-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-20-lizhi.hou@xxxxxxxxxx/
https://lore.kernel.org/lkml/20210512015339.5649-21-lizhi.hou@xxxxxxxxxx/

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