LinuxLists.cc - [PATCH V4 XRT Alveo Infrastructure 0/5] XRT Alveo driver infrastructure overview

[permalink] [raw]

Subject: [PATCH V4 XRT Alveo Infrastructure 2/5] Documentation: devicetree: bindings: add binding for Alveo platform

Create device tree binding document for partitions and pr isolation on
Xilinx Alveo platform.

Signed-off-by: Sonal Santan <[email protected]>
Signed-off-by: Max Zhen <[email protected]>
Signed-off-by: Lizhi Hou <[email protected]>
---
.../bindings/fpga/xlnx,alveo-partition.yaml | 76 +++++++++++++++++++
.../fpga/xlnx,alveo-pr-isolation.yaml | 40 ++++++++++
2 files changed, 116 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml

diff --git a/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml b/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
new file mode 100644
index 000000000000..ee50cb51d08e
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/xlnx,alveo-partition.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Alveo platform partition bindings
+
+description: |
+ Xilinx Alveo platform is a PCI device and has one or more partitions. A
+ partition is programmed dynamically and contains a set of hardware
+ peripherals also referred to as endpoints which appear on the PCI BARs.
+ This binding is defined for endpoint address translation which uses the
+ the following encoding:
+
+ 0xIooooooo 0xoooooooo
+
+ Where:
+
+ I = BAR index
+ oooooo oooooooo = BAR offset
+
+ As a PCI device, the Alveo platform is enumerated at runtime. Thus,
+ the partition node is created by Alveo device driver. The device driver
+ gets the BAR base address of the PCI device and creates the 'range'
+ property for address translation.
+
+allOf:
+ - $ref: /schemas/simple-bus.yaml#
+
+maintainers:
+ - Lizhi Hou <[email protected]>
+
+properties:
+ compatible:
+ contains:
+ const: xlnx,alveo-partition
+
+ "#address-cells":
+ const: 2
+
+ "#size-cells":
+ const: 2
+
+ ranges: true
+
+patternProperties:
+ "^.*@[0-9a-f]+$":
+ description: hardware endpoints belong to this partition.
+ type: object
+
+required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+ - ranges
+
+additionalProperties: false
+
+examples:
+ - |
+ bus {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ xrt-part-bus@0 {
+ compatible = "xlnx,alveo-partition", "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x0 0x0 0x0 0xe0000000 0x0 0x2000000
+ 0x20000000 0x0 0x0 0xe4200000 0x0 0x40000>;
+ pr-isolate-ulp@41000 {
+ compatible = "xlnx,alveo-pr-isolation";
+ reg = <0x0 0x41000 0 0x1000>;
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml b/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml
new file mode 100644
index 000000000000..8db949093ee0
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/xlnx,alveo-pr-isolation.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Partial Reconfig Isolation for Alveo platforms
+
+description: |
+ The Partial Reconfig ensures glitch free operation of the inputs from
+ a reconfigurable partition during partial reconfiguration on Alveo
+ platform.
+
+maintainers:
+ - Lizhi Hou <[email protected]>
+
+properties:
+ compatible:
+ const: xlnx,alveo-pr-isolation
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ bus {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pr-isolation-ulp@41000 {
+ compatible = "xlnx,alveo-pr-isolation";
+ reg = <0 0x41000 0 0x1000>;
+ };
+ };
--
2.27.0

2022-01-05 22:51:26

[permalink] [raw]

Subject: [PATCH V4 XRT Alveo Infrastructure 1/5] Documentation: fpga: Add a document describing XRT Alveo driver infrastructure

Describe XRT driver architecture and provide basic overview of
Xilinx Alveo platform.

Signed-off-by: Sonal Santan <[email protected]>
Signed-off-by: Max Zhen <[email protected]>
Signed-off-by: Lizhi Hou <[email protected]>
---
Documentation/fpga/index.rst | 1 +
Documentation/fpga/xrt.rst | 337 +++++++++++++++++++++++++++++++++++
MAINTAINERS | 10 ++
3 files changed, 348 insertions(+)
create mode 100644 Documentation/fpga/xrt.rst

diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
index f80f95667ca2..30134357b70d 100644
--- a/Documentation/fpga/index.rst
+++ b/Documentation/fpga/index.rst
@@ -8,6 +8,7 @@ fpga
:maxdepth: 1

dfl
+ xrt

.. only:: subproject and html

diff --git a/Documentation/fpga/xrt.rst b/Documentation/fpga/xrt.rst
new file mode 100644
index 000000000000..45d6f2e18af0
--- /dev/null
+++ b/Documentation/fpga/xrt.rst
@@ -0,0 +1,337 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==================================
+XRTV2 Linux Kernel Driver Overview
+==================================
+
+Authors:
+
+* Sonal Santan <[email protected]>
+* Max Zhen <[email protected]>
+* Lizhi Hou <[email protected]>
+
+XRTV2 drivers are second generation `XRT <https://github.com/Xilinx/XRT>`_
+drivers which support `Alveo <https://www.xilinx.com/products/boards-and-kits/alveo.html>`_
+PCIe platforms from Xilinx.
+
+XRTV2 drivers support *subsystem* style data driven platforms where the driver's
+configuration and behavior are determined by the metadata provided by the
+platform (in *device tree* format). Primary management physical function (MPF)
+driver is called **xrt-mgmt**. Primary user physical function (UPF) driver is
+called **xrt-user** and is under development. xrt_driver common APIs are packaged
+into a library module called **xrt-lib**, which is shared by **xrt-mgmt** and
+**xrt-user** (under development).
+
+Driver Modules
+==============
+
+xrt-lib.ko
+----------
+
+xrt-lib is the repository of functions that can potentially be shared between
+xrt-mgmt and xrt-user.
+
+Alveo platform consists of one or more FPGA partitions. Each partition has
+multiple HW peripherals (also referred to as endpoints) and metadata to describe
+the endpoints. This metadata is in flat device tree format. xrt-lib relies on OF
+kernel APIs to un-flatten the metadata and overlay the un-flattened device tree
+nodes to the system base device tree.
+
+xrt-mgmt.ko
+------------
+
+The xrt-mgmt driver is a PCIe device driver driving MPF found on Xilinx's Alveo
+PCIe device. It reads Alveo platform partition metadata and creates one or more
+partitions based on the hardware design. xrt-lib APIs are called to overlay the
+endpoint nodes to the system base tree. Eventually, platform devices are
+generated for each endpoint defined in the partition metadata.
+
+The xrt-mgmt driver uses xrt-lib APIs to manage the life cycle of partitions,
+which, in turn, manages multiple endpoints (platform devices) generated during
+partition creation. This flexibility allows xrt-mgmt.ko and xrt-lib.ko to support
+various HW subsystems exposed by different Alveo shells. The differences among
+these Alveo shells is handled in the endpoint (platform device) drivers.
+See :ref:`alveo_platform_overview`.
+
+The instantiation of a specific endpoint driver is completely data driven based
+on the metadata (in the device tree format). The flattened device tree is stored
+in a xsabin file which is discovered through the PCIe VSEC capability.
+
+
+Driver Object Model
+===================
+
+The system device tree after overlaying Alveo partitions looks like the
+following::
+
+ +-----------+
+ | of root |
+ +-----------+
+ |
+ +-------------------+-------------------+
+ | | |
+ v v v
+ +-------------+ +------------+ +---------+
+ | xrt-part0 | | xrt-partN | | |
+ |(simple-bus) | ... |(simple-bus)| | ... |
+ +-------------+ +------------+ +---------+
+ | |
+ | |
+ +-----+--------+ |
+ | | |
+ v v v
+ +-----------+ +-----------+ +------------+
+ |ep_foo@123 |..|ep_bar@456 | | ep_foo@789 |
+ +-----------+ +-----------+ +------------+
+
+partition node
+--------------
+
+The partition node is created and added to the system device tree when the driver
+creates a new partition. It is compatible with ``simple-bus`` which is a
+transparent bus node defined by Linux kernel. The partition node is used for
+translating the address of underneath endpoint to CPU address.
+
+endpoint node
+-------------
+
+During the partition creation, xrt driver un-flattens the partition metadata and
+adds all the endpoint nodes under the partition node to the system device tree.
+Eventually, all the endpoint nodes will be populated by the existing platform
+device and OF infrastructure. This means a platform device will be created for
+each endpoint node. The platform driver will be bound based on the ``compatible``
+property defined in the endpoint node.
+
+.. _alveo_platform_overview:
+
+Alveo Platform Overview
+=======================
+
+Alveo platforms are architected as two physical FPGA partitions: *Shell* and
+*User*. The Shell provides basic infrastructure for the Alveo platform like
+PCIe connectivity, board management, Dynamic Function Exchange (DFX), sensors,
+clocking, reset, and security. DFX, partial reconfiguration, is responsible for
+loading the user compiled FPGA binary.
+
+For DFX to work properly, physical partitions require strict HW compatibility
+with each other. Every physical partition has two interface UUIDs: the *parent*
+UUID and the *child* UUID. For simple single stage platforms, Shell → User forms
+the parent child relationship.
+
+.. note::
+ Partition compatibility matching is a key design component of the Alveo platforms
+ and XRT. Partitions have child and parent relationship. A loaded partition
+ exposes child partition UUID to advertise its compatibility requirement. When
+ loading a child partition, the xrt-mgmt driver matches the parent
+ UUID of the child partition against the child UUID exported by the parent.
+ The parent and child partition UUIDs are stored in the *xclbin* (for the user)
+ and the *xsabin* (for the shell). Except for the root UUID exported by VSEC,
+ the hardware itself does not know about the UUIDs. The UUIDs are stored in
+ xsabin and xclbin. The image format has a special node called Partition UUIDs
+ which define the compatibility UUIDs.
+
+
+The physical partitions and their loading are illustrated below::
+
+ SHELL USER
+ +-----------+ +-------------------+
+ | | | |
+ | VSEC UUID | CHILD PARENT | LOGIC UUID |
+ | o------->|<--------o |
+ | | UUID UUID | |
+ +-----+-----+ +--------+----------+
+ | |
+ . .
+ | |
+ +---+---+ +------+--------+
+ | POR | | USER COMPILED |
+ | FLASH | | XCLBIN |
+ +-------+ +---------------+
+
+
+Loading Sequence
+----------------
+
+The Shell partition is loaded from flash at system boot time. It establishes the
+PCIe link and exposes two physical functions to the BIOS. After the OS boots,
+the xrt-mgmt driver attaches to the PCIe physical function 0 exposed by the Shell
+and then looks for VSEC in the PCIe extended configuration space. Using VSEC, it
+determines the logic UUID of the Shell and uses the UUID to load matching *xsabin*
+file from Linux firmware directory. The xsabin file contains the metadata to
+discover the peripherals that are part of the Shell and the firmware for any
+embedded soft processors in the Shell. The xsabin file also contains Partition
+UUIDs.
+
+The Shell exports a child interface UUID which is used for the compatibility
+check when loading the user compiled xclbin over the User partition as part of DFX.
+When a user requests loading of a specific xclbin, the xrt-mgmt driver reads
+the parent interface UUID specified in the xclbin and matches it with the child
+interface UUID exported by the Shell to determine if the xclbin is compatible with
+the Shell. If the match fails, loading of xclbin is denied.
+
+xclbin loading is requested using the ICAP_DOWNLOAD_AXLF ioctl command. When loading
+a xclbin, the xrt-mgmt driver performs the following *logical* operations:
+
+1. Copy xclbin from user to kernel memory
+2. Sanity check the xclbin contents
+3. Isolate the User partition
+4. Download the bitstream using the FPGA config engine (ICAP)
+5. De-isolate the User partition
+6. Program the clocks (ClockWiz) driving the User partition
+7. Wait for the memory controller (MIG) calibration
+8. Return the loading status back to the caller
+
+`Platform Loading Overview <https://xilinx.github.io/XRT/master/html/platforms_partitions.html>`_
+provides more detailed information on platform loading.
+
+
+xsabin
+------
+
+Each Alveo platform comes packaged with its own xsabin. The xsabin is a trusted
+component of the platform. For format details refer to :ref:`xsabin_xclbin_container_format`
+below. xsabin contains basic information like UUIDs, platform name and metadata in the
+form of flat device tree.
+
+xclbin
+------
+
+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 an FPGA bitstream for the user partition, UUIDs, platform name, etc.
+
+
+.. _xsabin_xclbin_container_format:
+
+xsabin/xclbin Container Format
+------------------------------
+
+xclbin/xsabin is ELF-like binary container format. It is structured as series of
+sections. There is a file header followed by several section headers which is
+followed by sections. A section header points to an actual section. There is an
+optional signature at the end. The format is defined by the header file ``xclbin.h``.
+The following figure illustrates a typical xclbin::
+
+
+ +---------------------+
+ | |
+ | HEADER |
+ +---------------------+
+ | SECTION HEADER |
+ | |
+ +---------------------+
+ | ... |
+ | |
+ +---------------------+
+ | SECTION HEADER |
+ | |
+ +---------------------+
+ | SECTION |
+ | |
+ +---------------------+
+ | ... |
+ | |
+ +---------------------+
+ | SECTION |
+ | |
+ +---------------------+
+ | SIGNATURE |
+ | (OPTIONAL) |
+ +---------------------+
+
+
+xclbin/xsabin files can be packaged, un-packaged and inspected using an XRT
+utility called **xclbinutil**. xclbinutil is part of the XRT open source
+software stack. The source code for xclbinutil can be found at
+https://github.com/Xilinx/XRT/tree/master/src/runtime_src/tools/xclbinutil
+
+For example, to enumerate the contents of a xclbin/xsabin use the *--info* switch
+as shown below::
+
+
+ xclbinutil --info --input /opt/xilinx/firmware/u50/gen3x16-xdma/blp/test/bandwidth.xclbin
+ xclbinutil --info --input /lib/firmware/xilinx/862c7020a250293e32036f19956669e5/partition.xsabin
+
+Deployment Models
+=================
+
+Baremetal
+---------
+
+In bare-metal deployments, both MPF and UPF are visible and accessible. The
+xrt-mgmt driver binds to MPF. The xrt-mgmt driver operations are privileged and
+available to system administrator. The full stack is illustrated below::
+
+ HOST
+
+ [XRT-MGMT] [XRT-USER]
+ | |
+ | |
+ +-----+ +-----+
+ | MPF | | UPF |
+ | | | |
+ | PF0 | | PF1 |
+ +--+--+ +--+--+
+ ......... ^................. ^..........
+ | |
+ | PCIe DEVICE |
+ | |
+ +--+------------------+--+
+ | SHELL |
+ | |
+ +------------------------+
+ | USER |
+ | |
+ | |
+ | |
+ | |
+ +------------------------+
+
+
+
+Virtualized
+-----------
+
+In virtualized deployments, the privileged MPF is assigned to the host but the
+unprivileged UPF is assigned to a guest VM via PCIe pass-through. The xrt-mgmt
+driver in host binds to MPF. The xrt-mgmt driver operations are privileged and
+only accessible to the MPF. The full stack is illustrated below::
+
+
+ ..............
+ HOST . VM .
+ . .
+ [XRT-MGMT] . [XRT-USER] .
+ | . | .
+ | . | .
+ +-----+ . +-----+ .
+ | MPF | . | UPF | .
+ | | . | | .
+ | PF0 | . | PF1 | .
+ +--+--+ . +--+--+ .
+ ......... ^................. ^..........
+ | |
+ | PCIe DEVICE |
+ | |
+ +--+------------------+--+
+ | SHELL |
+ | |
+ +------------------------+
+ | USER |
+ | |
+ | |
+ | |
+ | |
+ +------------------------+
+
+
+
+
+
+Platform Security Considerations
+================================
+
+`Security of Alveo Platform <https://xilinx.github.io/XRT/master/html/security.html>`_
+discusses the deployment options and security implications in great detail.
diff --git a/MAINTAINERS b/MAINTAINERS
index 80eebc1d9ed5..fd7053bcfdb0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7369,6 +7369,16 @@ F: Documentation/fpga/
F: drivers/fpga/
F: include/linux/fpga/

+FPGA XRT DRIVERS
+M: Lizhi Hou <[email protected]>
+R: Max Zhen <[email protected]>
+R: Sonal Santan <[email protected]>
+L: [email protected]
+S: Supported
+W: https://github.com/Xilinx/XRT
+F: Documentation/fpga/xrt.rst
+F: drivers/fpga/xrt/
+
FPU EMULATOR
M: Bill Metzenthen <[email protected]>
S: Maintained
--
2.27.0

2022-01-05 22:52:11

[permalink] [raw]

Subject: [PATCH V4 XRT Alveo Infrastructure 4/5] fpga: xrt: xrt-lib common interfaces

The Alveo platform has to PCI fucntions. Each function has its own driver
attached. The common interfaces are created to support both drivers.

Signed-off-by: Sonal Santan <[email protected]>
Signed-off-by: Max Zhen <[email protected]>
Signed-off-by: Lizhi Hou <[email protected]>
---
drivers/fpga/Kconfig | 3 +
drivers/fpga/Makefile | 3 +
drivers/fpga/xrt/Kconfig | 6 +
drivers/fpga/xrt/include/xpartition.h | 28 ++++
drivers/fpga/xrt/lib/Kconfig | 17 +++
drivers/fpga/xrt/lib/Makefile | 15 +++
drivers/fpga/xrt/lib/lib-drv.c | 178 ++++++++++++++++++++++++++
drivers/fpga/xrt/lib/lib-drv.h | 15 +++
8 files changed, 265 insertions(+)
create mode 100644 drivers/fpga/xrt/Kconfig
create mode 100644 drivers/fpga/xrt/include/xpartition.h
create mode 100644 drivers/fpga/xrt/lib/Kconfig
create mode 100644 drivers/fpga/xrt/lib/Makefile
create mode 100644 drivers/fpga/xrt/lib/lib-drv.c
create mode 100644 drivers/fpga/xrt/lib/lib-drv.h

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 991b3f361ec9..93ae387c97c5 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -243,4 +243,7 @@ config FPGA_MGR_VERSAL_FPGA
configure the programmable logic(PL).

To compile this as a module, choose M here.
+
+source "drivers/fpga/xrt/Kconfig"
+
endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 0bff783d1b61..5bd41cf4c7ec 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -49,3 +49,6 @@ obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += dfl-n3000-nios.o

# Drivers for FPGAs which implement DFL
obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
+
+# XRT drivers for Alveo
+obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
new file mode 100644
index 000000000000..04c3bb5aaf4f
--- /dev/null
+++ b/drivers/fpga/xrt/Kconfig
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Xilinx Alveo FPGA device configuration
+#
+
+source "drivers/fpga/xrt/lib/Kconfig"
diff --git a/drivers/fpga/xrt/include/xpartition.h b/drivers/fpga/xrt/include/xpartition.h
new file mode 100644
index 000000000000..d72090ddfbee
--- /dev/null
+++ b/drivers/fpga/xrt/include/xpartition.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2022 Xilinx, Inc.
+ *
+ * Authors:
+ * Lizhi Hou <[email protected]>
+ */
+
+#ifndef _XRT_PARTITION_H_
+#define _XRT_PARTITION_H_
+
+struct xrt_partition_range {
+ u32 bar_idx;
+ u64 base;
+ u64 size;
+};
+
+struct xrt_partition_info {
+ int num_range;
+ struct xrt_partition_range *ranges;
+ void *fdt;
+ u32 fdt_len;
+};
+
+int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle);
+void xrt_partition_destroy(void *handle);
+
+#endif
diff --git a/drivers/fpga/xrt/lib/Kconfig b/drivers/fpga/xrt/lib/Kconfig
new file mode 100644
index 000000000000..73de1f50d5c6
--- /dev/null
+++ b/drivers/fpga/xrt/lib/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# XRT Alveo FPGA device configuration
+#
+
+config FPGA_XRT_LIB
+ tristate "XRT Alveo Driver Library"
+ depends on HWMON && PCI && HAS_IOMEM && OF
+ select REGMAP_MMIO
+ select OF_OVERLAY
+ help
+ Select this option to enable Xilinx XRT Alveo driver library. This
+ library is core infrastructure of XRT Alveo FPGA drivers which
+ provides functions for working with device nodes, iteration and
+ lookup of platform devices, common interfaces for platform devices,
+ plumbing of function call and ioctls between platform devices and
+ parent partitions.
diff --git a/drivers/fpga/xrt/lib/Makefile b/drivers/fpga/xrt/lib/Makefile
new file mode 100644
index 000000000000..698877c39657
--- /dev/null
+++ b/drivers/fpga/xrt/lib/Makefile
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
+#
+# Authors: [email protected]
+#
+
+FULL_XRT_PATH=$(srctree)/$(src)/..
+
+obj-$(CONFIG_FPGA_XRT_LIB) += xrt-lib.o
+
+xrt-lib-objs := \
+ lib-drv.o
+
+ccflags-y := -I$(FULL_XRT_PATH)/include
diff --git a/drivers/fpga/xrt/lib/lib-drv.c b/drivers/fpga/xrt/lib/lib-drv.c
new file mode 100644
index 000000000000..56334b2b9bec
--- /dev/null
+++ b/drivers/fpga/xrt/lib/lib-drv.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020-2022 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen <[email protected]>
+ * Lizhi Hou <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include "xpartition.h"
+#include "lib-drv.h"
+
+#define XRT_PARTITION_FDT_ALIGN 8
+#define XRT_PARTITION_NAME_LEN 64
+
+struct xrt_partition {
+ struct device *dev;
+ u32 id;
+ char name[XRT_PARTITION_NAME_LEN];
+ void *fdt;
+ struct property ranges;
+ struct of_changeset chgset;
+ bool chgset_applied;
+ void *dn_mem;
+};
+
+DEFINE_IDA(xrt_partition_id);
+
+static int xrt_partition_set_ranges(struct xrt_partition *xp, struct xrt_partition_range *ranges,
+ int num_range)
+{
+ __be64 *prop;
+ u32 prop_len;
+ int i;
+
+ prop_len = num_range * (sizeof(u64) * 3);
+ prop = kzalloc(prop_len, GFP_KERNEL);
+ if (!prop)
+ return -ENOMEM;
+
+ xp->ranges.name = "ranges";
+ xp->ranges.length = prop_len;
+ xp->ranges.value = prop;
+
+ for (i = 0; i < num_range; i++) {
+ *prop = cpu_to_be64((u64)ranges[i].bar_idx << 60);
+ prop++;
+ *prop = cpu_to_be64(ranges[i].base);
+ prop++;
+ *prop = cpu_to_be64(ranges[i].size);
+ prop++;
+ }
+
+ return 0;
+}
+
+void xrt_partition_destroy(void *handle)
+{
+ struct xrt_partition *xp = handle;
+
+ if (xp->chgset_applied)
+ of_changeset_revert(&xp->chgset);
+ of_changeset_destroy(&xp->chgset);
+
+ ida_free(&xrt_partition_id, xp->id);
+ kfree(xp->dn_mem);
+ kfree(xp->fdt);
+ kfree(xp->ranges.value);
+ kfree(xp);
+}
+EXPORT_SYMBOL_GPL(xrt_partition_destroy);
+
+int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle)
+{
+ struct device_node *parent_dn = NULL, *dn, *part_dn;
+ struct xrt_partition *xp = NULL;
+ void *fdt_aligned;
+ int ret;
+
+ xp = kzalloc(sizeof(*xp), GFP_KERNEL);
+ if (!xp)
+ return -ENOMEM;
+
+ ret = ida_alloc(&xrt_partition_id, GFP_KERNEL);
+ if (ret < 0) {
+ dev_err(dev, "alloc id failed, ret %d", ret);
+ kfree(xp);
+ return ret;
+ }
+ xp->id = ret;
+ of_changeset_init(&xp->chgset);
+
+ parent_dn = of_find_node_by_path("/");
+ if (!parent_dn) {
+ dev_err(dev, "did not find xrt node");
+ ret = -EINVAL;
+ goto failed;
+ }
+
+ xp->dev = dev;
+ snprintf(xp->name, XRT_PARTITION_NAME_LEN, "xrt-part@%x", xp->id);
+ ret = xrt_partition_set_ranges(xp, info->ranges, info->num_range);
+ if (ret)
+ goto failed;
+
+ xp->fdt = kmalloc(info->fdt_len + XRT_PARTITION_FDT_ALIGN, GFP_KERNEL);
+ if (!xp->fdt) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+ fdt_aligned = PTR_ALIGN(xp->fdt, XRT_PARTITION_FDT_ALIGN);
+ memcpy(fdt_aligned, info->fdt, info->fdt_len);
+
+ xp->dn_mem = of_fdt_unflatten_tree(fdt_aligned, NULL, &part_dn);
+ if (!xp->dn_mem) {
+ ret = -EINVAL;
+ goto failed;
+ }
+
+ of_node_get(part_dn);
+ part_dn->full_name = xp->name;
+ part_dn->parent = parent_dn;
+ for (dn = part_dn; dn; dn = of_find_all_nodes(dn))
+ of_changeset_attach_node(&xp->chgset, dn);
+
+ ret = of_changeset_add_property(&xp->chgset, part_dn, &xp->ranges);
+ if (ret) {
+ dev_err(dev, "failed to add property, ret %d", ret);
+ goto failed;
+ }
+
+ ret = of_changeset_apply(&xp->chgset);
+ if (ret) {
+ dev_err(dev, "failed to apply changeset, ret %d", ret);
+ goto failed;
+ }
+ xp->chgset_applied = true;
+ of_node_put(parent_dn);
+
+ ret = of_platform_populate(part_dn, NULL, NULL, dev);
+ if (ret) {
+ dev_err(dev, "failed to populate devices, ret %d", ret);
+ goto failed;
+ }
+
+ *handle = xp;
+ return 0;
+
+failed:
+ if (parent_dn)
+ of_node_put(parent_dn);
+ xrt_partition_destroy(xp);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_partition_create);
+
+static __init int xrt_lib_init(void)
+{
+ return 0;
+}
+
+static __exit void xrt_lib_fini(void)
+{
+}
+
+module_init(xrt_lib_init);
+module_exit(xrt_lib_fini);
+
+MODULE_AUTHOR("XRT Team <[email protected]>");
+MODULE_DESCRIPTION("Xilinx Alveo IP Lib driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/xrt/lib/lib-drv.h b/drivers/fpga/xrt/lib/lib-drv.h
new file mode 100644
index 000000000000..77ed5c399dcf
--- /dev/null
+++ b/drivers/fpga/xrt/lib/lib-drv.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2022 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen <[email protected]>
+ */
+
+#ifndef _LIB_DRV_H_
+#define _LIB_DRV_H_
+
+extern u8 __dtb_xrt_begin[];
+extern u8 __dtb_xrt_end[];
+
+#endif /* _LIB_DRV_H_ */
--
2.27.0

2022-01-05 22:52:42

[permalink] [raw]

Subject: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

When OF_FLATTREE is selected and there is not a device tree, create an
empty device tree root node. of/unittest.c code is referenced.

Signed-off-by: Sonal Santan <[email protected]>
Signed-off-by: Max Zhen <[email protected]>
Signed-off-by: Lizhi Hou <[email protected]>
---
drivers/of/Makefile | 5 +++
drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
drivers/of/fdt_default.dts | 5 +++
drivers/of/of_private.h | 17 +++++++
drivers/of/unittest.c | 72 ++----------------------------
5 files changed, 120 insertions(+), 69 deletions(-)
create mode 100644 drivers/of/fdt_default.dts

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c13b982084a3..a2989055c578 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
obj-y = base.o device.o platform.o property.o
+
obj-$(CONFIG_OF_KOBJ) += kobj.o
obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
obj-$(CONFIG_OF_FLATTREE) += fdt.o
@@ -20,4 +21,8 @@ obj-y += kexec.o
endif
endif

+ifndef CONFIG_OF_UNITTEST
+obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
+endif
+
obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4546572af24b..66ef9ac97829 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
}
EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);

+static int __init of_fdt_root_init(void)
+{
+ struct device_node *dt = NULL, *np;
+ void *fdt = NULL, *fdt_aligned;
+ struct property *prop = NULL;
+ __be32 *val = NULL;
+ int size, rc = 0;
+
+#if !defined(CONFIG_OF_UNITTEST)
+ if (of_root)
+ return 0;
+#endif
+ size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
+
+ fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
+ if (!fdt)
+ return -ENOMEM;
+
+ fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
+ memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
+
+ if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
+ NULL, &dt)) {
+ pr_warn("%s: unflatten default tree failed\n", __func__);
+ kfree(fdt);
+ return -ENODATA;
+ }
+ if (!dt) {
+ pr_warn("%s: empty default tree\n", __func__);
+ kfree(fdt);
+ return -ENODATA;
+ }
+
+ /*
+ * This lock normally encloses of_resolve_phandles()
+ */
+ of_overlay_mutex_lock();
+
+ rc = of_resolve_phandles(dt);
+ if (rc) {
+ pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
+ goto failed;
+ }
+
+ if (!of_root) {
+ prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ rc = -ENOMEM;
+ goto failed;
+ }
+ val = kzalloc(sizeof(*val), GFP_KERNEL);
+ if (!val) {
+ rc = -ENOMEM;
+ goto failed;
+ }
+ *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
+
+ prop->name = "#address-cells";
+ prop->value = val;
+ prop->length = sizeof(u32);
+ of_add_property(dt, prop);
+ prop++;
+ prop->name = "#size-cells";
+ prop->value = val;
+ prop->length = sizeof(u32);
+ of_add_property(dt, prop);
+ of_root = dt;
+ for_each_of_allnodes(np)
+ __of_attach_node_sysfs(np);
+ of_aliases = of_find_node_by_path("/aliases");
+ of_chosen = of_find_node_by_path("/chosen");
+ of_overlay_mutex_unlock();
+pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
+ return 0;
+ }
+
+ unittest_data_add(dt);
+
+ of_overlay_mutex_unlock();
+
+ return 0;
+
+failed:
+ of_overlay_mutex_unlock();
+ kfree(val);
+ kfree(prop);
+ return rc;
+}
+pure_initcall(of_fdt_root_init);
+
/* Everything below here references initial_boot_params directly. */
int __initdata dt_root_addr_cells;
int __initdata dt_root_size_cells;
diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
new file mode 100644
index 000000000000..d1f12a76dfc6
--- /dev/null
+++ b/drivers/of/fdt_default.dts
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+/ {
+};
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 631489f7f8c0..1ef93bccfdba 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -41,6 +41,18 @@ extern struct mutex of_mutex;
extern struct list_head aliases_lookup;
extern struct kset *of_kset;

+#if defined(CONFIG_OF_UNITTEST)
+extern u8 __dtb_testcases_begin[];
+extern u8 __dtb_testcases_end[];
+#define __dtb_fdt_default_begin __dtb_testcases_begin
+#define __dtb_fdt_default_end __dtb_testcases_end
+void __init unittest_data_add(struct device_node *dt);
+#else
+extern u8 __dtb_fdt_default_begin[];
+extern u8 __dtb_fdt_default_end[];
+static inline void unittest_data_add(struct device_node *dt) {}
+#endif
+
#if defined(CONFIG_OF_DYNAMIC)
extern int of_property_notify(int action, struct device_node *np,
struct property *prop, struct property *old_prop);
@@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}

#if defined(CONFIG_OF_RESOLVE)
int of_resolve_phandles(struct device_node *tree);
+#else
+static inline int of_resolve_phandles(struct device_node *tree)
+{
+ return 0;
+}
#endif

void __of_phandle_cache_inv_entry(phandle handle);
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 8c056972a6dd..745f455235cc 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
* unittest_data_add - Reads, copies data from
* linked tree and attaches it to the live tree
*/
-static int __init unittest_data_add(void)
+void __init unittest_data_add(struct device_node *dt)
{
- void *unittest_data;
- void *unittest_data_align;
- struct device_node *unittest_data_node = NULL, *np;
- /*
- * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
- * created by cmd_dt_S_dtb in scripts/Makefile.lib
- */
- extern uint8_t __dtb_testcases_begin[];
- extern uint8_t __dtb_testcases_end[];
- const int size = __dtb_testcases_end - __dtb_testcases_begin;
- int rc;
- void *ret;
-
- if (!size) {
- pr_warn("%s: testcases is empty\n", __func__);
- return -ENODATA;
- }
-
- /* creating copy */
- unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
- if (!unittest_data)
- return -ENOMEM;
-
- unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
- memcpy(unittest_data_align, __dtb_testcases_begin, size);
-
- ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
- if (!ret) {
- pr_warn("%s: unflatten testcases tree failed\n", __func__);
- kfree(unittest_data);
- return -ENODATA;
- }
- if (!unittest_data_node) {
- pr_warn("%s: testcases tree is empty\n", __func__);
- kfree(unittest_data);
- return -ENODATA;
- }
-
- /*
- * This lock normally encloses of_resolve_phandles()
- */
- of_overlay_mutex_lock();
-
- rc = of_resolve_phandles(unittest_data_node);
- if (rc) {
- pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
- of_overlay_mutex_unlock();
- return -EINVAL;
- }
-
- if (!of_root) {
- of_root = unittest_data_node;
- for_each_of_allnodes(np)
- __of_attach_node_sysfs(np);
- of_aliases = of_find_node_by_path("/aliases");
- of_chosen = of_find_node_by_path("/chosen");
- of_overlay_mutex_unlock();
- return 0;
- }
+ struct device_node *np;

EXPECT_BEGIN(KERN_INFO,
"Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");

/* attach the sub-tree to live tree */
- np = unittest_data_node->child;
+ np = dt->child;
while (np) {
struct device_node *next = np->sibling;

@@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)

EXPECT_END(KERN_INFO,
"Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
-
- of_overlay_mutex_unlock();
-
- return 0;
}

#ifdef CONFIG_OF_OVERLAY
@@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
static int __init of_unittest(void)
{
struct device_node *np;
- int res;

pr_info("start of unittest - you will see error messages\n");

@@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
if (IS_ENABLED(CONFIG_UML))
unittest_unflatten_overlay_base();

- res = unittest_data_add();
- if (res)
- return res;
if (!of_aliases)
of_aliases = of_find_node_by_path("/aliases");

--
2.27.0

2022-01-05 22:53:02

[permalink] [raw]

Subject: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

The PCIE device driver which attaches to management function on Alveo
devices. It instantiates one or more partition. Each partition consists
a set of hardward endpoints. A flat device tree is associated with each
partition. The first version of this driver uses test version flat device
tree and call xrt lib API to unflatten it.

Signed-off-by: Sonal Santan <[email protected]>
Signed-off-by: Max Zhen <[email protected]>
Signed-off-by: Lizhi Hou <[email protected]>
---
drivers/fpga/Makefile | 1 +
drivers/fpga/xrt/Kconfig | 1 +
drivers/fpga/xrt/mgmt/Kconfig | 14 +++
drivers/fpga/xrt/mgmt/Makefile | 16 +++
drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
7 files changed, 217 insertions(+)
create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
create mode 100644 drivers/fpga/xrt/mgmt/Makefile
create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c

diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 5bd41cf4c7ec..544e2144878f 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o

# XRT drivers for Alveo
obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
+obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
index 04c3bb5aaf4f..50422f77c6df 100644
--- a/drivers/fpga/xrt/Kconfig
+++ b/drivers/fpga/xrt/Kconfig
@@ -4,3 +4,4 @@
#

source "drivers/fpga/xrt/lib/Kconfig"
+source "drivers/fpga/xrt/mgmt/Kconfig"
diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
new file mode 100644
index 000000000000..a978747482be
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Xilinx XRT FPGA device configuration
+#
+
+config FPGA_XRT_XMGMT
+ tristate "Xilinx Alveo Management Driver"
+ depends on FPGA_XRT_LIB
+ select FPGA_BRIDGE
+ select FPGA_REGION
+ help
+ Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
+ This driver provides interfaces for userspace application to access
+ Alveo FPGA device.
diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
new file mode 100644
index 000000000000..c5134bf71cca
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/Makefile
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
+#
+# Authors: [email protected]
+#
+
+FULL_XRT_PATH=$(srctree)/$(src)/..
+
+obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
+
+xrt-mgmt-objs := \
+ xmgmt-drv.o \
+ dt-test.dtb.o
+
+ccflags-y := -I$(FULL_XRT_PATH)/include
diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
new file mode 100644
index 000000000000..68dbcb7fd79d
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/dt-test.dts
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+/ {
+ compatible = "xlnx,alveo-partition", "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ pr_isolate_ulp@0,41000 {
+ compatible = "xlnx,alveo-pr-isolation";
+ reg = <0x0 0x41000 0x0 0x1000>;
+ };
+};
diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
new file mode 100644
index 000000000000..6ec4203afbd2
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/dt-test.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2022 Xilinx, Inc.
+ *
+ * Authors:
+ * Lizhi Hou <[email protected]>
+ */
+
+#ifndef _DT_TEST_H_
+#define _DT_TEST_H_
+
+extern u8 __dtb_dt_test_begin[];
+extern u8 __dtb_dt_test_end[];
+
+#endif /* _DT_TEST_H_ */
diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
new file mode 100644
index 000000000000..87abe5b86e0b
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo Management Function Driver
+ *
+ * Copyright (C) 2020-2022 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen <[email protected]>
+ * Lizhi Hou <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/aer.h>
+#include <linux/vmalloc.h>
+#include <linux/delay.h>
+#include "xpartition.h"
+#include "dt-test.h"
+
+#define XMGMT_MODULE_NAME "xrt-mgmt"
+
+#define XMGMT_PDEV(xm) ((xm)->pdev)
+#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
+#define xmgmt_err(xm, fmt, args...) \
+ dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
+#define xmgmt_warn(xm, fmt, args...) \
+ dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
+#define xmgmt_info(xm, fmt, args...) \
+ dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
+#define xmgmt_dbg(xm, fmt, args...) \
+ dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
+#define XMGMT_DEV_ID(_pcidev) \
+ ({ typeof(_pcidev) (pcidev) = (_pcidev); \
+ ((pci_domain_nr((pcidev)->bus) << 16) | \
+ PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
+
+#define XRT_MAX_READRQ 512
+
+/* PCI Device IDs */
+#define PCI_DEVICE_ID_U50 0x5020
+static const struct pci_device_id xmgmt_pci_ids[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
+ { 0, }
+};
+
+struct xmgmt {
+ struct pci_dev *pdev;
+ void *base_partition;
+
+ bool ready;
+};
+
+static int xmgmt_config_pci(struct xmgmt *xm)
+{
+ struct pci_dev *pdev = XMGMT_PDEV(xm);
+ int rc;
+
+ rc = pcim_enable_device(pdev);
+ if (rc < 0) {
+ xmgmt_err(xm, "failed to enable device: %d", rc);
+ return rc;
+ }
+
+ rc = pci_enable_pcie_error_reporting(pdev);
+ if (rc)
+ xmgmt_warn(xm, "failed to enable AER: %d", rc);
+
+ pci_set_master(pdev);
+
+ rc = pcie_get_readrq(pdev);
+ if (rc > XRT_MAX_READRQ)
+ pcie_set_readrq(pdev, XRT_MAX_READRQ);
+ return 0;
+}
+
+static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
+ struct xrt_partition_info xp_info = { 0 };
+ struct device *dev = &pdev->dev;
+ int ret, i, idx = 0;
+ struct xmgmt *xm;
+
+ xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
+ if (!xm)
+ return -ENOMEM;
+ xm->pdev = pdev;
+ pci_set_drvdata(pdev, xm);
+
+ ret = xmgmt_config_pci(xm);
+ if (ret)
+ goto failed;
+
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+ if (pci_resource_len(pdev, i) > 0) {
+ ranges[idx].bar_idx = i;
+ ranges[idx].base = pci_resource_start(pdev, i);
+ ranges[idx].size = pci_resource_len(pdev, i);
+ idx++;
+ }
+ }
+ xp_info.num_range = idx;
+ xp_info.ranges = ranges;
+ xp_info.fdt = __dtb_dt_test_begin;
+ xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
+ ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
+ if (ret)
+ goto failed;
+
+ xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
+ return 0;
+
+failed:
+ if (xm->base_partition)
+ xrt_partition_destroy(xm->base_partition);
+ pci_set_drvdata(pdev, NULL);
+ return ret;
+}
+
+static void xmgmt_remove(struct pci_dev *pdev)
+{
+ struct xmgmt *xm = pci_get_drvdata(pdev);
+
+ xrt_partition_destroy(xm->base_partition);
+ pci_disable_pcie_error_reporting(xm->pdev);
+ xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
+}
+
+static struct pci_driver xmgmt_driver = {
+ .name = XMGMT_MODULE_NAME,
+ .id_table = xmgmt_pci_ids,
+ .probe = xmgmt_probe,
+ .remove = xmgmt_remove,
+};
+
+static int __init xmgmt_init(void)
+{
+ int res = 0;
+
+ res = pci_register_driver(&xmgmt_driver);
+ if (res)
+ return res;
+
+ return 0;
+}
+
+static __exit void xmgmt_exit(void)
+{
+ pci_unregister_driver(&xmgmt_driver);
+}
+
+module_init(xmgmt_init);
+module_exit(xmgmt_exit);
+
+MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
+MODULE_AUTHOR("XRT Team <[email protected]>");
+MODULE_DESCRIPTION("Xilinx Alveo management function driver");
+MODULE_LICENSE("GPL v2");
--
2.27.0

2022-01-08 03:35:38

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

Hi Lizhi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linux/master linus/master v5.16-rc8 next-20220107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Lizhi-Hou/XRT-Alveo-driver-infrastructure-overview/20220106-065312
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20220105 (https://download.01.org/0day-ci/archive/20220108/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

New smatch warnings:
drivers/of/fdt.c:541 of_fdt_root_init() warn: inconsistent indenting

Old smatch warnings:
drivers/of/fdt.c:1313 early_init_dt_add_memory_arch() warn: unsigned 'base + size' is never less than zero.
drivers/of/fdt.c:1318 early_init_dt_add_memory_arch() warn: unsigned 'base' is never less than zero.
drivers/of/fdt.c:1428 unflatten_and_copy_device_tree() warn: should '1 << (((__builtin_constant_p((((((7 * 4) + 4) + 4) + 4)) - 1)) ?((((((((7 * 4) + 4) + 4) + 4)) - 1) < 2) ?0:63 - __builtin_clzll((((((7 * 4) + 4) + 4) + 4)) - 1)):((4 <= 4)) ?__ilog2_u32((((((7 * 4) + 4) + 4) + 4)) - 1):__ilog2_u64((((((7 * 4) + 4) + 4) + 4)) - 1)) + 1)' be a 64 bit type?

vim +541 drivers/of/fdt.c

468
469 static int __init of_fdt_root_init(void)
470 {
471 struct device_node *dt = NULL, *np;
472 void *fdt = NULL, *fdt_aligned;
473 struct property *prop = NULL;
474 __be32 *val = NULL;
475 int size, rc = 0;
476
477 #if !defined(CONFIG_OF_UNITTEST)
478 if (of_root)
479 return 0;
480 #endif
481 size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
482
483 fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
484 if (!fdt)
485 return -ENOMEM;
486
487 fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
488 memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
489
490 if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
491 NULL, &dt)) {
492 pr_warn("%s: unflatten default tree failed\n", __func__);
493 kfree(fdt);
494 return -ENODATA;
495 }
496 if (!dt) {
497 pr_warn("%s: empty default tree\n", __func__);
498 kfree(fdt);
499 return -ENODATA;
500 }
501
502 /*
503 * This lock normally encloses of_resolve_phandles()
504 */
505 of_overlay_mutex_lock();
506
507 rc = of_resolve_phandles(dt);
508 if (rc) {
509 pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
510 goto failed;
511 }
512
513 if (!of_root) {
514 prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
515 if (!prop) {
516 rc = -ENOMEM;
517 goto failed;
518 }
519 val = kzalloc(sizeof(*val), GFP_KERNEL);
520 if (!val) {
521 rc = -ENOMEM;
522 goto failed;
523 }
524 *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
525
526 prop->name = "#address-cells";
527 prop->value = val;
528 prop->length = sizeof(u32);
529 of_add_property(dt, prop);
530 prop++;
531 prop->name = "#size-cells";
532 prop->value = val;
533 prop->length = sizeof(u32);
534 of_add_property(dt, prop);
535 of_root = dt;
536 for_each_of_allnodes(np)
537 __of_attach_node_sysfs(np);
538 of_aliases = of_find_node_by_path("/aliases");
539 of_chosen = of_find_node_by_path("/chosen");
540 of_overlay_mutex_unlock();
> 541 pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
542 return 0;
543 }
544
545 unittest_data_add(dt);
546
547 of_overlay_mutex_unlock();
548
549 return 0;
550
551 failed:
552 of_overlay_mutex_unlock();
553 kfree(val);
554 kfree(prop);
555 return rc;
556 }
557 pure_initcall(of_fdt_root_init);
558

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2022-01-09 17:38:24

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 1/5] Documentation: fpga: Add a document describing XRT Alveo driver infrastructure

On 1/5/22 2:50 PM, Lizhi Hou wrote:
> Describe XRT driver architecture and provide basic overview of
> Xilinx Alveo platform.
>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> Documentation/fpga/index.rst | 1 +
> Documentation/fpga/xrt.rst | 337 +++++++++++++++++++++++++++++++++++
> MAINTAINERS | 10 ++
> 3 files changed, 348 insertions(+)
> create mode 100644 Documentation/fpga/xrt.rst
>
> diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
> index f80f95667ca2..30134357b70d 100644
> --- a/Documentation/fpga/index.rst
> +++ b/Documentation/fpga/index.rst
> @@ -8,6 +8,7 @@ fpga
> :maxdepth: 1
>
> dfl
> + xrt
>
> .. only:: subproject and html
>
> diff --git a/Documentation/fpga/xrt.rst b/Documentation/fpga/xrt.rst
> new file mode 100644
> index 000000000000..45d6f2e18af0
> --- /dev/null
> +++ b/Documentation/fpga/xrt.rst
> @@ -0,0 +1,337 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==================================
> +XRTV2 Linux Kernel Driver Overview
> +==================================
> +
> +Authors:
> +
> +* Sonal Santan <[email protected]>
> +* Max Zhen <[email protected]>
> +* Lizhi Hou <[email protected]>
> +
> +XRTV2 drivers are second generation `XRT <https://github.com/Xilinx/XRT>`_
> +drivers which support `Alveo <https://www.xilinx.com/products/boards-and-kits/alveo.html>`_
> +PCIe platforms from Xilinx.
> +
> +XRTV2 drivers support *subsystem* style data driven platforms where the driver's
> +configuration and behavior are determined by the metadata provided by the
> +platform (in *device tree* format). Primary management physical function (MPF)
> +driver is called **xrt-mgmt**. Primary user physical function (UPF) driver is
> +called **xrt-user** and is under development. xrt_driver common APIs are packaged
> +into a library module called **xrt-lib**, which is shared by **xrt-mgmt** and
> +**xrt-user** (under development).
> +
> +Driver Modules
> +==============
> +
> +xrt-lib.ko
> +----------
> +
> +xrt-lib is the repository of functions that can potentially be shared between
> +xrt-mgmt and xrt-user.
> +
> +Alveo platform consists of one or more FPGA partitions. Each partition has
> +multiple HW peripherals (also referred to as endpoints) and metadata to describe
> +the endpoints. This metadata is in flat device tree format. xrt-lib relies on OF
> +kernel APIs to un-flatten the metadata and overlay the un-flattened device tree
> +nodes to the system base device tree.
to -> onto
> +
> +xrt-mgmt.ko
> +------------
> +
> +The xrt-mgmt driver is a PCIe device driver driving MPF found on Xilinx's Alveo
driving -> for a
> +PCIe device. It reads Alveo platform partition metadata and creates one or more
> +partitions based on the hardware design. xrt-lib APIs are called to overlay the
> +endpoint nodes to the system base tree. Eventually, platform devices are
> +generated for each endpoint defined in the partition metadata.
> +
> +The xrt-mgmt driver uses xrt-lib APIs to manage the life cycle of partitions,
> +which, in turn, manages multiple endpoints (platform devices) generated during
> +partition creation. This flexibility allows xrt-mgmt.ko and xrt-lib.ko to support
> +various HW subsystems exposed by different Alveo shells. The differences among
define 'shells'
> +these Alveo shells is handled in the endpoint (platform device) drivers.
> +See :ref:`alveo_platform_overview`.
> +
> +The instantiation of a specific endpoint driver is completely data driven based
> +on the metadata (in the device tree format). The flattened device tree is stored
> +in a xsabin file which is discovered through the PCIe VSEC capability.
> +
> +
> +Driver Object Model
> +===================
> +
> +The system device tree after overlaying Alveo partitions looks like the
> +following::
> +
> + +-----------+
> + | of root |
> + +-----------+
> + |
> + +-------------------+-------------------+
> + | | |
> + v v v
> + +-------------+ +------------+ +---------+
> + | xrt-part0 | | xrt-partN | | |
> + |(simple-bus) | ... |(simple-bus)| | ... |
> + +-------------+ +------------+ +---------+
> + | |
> + | |
> + +-----+--------+ |
> + | | |
> + v v v
> + +-----------+ +-----------+ +------------+
> + |ep_foo@123 |..|ep_bar@456 | | ep_foo@789 |
> + +-----------+ +-----------+ +------------+
> +
> +partition node
> +--------------
> +
> +The partition node is created and added to the system device tree when the driver
> +creates a new partition. It is compatible with ``simple-bus`` which is a
> +transparent bus node defined by Linux kernel. The partition node is used for
maybe drop 'defined by Linux kernel'
> +translating the address of underneath endpoint to CPU address.
address -> addresses
> +
> +endpoint node
> +-------------
> +
> +During the partition creation, xrt driver un-flattens the partition metadata and
> +adds all the endpoint nodes under the partition node to the system device tree.
> +Eventually, all the endpoint nodes will be populated by the existing platform
> +device and OF infrastructure. This means a platform device will be created for
> +each endpoint node. The platform driver will be bound based on the ``compatible``
> +property defined in the endpoint node.
> +
> +.. _alveo_platform_overview:
> +
> +Alveo Platform Overview
> +=======================
> +
> +Alveo platforms are architected as two physical FPGA partitions: *Shell* and
shell is defined here but needed earlier, maybe move this section to earlier
> +*User*. The Shell provides basic infrastructure for the Alveo platform like
> +PCIe connectivity, board management, Dynamic Function Exchange (DFX), sensors,
> +clocking, reset, and security. DFX, partial reconfiguration, is responsible for
DFX also known as partial reconfiguration,
> +loading the user compiled FPGA binary.
> +
> +For DFX to work properly, physical partitions require strict HW compatibility
> +with each other. Every physical partition has two interface UUIDs: the *parent*
> +UUID and the *child* UUID. For simple single stage platforms, Shell → User forms
> +the parent child relationship.
> +
> +.. note::
> + Partition compatibility matching is a key design component of the Alveo platforms
> + and XRT. Partitions have child and parent relationship. A loaded partition
drop 'Partitions have child and parent relationship' , you just said
this three lines up
> + exposes child partition UUID to advertise its compatibility requirement. When
> + loading a child partition, the xrt-mgmt driver matches the parent
> + UUID of the child partition against the child UUID exported by the parent.
> + The parent and child partition UUIDs are stored in the *xclbin* (for the user)
> + and the *xsabin* (for the shell). Except for the root UUID exported by VSEC,
> + the hardware itself does not know about the UUIDs. The UUIDs are stored in
> + xsabin and xclbin. The image format has a special node called Partition UUIDs
> + which define the compatibility UUIDs.
> +
> +
> +The physical partitions and their loading are illustrated below::
> +
> + SHELL USER
> + +-----------+ +-------------------+
> + | | | |
> + | VSEC UUID | CHILD PARENT | LOGIC UUID |
> + | o------->|<--------o |
> + | | UUID UUID | |
> + +-----+-----+ +--------+----------+
> + | |
> + . .
> + | |
> + +---+---+ +------+--------+
> + | POR | | USER COMPILED |
> + | FLASH | | XCLBIN |
> + +-------+ +---------------+
> +
> +
> +Loading Sequence
> +----------------
> +
> +The Shell partition is loaded from flash at system boot time. It establishes the
> +PCIe link and exposes two physical functions to the BIOS. After the OS boots,
> +the xrt-mgmt driver attaches to the PCIe physical function 0 exposed by the Shell
> +and then looks for VSEC in the PCIe extended configuration space. Using VSEC, it
> +determines the logic UUID of the Shell and uses the UUID to load matching *xsabin*
> +file from Linux firmware directory. The xsabin file contains the metadata to
> +discover the peripherals that are part of the Shell and the firmware for any
> +embedded soft processors in the Shell. The xsabin file also contains Partition
> +UUIDs.
> +
> +The Shell exports a child interface UUID which is used for the compatibility
> +check when loading the user compiled xclbin over the User partition as part of DFX.
> +When a user requests loading of a specific xclbin, the xrt-mgmt driver reads
> +the parent interface UUID specified in the xclbin and matches it with the child
> +interface UUID exported by the Shell to determine if the xclbin is compatible with
> +the Shell. If the match fails, loading of xclbin is denied.
> +
> +xclbin loading is requested using the ICAP_DOWNLOAD_AXLF ioctl command. When loading
> +a xclbin, the xrt-mgmt driver performs the following *logical* operations:
> +
> +1. Copy xclbin from user to kernel memory
> +2. Sanity check the xclbin contents
> +3. Isolate the User partition
> +4. Download the bitstream using the FPGA config engine (ICAP)
> +5. De-isolate the User partition
> +6. Program the clocks (ClockWiz) driving the User partition
> +7. Wait for the memory controller (MIG) calibration
> +8. Return the loading status back to the caller
> +
> +`Platform Loading Overview <https://xilinx.github.io/XRT/master/html/platforms_partitions.html>`_
> +provides more detailed information on platform loading.
> +
> +
> +xsabin
> +------
> +
> +Each Alveo platform comes packaged with its own xsabin. The xsabin is a trusted
> +component of the platform. For format details refer to :ref:`xsabin_xclbin_container_format`
> +below. xsabin contains basic information like UUIDs, platform name and metadata in the
> +form of flat device tree.
> +
> +xclbin
> +------
> +
> +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 an FPGA bitstream for the user partition, UUIDs, platform name, etc.
> +
> +
> +.. _xsabin_xclbin_container_format:
> +
> +xsabin/xclbin Container Format
> +------------------------------
> +
> +xclbin/xsabin is ELF-like binary container format. It is structured as series of
> +sections. There is a file header followed by several section headers which is
> +followed by sections. A section header points to an actual section. There is an
> +optional signature at the end. The format is defined by the header file ``xclbin.h``.
> +The following figure illustrates a typical xclbin::
> +
> +
> + +---------------------+
> + | |
> + | HEADER |
> + +---------------------+
> + | SECTION HEADER |
> + | |
> + +---------------------+
> + | ... |
> + | |
> + +---------------------+
> + | SECTION HEADER |
> + | |
> + +---------------------+
> + | SECTION |
> + | |
> + +---------------------+
> + | ... |
> + | |
> + +---------------------+
> + | SECTION |
> + | |
> + +---------------------+
> + | SIGNATURE |
> + | (OPTIONAL) |
> + +---------------------+
> +
> +
> +xclbin/xsabin files can be packaged, un-packaged and inspected using an XRT
> +utility called **xclbinutil**. xclbinutil is part of the XRT open source
> +software stack. The source code for xclbinutil can be found at
> +https://github.com/Xilinx/XRT/tree/master/src/runtime_src/tools/xclbinutil
> +
> +For example, to enumerate the contents of a xclbin/xsabin use the *--info* switch
> +as shown below::
> +
> +
> + xclbinutil --info --input /opt/xilinx/firmware/u50/gen3x16-xdma/blp/test/bandwidth.xclbin
> + xclbinutil --info --input /lib/firmware/xilinx/862c7020a250293e32036f19956669e5/partition.xsabin
Two similar calls aren't needed, could drop one.
> +
> +Deployment Models
> +=================
> +
> +Baremetal
> +---------
> +
> +In bare-metal deployments, both MPF and UPF are visible and accessible. The
> +xrt-mgmt driver binds to MPF. The xrt-mgmt driver operations are privileged and
> +available to system administrator. The full stack is illustrated below::
> +
> + HOST
> +
> + [XRT-MGMT] [XRT-USER]
> + | |
> + | |
> + +-----+ +-----+
> + | MPF | | UPF |
> + | | | |
> + | PF0 | | PF1 |
> + +--+--+ +--+--+
> + ......... ^................. ^..........
> + | |
> + | PCIe DEVICE |
> + | |
> + +--+------------------+--+
> + | SHELL |
> + | |
> + +------------------------+
> + | USER |
> + | |
> + | |
> + | |
> + | |
> + +------------------------+
> +
> +
> +
> +Virtualized
> +-----------
> +
> +In virtualized deployments, the privileged MPF is assigned to the host but the
> +unprivileged UPF is assigned to a guest VM via PCIe pass-through. The xrt-mgmt
> +driver in host binds to MPF. The xrt-mgmt driver operations are privileged and
> +only accessible to the MPF. The full stack is illustrated below::
> +
> +
> + ..............
> + HOST . VM .
> + . .
> + [XRT-MGMT] . [XRT-USER] .
> + | . | .
> + | . | .
> + +-----+ . +-----+ .
> + | MPF | . | UPF | .
> + | | . | | .
> + | PF0 | . | PF1 | .
> + +--+--+ . +--+--+ .
> + ......... ^................. ^..........
> + | |
> + | PCIe DEVICE |
> + | |
> + +--+------------------+--+
> + | SHELL |
> + | |
> + +------------------------+
> + | USER |
> + | |
> + | |
> + | |
> + | |
> + +------------------------+
> +
> +
> +
> +
> +
> +Platform Security Considerations
> +================================
> +
> +`Security of Alveo Platform <https://xilinx.github.io/XRT/master/html/security.html>`_
> +discusses the deployment options and security implications in great detail.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80eebc1d9ed5..fd7053bcfdb0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7369,6 +7369,16 @@ F: Documentation/fpga/
> F: drivers/fpga/
> F: include/linux/fpga/
>
> +FPGA XRT DRIVERS
> +M: Lizhi Hou <[email protected]>
> +R: Max Zhen <[email protected]>
> +R: Sonal Santan <[email protected]>

Can you add me ?

R: Tom Rix <[email protected]>

> +L: [email protected]
> +S: Supported
> +W: https://github.com/Xilinx/XRT

This should maybe be a T: since it is a git repo.

A better W: may be

https://xilinx.github.io/XRT/master/html/index.html

Tom

> +F: Documentation/fpga/xrt.rst
> +F: drivers/fpga/xrt/
> +
> FPU EMULATOR
> M: Bill Metzenthen <[email protected]>
> S: Maintained

2022-01-09 17:47:00

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 2/5] Documentation: devicetree: bindings: add binding for Alveo platform

On 1/5/22 2:50 PM, Lizhi Hou wrote:
> Create device tree binding document for partitions and pr isolation on
> Xilinx Alveo platform.
>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> .../bindings/fpga/xlnx,alveo-partition.yaml | 76 +++++++++++++++++++
> .../fpga/xlnx,alveo-pr-isolation.yaml | 40 ++++++++++
> 2 files changed, 116 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
> create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml
>
> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml b/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
> new file mode 100644
> index 000000000000..ee50cb51d08e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xlnx,alveo-partition.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/xlnx,alveo-partition.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Alveo platform partition bindings
> +
> +description: |
> + Xilinx Alveo platform is a PCI device and has one or more partitions. A
PCIe
> + partition is programmed dynamically and contains a set of hardware
> + peripherals also referred to as endpoints which appear on the PCI BARs.
> + This binding is defined for endpoint address translation which uses the
> + the following encoding:
> +
> + 0xIooooooo 0xoooooooo
> +
> + Where:
> +
> + I = BAR index
> + oooooo oooooooo = BAR offset
> +
> + As a PCI device, the Alveo platform is enumerated at runtime. Thus,
> + the partition node is created by Alveo device driver. The device driver
> + gets the BAR base address of the PCI device and creates the 'range'
> + property for address translation.
> +
> +allOf:
> + - $ref: /schemas/simple-bus.yaml#
> +
> +maintainers:
> + - Lizhi Hou <[email protected]>
> +
> +properties:
> + compatible:
> + contains:
> + const: xlnx,alveo-partition
> +
> + "#address-cells":
> + const: 2
> +
> + "#size-cells":
> + const: 2
> +
> + ranges: true
> +
> +patternProperties:
> + "^.*@[0-9a-f]+$":
> + description: hardware endpoints belong to this partition.
> + type: object
> +
> +required:
> + - compatible
> + - "#address-cells"
> + - "#size-cells"
> + - ranges
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + bus {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + xrt-part-bus@0 {
> + compatible = "xlnx,alveo-partition", "simple-bus";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges = <0x0 0x0 0x0 0xe0000000 0x0 0x2000000
> + 0x20000000 0x0 0x0 0xe4200000 0x0 0x40000>;
> + pr-isolate-ulp@41000 {
> + compatible = "xlnx,alveo-pr-isolation";
> + reg = <0x0 0x41000 0 0x1000>;
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml b/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml
> new file mode 100644
> index 000000000000..8db949093ee0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xlnx,alveo-pr-isolation.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/xlnx,alveo-pr-isolation.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Partial Reconfig Isolation for Alveo platforms
Expand 'Partial Reconfig' to 'Partial Reconfiguration'
> +
> +description: |
> + The Partial Reconfig ensures glitch free operation of the inputs from
> + a reconfigurable partition during partial reconfiguration on Alveo
> + platform.

glitch free is not descriptive. maybe describe what that reg is used for.

Tom

> +
> +maintainers:
> + - Lizhi Hou <[email protected]>
> +
> +properties:
> + compatible:
> + const: xlnx,alveo-pr-isolation
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + bus {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pr-isolation-ulp@41000 {
> + compatible = "xlnx,alveo-pr-isolation";
> + reg = <0 0x41000 0 0x1000>;
> + };
> + };

2022-01-09 18:40:03

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

On 1/5/22 2:50 PM, Lizhi Hou wrote:
> When OF_FLATTREE is selected and there is not a device tree, create an
> empty device tree root node. of/unittest.c code is referenced.

Looks like this patch is doing two things, creating the empty node and
refactoring the unit tests.

This patch should be split.

It is not clear why the unit test should be refactored.

Tom

>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> drivers/of/Makefile | 5 +++
> drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
> drivers/of/fdt_default.dts | 5 +++
> drivers/of/of_private.h | 17 +++++++
> drivers/of/unittest.c | 72 ++----------------------------
> 5 files changed, 120 insertions(+), 69 deletions(-)
> create mode 100644 drivers/of/fdt_default.dts
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..a2989055c578 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-y = base.o device.o platform.o property.o
> +
extra lf, remove
> obj-$(CONFIG_OF_KOBJ) += kobj.o
> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> obj-$(CONFIG_OF_FLATTREE) += fdt.o
> @@ -20,4 +21,8 @@ obj-y += kexec.o
> endif
> endif
>
> +ifndef CONFIG_OF_UNITTEST
> +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
> +endif
> +
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4546572af24b..66ef9ac97829 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> }
> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>
> +static int __init of_fdt_root_init(void)
> +{
> + struct device_node *dt = NULL, *np;
> + void *fdt = NULL, *fdt_aligned;
> + struct property *prop = NULL;
> + __be32 *val = NULL;
> + int size, rc = 0;
> +
> +#if !defined(CONFIG_OF_UNITTEST)
> + if (of_root)
> + return 0;
> +#endif
> + size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
> +
> + fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> + if (!fdt)
> + return -ENOMEM;
> +
> + fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
> + memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
> +
> + if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
> + NULL, &dt)) {
> + pr_warn("%s: unflatten default tree failed\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> + if (!dt) {
> + pr_warn("%s: empty default tree\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> +
> + /*
> + * This lock normally encloses of_resolve_phandles()
> + */
> + of_overlay_mutex_lock();
> +
> + rc = of_resolve_phandles(dt);
> + if (rc) {
> + pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> + goto failed;
> + }
> +
> + if (!of_root) {
> + prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + val = kzalloc(sizeof(*val), GFP_KERNEL);
> + if (!val) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
> +
> + prop->name = "#address-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + prop++;
> + prop->name = "#size-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + of_root = dt;
> + for_each_of_allnodes(np)
> + __of_attach_node_sysfs(np);
> + of_aliases = of_find_node_by_path("/aliases");
> + of_chosen = of_find_node_by_path("/chosen");
> + of_overlay_mutex_unlock();
> +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
> + return 0;
> + }
> +
> + unittest_data_add(dt);
> +
> + of_overlay_mutex_unlock();
> +
> + return 0;
> +
> +failed:
> + of_overlay_mutex_unlock();
> + kfree(val);
> + kfree(prop);
> + return rc;
> +}
> +pure_initcall(of_fdt_root_init);
> +
> /* Everything below here references initial_boot_params directly. */
> int __initdata dt_root_addr_cells;
> int __initdata dt_root_size_cells;
> diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
> new file mode 100644
> index 000000000000..d1f12a76dfc6
> --- /dev/null
> +++ b/drivers/of/fdt_default.dts
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> +};
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 631489f7f8c0..1ef93bccfdba 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
> extern struct list_head aliases_lookup;
> extern struct kset *of_kset;
>
> +#if defined(CONFIG_OF_UNITTEST)
> +extern u8 __dtb_testcases_begin[];
> +extern u8 __dtb_testcases_end[];
> +#define __dtb_fdt_default_begin __dtb_testcases_begin
> +#define __dtb_fdt_default_end __dtb_testcases_end
> +void __init unittest_data_add(struct device_node *dt);
> +#else
> +extern u8 __dtb_fdt_default_begin[];
> +extern u8 __dtb_fdt_default_end[];
> +static inline void unittest_data_add(struct device_node *dt) {}
> +#endif
> +
> #if defined(CONFIG_OF_DYNAMIC)
> extern int of_property_notify(int action, struct device_node *np,
> struct property *prop, struct property *old_prop);
> @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
>
> #if defined(CONFIG_OF_RESOLVE)
> int of_resolve_phandles(struct device_node *tree);
> +#else
> +static inline int of_resolve_phandles(struct device_node *tree)
> +{
> + return 0;
> +}
> #endif
>
> void __of_phandle_cache_inv_entry(phandle handle);
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 8c056972a6dd..745f455235cc 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
> * unittest_data_add - Reads, copies data from
> * linked tree and attaches it to the live tree
> */
> -static int __init unittest_data_add(void)
> +void __init unittest_data_add(struct device_node *dt)
> {
> - void *unittest_data;
> - void *unittest_data_align;
> - struct device_node *unittest_data_node = NULL, *np;
> - /*
> - * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> - * created by cmd_dt_S_dtb in scripts/Makefile.lib
> - */
> - extern uint8_t __dtb_testcases_begin[];
> - extern uint8_t __dtb_testcases_end[];
> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
> - int rc;
> - void *ret;
> -
> - if (!size) {
> - pr_warn("%s: testcases is empty\n", __func__);
> - return -ENODATA;
> - }
> -
> - /* creating copy */
> - unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> - if (!unittest_data)
> - return -ENOMEM;
> -
> - unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> - memcpy(unittest_data_align, __dtb_testcases_begin, size);
> -
> - ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> - if (!ret) {
> - pr_warn("%s: unflatten testcases tree failed\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> - if (!unittest_data_node) {
> - pr_warn("%s: testcases tree is empty\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> -
> - /*
> - * This lock normally encloses of_resolve_phandles()
> - */
> - of_overlay_mutex_lock();
> -
> - rc = of_resolve_phandles(unittest_data_node);
> - if (rc) {
> - pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> - of_overlay_mutex_unlock();
> - return -EINVAL;
> - }
> -
> - if (!of_root) {
> - of_root = unittest_data_node;
> - for_each_of_allnodes(np)
> - __of_attach_node_sysfs(np);
> - of_aliases = of_find_node_by_path("/aliases");
> - of_chosen = of_find_node_by_path("/chosen");
> - of_overlay_mutex_unlock();
> - return 0;
> - }
> + struct device_node *np;
>
> EXPECT_BEGIN(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
>
> /* attach the sub-tree to live tree */
> - np = unittest_data_node->child;
> + np = dt->child;
> while (np) {
> struct device_node *next = np->sibling;
>
> @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
>
> EXPECT_END(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
> -
> - of_overlay_mutex_unlock();
> -
> - return 0;
> }
>
> #ifdef CONFIG_OF_OVERLAY
> @@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
> static int __init of_unittest(void)
> {
> struct device_node *np;
> - int res;
>
> pr_info("start of unittest - you will see error messages\n");
>
> @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
> if (IS_ENABLED(CONFIG_UML))
> unittest_unflatten_overlay_base();
>
> - res = unittest_data_add();
> - if (res)
> - return res;
> if (!of_aliases)
> of_aliases = of_find_node_by_path("/aliases");
>

2022-01-09 21:16:33

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 4/5] fpga: xrt: xrt-lib common interfaces

On 1/5/22 2:50 PM, Lizhi Hou wrote:
> The Alveo platform has to PCI fucntions. Each function has its own driver
> attached. The common interfaces are created to support both drivers.

The commit log should be more descriptive since this introduces a new class

of drivers. Reuse the cover letter content.

>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> drivers/fpga/Kconfig | 3 +
> drivers/fpga/Makefile | 3 +
> drivers/fpga/xrt/Kconfig | 6 +
> drivers/fpga/xrt/include/xpartition.h | 28 ++++
> drivers/fpga/xrt/lib/Kconfig | 17 +++
> drivers/fpga/xrt/lib/Makefile | 15 +++
> drivers/fpga/xrt/lib/lib-drv.c | 178 ++++++++++++++++++++++++++
> drivers/fpga/xrt/lib/lib-drv.h | 15 +++
> 8 files changed, 265 insertions(+)
> create mode 100644 drivers/fpga/xrt/Kconfig
> create mode 100644 drivers/fpga/xrt/include/xpartition.h
> create mode 100644 drivers/fpga/xrt/lib/Kconfig
> create mode 100644 drivers/fpga/xrt/lib/Makefile
> create mode 100644 drivers/fpga/xrt/lib/lib-drv.c
> create mode 100644 drivers/fpga/xrt/lib/lib-drv.h
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 991b3f361ec9..93ae387c97c5 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -243,4 +243,7 @@ config FPGA_MGR_VERSAL_FPGA
> configure the programmable logic(PL).
>
> To compile this as a module, choose M here.
> +
> +source "drivers/fpga/xrt/Kconfig"

This patchset will have 2 new Kconfigs and only 2 config setting.

To simplify, add the 2 config settings directly to fpga/Kconfig

> +
> endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 0bff783d1b61..5bd41cf4c7ec 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -49,3 +49,6 @@ obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += dfl-n3000-nios.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> +
> +# XRT drivers for Alveo
> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
> diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> new file mode 100644
> index 000000000000..04c3bb5aaf4f
> --- /dev/null
> +++ b/drivers/fpga/xrt/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Xilinx Alveo FPGA device configuration
> +#
> +
> +source "drivers/fpga/xrt/lib/Kconfig"
> diff --git a/drivers/fpga/xrt/include/xpartition.h b/drivers/fpga/xrt/include/xpartition.h
> new file mode 100644
> index 000000000000..d72090ddfbee
> --- /dev/null
> +++ b/drivers/fpga/xrt/include/xpartition.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#ifndef _XRT_PARTITION_H_
> +#define _XRT_PARTITION_H_
> +
> +struct xrt_partition_range {
> + u32 bar_idx;
> + u64 base;
> + u64 size;
> +};
> +
> +struct xrt_partition_info {
> + int num_range;
> + struct xrt_partition_range *ranges;
> + void *fdt;
> + u32 fdt_len;
> +};
> +
> +int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle);
> +void xrt_partition_destroy(void *handle);
> +
> +#endif
> diff --git a/drivers/fpga/xrt/lib/Kconfig b/drivers/fpga/xrt/lib/Kconfig
> new file mode 100644
> index 000000000000..73de1f50d5c6
> --- /dev/null
> +++ b/drivers/fpga/xrt/lib/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# XRT Alveo FPGA device configuration
> +#
> +
> +config FPGA_XRT_LIB
> + tristate "XRT Alveo Driver Library"
> + depends on HWMON && PCI && HAS_IOMEM && OF
> + select REGMAP_MMIO
> + select OF_OVERLAY
> + help
> + Select this option to enable Xilinx XRT Alveo driver library. This
> + library is core infrastructure of XRT Alveo FPGA drivers which
> + provides functions for working with device nodes, iteration and
> + lookup of platform devices, common interfaces for platform devices,
> + plumbing of function call and ioctls between platform devices and
> + parent partitions.
> diff --git a/drivers/fpga/xrt/lib/Makefile b/drivers/fpga/xrt/lib/Makefile
> new file mode 100644
> index 000000000000..698877c39657
> --- /dev/null
> +++ b/drivers/fpga/xrt/lib/Makefile
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> +#
> +# Authors: [email protected]
> +#
> +
> +FULL_XRT_PATH=$(srctree)/$(src)/..
> +
> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-lib.o
> +
> +xrt-lib-objs := \
> + lib-drv.o
> +
> +ccflags-y := -I$(FULL_XRT_PATH)/include
> diff --git a/drivers/fpga/xrt/lib/lib-drv.c b/drivers/fpga/xrt/lib/lib-drv.c
> new file mode 100644
> index 000000000000..56334b2b9bec
> --- /dev/null
> +++ b/drivers/fpga/xrt/lib/lib-drv.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Cheng Zhen <[email protected]>
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include "xpartition.h"
> +#include "lib-drv.h"
> +
> +#define XRT_PARTITION_FDT_ALIGN 8
> +#define XRT_PARTITION_NAME_LEN 64
> +
> +struct xrt_partition {
> + struct device *dev;
> + u32 id;
> + char name[XRT_PARTITION_NAME_LEN];
> + void *fdt;
> + struct property ranges;
> + struct of_changeset chgset;
> + bool chgset_applied;
> + void *dn_mem;
> +};
> +
> +DEFINE_IDA(xrt_partition_id);
> +
> +static int xrt_partition_set_ranges(struct xrt_partition *xp, struct xrt_partition_range *ranges,
> + int num_range)
> +{
> + __be64 *prop;
> + u32 prop_len;
> + int i;
> +
> + prop_len = num_range * (sizeof(u64) * 3);
> + prop = kzalloc(prop_len, GFP_KERNEL);
> + if (!prop)
> + return -ENOMEM;
> +
> + xp->ranges.name = "ranges";
> + xp->ranges.length = prop_len;
> + xp->ranges.value = prop;
> +
> + for (i = 0; i < num_range; i++) {
> + *prop = cpu_to_be64((u64)ranges[i].bar_idx << 60);
> + prop++;
> + *prop = cpu_to_be64(ranges[i].base);
> + prop++;
> + *prop = cpu_to_be64(ranges[i].size);
> + prop++;
> + }
> +
> + return 0;
> +}
> +
> +void xrt_partition_destroy(void *handle)
> +{
> + struct xrt_partition *xp = handle;
> +
> + if (xp->chgset_applied)
> + of_changeset_revert(&xp->chgset);
> + of_changeset_destroy(&xp->chgset);
> +
> + ida_free(&xrt_partition_id, xp->id);
> + kfree(xp->dn_mem);
> + kfree(xp->fdt);
> + kfree(xp->ranges.value);
> + kfree(xp);
> +}
> +EXPORT_SYMBOL_GPL(xrt_partition_destroy);
> +
> +int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle)
> +{
> + struct device_node *parent_dn = NULL, *dn, *part_dn;
> + struct xrt_partition *xp = NULL;
> + void *fdt_aligned;
> + int ret;
> +
> + xp = kzalloc(sizeof(*xp), GFP_KERNEL);
> + if (!xp)
> + return -ENOMEM;
> +
> + ret = ida_alloc(&xrt_partition_id, GFP_KERNEL);
> + if (ret < 0) {
> + dev_err(dev, "alloc id failed, ret %d", ret);
> + kfree(xp);
> + return ret;
> + }
> + xp->id = ret;
> + of_changeset_init(&xp->chgset);
> +
> + parent_dn = of_find_node_by_path("/");
> + if (!parent_dn) {
> + dev_err(dev, "did not find xrt node");
> + ret = -EINVAL;
> + goto failed;
> + }
> +
> + xp->dev = dev;
> + snprintf(xp->name, XRT_PARTITION_NAME_LEN, "xrt-part@%x", xp->id);
> + ret = xrt_partition_set_ranges(xp, info->ranges, info->num_range);
> + if (ret)
> + goto failed;
> +
> + xp->fdt = kmalloc(info->fdt_len + XRT_PARTITION_FDT_ALIGN, GFP_KERNEL);
> + if (!xp->fdt) {
> + ret = -ENOMEM;
> + goto failed;
> + }
> + fdt_aligned = PTR_ALIGN(xp->fdt, XRT_PARTITION_FDT_ALIGN);
> + memcpy(fdt_aligned, info->fdt, info->fdt_len);
> +
> + xp->dn_mem = of_fdt_unflatten_tree(fdt_aligned, NULL, &part_dn);
> + if (!xp->dn_mem) {
> + ret = -EINVAL;
> + goto failed;
> + }
> +
> + of_node_get(part_dn);
> + part_dn->full_name = xp->name;
> + part_dn->parent = parent_dn;
> + for (dn = part_dn; dn; dn = of_find_all_nodes(dn))
> + of_changeset_attach_node(&xp->chgset, dn);
> +
> + ret = of_changeset_add_property(&xp->chgset, part_dn, &xp->ranges);
> + if (ret) {
> + dev_err(dev, "failed to add property, ret %d", ret);
> + goto failed;
> + }
> +
> + ret = of_changeset_apply(&xp->chgset);
> + if (ret) {
> + dev_err(dev, "failed to apply changeset, ret %d", ret);
> + goto failed;
> + }
> + xp->chgset_applied = true;
> + of_node_put(parent_dn);
> +
> + ret = of_platform_populate(part_dn, NULL, NULL, dev);
> + if (ret) {
> + dev_err(dev, "failed to populate devices, ret %d", ret);
> + goto failed;
> + }
> +
> + *handle = xp;
> + return 0;
> +
> +failed:
> + if (parent_dn)
> + of_node_put(parent_dn);
> + xrt_partition_destroy(xp);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(xrt_partition_create);
> +
> +static __init int xrt_lib_init(void)
> +{
> + return 0;
> +}
> +
> +static __exit void xrt_lib_fini(void)
> +{
> +}
> +
> +module_init(xrt_lib_init);
> +module_exit(xrt_lib_fini);
noops
> +
> +MODULE_AUTHOR("XRT Team <[email protected]>");
> +MODULE_DESCRIPTION("Xilinx Alveo IP Lib driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/fpga/xrt/lib/lib-drv.h b/drivers/fpga/xrt/lib/lib-drv.h
> new file mode 100644
> index 000000000000..77ed5c399dcf
> --- /dev/null
> +++ b/drivers/fpga/xrt/lib/lib-drv.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Cheng Zhen <[email protected]>
> + */
> +
> +#ifndef _LIB_DRV_H_
> +#define _LIB_DRV_H_
header guards should have a consistent prefix and this one is a little
generic, append _XRT
> +
> +extern u8 __dtb_xrt_begin[];
> +extern u8 __dtb_xrt_end[];

I could not find where these were used in the patch.

Maybe consolidate all the xrt/lib/*.h to just this one file.

Tom

> +
> +#endif /* _LIB_DRV_H_ */

2022-01-09 21:34:39

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On 1/5/22 2:50 PM, Lizhi Hou wrote:
> The PCIE device driver which attaches to management function on Alveo
> devices. It instantiates one or more partition. Each partition consists
> a set of hardward endpoints. A flat device tree is associated with each
> partition. The first version of this driver uses test version flat device
> tree and call xrt lib API to unflatten it.
>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> drivers/fpga/Makefile | 1 +
> drivers/fpga/xrt/Kconfig | 1 +
> drivers/fpga/xrt/mgmt/Kconfig | 14 +++
> drivers/fpga/xrt/mgmt/Makefile | 16 +++
> drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
> drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
> drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
> 7 files changed, 217 insertions(+)
> create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
> create mode 100644 drivers/fpga/xrt/mgmt/Makefile
> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
> create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
>
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 5bd41cf4c7ec..544e2144878f 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
>
> # XRT drivers for Alveo
> obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
> +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
> diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> index 04c3bb5aaf4f..50422f77c6df 100644
> --- a/drivers/fpga/xrt/Kconfig
> +++ b/drivers/fpga/xrt/Kconfig
> @@ -4,3 +4,4 @@
> #
>
> source "drivers/fpga/xrt/lib/Kconfig"
> +source "drivers/fpga/xrt/mgmt/Kconfig"
> diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
> new file mode 100644
> index 000000000000..a978747482be
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Xilinx XRT FPGA device configuration
> +#
> +
> +config FPGA_XRT_XMGMT
> + tristate "Xilinx Alveo Management Driver"
> + depends on FPGA_XRT_LIB
> + select FPGA_BRIDGE
> + select FPGA_REGION
> + help
> + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
> + This driver provides interfaces for userspace application to access
> + Alveo FPGA device.
> diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
> new file mode 100644
> index 000000000000..c5134bf71cca
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/Makefile
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> +#
> +# Authors: [email protected]
> +#
> +
> +FULL_XRT_PATH=$(srctree)/$(src)/..
> +
> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
> +
> +xrt-mgmt-objs := \
> + xmgmt-drv.o \
> + dt-test.dtb.o
> +
> +ccflags-y := -I$(FULL_XRT_PATH)/include
> diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
> new file mode 100644
> index 000000000000..68dbcb7fd79d
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> + compatible = "xlnx,alveo-partition", "simple-bus";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + pr_isolate_ulp@0,41000 {
> + compatible = "xlnx,alveo-pr-isolation";
> + reg = <0x0 0x41000 0x0 0x1000>;
> + };
> +};
> diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
> new file mode 100644
> index 000000000000..6ec4203afbd2
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/dt-test.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#ifndef _DT_TEST_H_
> +#define _DT_TEST_H_
> +
> +extern u8 __dtb_dt_test_begin[];
> +extern u8 __dtb_dt_test_end[];
these externs are also in lib-drv.h, this could be a duplicate file
> +
> +#endif /* _DT_TEST_H_ */
> diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> new file mode 100644
> index 000000000000..87abe5b86e0b
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx Alveo Management Function Driver
> + *
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Cheng Zhen <[email protected]>
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/aer.h>
> +#include <linux/vmalloc.h>
> +#include <linux/delay.h>
> +#include "xpartition.h"
> +#include "dt-test.h"
> +
> +#define XMGMT_MODULE_NAME "xrt-mgmt"
> +
> +#define XMGMT_PDEV(xm) ((xm)->pdev)
> +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
> +#define xmgmt_err(xm, fmt, args...) \
> + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_warn(xm, fmt, args...) \
> + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_info(xm, fmt, args...) \
> + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_dbg(xm, fmt, args...) \
> + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define XMGMT_DEV_ID(_pcidev) \
> + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
> + ((pci_domain_nr((pcidev)->bus) << 16) | \
> + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
> +
> +#define XRT_MAX_READRQ 512
> +
> +/* PCI Device IDs */
> +#define PCI_DEVICE_ID_U50 0x5020
> +static const struct pci_device_id xmgmt_pci_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
> + { 0, }
> +};
> +
> +struct xmgmt {
> + struct pci_dev *pdev;
> + void *base_partition;
> +
remove empty nl
> + bool ready;
> +};
> +
> +static int xmgmt_config_pci(struct xmgmt *xm)
> +{
> + struct pci_dev *pdev = XMGMT_PDEV(xm);
> + int rc;
> +
> + rc = pcim_enable_device(pdev);
> + if (rc < 0) {
> + xmgmt_err(xm, "failed to enable device: %d", rc);
> + return rc;
> + }
> +
> + rc = pci_enable_pcie_error_reporting(pdev);
> + if (rc)
> + xmgmt_warn(xm, "failed to enable AER: %d", rc);
> +
> + pci_set_master(pdev);
> +
> + rc = pcie_get_readrq(pdev);
> + if (rc > XRT_MAX_READRQ)
> + pcie_set_readrq(pdev, XRT_MAX_READRQ);
> + return 0;
> +}
> +
> +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
> + struct xrt_partition_info xp_info = { 0 };
> + struct device *dev = &pdev->dev;
> + int ret, i, idx = 0;
> + struct xmgmt *xm;
> +
> + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
> + if (!xm)
> + return -ENOMEM;
> + xm->pdev = pdev;
> + pci_set_drvdata(pdev, xm);
> +
> + ret = xmgmt_config_pci(xm);
> + if (ret)
> + goto failed;
> +
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + if (pci_resource_len(pdev, i) > 0) {
> + ranges[idx].bar_idx = i;
> + ranges[idx].base = pci_resource_start(pdev, i);
> + ranges[idx].size = pci_resource_len(pdev, i);
> + idx++;
> + }
> + }
> + xp_info.num_range = idx;
> + xp_info.ranges = ranges;
> + xp_info.fdt = __dtb_dt_test_begin;
> + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
> + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
> + if (ret)
> + goto failed;
> +
> + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
> + return 0;
> +
> +failed:
> + if (xm->base_partition)

This if-check and xrt_partition can be removed.

xm->base_partition is only set when xrt_partition_create is successful

> + xrt_partition_destroy(xm->base_partition);
> + pci_set_drvdata(pdev, NULL);
> + return ret;
> +}
> +
> +static void xmgmt_remove(struct pci_dev *pdev)
> +{
> + struct xmgmt *xm = pci_get_drvdata(pdev);
> +
> + xrt_partition_destroy(xm->base_partition);
> + pci_disable_pcie_error_reporting(xm->pdev);
> + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
> +}
> +
> +static struct pci_driver xmgmt_driver = {
> + .name = XMGMT_MODULE_NAME,
> + .id_table = xmgmt_pci_ids,
> + .probe = xmgmt_probe,
> + .remove = xmgmt_remove,
> +};
> +
> +static int __init xmgmt_init(void)
> +{
> + int res = 0;

This is a dead assignment, keep if you want.

Tom

> +
> + res = pci_register_driver(&xmgmt_driver);
> + if (res)
> + return res;
> +
> + return 0;
> +}
> +
> +static __exit void xmgmt_exit(void)
> +{
> + pci_unregister_driver(&xmgmt_driver);
> +}
> +
> +module_init(xmgmt_init);
> +module_exit(xmgmt_exit);
> +
> +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
> +MODULE_AUTHOR("XRT Team <[email protected]>");
> +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
> +MODULE_LICENSE("GPL v2");

2022-01-11 01:14:27

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

Hi Rob,

We have modified the required device tree change according to your
latest comments.
https://lore.kernel.org/lkml/YaWE%[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Waiting for your feedback on the design and code changes.

Tom, thanks for your suggestions. Will incorporate your feedback in the
next patch series.

Thanks,
Lizhi

On 1/9/22 10:39 AM, Tom Rix wrote:
>
> On 1/5/22 2:50 PM, Lizhi Hou wrote:
>> When OF_FLATTREE is selected and there is not a device tree, create an
>> empty device tree root node. of/unittest.c code is referenced.
>
> Looks like this patch is doing two things, creating the empty node and
> refactoring the unit tests.
>
> This patch should be split.
>
> It is not clear why the unit test should be refactored.
>
> Tom
>
>>
>> Signed-off-by: Sonal Santan <[email protected]>
>> Signed-off-by: Max Zhen <[email protected]>
>> Signed-off-by: Lizhi Hou <[email protected]>
>> ---
>> drivers/of/Makefile        | 5 +++
>> drivers/of/fdt.c           | 90 ++++++++++++++++++++++++++++++++++++++
>> drivers/of/fdt_default.dts | 5 +++
>> drivers/of/of_private.h    | 17 +++++++
>> drivers/of/unittest.c      | 72 ++----------------------------
>> 5 files changed, 120 insertions(+), 69 deletions(-)
>> create mode 100644 drivers/of/fdt_default.dts
>>
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index c13b982084a3..a2989055c578 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0
>> obj-y = base.o device.o platform.o property.o
>> +
> extra lf, remove
>> obj-$(CONFIG_OF_KOBJ) += kobj.o
>> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> @@ -20,4 +21,8 @@ obj-y       += kexec.o
>> endif
>> endif
>>
>> +ifndef CONFIG_OF_UNITTEST
>> +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
>> +endif
>> +
>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4546572af24b..66ef9ac97829 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long
>> *blob,
>> }
>> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>
>> +static int __init of_fdt_root_init(void)
>> +{
>> +     struct device_node *dt = NULL, *np;
>> +     void *fdt = NULL, *fdt_aligned;
>> +     struct property *prop = NULL;
>> +     __be32 *val = NULL;
>> +     int size, rc = 0;
>> +
>> +#if !defined(CONFIG_OF_UNITTEST)
>> +     if (of_root)
>> +             return 0;
>> +#endif
>> +     size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
>> +
>> +     fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
>> +     if (!fdt)
>> +             return -ENOMEM;
>> +
>> +     fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
>> +     memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
>> +
>> +     if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
>> +                                NULL, &dt)) {
>> +             pr_warn("%s: unflatten default tree failed\n", __func__);
>> +             kfree(fdt);
>> +             return -ENODATA;
>> +     }
>> +     if (!dt) {
>> +             pr_warn("%s: empty default tree\n", __func__);
>> +             kfree(fdt);
>> +             return -ENODATA;
>> +     }
>> +
>> +     /*
>> +      * This lock normally encloses of_resolve_phandles()
>> +      */
>> +     of_overlay_mutex_lock();
>> +
>> +     rc = of_resolve_phandles(dt);
>> +     if (rc) {
>> +             pr_err("%s: Failed to resolve phandles (rc=%i)\n",
>> __func__, rc);
>> +             goto failed;
>> +     }
>> +
>> +     if (!of_root) {
>> +             prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
>> +             if (!prop) {
>> +                     rc = -ENOMEM;
>> +                     goto failed;
>> +             }
>> +             val = kzalloc(sizeof(*val), GFP_KERNEL);
>> +             if (!val) {
>> +                     rc = -ENOMEM;
>> +                     goto failed;
>> +             }
>> +             *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
>> +
>> +             prop->name = "#address-cells";
>> +             prop->value = val;
>> +             prop->length = sizeof(u32);
>> +             of_add_property(dt, prop);
>> +             prop++;
>> +             prop->name = "#size-cells";
>> +             prop->value = val;
>> +             prop->length = sizeof(u32);
>> +             of_add_property(dt, prop);
>> +             of_root = dt;
>> +             for_each_of_allnodes(np)
>> +                     __of_attach_node_sysfs(np);
>> +             of_aliases = of_find_node_by_path("/aliases");
>> +             of_chosen = of_find_node_by_path("/chosen");
>> +             of_overlay_mutex_unlock();
>> +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
>> +             return 0;
>> +     }
>> +
>> +     unittest_data_add(dt);
>> +
>> +     of_overlay_mutex_unlock();
>> +
>> +     return 0;
>> +
>> +failed:
>> +     of_overlay_mutex_unlock();
>> +     kfree(val);
>> +     kfree(prop);
>> +     return rc;
>> +}
>> +pure_initcall(of_fdt_root_init);
>> +
>> /* Everything below here references initial_boot_params directly. */
>> int __initdata dt_root_addr_cells;
>> int __initdata dt_root_size_cells;
>> diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
>> new file mode 100644
>> index 000000000000..d1f12a76dfc6
>> --- /dev/null
>> +++ b/drivers/of/fdt_default.dts
>> @@ -0,0 +1,5 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +/ {
>> +};
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 631489f7f8c0..1ef93bccfdba 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
>> extern struct list_head aliases_lookup;
>> extern struct kset *of_kset;
>>
>> +#if defined(CONFIG_OF_UNITTEST)
>> +extern u8 __dtb_testcases_begin[];
>> +extern u8 __dtb_testcases_end[];
>> +#define __dtb_fdt_default_begin __dtb_testcases_begin
>> +#define __dtb_fdt_default_end __dtb_testcases_end
>> +void __init unittest_data_add(struct device_node *dt);
>> +#else
>> +extern u8 __dtb_fdt_default_begin[];
>> +extern u8 __dtb_fdt_default_end[];
>> +static inline void unittest_data_add(struct device_node *dt) {}
>> +#endif
>> +
>> #if defined(CONFIG_OF_DYNAMIC)
>> extern int of_property_notify(int action, struct device_node *np,
>>                             struct property *prop, struct property
>> *old_prop);
>> @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct
>> device_node *np) {}
>>
>> #if defined(CONFIG_OF_RESOLVE)
>> int of_resolve_phandles(struct device_node *tree);
>> +#else
>> +static inline int of_resolve_phandles(struct device_node *tree)
>> +{
>> +     return 0;
>> +}
>> #endif
>>
>> void __of_phandle_cache_inv_entry(phandle handle);
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 8c056972a6dd..745f455235cc 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct
>> device_node *np)
>>    * unittest_data_add - Reads, copies data from
>>    * linked tree and attaches it to the live tree
>>    */
>> -static int __init unittest_data_add(void)
>> +void __init unittest_data_add(struct device_node *dt)
>> {
>> -     void *unittest_data;
>> -     void *unittest_data_align;
>> -     struct device_node *unittest_data_node = NULL, *np;
>> -     /*
>> -      * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
>> -      * created by cmd_dt_S_dtb in scripts/Makefile.lib
>> -      */
>> -     extern uint8_t __dtb_testcases_begin[];
>> -     extern uint8_t __dtb_testcases_end[];
>> -     const int size = __dtb_testcases_end - __dtb_testcases_begin;
>> -     int rc;
>> -     void *ret;
>> -
>> -     if (!size) {
>> -             pr_warn("%s: testcases is empty\n", __func__);
>> -             return -ENODATA;
>> -     }
>> -
>> -     /* creating copy */
>> -     unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
>> -     if (!unittest_data)
>> -             return -ENOMEM;
>> -
>> -     unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> -     memcpy(unittest_data_align, __dtb_testcases_begin, size);
>> -
>> -     ret = of_fdt_unflatten_tree(unittest_data_align, NULL,
>> &unittest_data_node);
>> -     if (!ret) {
>> -             pr_warn("%s: unflatten testcases tree failed\n",
>> __func__);
>> -             kfree(unittest_data);
>> -             return -ENODATA;
>> -     }
>> -     if (!unittest_data_node) {
>> -             pr_warn("%s: testcases tree is empty\n", __func__);
>> -             kfree(unittest_data);
>> -             return -ENODATA;
>> -     }
>> -
>> -     /*
>> -      * This lock normally encloses of_resolve_phandles()
>> -      */
>> -     of_overlay_mutex_lock();
>> -
>> -     rc = of_resolve_phandles(unittest_data_node);
>> -     if (rc) {
>> -             pr_err("%s: Failed to resolve phandles (rc=%i)\n",
>> __func__, rc);
>> -             of_overlay_mutex_unlock();
>> -             return -EINVAL;
>> -     }
>> -
>> -     if (!of_root) {
>> -             of_root = unittest_data_node;
>> -             for_each_of_allnodes(np)
>> -                     __of_attach_node_sysfs(np);
>> -             of_aliases = of_find_node_by_path("/aliases");
>> -             of_chosen = of_find_node_by_path("/chosen");
>> -             of_overlay_mutex_unlock();
>> -             return 0;
>> -     }
>> +     struct device_node *np;
>>
>>       EXPECT_BEGIN(KERN_INFO,
>>                    "Duplicate name in testcase-data, renamed to
>> \"duplicate-name#1\"");
>>
>>       /* attach the sub-tree to live tree */
>> -     np = unittest_data_node->child;
>> +     np = dt->child;
>>       while (np) {
>>               struct device_node *next = np->sibling;
>>
>> @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
>>
>>       EXPECT_END(KERN_INFO,
>>                  "Duplicate name in testcase-data, renamed to
>> \"duplicate-name#1\"");
>> -
>> -     of_overlay_mutex_unlock();
>> -
>> -     return 0;
>> }
>>
>> #ifdef CONFIG_OF_OVERLAY
>> @@ -3258,7 +3196,6 @@ static inline __init void
>> of_unittest_overlay_high_level(void) {}
>> static int __init of_unittest(void)
>> {
>>       struct device_node *np;
>> -     int res;
>>
>>       pr_info("start of unittest - you will see error messages\n");
>>
>> @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
>>       if (IS_ENABLED(CONFIG_UML))
>>               unittest_unflatten_overlay_base();
>>
>> -     res = unittest_data_add();
>> -     if (res)
>> -             return res;
>>       if (!of_aliases)
>>               of_aliases = of_find_node_by_path("/aliases");
>>
>

2022-01-11 04:37:24

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

On Wed, Jan 05, 2022 at 02:50:11PM -0800, Lizhi Hou wrote:
> When OF_FLATTREE is selected and there is not a device tree, create an
> empty device tree root node. of/unittest.c code is referenced.
>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> drivers/of/Makefile | 5 +++
> drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
> drivers/of/fdt_default.dts | 5 +++
> drivers/of/of_private.h | 17 +++++++
> drivers/of/unittest.c | 72 ++----------------------------
> 5 files changed, 120 insertions(+), 69 deletions(-)
> create mode 100644 drivers/of/fdt_default.dts
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..a2989055c578 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-y = base.o device.o platform.o property.o
> +

remove the blank line.

> obj-$(CONFIG_OF_KOBJ) += kobj.o
> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> obj-$(CONFIG_OF_FLATTREE) += fdt.o
> @@ -20,4 +21,8 @@ obj-y += kexec.o
> endif
> endif
>
> +ifndef CONFIG_OF_UNITTEST
> +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
> +endif
> +

Same question as Tom, the unittest should work well with or without
of_root, is it? So creating an empty root will not affect unittest, so
why so many ifdefs for CONFIG_OF_UNITTEST?

> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4546572af24b..66ef9ac97829 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> }
> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>
> +static int __init of_fdt_root_init(void)
> +{
> + struct device_node *dt = NULL, *np;
> + void *fdt = NULL, *fdt_aligned;
> + struct property *prop = NULL;
> + __be32 *val = NULL;
> + int size, rc = 0;
> +
> +#if !defined(CONFIG_OF_UNITTEST)
> + if (of_root)
> + return 0;
> +#endif
> + size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
> +
> + fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> + if (!fdt)
> + return -ENOMEM;
> +
> + fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
> + memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
> +
> + if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
> + NULL, &dt)) {
> + pr_warn("%s: unflatten default tree failed\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> + if (!dt) {
> + pr_warn("%s: empty default tree\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> +
> + /*
> + * This lock normally encloses of_resolve_phandles()
> + */
> + of_overlay_mutex_lock();
> +
> + rc = of_resolve_phandles(dt);
> + if (rc) {
> + pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> + goto failed;
> + }
> +
> + if (!of_root) {
> + prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + val = kzalloc(sizeof(*val), GFP_KERNEL);
> + if (!val) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
> +
> + prop->name = "#address-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + prop++;
> + prop->name = "#size-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + of_root = dt;
> + for_each_of_allnodes(np)
> + __of_attach_node_sysfs(np);
> + of_aliases = of_find_node_by_path("/aliases");
> + of_chosen = of_find_node_by_path("/chosen");
> + of_overlay_mutex_unlock();
> +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
> + return 0;
> + }
> +
> + unittest_data_add(dt);

It's confusing to me. If we need to share some functions with unittest,
make a new clearly defined (and named) function.

> +
> + of_overlay_mutex_unlock();
> +
> + return 0;
> +
> +failed:
> + of_overlay_mutex_unlock();
> + kfree(val);
> + kfree(prop);
> + return rc;
> +}
> +pure_initcall(of_fdt_root_init);

Is it better we have a new Kconfig option for the empty tree creation.

> +
> /* Everything below here references initial_boot_params directly. */
> int __initdata dt_root_addr_cells;
> int __initdata dt_root_size_cells;
> diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
> new file mode 100644
> index 000000000000..d1f12a76dfc6
> --- /dev/null
> +++ b/drivers/of/fdt_default.dts
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> +};
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 631489f7f8c0..1ef93bccfdba 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
> extern struct list_head aliases_lookup;
> extern struct kset *of_kset;
>
> +#if defined(CONFIG_OF_UNITTEST)
> +extern u8 __dtb_testcases_begin[];
> +extern u8 __dtb_testcases_end[];
> +#define __dtb_fdt_default_begin __dtb_testcases_begin
> +#define __dtb_fdt_default_end __dtb_testcases_end

Maybe we don't have to use the test dt data, stick to the default empty
fdt is fine?

> +void __init unittest_data_add(struct device_node *dt);
> +#else
> +extern u8 __dtb_fdt_default_begin[];
> +extern u8 __dtb_fdt_default_end[];
> +static inline void unittest_data_add(struct device_node *dt) {}
> +#endif
> +
> #if defined(CONFIG_OF_DYNAMIC)
> extern int of_property_notify(int action, struct device_node *np,
> struct property *prop, struct property *old_prop);
> @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
>
> #if defined(CONFIG_OF_RESOLVE)
> int of_resolve_phandles(struct device_node *tree);
> +#else
> +static inline int of_resolve_phandles(struct device_node *tree)
> +{
> + return 0;
> +}

If we have an empty of_resolve_phandles, does the empty tree creation
still works? Or if we don't need this func, just delete in the code.

Thanks,
Yilun

> #endif
>
> void __of_phandle_cache_inv_entry(phandle handle);
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 8c056972a6dd..745f455235cc 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
> * unittest_data_add - Reads, copies data from
> * linked tree and attaches it to the live tree
> */
> -static int __init unittest_data_add(void)
> +void __init unittest_data_add(struct device_node *dt)
> {
> - void *unittest_data;
> - void *unittest_data_align;
> - struct device_node *unittest_data_node = NULL, *np;
> - /*
> - * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> - * created by cmd_dt_S_dtb in scripts/Makefile.lib
> - */
> - extern uint8_t __dtb_testcases_begin[];
> - extern uint8_t __dtb_testcases_end[];
> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
> - int rc;
> - void *ret;
> -
> - if (!size) {
> - pr_warn("%s: testcases is empty\n", __func__);
> - return -ENODATA;
> - }
> -
> - /* creating copy */
> - unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> - if (!unittest_data)
> - return -ENOMEM;
> -
> - unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> - memcpy(unittest_data_align, __dtb_testcases_begin, size);
> -
> - ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> - if (!ret) {
> - pr_warn("%s: unflatten testcases tree failed\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> - if (!unittest_data_node) {
> - pr_warn("%s: testcases tree is empty\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> -
> - /*
> - * This lock normally encloses of_resolve_phandles()
> - */
> - of_overlay_mutex_lock();
> -
> - rc = of_resolve_phandles(unittest_data_node);
> - if (rc) {
> - pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> - of_overlay_mutex_unlock();
> - return -EINVAL;
> - }
> -
> - if (!of_root) {
> - of_root = unittest_data_node;
> - for_each_of_allnodes(np)
> - __of_attach_node_sysfs(np);
> - of_aliases = of_find_node_by_path("/aliases");
> - of_chosen = of_find_node_by_path("/chosen");
> - of_overlay_mutex_unlock();
> - return 0;
> - }
> + struct device_node *np;
>
> EXPECT_BEGIN(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
>
> /* attach the sub-tree to live tree */
> - np = unittest_data_node->child;
> + np = dt->child;
> while (np) {
> struct device_node *next = np->sibling;
>
> @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
>
> EXPECT_END(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
> -
> - of_overlay_mutex_unlock();
> -
> - return 0;
> }
>
> #ifdef CONFIG_OF_OVERLAY
> @@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
> static int __init of_unittest(void)
> {
> struct device_node *np;
> - int res;
>
> pr_info("start of unittest - you will see error messages\n");
>
> @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
> if (IS_ENABLED(CONFIG_UML))
> unittest_unflatten_overlay_base();
>
> - res = unittest_data_add();
> - if (res)
> - return res;
> if (!of_aliases)
> of_aliases = of_find_node_by_path("/aliases");
>
> --
> 2.27.0

2022-01-11 06:43:35

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 4/5] fpga: xrt: xrt-lib common interfaces

On Sun, Jan 09, 2022 at 01:16:23PM -0800, Tom Rix wrote:
>
> On 1/5/22 2:50 PM, Lizhi Hou wrote:
> > The Alveo platform has to PCI fucntions. Each function has its own driver
two
> > attached. The common interfaces are created to support both drivers.
>
> The commit log should be more descriptive since this introduces a new class
>
> of drivers.? Reuse the cover letter content.
>
> >
> > Signed-off-by: Sonal Santan <[email protected]>
> > Signed-off-by: Max Zhen <[email protected]>
> > Signed-off-by: Lizhi Hou <[email protected]>
> > ---
> > drivers/fpga/Kconfig | 3 +
> > drivers/fpga/Makefile | 3 +
> > drivers/fpga/xrt/Kconfig | 6 +
> > drivers/fpga/xrt/include/xpartition.h | 28 ++++
> > drivers/fpga/xrt/lib/Kconfig | 17 +++
> > drivers/fpga/xrt/lib/Makefile | 15 +++
> > drivers/fpga/xrt/lib/lib-drv.c | 178 ++++++++++++++++++++++++++
> > drivers/fpga/xrt/lib/lib-drv.h | 15 +++
> > 8 files changed, 265 insertions(+)
> > create mode 100644 drivers/fpga/xrt/Kconfig
> > create mode 100644 drivers/fpga/xrt/include/xpartition.h
> > create mode 100644 drivers/fpga/xrt/lib/Kconfig
> > create mode 100644 drivers/fpga/xrt/lib/Makefile
> > create mode 100644 drivers/fpga/xrt/lib/lib-drv.c
> > create mode 100644 drivers/fpga/xrt/lib/lib-drv.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 991b3f361ec9..93ae387c97c5 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -243,4 +243,7 @@ config FPGA_MGR_VERSAL_FPGA
> > configure the programmable logic(PL).
> > To compile this as a module, choose M here.
> > +
> > +source "drivers/fpga/xrt/Kconfig"
>
> This patchset will have 2 new Kconfigs and only 2 config setting.
>
> To simplify, add the 2 config settings directly to fpga/Kconfig

Or have a xrt/Kconfig that records all config settings.

>
> > +
> > endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 0bff783d1b61..5bd41cf4c7ec 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -49,3 +49,6 @@ obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += dfl-n3000-nios.o
> > # Drivers for FPGAs which implement DFL
> > obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > +
> > +# XRT drivers for Alveo
> > +obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/

Have a xrt/Makefile that records all makings?

> > diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> > new file mode 100644
> > index 000000000000..04c3bb5aaf4f
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/Kconfig
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Xilinx Alveo FPGA device configuration
> > +#
> > +
> > +source "drivers/fpga/xrt/lib/Kconfig"
> > diff --git a/drivers/fpga/xrt/include/xpartition.h b/drivers/fpga/xrt/include/xpartition.h
> > new file mode 100644
> > index 000000000000..d72090ddfbee
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/include/xpartition.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > + *
> > + * Authors:
> > + * Lizhi Hou <[email protected]>
> > + */
> > +
> > +#ifndef _XRT_PARTITION_H_
> > +#define _XRT_PARTITION_H_
> > +
> > +struct xrt_partition_range {
> > + u32 bar_idx;
> > + u64 base;
> > + u64 size;
> > +};
> > +
> > +struct xrt_partition_info {
> > + int num_range;
> > + struct xrt_partition_range *ranges;
> > + void *fdt;
> > + u32 fdt_len;
> > +};
> > +
> > +int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle);
> > +void xrt_partition_destroy(void *handle);
> > +
> > +#endif
> > diff --git a/drivers/fpga/xrt/lib/Kconfig b/drivers/fpga/xrt/lib/Kconfig
> > new file mode 100644
> > index 000000000000..73de1f50d5c6
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/Kconfig
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# XRT Alveo FPGA device configuration
> > +#
> > +
> > +config FPGA_XRT_LIB
> > + tristate "XRT Alveo Driver Library"
> > + depends on HWMON && PCI && HAS_IOMEM && OF

depends on HWMON & PCI?

> > + select REGMAP_MMIO
> > + select OF_OVERLAY

Same concern, add options when you really need them.

> > + help
> > + Select this option to enable Xilinx XRT Alveo driver library. This
> > + library is core infrastructure of XRT Alveo FPGA drivers which
> > + provides functions for working with device nodes, iteration and
> > + lookup of platform devices, common interfaces for platform devices,
> > + plumbing of function call and ioctls between platform devices and
> > + parent partitions.
> > diff --git a/drivers/fpga/xrt/lib/Makefile b/drivers/fpga/xrt/lib/Makefile
> > new file mode 100644
> > index 000000000000..698877c39657
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/Makefile
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> > +#
> > +# Authors: [email protected]
> > +#
> > +
> > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > +
> > +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-lib.o
> > +
> > +xrt-lib-objs := \
> > + lib-drv.o
> > +
> > +ccflags-y := -I$(FULL_XRT_PATH)/include
> > diff --git a/drivers/fpga/xrt/lib/lib-drv.c b/drivers/fpga/xrt/lib/lib-drv.c
> > new file mode 100644
> > index 000000000000..56334b2b9bec
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/lib-drv.c
> > @@ -0,0 +1,178 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > + *
> > + * Authors:
> > + * Cheng Zhen <[email protected]>
> > + * Lizhi Hou <[email protected]>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> > +#include "xpartition.h"
> > +#include "lib-drv.h"
> > +
> > +#define XRT_PARTITION_FDT_ALIGN 8
> > +#define XRT_PARTITION_NAME_LEN 64
> > +
> > +struct xrt_partition {
> > + struct device *dev;
> > + u32 id;
> > + char name[XRT_PARTITION_NAME_LEN];
> > + void *fdt;
> > + struct property ranges;
> > + struct of_changeset chgset;
> > + bool chgset_applied;
> > + void *dn_mem;
> > +};
> > +
> > +DEFINE_IDA(xrt_partition_id);
> > +
> > +static int xrt_partition_set_ranges(struct xrt_partition *xp, struct xrt_partition_range *ranges,
> > + int num_range)
> > +{
> > + __be64 *prop;
> > + u32 prop_len;
> > + int i;
> > +
> > + prop_len = num_range * (sizeof(u64) * 3);
> > + prop = kzalloc(prop_len, GFP_KERNEL);
> > + if (!prop)
> > + return -ENOMEM;
> > +
> > + xp->ranges.name = "ranges";
> > + xp->ranges.length = prop_len;
> > + xp->ranges.value = prop;
> > +
> > + for (i = 0; i < num_range; i++) {
> > + *prop = cpu_to_be64((u64)ranges[i].bar_idx << 60);
> > + prop++;
> > + *prop = cpu_to_be64(ranges[i].base);
> > + prop++;
> > + *prop = cpu_to_be64(ranges[i].size);
> > + prop++;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void xrt_partition_destroy(void *handle)
> > +{
> > + struct xrt_partition *xp = handle;
> > +
> > + if (xp->chgset_applied)
> > + of_changeset_revert(&xp->chgset);
> > + of_changeset_destroy(&xp->chgset);
> > +
> > + ida_free(&xrt_partition_id, xp->id);
> > + kfree(xp->dn_mem);
> > + kfree(xp->fdt);
> > + kfree(xp->ranges.value);
> > + kfree(xp);
> > +}
> > +EXPORT_SYMBOL_GPL(xrt_partition_destroy);
> > +
> > +int xrt_partition_create(struct device *dev, struct xrt_partition_info *info, void **handle)
> > +{
> > + struct device_node *parent_dn = NULL, *dn, *part_dn;
> > + struct xrt_partition *xp = NULL;
> > + void *fdt_aligned;
> > + int ret;
> > +
> > + xp = kzalloc(sizeof(*xp), GFP_KERNEL);
> > + if (!xp)
> > + return -ENOMEM;
> > +
> > + ret = ida_alloc(&xrt_partition_id, GFP_KERNEL);
> > + if (ret < 0) {
> > + dev_err(dev, "alloc id failed, ret %d", ret);
> > + kfree(xp);
> > + return ret;
> > + }
> > + xp->id = ret;
> > + of_changeset_init(&xp->chgset);
> > +
> > + parent_dn = of_find_node_by_path("/");
> > + if (!parent_dn) {
> > + dev_err(dev, "did not find xrt node");
> > + ret = -EINVAL;
> > + goto failed;
> > + }
> > +
> > + xp->dev = dev;
> > + snprintf(xp->name, XRT_PARTITION_NAME_LEN, "xrt-part@%x", xp->id);
> > + ret = xrt_partition_set_ranges(xp, info->ranges, info->num_range);
> > + if (ret)
> > + goto failed;
> > +
> > + xp->fdt = kmalloc(info->fdt_len + XRT_PARTITION_FDT_ALIGN, GFP_KERNEL);
> > + if (!xp->fdt) {
> > + ret = -ENOMEM;
> > + goto failed;
> > + }
> > + fdt_aligned = PTR_ALIGN(xp->fdt, XRT_PARTITION_FDT_ALIGN);
> > + memcpy(fdt_aligned, info->fdt, info->fdt_len);
> > +
> > + xp->dn_mem = of_fdt_unflatten_tree(fdt_aligned, NULL, &part_dn);
> > + if (!xp->dn_mem) {
> > + ret = -EINVAL;
> > + goto failed;
> > + }
> > +
> > + of_node_get(part_dn);
> > + part_dn->full_name = xp->name;
> > + part_dn->parent = parent_dn;
> > + for (dn = part_dn; dn; dn = of_find_all_nodes(dn))
> > + of_changeset_attach_node(&xp->chgset, dn);
> > +
> > + ret = of_changeset_add_property(&xp->chgset, part_dn, &xp->ranges);
> > + if (ret) {
> > + dev_err(dev, "failed to add property, ret %d", ret);
> > + goto failed;
> > + }
> > +
> > + ret = of_changeset_apply(&xp->chgset);
> > + if (ret) {
> > + dev_err(dev, "failed to apply changeset, ret %d", ret);
> > + goto failed;
> > + }
> > + xp->chgset_applied = true;
> > + of_node_put(parent_dn);
> > +
> > + ret = of_platform_populate(part_dn, NULL, NULL, dev);
> > + if (ret) {
> > + dev_err(dev, "failed to populate devices, ret %d", ret);
> > + goto failed;
> > + }
> > +
> > + *handle = xp;
> > + return 0;
> > +
> > +failed:
> > + if (parent_dn)
> > + of_node_put(parent_dn);
> > + xrt_partition_destroy(xp);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(xrt_partition_create);

Maybe you could put the necessary lib functions and the callers in the
same patch next time, for easier review.

> > +
> > +static __init int xrt_lib_init(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static __exit void xrt_lib_fini(void)
> > +{
> > +}
> > +
> > +module_init(xrt_lib_init);
> > +module_exit(xrt_lib_fini);
> noops
> > +
> > +MODULE_AUTHOR("XRT Team <[email protected]>");
> > +MODULE_DESCRIPTION("Xilinx Alveo IP Lib driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/fpga/xrt/lib/lib-drv.h b/drivers/fpga/xrt/lib/lib-drv.h
> > new file mode 100644
> > index 000000000000..77ed5c399dcf
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/lib-drv.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > + *
> > + * Authors:
> > + * Cheng Zhen <[email protected]>
> > + */
> > +
> > +#ifndef _LIB_DRV_H_
> > +#define _LIB_DRV_H_
> header guards should have a consistent prefix and this one is a little
> generic, append _XRT
> > +
> > +extern u8 __dtb_xrt_begin[];
> > +extern u8 __dtb_xrt_end[];
>
> I could not find where these were used in the patch.
>
> Maybe consolidate all the xrt/lib/*.h to just this one file.
>
> Tom

Thanks,
Yilun

>
> > +
> > +#endif /* _LIB_DRV_H_ */

2022-01-11 07:08:00

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On Wed, Jan 05, 2022 at 02:50:13PM -0800, Lizhi Hou wrote:
> The PCIE device driver which attaches to management function on Alveo
> devices. It instantiates one or more partition. Each partition consists
> a set of hardward endpoints. A flat device tree is associated with each
> partition. The first version of this driver uses test version flat device
> tree and call xrt lib API to unflatten it.
>
> Signed-off-by: Sonal Santan <[email protected]>
> Signed-off-by: Max Zhen <[email protected]>
> Signed-off-by: Lizhi Hou <[email protected]>
> ---
> drivers/fpga/Makefile | 1 +
> drivers/fpga/xrt/Kconfig | 1 +
> drivers/fpga/xrt/mgmt/Kconfig | 14 +++
> drivers/fpga/xrt/mgmt/Makefile | 16 +++
> drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
> drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
> drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
> 7 files changed, 217 insertions(+)
> create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
> create mode 100644 drivers/fpga/xrt/mgmt/Makefile
> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
> create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
>
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 5bd41cf4c7ec..544e2144878f 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
>
> # XRT drivers for Alveo
> obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
> +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
> diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> index 04c3bb5aaf4f..50422f77c6df 100644
> --- a/drivers/fpga/xrt/Kconfig
> +++ b/drivers/fpga/xrt/Kconfig
> @@ -4,3 +4,4 @@
> #
>
> source "drivers/fpga/xrt/lib/Kconfig"
> +source "drivers/fpga/xrt/mgmt/Kconfig"
> diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
> new file mode 100644
> index 000000000000..a978747482be
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Xilinx XRT FPGA device configuration
> +#
> +
> +config FPGA_XRT_XMGMT
> + tristate "Xilinx Alveo Management Driver"
> + depends on FPGA_XRT_LIB
> + select FPGA_BRIDGE
> + select FPGA_REGION
> + help
> + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
> + This driver provides interfaces for userspace application to access
> + Alveo FPGA device.
> diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
> new file mode 100644
> index 000000000000..c5134bf71cca
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/Makefile
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> +#
> +# Authors: [email protected]
> +#
> +
> +FULL_XRT_PATH=$(srctree)/$(src)/..
> +
> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
> +
> +xrt-mgmt-objs := \
> + xmgmt-drv.o \
> + dt-test.dtb.o
> +
> +ccflags-y := -I$(FULL_XRT_PATH)/include
> diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
> new file mode 100644
> index 000000000000..68dbcb7fd79d
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> + compatible = "xlnx,alveo-partition", "simple-bus";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + pr_isolate_ulp@0,41000 {
> + compatible = "xlnx,alveo-pr-isolation";
> + reg = <0x0 0x41000 0x0 0x1000>;
> + };
> +};

I remember Rob's comments:

"we'd need to create a base tree (if there isn't one) with nodes
for the USB or PCI device(s) and then an overlay for the device can be
applied to those nodes."

https://lore.kernel.org/linux-fpga/CAL_JsqJfyRymB=VxLuQqLpep+Q1Eie48dobv9sC5OizDz0d2DQ@mail.gmail.com/

So could we firstly create a pci device node under the of_root when
the driver probing, then add the partition DTs under the pci device node
by overlay machenism.

I'm considering if finally we could leverage the existing of-fpga-region
that reprograms and enumerates FPGA sub devices by overlay.

Open for discussion.

Thanks,
Yilun

> diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
> new file mode 100644
> index 000000000000..6ec4203afbd2
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/dt-test.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#ifndef _DT_TEST_H_
> +#define _DT_TEST_H_
> +
> +extern u8 __dtb_dt_test_begin[];
> +extern u8 __dtb_dt_test_end[];
> +
> +#endif /* _DT_TEST_H_ */
> diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> new file mode 100644
> index 000000000000..87abe5b86e0b
> --- /dev/null
> +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx Alveo Management Function Driver
> + *
> + * Copyright (C) 2020-2022 Xilinx, Inc.
> + *
> + * Authors:
> + * Cheng Zhen <[email protected]>
> + * Lizhi Hou <[email protected]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/aer.h>
> +#include <linux/vmalloc.h>
> +#include <linux/delay.h>
> +#include "xpartition.h"
> +#include "dt-test.h"
> +
> +#define XMGMT_MODULE_NAME "xrt-mgmt"
> +
> +#define XMGMT_PDEV(xm) ((xm)->pdev)
> +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
> +#define xmgmt_err(xm, fmt, args...) \
> + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_warn(xm, fmt, args...) \
> + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_info(xm, fmt, args...) \
> + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define xmgmt_dbg(xm, fmt, args...) \
> + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> +#define XMGMT_DEV_ID(_pcidev) \
> + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
> + ((pci_domain_nr((pcidev)->bus) << 16) | \
> + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
> +
> +#define XRT_MAX_READRQ 512
> +
> +/* PCI Device IDs */
> +#define PCI_DEVICE_ID_U50 0x5020
> +static const struct pci_device_id xmgmt_pci_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
> + { 0, }
> +};
> +
> +struct xmgmt {
> + struct pci_dev *pdev;
> + void *base_partition;
> +
> + bool ready;
> +};
> +
> +static int xmgmt_config_pci(struct xmgmt *xm)
> +{
> + struct pci_dev *pdev = XMGMT_PDEV(xm);
> + int rc;
> +
> + rc = pcim_enable_device(pdev);
> + if (rc < 0) {
> + xmgmt_err(xm, "failed to enable device: %d", rc);
> + return rc;
> + }
> +
> + rc = pci_enable_pcie_error_reporting(pdev);
> + if (rc)
> + xmgmt_warn(xm, "failed to enable AER: %d", rc);
> +
> + pci_set_master(pdev);
> +
> + rc = pcie_get_readrq(pdev);
> + if (rc > XRT_MAX_READRQ)
> + pcie_set_readrq(pdev, XRT_MAX_READRQ);
> + return 0;
> +}
> +
> +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
> + struct xrt_partition_info xp_info = { 0 };
> + struct device *dev = &pdev->dev;
> + int ret, i, idx = 0;
> + struct xmgmt *xm;
> +
> + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
> + if (!xm)
> + return -ENOMEM;
> + xm->pdev = pdev;
> + pci_set_drvdata(pdev, xm);
> +
> + ret = xmgmt_config_pci(xm);
> + if (ret)
> + goto failed;
> +
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + if (pci_resource_len(pdev, i) > 0) {
> + ranges[idx].bar_idx = i;
> + ranges[idx].base = pci_resource_start(pdev, i);
> + ranges[idx].size = pci_resource_len(pdev, i);
> + idx++;
> + }
> + }
> + xp_info.num_range = idx;
> + xp_info.ranges = ranges;
> + xp_info.fdt = __dtb_dt_test_begin;
> + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
> + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
> + if (ret)
> + goto failed;
> +
> + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
> + return 0;
> +
> +failed:
> + if (xm->base_partition)
> + xrt_partition_destroy(xm->base_partition);
> + pci_set_drvdata(pdev, NULL);
> + return ret;
> +}
> +
> +static void xmgmt_remove(struct pci_dev *pdev)
> +{
> + struct xmgmt *xm = pci_get_drvdata(pdev);
> +
> + xrt_partition_destroy(xm->base_partition);
> + pci_disable_pcie_error_reporting(xm->pdev);
> + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
> +}
> +
> +static struct pci_driver xmgmt_driver = {
> + .name = XMGMT_MODULE_NAME,
> + .id_table = xmgmt_pci_ids,
> + .probe = xmgmt_probe,
> + .remove = xmgmt_remove,
> +};
> +
> +static int __init xmgmt_init(void)
> +{
> + int res = 0;
> +
> + res = pci_register_driver(&xmgmt_driver);
> + if (res)
> + return res;
> +
> + return 0;
> +}
> +
> +static __exit void xmgmt_exit(void)
> +{
> + pci_unregister_driver(&xmgmt_driver);
> +}
> +
> +module_init(xmgmt_init);
> +module_exit(xmgmt_exit);
> +
> +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
> +MODULE_AUTHOR("XRT Team <[email protected]>");
> +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.27.0

2022-01-13 23:41:54

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On 1/10/22 11:00 PM, Xu Yilun wrote:
> =
>
> On Wed, Jan 05, 2022 at 02:50:13PM -0800, Lizhi Hou wrote:
>> The PCIE device driver which attaches to management function on Alveo
>> devices. It instantiates one or more partition. Each partition consists
>> a set of hardward endpoints. A flat device tree is associated with each
>> partition. The first version of this driver uses test version flat device
>> tree and call xrt lib API to unflatten it.
>>
>> Signed-off-by: Sonal Santan <[email protected]>
>> Signed-off-by: Max Zhen <[email protected]>
>> Signed-off-by: Lizhi Hou <[email protected]>
>> ---
>> drivers/fpga/Makefile | 1 +
>> drivers/fpga/xrt/Kconfig | 1 +
>> drivers/fpga/xrt/mgmt/Kconfig | 14 +++
>> drivers/fpga/xrt/mgmt/Makefile | 16 +++
>> drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
>> drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
>> drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
>> 7 files changed, 217 insertions(+)
>> create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
>> create mode 100644 drivers/fpga/xrt/mgmt/Makefile
>> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
>> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
>> create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
>>
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 5bd41cf4c7ec..544e2144878f 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
>>
>> # XRT drivers for Alveo
>> obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
>> +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
>> diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
>> index 04c3bb5aaf4f..50422f77c6df 100644
>> --- a/drivers/fpga/xrt/Kconfig
>> +++ b/drivers/fpga/xrt/Kconfig
>> @@ -4,3 +4,4 @@
>> #
>>
>> source "drivers/fpga/xrt/lib/Kconfig"
>> +source "drivers/fpga/xrt/mgmt/Kconfig"
>> diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
>> new file mode 100644
>> index 000000000000..a978747482be
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/mgmt/Kconfig
>> @@ -0,0 +1,14 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Xilinx XRT FPGA device configuration
>> +#
>> +
>> +config FPGA_XRT_XMGMT
>> + tristate "Xilinx Alveo Management Driver"
>> + depends on FPGA_XRT_LIB
>> + select FPGA_BRIDGE
>> + select FPGA_REGION
>> + help
>> + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
>> + This driver provides interfaces for userspace application to access
>> + Alveo FPGA device.
>> diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
>> new file mode 100644
>> index 000000000000..c5134bf71cca
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/mgmt/Makefile
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
>> +#
>> +# Authors: [email protected]
>> +#
>> +
>> +FULL_XRT_PATH=$(srctree)/$(src)/..
>> +
>> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
>> +
>> +xrt-mgmt-objs := \
>> + xmgmt-drv.o \
>> + dt-test.dtb.o
>> +
>> +ccflags-y := -I$(FULL_XRT_PATH)/include
>> diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
>> new file mode 100644
>> index 000000000000..68dbcb7fd79d
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
>> @@ -0,0 +1,12 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +/ {
>> + compatible = "xlnx,alveo-partition", "simple-bus";
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + pr_isolate_ulp@0,41000 {
>> + compatible = "xlnx,alveo-pr-isolation";
>> + reg = <0x0 0x41000 0x0 0x1000>;
>> + };
>> +};
> I remember Rob's comments:
>
> "we'd need to create a base tree (if there isn't one) with nodes
> for the USB or PCI device(s) and then an overlay for the device can be
> applied to those nodes."
>
> https://lore.kernel.org/linux-fpga/CAL_JsqJfyRymB=VxLuQqLpep+Q1Eie48dobv9sC5OizDz0d2DQ@mail.gmail.com/
>
> So could we firstly create a pci device node under the of_root when
> the driver probing, then add the partition DTs under the pci device node
> by overlay machenism.
>
> I'm considering if finally we could leverage the existing of-fpga-region
> that reprograms and enumerates FPGA sub devices by overlay.
>
> Open for discussion.

I would list the main ideas of our implementation

1. “alveo-partition (simple-bus)” node is created under of_root in xmgmt
probe function. “simple-bus” is used as it fits well and we can modify
it to “fpga-region” when we refactor fpga-mgr/region/bridge part.
2. we are the first pcie device using flatten device tree to describe
sub devices. Adding device tree nodes for all pcie device sounds
intrusive. We can expend to all pcie device when we have another user of
this feature.

3. of_overlay_fdt_apply() does not support overlaying to a dynamic
generated parent node. Thus, we use of_dynamic functions
of_changeset_attach_node() instead. (similar to
drivers/pci/hotplug/pnv_php.c,) . If needed ,we can consider to add
parameter “parent node” to of_overlay_fdt_apply() to support dynamical
overlay case.

Thanks,

Lizhi

>
> Thanks,
> Yilun
>
>> diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
>> new file mode 100644
>> index 000000000000..6ec4203afbd2
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/mgmt/dt-test.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2020-2022 Xilinx, Inc.
>> + *
>> + * Authors:
>> + * Lizhi Hou <[email protected]>
>> + */
>> +
>> +#ifndef _DT_TEST_H_
>> +#define _DT_TEST_H_
>> +
>> +extern u8 __dtb_dt_test_begin[];
>> +extern u8 __dtb_dt_test_end[];
>> +
>> +#endif /* _DT_TEST_H_ */
>> diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
>> new file mode 100644
>> index 000000000000..87abe5b86e0b
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
>> @@ -0,0 +1,158 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx Alveo Management Function Driver
>> + *
>> + * Copyright (C) 2020-2022 Xilinx, Inc.
>> + *
>> + * Authors:
>> + * Cheng Zhen <[email protected]>
>> + * Lizhi Hou <[email protected]>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/aer.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/delay.h>
>> +#include "xpartition.h"
>> +#include "dt-test.h"
>> +
>> +#define XMGMT_MODULE_NAME "xrt-mgmt"
>> +
>> +#define XMGMT_PDEV(xm) ((xm)->pdev)
>> +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
>> +#define xmgmt_err(xm, fmt, args...) \
>> + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>> +#define xmgmt_warn(xm, fmt, args...) \
>> + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>> +#define xmgmt_info(xm, fmt, args...) \
>> + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>> +#define xmgmt_dbg(xm, fmt, args...) \
>> + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>> +#define XMGMT_DEV_ID(_pcidev) \
>> + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
>> + ((pci_domain_nr((pcidev)->bus) << 16) | \
>> + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
>> +
>> +#define XRT_MAX_READRQ 512
>> +
>> +/* PCI Device IDs */
>> +#define PCI_DEVICE_ID_U50 0x5020
>> +static const struct pci_device_id xmgmt_pci_ids[] = {
>> + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
>> + { 0, }
>> +};
>> +
>> +struct xmgmt {
>> + struct pci_dev *pdev;
>> + void *base_partition;
>> +
>> + bool ready;
>> +};
>> +
>> +static int xmgmt_config_pci(struct xmgmt *xm)
>> +{
>> + struct pci_dev *pdev = XMGMT_PDEV(xm);
>> + int rc;
>> +
>> + rc = pcim_enable_device(pdev);
>> + if (rc < 0) {
>> + xmgmt_err(xm, "failed to enable device: %d", rc);
>> + return rc;
>> + }
>> +
>> + rc = pci_enable_pcie_error_reporting(pdev);
>> + if (rc)
>> + xmgmt_warn(xm, "failed to enable AER: %d", rc);
>> +
>> + pci_set_master(pdev);
>> +
>> + rc = pcie_get_readrq(pdev);
>> + if (rc > XRT_MAX_READRQ)
>> + pcie_set_readrq(pdev, XRT_MAX_READRQ);
>> + return 0;
>> +}
>> +
>> +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
>> + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
>> + struct xrt_partition_info xp_info = { 0 };
>> + struct device *dev = &pdev->dev;
>> + int ret, i, idx = 0;
>> + struct xmgmt *xm;
>> +
>> + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
>> + if (!xm)
>> + return -ENOMEM;
>> + xm->pdev = pdev;
>> + pci_set_drvdata(pdev, xm);
>> +
>> + ret = xmgmt_config_pci(xm);
>> + if (ret)
>> + goto failed;
>> +
>> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>> + if (pci_resource_len(pdev, i) > 0) {
>> + ranges[idx].bar_idx = i;
>> + ranges[idx].base = pci_resource_start(pdev, i);
>> + ranges[idx].size = pci_resource_len(pdev, i);
>> + idx++;
>> + }
>> + }
>> + xp_info.num_range = idx;
>> + xp_info.ranges = ranges;
>> + xp_info.fdt = __dtb_dt_test_begin;
>> + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
>> + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
>> + if (ret)
>> + goto failed;
>> +
>> + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
>> + return 0;
>> +
>> +failed:
>> + if (xm->base_partition)
>> + xrt_partition_destroy(xm->base_partition);
>> + pci_set_drvdata(pdev, NULL);
>> + return ret;
>> +}
>> +
>> +static void xmgmt_remove(struct pci_dev *pdev)
>> +{
>> + struct xmgmt *xm = pci_get_drvdata(pdev);
>> +
>> + xrt_partition_destroy(xm->base_partition);
>> + pci_disable_pcie_error_reporting(xm->pdev);
>> + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
>> +}
>> +
>> +static struct pci_driver xmgmt_driver = {
>> + .name = XMGMT_MODULE_NAME,
>> + .id_table = xmgmt_pci_ids,
>> + .probe = xmgmt_probe,
>> + .remove = xmgmt_remove,
>> +};
>> +
>> +static int __init xmgmt_init(void)
>> +{
>> + int res = 0;
>> +
>> + res = pci_register_driver(&xmgmt_driver);
>> + if (res)
>> + return res;
>> +
>> + return 0;
>> +}
>> +
>> +static __exit void xmgmt_exit(void)
>> +{
>> + pci_unregister_driver(&xmgmt_driver);
>> +}
>> +
>> +module_init(xmgmt_init);
>> +module_exit(xmgmt_exit);
>> +
>> +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
>> +MODULE_AUTHOR("XRT Team <[email protected]>");
>> +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.27.0

2022-01-14 01:51:44

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On Thu, Jan 13, 2022 at 03:41:47PM -0800, Lizhi Hou wrote:
>
> On 1/10/22 11:00 PM, Xu Yilun wrote:
> > =
> >
> > On Wed, Jan 05, 2022 at 02:50:13PM -0800, Lizhi Hou wrote:
> > > The PCIE device driver which attaches to management function on Alveo
> > > devices. It instantiates one or more partition. Each partition consists
> > > a set of hardward endpoints. A flat device tree is associated with each
> > > partition. The first version of this driver uses test version flat device
> > > tree and call xrt lib API to unflatten it.
> > >
> > > Signed-off-by: Sonal Santan <[email protected]>
> > > Signed-off-by: Max Zhen <[email protected]>
> > > Signed-off-by: Lizhi Hou <[email protected]>
> > > ---
> > > drivers/fpga/Makefile | 1 +
> > > drivers/fpga/xrt/Kconfig | 1 +
> > > drivers/fpga/xrt/mgmt/Kconfig | 14 +++
> > > drivers/fpga/xrt/mgmt/Makefile | 16 +++
> > > drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
> > > drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
> > > drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
> > > 7 files changed, 217 insertions(+)
> > > create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
> > > create mode 100644 drivers/fpga/xrt/mgmt/Makefile
> > > create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
> > > create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
> > > create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > >
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > index 5bd41cf4c7ec..544e2144878f 100644
> > > --- a/drivers/fpga/Makefile
> > > +++ b/drivers/fpga/Makefile
> > > @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > >
> > > # XRT drivers for Alveo
> > > obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
> > > +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
> > > diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> > > index 04c3bb5aaf4f..50422f77c6df 100644
> > > --- a/drivers/fpga/xrt/Kconfig
> > > +++ b/drivers/fpga/xrt/Kconfig
> > > @@ -4,3 +4,4 @@
> > > #
> > >
> > > source "drivers/fpga/xrt/lib/Kconfig"
> > > +source "drivers/fpga/xrt/mgmt/Kconfig"
> > > diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
> > > new file mode 100644
> > > index 000000000000..a978747482be
> > > --- /dev/null
> > > +++ b/drivers/fpga/xrt/mgmt/Kconfig
> > > @@ -0,0 +1,14 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# Xilinx XRT FPGA device configuration
> > > +#
> > > +
> > > +config FPGA_XRT_XMGMT
> > > + tristate "Xilinx Alveo Management Driver"
> > > + depends on FPGA_XRT_LIB
> > > + select FPGA_BRIDGE
> > > + select FPGA_REGION
> > > + help
> > > + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
> > > + This driver provides interfaces for userspace application to access
> > > + Alveo FPGA device.
> > > diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
> > > new file mode 100644
> > > index 000000000000..c5134bf71cca
> > > --- /dev/null
> > > +++ b/drivers/fpga/xrt/mgmt/Makefile
> > > @@ -0,0 +1,16 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> > > +#
> > > +# Authors: [email protected]
> > > +#
> > > +
> > > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > > +
> > > +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
> > > +
> > > +xrt-mgmt-objs := \
> > > + xmgmt-drv.o \
> > > + dt-test.dtb.o
> > > +
> > > +ccflags-y := -I$(FULL_XRT_PATH)/include
> > > diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
> > > new file mode 100644
> > > index 000000000000..68dbcb7fd79d
> > > --- /dev/null
> > > +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
> > > @@ -0,0 +1,12 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/dts-v1/;
> > > +
> > > +/ {
> > > + compatible = "xlnx,alveo-partition", "simple-bus";
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > + pr_isolate_ulp@0,41000 {
> > > + compatible = "xlnx,alveo-pr-isolation";
> > > + reg = <0x0 0x41000 0x0 0x1000>;
> > > + };
> > > +};
> > I remember Rob's comments:
> >
> > "we'd need to create a base tree (if there isn't one) with nodes
> > for the USB or PCI device(s) and then an overlay for the device can be
> > applied to those nodes."
> >
> > https://lore.kernel.org/linux-fpga/CAL_JsqJfyRymB=VxLuQqLpep+Q1Eie48dobv9sC5OizDz0d2DQ@mail.gmail.com/
> >
> > So could we firstly create a pci device node under the of_root when
> > the driver probing, then add the partition DTs under the pci device node
> > by overlay machenism.
> >
> > I'm considering if finally we could leverage the existing of-fpga-region
> > that reprograms and enumerates FPGA sub devices by overlay.
> >
> > Open for discussion.
>
> I would list the main ideas of our implementation
>
> 1. “alveo-partition (simple-bus)” node is created under of_root in xmgmt
> probe function. “simple-bus” is used as it fits well and we can modify it to
> “fpga-region” when we refactor fpga-mgr/region/bridge part.

OK. "fpga-region" for an independent reprogramable region. And
"simple-bus" for static board components.

> 2. we are the first pcie device using flatten device tree to describe sub
> devices. Adding device tree nodes for all pcie device sounds intrusive. We
> can expend to all pcie device when we have another user of this feature.

I don't mean we create DT nodes for all pcie devices in system, it is the
device driver's choice to create the node for DT style enumeration of its
sub devices. This is mainly for pcie based fpga cards. Now there exsits
plenty of these cards in the world, and the enumeration of their sub
devices could be a common concern. I think it's good we have a clear
design at the beginning.

>
> 3. of_overlay_fdt_apply() does not support overlaying to a dynamic generated

Yes that's the problem we have.

> parent node. Thus, we use of_dynamic functions of_changeset_attach_node()
> instead. (similar to drivers/pci/hotplug/pnv_php.c,) . If needed ,we can
> consider to add parameter “parent node” to of_overlay_fdt_apply() to support
> dynamical overlay case.

I agree we do some investigation for this.

Thanks,
Yilun

>
>
> Thanks,
>
> Lizhi
>
> >
> > Thanks,
> > Yilun
> >
> > > diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
> > > new file mode 100644
> > > index 000000000000..6ec4203afbd2
> > > --- /dev/null
> > > +++ b/drivers/fpga/xrt/mgmt/dt-test.h
> > > @@ -0,0 +1,15 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > > + *
> > > + * Authors:
> > > + * Lizhi Hou <[email protected]>
> > > + */
> > > +
> > > +#ifndef _DT_TEST_H_
> > > +#define _DT_TEST_H_
> > > +
> > > +extern u8 __dtb_dt_test_begin[];
> > > +extern u8 __dtb_dt_test_end[];
> > > +
> > > +#endif /* _DT_TEST_H_ */
> > > diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > > new file mode 100644
> > > index 000000000000..87abe5b86e0b
> > > --- /dev/null
> > > +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > > @@ -0,0 +1,158 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Xilinx Alveo Management Function Driver
> > > + *
> > > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > > + *
> > > + * Authors:
> > > + * Cheng Zhen <[email protected]>
> > > + * Lizhi Hou <[email protected]>
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/aer.h>
> > > +#include <linux/vmalloc.h>
> > > +#include <linux/delay.h>
> > > +#include "xpartition.h"
> > > +#include "dt-test.h"
> > > +
> > > +#define XMGMT_MODULE_NAME "xrt-mgmt"
> > > +
> > > +#define XMGMT_PDEV(xm) ((xm)->pdev)
> > > +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
> > > +#define xmgmt_err(xm, fmt, args...) \
> > > + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > +#define xmgmt_warn(xm, fmt, args...) \
> > > + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > +#define xmgmt_info(xm, fmt, args...) \
> > > + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > +#define xmgmt_dbg(xm, fmt, args...) \
> > > + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > +#define XMGMT_DEV_ID(_pcidev) \
> > > + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
> > > + ((pci_domain_nr((pcidev)->bus) << 16) | \
> > > + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
> > > +
> > > +#define XRT_MAX_READRQ 512
> > > +
> > > +/* PCI Device IDs */
> > > +#define PCI_DEVICE_ID_U50 0x5020
> > > +static const struct pci_device_id xmgmt_pci_ids[] = {
> > > + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
> > > + { 0, }
> > > +};
> > > +
> > > +struct xmgmt {
> > > + struct pci_dev *pdev;
> > > + void *base_partition;
> > > +
> > > + bool ready;
> > > +};
> > > +
> > > +static int xmgmt_config_pci(struct xmgmt *xm)
> > > +{
> > > + struct pci_dev *pdev = XMGMT_PDEV(xm);
> > > + int rc;
> > > +
> > > + rc = pcim_enable_device(pdev);
> > > + if (rc < 0) {
> > > + xmgmt_err(xm, "failed to enable device: %d", rc);
> > > + return rc;
> > > + }
> > > +
> > > + rc = pci_enable_pcie_error_reporting(pdev);
> > > + if (rc)
> > > + xmgmt_warn(xm, "failed to enable AER: %d", rc);
> > > +
> > > + pci_set_master(pdev);
> > > +
> > > + rc = pcie_get_readrq(pdev);
> > > + if (rc > XRT_MAX_READRQ)
> > > + pcie_set_readrq(pdev, XRT_MAX_READRQ);
> > > + return 0;
> > > +}
> > > +
> > > +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > > + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
> > > + struct xrt_partition_info xp_info = { 0 };
> > > + struct device *dev = &pdev->dev;
> > > + int ret, i, idx = 0;
> > > + struct xmgmt *xm;
> > > +
> > > + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
> > > + if (!xm)
> > > + return -ENOMEM;
> > > + xm->pdev = pdev;
> > > + pci_set_drvdata(pdev, xm);
> > > +
> > > + ret = xmgmt_config_pci(xm);
> > > + if (ret)
> > > + goto failed;
> > > +
> > > + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > > + if (pci_resource_len(pdev, i) > 0) {
> > > + ranges[idx].bar_idx = i;
> > > + ranges[idx].base = pci_resource_start(pdev, i);
> > > + ranges[idx].size = pci_resource_len(pdev, i);
> > > + idx++;
> > > + }
> > > + }
> > > + xp_info.num_range = idx;
> > > + xp_info.ranges = ranges;
> > > + xp_info.fdt = __dtb_dt_test_begin;
> > > + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
> > > + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
> > > + if (ret)
> > > + goto failed;
> > > +
> > > + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
> > > + return 0;
> > > +
> > > +failed:
> > > + if (xm->base_partition)
> > > + xrt_partition_destroy(xm->base_partition);
> > > + pci_set_drvdata(pdev, NULL);
> > > + return ret;
> > > +}
> > > +
> > > +static void xmgmt_remove(struct pci_dev *pdev)
> > > +{
> > > + struct xmgmt *xm = pci_get_drvdata(pdev);
> > > +
> > > + xrt_partition_destroy(xm->base_partition);
> > > + pci_disable_pcie_error_reporting(xm->pdev);
> > > + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
> > > +}
> > > +
> > > +static struct pci_driver xmgmt_driver = {
> > > + .name = XMGMT_MODULE_NAME,
> > > + .id_table = xmgmt_pci_ids,
> > > + .probe = xmgmt_probe,
> > > + .remove = xmgmt_remove,
> > > +};
> > > +
> > > +static int __init xmgmt_init(void)
> > > +{
> > > + int res = 0;
> > > +
> > > + res = pci_register_driver(&xmgmt_driver);
> > > + if (res)
> > > + return res;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static __exit void xmgmt_exit(void)
> > > +{
> > > + pci_unregister_driver(&xmgmt_driver);
> > > +}
> > > +
> > > +module_init(xmgmt_init);
> > > +module_exit(xmgmt_exit);
> > > +
> > > +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
> > > +MODULE_AUTHOR("XRT Team <[email protected]>");
> > > +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.27.0

2022-01-18 02:54:25

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On 1/13/22 5:43 PM, Xu Yilun wrote:
> On Thu, Jan 13, 2022 at 03:41:47PM -0800, Lizhi Hou wrote:
>> On 1/10/22 11:00 PM, Xu Yilun wrote:
>>> =
>>>
>>> On Wed, Jan 05, 2022 at 02:50:13PM -0800, Lizhi Hou wrote:
>>>> The PCIE device driver which attaches to management function on Alveo
>>>> devices. It instantiates one or more partition. Each partition consists
>>>> a set of hardward endpoints. A flat device tree is associated with each
>>>> partition. The first version of this driver uses test version flat device
>>>> tree and call xrt lib API to unflatten it.
>>>>
>>>> Signed-off-by: Sonal Santan <[email protected]>
>>>> Signed-off-by: Max Zhen <[email protected]>
>>>> Signed-off-by: Lizhi Hou <[email protected]>
>>>> ---
>>>> drivers/fpga/Makefile | 1 +
>>>> drivers/fpga/xrt/Kconfig | 1 +
>>>> drivers/fpga/xrt/mgmt/Kconfig | 14 +++
>>>> drivers/fpga/xrt/mgmt/Makefile | 16 +++
>>>> drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
>>>> drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
>>>> drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
>>>> 7 files changed, 217 insertions(+)
>>>> create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
>>>> create mode 100644 drivers/fpga/xrt/mgmt/Makefile
>>>> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
>>>> create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
>>>> create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
>>>>
>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>>> index 5bd41cf4c7ec..544e2144878f 100644
>>>> --- a/drivers/fpga/Makefile
>>>> +++ b/drivers/fpga/Makefile
>>>> @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
>>>>
>>>> # XRT drivers for Alveo
>>>> obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
>>>> +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
>>>> diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
>>>> index 04c3bb5aaf4f..50422f77c6df 100644
>>>> --- a/drivers/fpga/xrt/Kconfig
>>>> +++ b/drivers/fpga/xrt/Kconfig
>>>> @@ -4,3 +4,4 @@
>>>> #
>>>>
>>>> source "drivers/fpga/xrt/lib/Kconfig"
>>>> +source "drivers/fpga/xrt/mgmt/Kconfig"
>>>> diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..a978747482be
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/xrt/mgmt/Kconfig
>>>> @@ -0,0 +1,14 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +#
>>>> +# Xilinx XRT FPGA device configuration
>>>> +#
>>>> +
>>>> +config FPGA_XRT_XMGMT
>>>> + tristate "Xilinx Alveo Management Driver"
>>>> + depends on FPGA_XRT_LIB
>>>> + select FPGA_BRIDGE
>>>> + select FPGA_REGION
>>>> + help
>>>> + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
>>>> + This driver provides interfaces for userspace application to access
>>>> + Alveo FPGA device.
>>>> diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
>>>> new file mode 100644
>>>> index 000000000000..c5134bf71cca
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/xrt/mgmt/Makefile
>>>> @@ -0,0 +1,16 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +#
>>>> +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
>>>> +#
>>>> +# Authors: [email protected]
>>>> +#
>>>> +
>>>> +FULL_XRT_PATH=$(srctree)/$(src)/..
>>>> +
>>>> +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
>>>> +
>>>> +xrt-mgmt-objs := \
>>>> + xmgmt-drv.o \
>>>> + dt-test.dtb.o
>>>> +
>>>> +ccflags-y := -I$(FULL_XRT_PATH)/include
>>>> diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
>>>> new file mode 100644
>>>> index 000000000000..68dbcb7fd79d
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
>>>> @@ -0,0 +1,12 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> + compatible = "xlnx,alveo-partition", "simple-bus";
>>>> + #address-cells = <2>;
>>>> + #size-cells = <2>;
>>>> + pr_isolate_ulp@0,41000 {
>>>> + compatible = "xlnx,alveo-pr-isolation";
>>>> + reg = <0x0 0x41000 0x0 0x1000>;
>>>> + };
>>>> +};
>>> I remember Rob's comments:
>>>
>>> "we'd need to create a base tree (if there isn't one) with nodes
>>> for the USB or PCI device(s) and then an overlay for the device can be
>>> applied to those nodes."
>>>
>>> https://lore.kernel.org/linux-fpga/CAL_JsqJfyRymB=VxLuQqLpep+Q1Eie48dobv9sC5OizDz0d2DQ@mail.gmail.com/
>>>
>>> So could we firstly create a pci device node under the of_root when
>>> the driver probing, then add the partition DTs under the pci device node
>>> by overlay machenism.
>>>
>>> I'm considering if finally we could leverage the existing of-fpga-region
>>> that reprograms and enumerates FPGA sub devices by overlay.
>>>
>>> Open for discussion.
>> I would list the main ideas of our implementation
>>
>> 1. “alveo-partition (simple-bus)” node is created under of_root in xmgmt
>> probe function. “simple-bus” is used as it fits well and we can modify it to
>> “fpga-region” when we refactor fpga-mgr/region/bridge part.
> OK. "fpga-region" for an independent reprogramable region. And
> "simple-bus" for static board components.
>
>> 2. we are the first pcie device using flatten device tree to describe sub
>> devices. Adding device tree nodes for all pcie device sounds intrusive. We
>> can expend to all pcie device when we have another user of this feature.
> I don't mean we create DT nodes for all pcie devices in system, it is the
> device driver's choice to create the node for DT style enumeration of its
> sub devices. This is mainly for pcie based fpga cards. Now there exsits
Glad to know this.
> plenty of these cards in the world, and the enumeration of their sub
> devices could be a common concern. I think it's good we have a clear
> design at the beginning.

For single Alveo card on system, we are thinking this structure

of_root -----alveo-part@0 (simple-bus or fpga-region)

             |---alveo-part@1 (fpga-region or fpga-region)

              .....

Are you suggesting to add a pcie device layer. e.g.

of_root----alveo-fpga@<addr>----alveo-part@0

                                                 |--alveo-part@1

Thanks,

Lizhi

>
>> 3. of_overlay_fdt_apply() does not support overlaying to a dynamic generated
> Yes that's the problem we have.
>
>> parent node. Thus, we use of_dynamic functions of_changeset_attach_node()
>> instead. (similar to drivers/pci/hotplug/pnv_php.c,) . If needed ,we can
>> consider to add parameter “parent node” to of_overlay_fdt_apply() to support
>> dynamical overlay case.
> I agree we do some investigation for this.
>
> Thanks,
> Yilun
>
>>
>> Thanks,
>>
>> Lizhi
>>
>>> Thanks,
>>> Yilun
>>>
>>>> diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
>>>> new file mode 100644
>>>> index 000000000000..6ec4203afbd2
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/xrt/mgmt/dt-test.h
>>>> @@ -0,0 +1,15 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Copyright (C) 2020-2022 Xilinx, Inc.
>>>> + *
>>>> + * Authors:
>>>> + * Lizhi Hou <[email protected]>
>>>> + */
>>>> +
>>>> +#ifndef _DT_TEST_H_
>>>> +#define _DT_TEST_H_
>>>> +
>>>> +extern u8 __dtb_dt_test_begin[];
>>>> +extern u8 __dtb_dt_test_end[];
>>>> +
>>>> +#endif /* _DT_TEST_H_ */
>>>> diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
>>>> new file mode 100644
>>>> index 000000000000..87abe5b86e0b
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
>>>> @@ -0,0 +1,158 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Xilinx Alveo Management Function Driver
>>>> + *
>>>> + * Copyright (C) 2020-2022 Xilinx, Inc.
>>>> + *
>>>> + * Authors:
>>>> + * Cheng Zhen <[email protected]>
>>>> + * Lizhi Hou <[email protected]>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/aer.h>
>>>> +#include <linux/vmalloc.h>
>>>> +#include <linux/delay.h>
>>>> +#include "xpartition.h"
>>>> +#include "dt-test.h"
>>>> +
>>>> +#define XMGMT_MODULE_NAME "xrt-mgmt"
>>>> +
>>>> +#define XMGMT_PDEV(xm) ((xm)->pdev)
>>>> +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
>>>> +#define xmgmt_err(xm, fmt, args...) \
>>>> + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>>>> +#define xmgmt_warn(xm, fmt, args...) \
>>>> + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>>>> +#define xmgmt_info(xm, fmt, args...) \
>>>> + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>>>> +#define xmgmt_dbg(xm, fmt, args...) \
>>>> + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
>>>> +#define XMGMT_DEV_ID(_pcidev) \
>>>> + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
>>>> + ((pci_domain_nr((pcidev)->bus) << 16) | \
>>>> + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
>>>> +
>>>> +#define XRT_MAX_READRQ 512
>>>> +
>>>> +/* PCI Device IDs */
>>>> +#define PCI_DEVICE_ID_U50 0x5020
>>>> +static const struct pci_device_id xmgmt_pci_ids[] = {
>>>> + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
>>>> + { 0, }
>>>> +};
>>>> +
>>>> +struct xmgmt {
>>>> + struct pci_dev *pdev;
>>>> + void *base_partition;
>>>> +
>>>> + bool ready;
>>>> +};
>>>> +
>>>> +static int xmgmt_config_pci(struct xmgmt *xm)
>>>> +{
>>>> + struct pci_dev *pdev = XMGMT_PDEV(xm);
>>>> + int rc;
>>>> +
>>>> + rc = pcim_enable_device(pdev);
>>>> + if (rc < 0) {
>>>> + xmgmt_err(xm, "failed to enable device: %d", rc);
>>>> + return rc;
>>>> + }
>>>> +
>>>> + rc = pci_enable_pcie_error_reporting(pdev);
>>>> + if (rc)
>>>> + xmgmt_warn(xm, "failed to enable AER: %d", rc);
>>>> +
>>>> + pci_set_master(pdev);
>>>> +
>>>> + rc = pcie_get_readrq(pdev);
>>>> + if (rc > XRT_MAX_READRQ)
>>>> + pcie_set_readrq(pdev, XRT_MAX_READRQ);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>> +{
>>>> + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
>>>> + struct xrt_partition_info xp_info = { 0 };
>>>> + struct device *dev = &pdev->dev;
>>>> + int ret, i, idx = 0;
>>>> + struct xmgmt *xm;
>>>> +
>>>> + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
>>>> + if (!xm)
>>>> + return -ENOMEM;
>>>> + xm->pdev = pdev;
>>>> + pci_set_drvdata(pdev, xm);
>>>> +
>>>> + ret = xmgmt_config_pci(xm);
>>>> + if (ret)
>>>> + goto failed;
>>>> +
>>>> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>>> + if (pci_resource_len(pdev, i) > 0) {
>>>> + ranges[idx].bar_idx = i;
>>>> + ranges[idx].base = pci_resource_start(pdev, i);
>>>> + ranges[idx].size = pci_resource_len(pdev, i);
>>>> + idx++;
>>>> + }
>>>> + }
>>>> + xp_info.num_range = idx;
>>>> + xp_info.ranges = ranges;
>>>> + xp_info.fdt = __dtb_dt_test_begin;
>>>> + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
>>>> + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
>>>> + if (ret)
>>>> + goto failed;
>>>> +
>>>> + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
>>>> + return 0;
>>>> +
>>>> +failed:
>>>> + if (xm->base_partition)
>>>> + xrt_partition_destroy(xm->base_partition);
>>>> + pci_set_drvdata(pdev, NULL);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void xmgmt_remove(struct pci_dev *pdev)
>>>> +{
>>>> + struct xmgmt *xm = pci_get_drvdata(pdev);
>>>> +
>>>> + xrt_partition_destroy(xm->base_partition);
>>>> + pci_disable_pcie_error_reporting(xm->pdev);
>>>> + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
>>>> +}
>>>> +
>>>> +static struct pci_driver xmgmt_driver = {
>>>> + .name = XMGMT_MODULE_NAME,
>>>> + .id_table = xmgmt_pci_ids,
>>>> + .probe = xmgmt_probe,
>>>> + .remove = xmgmt_remove,
>>>> +};
>>>> +
>>>> +static int __init xmgmt_init(void)
>>>> +{
>>>> + int res = 0;
>>>> +
>>>> + res = pci_register_driver(&xmgmt_driver);
>>>> + if (res)
>>>> + return res;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static __exit void xmgmt_exit(void)
>>>> +{
>>>> + pci_unregister_driver(&xmgmt_driver);
>>>> +}
>>>> +
>>>> +module_init(xmgmt_init);
>>>> +module_exit(xmgmt_exit);
>>>> +
>>>> +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
>>>> +MODULE_AUTHOR("XRT Team <[email protected]>");
>>>> +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> --
>>>> 2.27.0

2022-01-18 03:12:35

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 5/5] fpga: xrt: management physical function driver

On Mon, Jan 17, 2022 at 09:43:40AM -0800, Lizhi Hou wrote:
>
> On 1/13/22 5:43 PM, Xu Yilun wrote:
> > On Thu, Jan 13, 2022 at 03:41:47PM -0800, Lizhi Hou wrote:
> > > On 1/10/22 11:00 PM, Xu Yilun wrote:
> > > > =
> > > >
> > > > On Wed, Jan 05, 2022 at 02:50:13PM -0800, Lizhi Hou wrote:
> > > > > The PCIE device driver which attaches to management function on Alveo
> > > > > devices. It instantiates one or more partition. Each partition consists
> > > > > a set of hardward endpoints. A flat device tree is associated with each
> > > > > partition. The first version of this driver uses test version flat device
> > > > > tree and call xrt lib API to unflatten it.
> > > > >
> > > > > Signed-off-by: Sonal Santan <[email protected]>
> > > > > Signed-off-by: Max Zhen <[email protected]>
> > > > > Signed-off-by: Lizhi Hou <[email protected]>
> > > > > ---
> > > > > drivers/fpga/Makefile | 1 +
> > > > > drivers/fpga/xrt/Kconfig | 1 +
> > > > > drivers/fpga/xrt/mgmt/Kconfig | 14 +++
> > > > > drivers/fpga/xrt/mgmt/Makefile | 16 +++
> > > > > drivers/fpga/xrt/mgmt/dt-test.dts | 12 +++
> > > > > drivers/fpga/xrt/mgmt/dt-test.h | 15 +++
> > > > > drivers/fpga/xrt/mgmt/xmgmt-drv.c | 158 ++++++++++++++++++++++++++++++
> > > > > 7 files changed, 217 insertions(+)
> > > > > create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
> > > > > create mode 100644 drivers/fpga/xrt/mgmt/Makefile
> > > > > create mode 100644 drivers/fpga/xrt/mgmt/dt-test.dts
> > > > > create mode 100644 drivers/fpga/xrt/mgmt/dt-test.h
> > > > > create mode 100644 drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > > > >
> > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > index 5bd41cf4c7ec..544e2144878f 100644
> > > > > --- a/drivers/fpga/Makefile
> > > > > +++ b/drivers/fpga/Makefile
> > > > > @@ -52,3 +52,4 @@ obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > > > >
> > > > > # XRT drivers for Alveo
> > > > > obj-$(CONFIG_FPGA_XRT_LIB) += xrt/lib/
> > > > > +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt/mgmt/
> > > > > diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> > > > > index 04c3bb5aaf4f..50422f77c6df 100644
> > > > > --- a/drivers/fpga/xrt/Kconfig
> > > > > +++ b/drivers/fpga/xrt/Kconfig
> > > > > @@ -4,3 +4,4 @@
> > > > > #
> > > > >
> > > > > source "drivers/fpga/xrt/lib/Kconfig"
> > > > > +source "drivers/fpga/xrt/mgmt/Kconfig"
> > > > > diff --git a/drivers/fpga/xrt/mgmt/Kconfig b/drivers/fpga/xrt/mgmt/Kconfig
> > > > > new file mode 100644
> > > > > index 000000000000..a978747482be
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/xrt/mgmt/Kconfig
> > > > > @@ -0,0 +1,14 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-only
> > > > > +#
> > > > > +# Xilinx XRT FPGA device configuration
> > > > > +#
> > > > > +
> > > > > +config FPGA_XRT_XMGMT
> > > > > + tristate "Xilinx Alveo Management Driver"
> > > > > + depends on FPGA_XRT_LIB
> > > > > + select FPGA_BRIDGE
> > > > > + select FPGA_REGION
> > > > > + help
> > > > > + Select this option to enable XRT PCIe driver for Xilinx Alveo FPGA.
> > > > > + This driver provides interfaces for userspace application to access
> > > > > + Alveo FPGA device.
> > > > > diff --git a/drivers/fpga/xrt/mgmt/Makefile b/drivers/fpga/xrt/mgmt/Makefile
> > > > > new file mode 100644
> > > > > index 000000000000..c5134bf71cca
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/xrt/mgmt/Makefile
> > > > > @@ -0,0 +1,16 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +#
> > > > > +# Copyright (C) 2020-2022 Xilinx, Inc. All rights reserved.
> > > > > +#
> > > > > +# Authors: [email protected]
> > > > > +#
> > > > > +
> > > > > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > > > > +
> > > > > +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-mgmt.o
> > > > > +
> > > > > +xrt-mgmt-objs := \
> > > > > + xmgmt-drv.o \
> > > > > + dt-test.dtb.o
> > > > > +
> > > > > +ccflags-y := -I$(FULL_XRT_PATH)/include
> > > > > diff --git a/drivers/fpga/xrt/mgmt/dt-test.dts b/drivers/fpga/xrt/mgmt/dt-test.dts
> > > > > new file mode 100644
> > > > > index 000000000000..68dbcb7fd79d
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/xrt/mgmt/dt-test.dts
> > > > > @@ -0,0 +1,12 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/dts-v1/;
> > > > > +
> > > > > +/ {
> > > > > + compatible = "xlnx,alveo-partition", "simple-bus";
> > > > > + #address-cells = <2>;
> > > > > + #size-cells = <2>;
> > > > > + pr_isolate_ulp@0,41000 {
> > > > > + compatible = "xlnx,alveo-pr-isolation";
> > > > > + reg = <0x0 0x41000 0x0 0x1000>;
> > > > > + };
> > > > > +};
> > > > I remember Rob's comments:
> > > >
> > > > "we'd need to create a base tree (if there isn't one) with nodes
> > > > for the USB or PCI device(s) and then an overlay for the device can be
> > > > applied to those nodes."
> > > >
> > > > https://lore.kernel.org/linux-fpga/CAL_JsqJfyRymB=VxLuQqLpep+Q1Eie48dobv9sC5OizDz0d2DQ@mail.gmail.com/
> > > >
> > > > So could we firstly create a pci device node under the of_root when
> > > > the driver probing, then add the partition DTs under the pci device node
> > > > by overlay machenism.
> > > >
> > > > I'm considering if finally we could leverage the existing of-fpga-region
> > > > that reprograms and enumerates FPGA sub devices by overlay.
> > > >
> > > > Open for discussion.
> > > I would list the main ideas of our implementation
> > >
> > > 1. “alveo-partition (simple-bus)” node is created under of_root in xmgmt
> > > probe function. “simple-bus” is used as it fits well and we can modify it to
> > > “fpga-region” when we refactor fpga-mgr/region/bridge part.
> > OK. "fpga-region" for an independent reprogramable region. And
> > "simple-bus" for static board components.
> >
> > > 2. we are the first pcie device using flatten device tree to describe sub
> > > devices. Adding device tree nodes for all pcie device sounds intrusive. We
> > > can expend to all pcie device when we have another user of this feature.
> > I don't mean we create DT nodes for all pcie devices in system, it is the
> > device driver's choice to create the node for DT style enumeration of its
> > sub devices. This is mainly for pcie based fpga cards. Now there exsits
> Glad to know this.
> > plenty of these cards in the world, and the enumeration of their sub
> > devices could be a common concern. I think it's good we have a clear
> > design at the beginning.
>
> For single Alveo card on system, we are thinking this structure
>
> of_root -----alveo-part@0 (simple-bus or fpga-region)
>
>              |---alveo-part@1 (fpga-region or fpga-region)
>
>               .....
>
> Are you suggesting to add a pcie device layer. e.g.
>
> of_root----alveo-fpga@<addr>----alveo-part@0
>
>                                                  |--alveo-part@1
>

Yes, my idea is that we don't build the whole PCI/e tree (RPs, Bridges,
switches). Instead we add a PCI/e DT node directly under of_root if the
driver wants to append its DT overlay for its sub devices.

There could be changes to common pci/of or DT domain, so for sure we
need to discuss it with the PCI/DT maintainers.

Thanks,
Yilun

>
> Thanks,
>
> Lizhi
>
> >
> > > 3. of_overlay_fdt_apply() does not support overlaying to a dynamic generated
> > Yes that's the problem we have.
> >
> > > parent node. Thus, we use of_dynamic functions of_changeset_attach_node()
> > > instead. (similar to drivers/pci/hotplug/pnv_php.c,) . If needed ,we can
> > > consider to add parameter “parent node” to of_overlay_fdt_apply() to support
> > > dynamical overlay case.
> > I agree we do some investigation for this.
> >
> > Thanks,
> > Yilun
> >
> > >
> > > Thanks,
> > >
> > > Lizhi
> > >
> > > > Thanks,
> > > > Yilun
> > > >
> > > > > diff --git a/drivers/fpga/xrt/mgmt/dt-test.h b/drivers/fpga/xrt/mgmt/dt-test.h
> > > > > new file mode 100644
> > > > > index 000000000000..6ec4203afbd2
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/xrt/mgmt/dt-test.h
> > > > > @@ -0,0 +1,15 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +/*
> > > > > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > > > > + *
> > > > > + * Authors:
> > > > > + * Lizhi Hou <[email protected]>
> > > > > + */
> > > > > +
> > > > > +#ifndef _DT_TEST_H_
> > > > > +#define _DT_TEST_H_
> > > > > +
> > > > > +extern u8 __dtb_dt_test_begin[];
> > > > > +extern u8 __dtb_dt_test_end[];
> > > > > +
> > > > > +#endif /* _DT_TEST_H_ */
> > > > > diff --git a/drivers/fpga/xrt/mgmt/xmgmt-drv.c b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > > > > new file mode 100644
> > > > > index 000000000000..87abe5b86e0b
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/xrt/mgmt/xmgmt-drv.c
> > > > > @@ -0,0 +1,158 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Xilinx Alveo Management Function Driver
> > > > > + *
> > > > > + * Copyright (C) 2020-2022 Xilinx, Inc.
> > > > > + *
> > > > > + * Authors:
> > > > > + * Cheng Zhen <[email protected]>
> > > > > + * Lizhi Hou <[email protected]>
> > > > > + */
> > > > > +
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/pci.h>
> > > > > +#include <linux/aer.h>
> > > > > +#include <linux/vmalloc.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include "xpartition.h"
> > > > > +#include "dt-test.h"
> > > > > +
> > > > > +#define XMGMT_MODULE_NAME "xrt-mgmt"
> > > > > +
> > > > > +#define XMGMT_PDEV(xm) ((xm)->pdev)
> > > > > +#define XMGMT_DEV(xm) (&(XMGMT_PDEV(xm)->dev))
> > > > > +#define xmgmt_err(xm, fmt, args...) \
> > > > > + dev_err(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > > > +#define xmgmt_warn(xm, fmt, args...) \
> > > > > + dev_warn(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > > > +#define xmgmt_info(xm, fmt, args...) \
> > > > > + dev_info(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > > > +#define xmgmt_dbg(xm, fmt, args...) \
> > > > > + dev_dbg(XMGMT_DEV(xm), "%s: " fmt, __func__, ##args)
> > > > > +#define XMGMT_DEV_ID(_pcidev) \
> > > > > + ({ typeof(_pcidev) (pcidev) = (_pcidev); \
> > > > > + ((pci_domain_nr((pcidev)->bus) << 16) | \
> > > > > + PCI_DEVID((pcidev)->bus->number, (pcidev)->devfn)); })
> > > > > +
> > > > > +#define XRT_MAX_READRQ 512
> > > > > +
> > > > > +/* PCI Device IDs */
> > > > > +#define PCI_DEVICE_ID_U50 0x5020
> > > > > +static const struct pci_device_id xmgmt_pci_ids[] = {
> > > > > + { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
> > > > > + { 0, }
> > > > > +};
> > > > > +
> > > > > +struct xmgmt {
> > > > > + struct pci_dev *pdev;
> > > > > + void *base_partition;
> > > > > +
> > > > > + bool ready;
> > > > > +};
> > > > > +
> > > > > +static int xmgmt_config_pci(struct xmgmt *xm)
> > > > > +{
> > > > > + struct pci_dev *pdev = XMGMT_PDEV(xm);
> > > > > + int rc;
> > > > > +
> > > > > + rc = pcim_enable_device(pdev);
> > > > > + if (rc < 0) {
> > > > > + xmgmt_err(xm, "failed to enable device: %d", rc);
> > > > > + return rc;
> > > > > + }
> > > > > +
> > > > > + rc = pci_enable_pcie_error_reporting(pdev);
> > > > > + if (rc)
> > > > > + xmgmt_warn(xm, "failed to enable AER: %d", rc);
> > > > > +
> > > > > + pci_set_master(pdev);
> > > > > +
> > > > > + rc = pcie_get_readrq(pdev);
> > > > > + if (rc > XRT_MAX_READRQ)
> > > > > + pcie_set_readrq(pdev, XRT_MAX_READRQ);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int xmgmt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > > +{
> > > > > + struct xrt_partition_range ranges[PCI_NUM_RESOURCES];
> > > > > + struct xrt_partition_info xp_info = { 0 };
> > > > > + struct device *dev = &pdev->dev;
> > > > > + int ret, i, idx = 0;
> > > > > + struct xmgmt *xm;
> > > > > +
> > > > > + xm = devm_kzalloc(dev, sizeof(*xm), GFP_KERNEL);
> > > > > + if (!xm)
> > > > > + return -ENOMEM;
> > > > > + xm->pdev = pdev;
> > > > > + pci_set_drvdata(pdev, xm);
> > > > > +
> > > > > + ret = xmgmt_config_pci(xm);
> > > > > + if (ret)
> > > > > + goto failed;
> > > > > +
> > > > > + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > > > > + if (pci_resource_len(pdev, i) > 0) {
> > > > > + ranges[idx].bar_idx = i;
> > > > > + ranges[idx].base = pci_resource_start(pdev, i);
> > > > > + ranges[idx].size = pci_resource_len(pdev, i);
> > > > > + idx++;
> > > > > + }
> > > > > + }
> > > > > + xp_info.num_range = idx;
> > > > > + xp_info.ranges = ranges;
> > > > > + xp_info.fdt = __dtb_dt_test_begin;
> > > > > + xp_info.fdt_len = (u32)(__dtb_dt_test_end - __dtb_dt_test_begin);
> > > > > + ret = xrt_partition_create(&pdev->dev, &xp_info, &xm->base_partition);
> > > > > + if (ret)
> > > > > + goto failed;
> > > > > +
> > > > > + xmgmt_info(xm, "%s started successfully", XMGMT_MODULE_NAME);
> > > > > + return 0;
> > > > > +
> > > > > +failed:
> > > > > + if (xm->base_partition)
> > > > > + xrt_partition_destroy(xm->base_partition);
> > > > > + pci_set_drvdata(pdev, NULL);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static void xmgmt_remove(struct pci_dev *pdev)
> > > > > +{
> > > > > + struct xmgmt *xm = pci_get_drvdata(pdev);
> > > > > +
> > > > > + xrt_partition_destroy(xm->base_partition);
> > > > > + pci_disable_pcie_error_reporting(xm->pdev);
> > > > > + xmgmt_info(xm, "%s cleaned up successfully", XMGMT_MODULE_NAME);
> > > > > +}
> > > > > +
> > > > > +static struct pci_driver xmgmt_driver = {
> > > > > + .name = XMGMT_MODULE_NAME,
> > > > > + .id_table = xmgmt_pci_ids,
> > > > > + .probe = xmgmt_probe,
> > > > > + .remove = xmgmt_remove,
> > > > > +};
> > > > > +
> > > > > +static int __init xmgmt_init(void)
> > > > > +{
> > > > > + int res = 0;
> > > > > +
> > > > > + res = pci_register_driver(&xmgmt_driver);
> > > > > + if (res)
> > > > > + return res;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static __exit void xmgmt_exit(void)
> > > > > +{
> > > > > + pci_unregister_driver(&xmgmt_driver);
> > > > > +}
> > > > > +
> > > > > +module_init(xmgmt_init);
> > > > > +module_exit(xmgmt_exit);
> > > > > +
> > > > > +MODULE_DEVICE_TABLE(pci, xmgmt_pci_ids);
> > > > > +MODULE_AUTHOR("XRT Team <[email protected]>");
> > > > > +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
> > > > > +MODULE_LICENSE("GPL v2");
> > > > > --
> > > > > 2.27.0

2022-01-21 19:57:32

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

Hi Yilun,

Thanks for your comments. Overall, we made the code change based on
Rob's comments on previous patch set. Please see my inline comments for
detail.

Rob, please provide your guidance here.

On 1/10/22 8:29 PM, Xu Yilun wrote:
> On Wed, Jan 05, 2022 at 02:50:11PM -0800, Lizhi Hou wrote:
>> When OF_FLATTREE is selected and there is not a device tree, create an
>> empty device tree root node. of/unittest.c code is referenced.
>>
>> Signed-off-by: Sonal Santan <[email protected]>
>> Signed-off-by: Max Zhen <[email protected]>
>> Signed-off-by: Lizhi Hou <[email protected]>
>> ---
>> drivers/of/Makefile | 5 +++
>> drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
>> drivers/of/fdt_default.dts | 5 +++
>> drivers/of/of_private.h | 17 +++++++
>> drivers/of/unittest.c | 72 ++----------------------------
>> 5 files changed, 120 insertions(+), 69 deletions(-)
>> create mode 100644 drivers/of/fdt_default.dts
>>
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index c13b982084a3..a2989055c578 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0
>> obj-y = base.o device.o platform.o property.o
>> +
> remove the blank line.
Will remove.
>
>> obj-$(CONFIG_OF_KOBJ) += kobj.o
>> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> @@ -20,4 +21,8 @@ obj-y += kexec.o
>> endif
>> endif
>>
>> +ifndef CONFIG_OF_UNITTEST
>> +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
>> +endif
>> +
> Same question as Tom, the unittest should work well with or without
> of_root, is it? So creating an empty root will not affect unittest, so
> why so many ifdefs for CONFIG_OF_UNITTEST?

Based on Rob's comment in
https://lore.kernel.org/lkml/[email protected]/, it
needs to have a unified code to set of_root with or without
CONFIG_OF_UNITTEST defined. So the unified code works as this during boot

1. With CONFIG_OF_UNITEST define, of_root is set to base tree
defined/compiled in testcases.dtb.o

2. Without CONFIG_OF_UNITEST, of_root is set to base tree
defined/compiled in fdt_default.dtb.o

>
>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4546572af24b..66ef9ac97829 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>> }
>> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>
>> +static int __init of_fdt_root_init(void)
>> +{
>> + struct device_node *dt = NULL, *np;
>> + void *fdt = NULL, *fdt_aligned;
>> + struct property *prop = NULL;
>> + __be32 *val = NULL;
>> + int size, rc = 0;
>> +
>> +#if !defined(CONFIG_OF_UNITTEST)
>> + if (of_root)
>> + return 0;
>> +#endif
>> + size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
>> +
>> + fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
>> + if (!fdt)
>> + return -ENOMEM;
>> +
>> + fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
>> + memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
>> +
>> + if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
>> + NULL, &dt)) {
>> + pr_warn("%s: unflatten default tree failed\n", __func__);
>> + kfree(fdt);
>> + return -ENODATA;
>> + }
>> + if (!dt) {
>> + pr_warn("%s: empty default tree\n", __func__);
>> + kfree(fdt);
>> + return -ENODATA;
>> + }
>> +
>> + /*
>> + * This lock normally encloses of_resolve_phandles()
>> + */
>> + of_overlay_mutex_lock();
>> +
>> + rc = of_resolve_phandles(dt);
>> + if (rc) {
>> + pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
>> + goto failed;
>> + }
>> +
>> + if (!of_root) {
>> + prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
>> + if (!prop) {
>> + rc = -ENOMEM;
>> + goto failed;
>> + }
>> + val = kzalloc(sizeof(*val), GFP_KERNEL);
>> + if (!val) {
>> + rc = -ENOMEM;
>> + goto failed;
>> + }
>> + *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
>> +
>> + prop->name = "#address-cells";
>> + prop->value = val;
>> + prop->length = sizeof(u32);
>> + of_add_property(dt, prop);
>> + prop++;
>> + prop->name = "#size-cells";
>> + prop->value = val;
>> + prop->length = sizeof(u32);
>> + of_add_property(dt, prop);
>> + of_root = dt;
>> + for_each_of_allnodes(np)
>> + __of_attach_node_sysfs(np);
>> + of_aliases = of_find_node_by_path("/aliases");
>> + of_chosen = of_find_node_by_path("/chosen");
>> + of_overlay_mutex_unlock();
>> +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
>> + return 0;
>> + }
>> +
>> + unittest_data_add(dt);
> It's confusing to me. If we need to share some functions with unittest,
> make a new clearly defined (and named) function.

unittest_data_add() is not shared function. If CONFIG_OF_UNITTEST is not
defined, this is a null function (please see of_private.h). I just
followed the existing code style. e.g. of_property_notify() in of_private.h.

Would adding some comments to describe this be good enough?

>
>> +
>> + of_overlay_mutex_unlock();
>> +
>> + return 0;
>> +
>> +failed:
>> + of_overlay_mutex_unlock();
>> + kfree(val);
>> + kfree(prop);
>> + return rc;
>> +}
>> +pure_initcall(of_fdt_root_init);
> Is it better we have a new Kconfig option for the empty tree creation.
Sure, if needed.
>
>> +
>> /* Everything below here references initial_boot_params directly. */
>> int __initdata dt_root_addr_cells;
>> int __initdata dt_root_size_cells;
>> diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
>> new file mode 100644
>> index 000000000000..d1f12a76dfc6
>> --- /dev/null
>> +++ b/drivers/of/fdt_default.dts
>> @@ -0,0 +1,5 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +/ {
>> +};
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 631489f7f8c0..1ef93bccfdba 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
>> extern struct list_head aliases_lookup;
>> extern struct kset *of_kset;
>>
>> +#if defined(CONFIG_OF_UNITTEST)
>> +extern u8 __dtb_testcases_begin[];
>> +extern u8 __dtb_testcases_end[];
>> +#define __dtb_fdt_default_begin __dtb_testcases_begin
>> +#define __dtb_fdt_default_end __dtb_testcases_end
> Maybe we don't have to use the test dt data, stick to the default empty
> fdt is fine?

I am not sure I understand the point. test dt data contains a lot test
nodes and we should not create those nodes if CONFIG_OF_UNITTEST is not
defined.

Are you asking that we create empty of_root here and test nodes are
created in unittest.c? I believe that was we tried to do with previous
patch. Is this the same ask within your second comment?

We are open to change it back as previous patch does if needed.

Rob, do you have any comment here?

>
>> +void __init unittest_data_add(struct device_node *dt);
>> +#else
>> +extern u8 __dtb_fdt_default_begin[];
>> +extern u8 __dtb_fdt_default_end[];
>> +static inline void unittest_data_add(struct device_node *dt) {}
>> +#endif
>> +
>> #if defined(CONFIG_OF_DYNAMIC)
>> extern int of_property_notify(int action, struct device_node *np,
>> struct property *prop, struct property *old_prop);
>> @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
>>
>> #if defined(CONFIG_OF_RESOLVE)
>> int of_resolve_phandles(struct device_node *tree);
>> +#else
>> +static inline int of_resolve_phandles(struct device_node *tree)
>> +{
>> + return 0;
>> +}
> If we have an empty of_resolve_phandles, does the empty tree creation
> still works? Or if we don't need this func, just delete in the code.

test nodes creation requires of_resolve_phandles() and creating empty
of_root does not. This define is added for unifying the code.

Thanks,

Lizhi

>
> Thanks,
> Yilun
>
>> #endif
>>
>> void __of_phandle_cache_inv_entry(phandle handle);
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 8c056972a6dd..745f455235cc 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
>> * unittest_data_add - Reads, copies data from
>> * linked tree and attaches it to the live tree
>> */
>> -static int __init unittest_data_add(void)
>> +void __init unittest_data_add(struct device_node *dt)
>> {
>> - void *unittest_data;
>> - void *unittest_data_align;
>> - struct device_node *unittest_data_node = NULL, *np;
>> - /*
>> - * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
>> - * created by cmd_dt_S_dtb in scripts/Makefile.lib
>> - */
>> - extern uint8_t __dtb_testcases_begin[];
>> - extern uint8_t __dtb_testcases_end[];
>> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
>> - int rc;
>> - void *ret;
>> -
>> - if (!size) {
>> - pr_warn("%s: testcases is empty\n", __func__);
>> - return -ENODATA;
>> - }
>> -
>> - /* creating copy */
>> - unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
>> - if (!unittest_data)
>> - return -ENOMEM;
>> -
>> - unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> - memcpy(unittest_data_align, __dtb_testcases_begin, size);
>> -
>> - ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
>> - if (!ret) {
>> - pr_warn("%s: unflatten testcases tree failed\n", __func__);
>> - kfree(unittest_data);
>> - return -ENODATA;
>> - }
>> - if (!unittest_data_node) {
>> - pr_warn("%s: testcases tree is empty\n", __func__);
>> - kfree(unittest_data);
>> - return -ENODATA;
>> - }
>> -
>> - /*
>> - * This lock normally encloses of_resolve_phandles()
>> - */
>> - of_overlay_mutex_lock();
>> -
>> - rc = of_resolve_phandles(unittest_data_node);
>> - if (rc) {
>> - pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
>> - of_overlay_mutex_unlock();
>> - return -EINVAL;
>> - }
>> -
>> - if (!of_root) {
>> - of_root = unittest_data_node;
>> - for_each_of_allnodes(np)
>> - __of_attach_node_sysfs(np);
>> - of_aliases = of_find_node_by_path("/aliases");
>> - of_chosen = of_find_node_by_path("/chosen");
>> - of_overlay_mutex_unlock();
>> - return 0;
>> - }
>> + struct device_node *np;
>>
>> EXPECT_BEGIN(KERN_INFO,
>> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
>>
>> /* attach the sub-tree to live tree */
>> - np = unittest_data_node->child;
>> + np = dt->child;
>> while (np) {
>> struct device_node *next = np->sibling;
>>
>> @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
>>
>> EXPECT_END(KERN_INFO,
>> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
>> -
>> - of_overlay_mutex_unlock();
>> -
>> - return 0;
>> }
>>
>> #ifdef CONFIG_OF_OVERLAY
>> @@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
>> static int __init of_unittest(void)
>> {
>> struct device_node *np;
>> - int res;
>>
>> pr_info("start of unittest - you will see error messages\n");
>>
>> @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
>> if (IS_ENABLED(CONFIG_UML))
>> unittest_unflatten_overlay_base();
>>
>> - res = unittest_data_add();
>> - if (res)
>> - return res;
>> if (!of_aliases)
>> of_aliases = of_find_node_by_path("/aliases");
>>
>> --
>> 2.27.0

2022-01-22 00:28:59

[permalink] [raw]

Subject: Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

On Wed, Jan 19, 2022 at 10:59:48AM -0800, Lizhi Hou wrote:
> Hi Yilun,
>
> Thanks for your comments. Overall, we made the code change based on Rob's
> comments on previous patch set. Please see my inline comments for detail.
>
> Rob, please provide your guidance here.
>
> On 1/10/22 8:29 PM, Xu Yilun wrote:
> > On Wed, Jan 05, 2022 at 02:50:11PM -0800, Lizhi Hou wrote:
> > > When OF_FLATTREE is selected and there is not a device tree, create an
> > > empty device tree root node. of/unittest.c code is referenced.
> > >
> > > Signed-off-by: Sonal Santan <[email protected]>
> > > Signed-off-by: Max Zhen <[email protected]>
> > > Signed-off-by: Lizhi Hou <[email protected]>
> > > ---
> > > drivers/of/Makefile | 5 +++
> > > drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
> > > drivers/of/fdt_default.dts | 5 +++
> > > drivers/of/of_private.h | 17 +++++++
> > > drivers/of/unittest.c | 72 ++----------------------------
> > > 5 files changed, 120 insertions(+), 69 deletions(-)
> > > create mode 100644 drivers/of/fdt_default.dts
> > >
> > > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > > index c13b982084a3..a2989055c578 100644
> > > --- a/drivers/of/Makefile
> > > +++ b/drivers/of/Makefile
> > > @@ -1,5 +1,6 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > > obj-y = base.o device.o platform.o property.o
> > > +
> > remove the blank line.
> Will remove.
> >
> > > obj-$(CONFIG_OF_KOBJ) += kobj.o
> > > obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> > > obj-$(CONFIG_OF_FLATTREE) += fdt.o
> > > @@ -20,4 +21,8 @@ obj-y += kexec.o
> > > endif
> > > endif
> > >
> > > +ifndef CONFIG_OF_UNITTEST
> > > +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
> > > +endif
> > > +
> > Same question as Tom, the unittest should work well with or without
> > of_root, is it? So creating an empty root will not affect unittest, so
> > why so many ifdefs for CONFIG_OF_UNITTEST?
>
> Based on Rob's comment in
> https://lore.kernel.org/lkml/[email protected]/, it needs
> to have a unified code to set of_root with or without CONFIG_OF_UNITTEST
> defined.? So the unified code works as this during boot
>
> 1. With CONFIG_OF_UNITEST define, of_root is set to base tree
> defined/compiled in testcases.dtb.o
>
> 2. Without CONFIG_OF_UNITEST, of_root is set to base tree defined/compiled
> in fdt_default.dtb.o
>
> >
> > > obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 4546572af24b..66ef9ac97829 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> > > }
> > > EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> > >
> > > +static int __init of_fdt_root_init(void)
> > > +{
> > > + struct device_node *dt = NULL, *np;
> > > + void *fdt = NULL, *fdt_aligned;
> > > + struct property *prop = NULL;
> > > + __be32 *val = NULL;
> > > + int size, rc = 0;
> > > +
> > > +#if !defined(CONFIG_OF_UNITTEST)
> > > + if (of_root)
> > > + return 0;
> > > +#endif
> > > + size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
> > > +
> > > + fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> > > + if (!fdt)
> > > + return -ENOMEM;
> > > +
> > > + fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
> > > + memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
> > > +
> > > + if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
> > > + NULL, &dt)) {
> > > + pr_warn("%s: unflatten default tree failed\n", __func__);
> > > + kfree(fdt);
> > > + return -ENODATA;
> > > + }
> > > + if (!dt) {
> > > + pr_warn("%s: empty default tree\n", __func__);
> > > + kfree(fdt);
> > > + return -ENODATA;
> > > + }
> > > +
> > > + /*
> > > + * This lock normally encloses of_resolve_phandles()
> > > + */
> > > + of_overlay_mutex_lock();
> > > +
> > > + rc = of_resolve_phandles(dt);
> > > + if (rc) {
> > > + pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> > > + goto failed;
> > > + }
> > > +
> > > + if (!of_root) {
> > > + prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
> > > + if (!prop) {
> > > + rc = -ENOMEM;
> > > + goto failed;
> > > + }
> > > + val = kzalloc(sizeof(*val), GFP_KERNEL);
> > > + if (!val) {
> > > + rc = -ENOMEM;
> > > + goto failed;
> > > + }
> > > + *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
> > > +
> > > + prop->name = "#address-cells";
> > > + prop->value = val;
> > > + prop->length = sizeof(u32);
> > > + of_add_property(dt, prop);
> > > + prop++;
> > > + prop->name = "#size-cells";
> > > + prop->value = val;
> > > + prop->length = sizeof(u32);
> > > + of_add_property(dt, prop);
> > > + of_root = dt;
> > > + for_each_of_allnodes(np)
> > > + __of_attach_node_sysfs(np);
> > > + of_aliases = of_find_node_by_path("/aliases");
> > > + of_chosen = of_find_node_by_path("/chosen");
> > > + of_overlay_mutex_unlock();
> > > +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
> > > + return 0;
> > > + }
> > > +
> > > + unittest_data_add(dt);
> > It's confusing to me. If we need to share some functions with unittest,
> > make a new clearly defined (and named) function.
>
> unittest_data_add() is not shared function. If CONFIG_OF_UNITTEST is not
> defined, this is a null function (please see of_private.h). I just followed
> the existing code style. e.g. of_property_notify() in of_private.h.
>
> Would adding some comments to describe this be good enough?
>
> >
> > > +
> > > + of_overlay_mutex_unlock();
> > > +
> > > + return 0;
> > > +
> > > +failed:
> > > + of_overlay_mutex_unlock();
> > > + kfree(val);
> > > + kfree(prop);
> > > + return rc;
> > > +}
> > > +pure_initcall(of_fdt_root_init);
> > Is it better we have a new Kconfig option for the empty tree creation.
> Sure, if needed.
> >
> > > +
> > > /* Everything below here references initial_boot_params directly. */
> > > int __initdata dt_root_addr_cells;
> > > int __initdata dt_root_size_cells;
> > > diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
> > > new file mode 100644
> > > index 000000000000..d1f12a76dfc6
> > > --- /dev/null
> > > +++ b/drivers/of/fdt_default.dts
> > > @@ -0,0 +1,5 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/dts-v1/;
> > > +
> > > +/ {
> > > +};
> > > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> > > index 631489f7f8c0..1ef93bccfdba 100644
> > > --- a/drivers/of/of_private.h
> > > +++ b/drivers/of/of_private.h
> > > @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
> > > extern struct list_head aliases_lookup;
> > > extern struct kset *of_kset;
> > >
> > > +#if defined(CONFIG_OF_UNITTEST)
> > > +extern u8 __dtb_testcases_begin[];
> > > +extern u8 __dtb_testcases_end[];
> > > +#define __dtb_fdt_default_begin __dtb_testcases_begin
> > > +#define __dtb_fdt_default_end __dtb_testcases_end
> > Maybe we don't have to use the test dt data, stick to the default empty
> > fdt is fine?
>
> I am not sure I understand the point. test dt data contains a lot test nodes
> and we should not create those nodes if CONFIG_OF_UNITTEST is not defined.
>
> Are you asking that we create empty of_root here and test nodes are created
> in unittest.c? I believe that was we tried to do with previous patch. Is
> this the same ask within your second comment?

Yes, generally this is what I mean. I think you may have some
misunderstanding for Rob's comments. My understanding is, to move the code
from unittest to the of core, refactor them and make clearly defined
functions, and let unittest call these functions.

It is generally not reasonable the core uses help functions or test data
from unittest.

>
> We are open to change it back as previous patch does if needed.
>
> Rob, do you have any comment here?
>
> >
> > > +void __init unittest_data_add(struct device_node *dt);
> > > +#else
> > > +extern u8 __dtb_fdt_default_begin[];
> > > +extern u8 __dtb_fdt_default_end[];
> > > +static inline void unittest_data_add(struct device_node *dt) {}
> > > +#endif
> > > +
> > > #if defined(CONFIG_OF_DYNAMIC)
> > > extern int of_property_notify(int action, struct device_node *np,
> > > struct property *prop, struct property *old_prop);
> > > @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
> > >
> > > #if defined(CONFIG_OF_RESOLVE)
> > > int of_resolve_phandles(struct device_node *tree);
> > > +#else
> > > +static inline int of_resolve_phandles(struct device_node *tree)
> > > +{
> > > + return 0;
> > > +}
> > If we have an empty of_resolve_phandles, does the empty tree creation
> > still works? Or if we don't need this func, just delete in the code.
>
> test nodes creation requires of_resolve_phandles() and creating empty
> of_root does not. This define is added for unifying the code.

If you don't need the of_resolve_phandles in core code, don't call it.

Thanks,
Yilun

>
>
> Thanks,
>
> Lizhi
>
> >
> > Thanks,
> > Yilun
> >
> > > #endif
> > >
> > > void __of_phandle_cache_inv_entry(phandle handle);
> > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > index 8c056972a6dd..745f455235cc 100644
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
> > > @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
> > > * unittest_data_add - Reads, copies data from
> > > * linked tree and attaches it to the live tree
> > > */
> > > -static int __init unittest_data_add(void)
> > > +void __init unittest_data_add(struct device_node *dt)
> > > {
> > > - void *unittest_data;
> > > - void *unittest_data_align;
> > > - struct device_node *unittest_data_node = NULL, *np;
> > > - /*
> > > - * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> > > - * created by cmd_dt_S_dtb in scripts/Makefile.lib
> > > - */
> > > - extern uint8_t __dtb_testcases_begin[];
> > > - extern uint8_t __dtb_testcases_end[];
> > > - const int size = __dtb_testcases_end - __dtb_testcases_begin;
> > > - int rc;
> > > - void *ret;
> > > -
> > > - if (!size) {
> > > - pr_warn("%s: testcases is empty\n", __func__);
> > > - return -ENODATA;
> > > - }
> > > -
> > > - /* creating copy */
> > > - unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> > > - if (!unittest_data)
> > > - return -ENOMEM;
> > > -
> > > - unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> > > - memcpy(unittest_data_align, __dtb_testcases_begin, size);
> > > -
> > > - ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> > > - if (!ret) {
> > > - pr_warn("%s: unflatten testcases tree failed\n", __func__);
> > > - kfree(unittest_data);
> > > - return -ENODATA;
> > > - }
> > > - if (!unittest_data_node) {
> > > - pr_warn("%s: testcases tree is empty\n", __func__);
> > > - kfree(unittest_data);
> > > - return -ENODATA;
> > > - }
> > > -
> > > - /*
> > > - * This lock normally encloses of_resolve_phandles()
> > > - */
> > > - of_overlay_mutex_lock();
> > > -
> > > - rc = of_resolve_phandles(unittest_data_node);
> > > - if (rc) {
> > > - pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> > > - of_overlay_mutex_unlock();
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (!of_root) {
> > > - of_root = unittest_data_node;
> > > - for_each_of_allnodes(np)
> > > - __of_attach_node_sysfs(np);
> > > - of_aliases = of_find_node_by_path("/aliases");
> > > - of_chosen = of_find_node_by_path("/chosen");
> > > - of_overlay_mutex_unlock();
> > > - return 0;
> > > - }
> > > + struct device_node *np;
> > >
> > > EXPECT_BEGIN(KERN_INFO,
> > > "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
> > >
> > > /* attach the sub-tree to live tree */
> > > - np = unittest_data_node->child;
> > > + np = dt->child;
> > > while (np) {
> > > struct device_node *next = np->sibling;
> > >
> > > @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
> > >
> > > EXPECT_END(KERN_INFO,
> > > "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
> > > -
> > > - of_overlay_mutex_unlock();
> > > -
> > > - return 0;
> > > }
> > >
> > > #ifdef CONFIG_OF_OVERLAY
> > > @@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
> > > static int __init of_unittest(void)
> > > {
> > > struct device_node *np;
> > > - int res;
> > >
> > > pr_info("start of unittest - you will see error messages\n");
> > >
> > > @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
> > > if (IS_ENABLED(CONFIG_UML))
> > > unittest_unflatten_overlay_base();
> > >
> > > - res = unittest_data_add();
> > > - if (res)
> > > - return res;
> > > if (!of_aliases)
> > > of_aliases = of_find_node_by_path("/aliases");
> > >
> > > --
> > > 2.27.0

2022-01-22 02:01:13