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

[Tom Rix]

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