Message-ID: <7290da0c-0e6f-81e3-e1b8-087536110569@redhat.com>
Date:   Fri, 16 Jul 2021 11:27:29 -0700
From:   Tom Rix <trix@...hat.com>
To:     Lizhi Hou <lizhi.hou@...inx.com>, linux-kernel@...r.kernel.org,
        "mdf@...nel.org" <mdf@...nel.org>
Cc:     linux-fpga@...r.kernel.org, maxz@...inx.com,
        sonal.santan@...inx.com, yliu@...inx.com, michal.simek@...inx.com,
        stefanos@...inx.com, devicetree@...r.kernel.org, robh@...nel.org
Subject: Re: [PATCH V7 XRT Alveo 00/20] XRT Alveo driver overview


On 7/1/21 1:32 PM, Tom Rix wrote:
> 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.

Lizhi

This pathset has stalled.

The spelling changes should be fixed.

Some of xleafs may need to be moved and reworked.

The addition of building outside of fpga/xrt is a new feature, and I 
think is best considered when the first foundational change (this 
patchset) is in tree.

Could the xleafs' below be dropped from this set ?

Tom

>
> 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
>>

Powered by blists - more mailing lists