Re: [PATCH V4 XRT Alveo 09/20] fpga: xrt: management physical function driver (root)
Hi Tom,
On 3/31/21 6:03 AM, Tom Rix wrote:
On 3/23/21 10:29 PM, Lizhi Hou wrote:
The PCIE device driver which attaches to management function on Alveo
devices. It instantiates one or more group drivers which, in turn,
instantiate platform drivers. The instantiation of group and platform
drivers is completely dtb driven.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/mgmt/root.c | 333 +++
1 file changed, 333 insertions(+)
create mode 100644 drivers/fpga/xrt/mgmt/root.c
diff --git a/drivers/fpga/xrt/mgmt/root.c b/drivers/fpga/xrt/mgmt/root.c
new file mode 100644
index ..f97f92807c01
--- /dev/null
+++ b/drivers/fpga/xrt/mgmt/root.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo Management Function Driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "xroot.h"
+#include "xmgnt.h"
+#include "metadata.h"
+
+#define XMGMT_MODULE_NAME"xrt-mgmt"
ok
+#define XMGMT_DRIVER_VERSION "4.0.0"
+
+#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, 0)); })
+
+static struct class *xmgmt_class;
+
+/* PCI Device IDs */
add a comment on what a golden image is here something like
/*
* Golden image is preloaded on the device when it is shipped to customer.
* Then, customer can load other shells (from Xilinx or some other vendor).
* If something goes wrong with the shell, customer can always go back to
* golden and start over again.
*/
Will do.
+#define PCI_DEVICE_ID_U50_GOLDEN 0xD020
+#define PCI_DEVICE_ID_U500x5020
+static const struct pci_device_id xmgmt_pci_ids[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50_GOLDEN), }, /* Alveo
U50 (golden) */
+ { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_U50), }, /* Alveo U50 */
+ { 0, }
+};
+
+struct xmgmt {
+ struct pci_dev *pdev;
+ void *root;
+
+ 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)
ok
+ xmgmt_warn(xm, "failed to enable AER: %d", rc);
+
+ pci_set_master(pdev);
+
+ rc = pcie_get_readrq(pdev);
+ if (rc > 512)
512 is magic number, change this to a #define
Will do.
+ pcie_set_readrq(pdev, 512);
+ return 0;
+}
+
+static int xmgmt_match_slot_and_save(struct device *dev, void *data)
+{
+ struct xmgmt *xm = data;
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (XMGMT_DEV_ID(pdev) == XMGMT_DEV_ID(xm->pdev)) {
+ pci_cfg_access_lock(pdev);
+ pci_save_state(pdev);
+ }
+
+ return 0;
+}
+
+static void xmgmt_pci_save_config_all(struct xmgmt *xm)
+{
+ bus_for_each_dev(_bus_type, NULL, xm, xmgmt_match_slot_and_save);
refactor expected in v5 when pseudo bus change happens.
There might be some mis-understanding here...
No matter how we reorganize our code (using platform_device bus type or
defining our own bus type), it's a driver that drives a PCIE device
after all. So, this mgmt/root.c must be a PCIE driver, which may
interact with a whole bunch of IP drivers through a pseudo bus we are
about to create.
What this code is doing here is completely of PCIE business (PCIE config
space access). So, I think it is appropriate code in a PCIE driver.
The PCIE device we are driving is a multi-function device. The mgmt pf
is of function 0, which, according to PCIE spec, can manage other
functions on the same device. So, I think it's appropriate for mgmt pf
driver (this root driver) to find it's peer function (through PCIE bus
type) on the same device and do something about it in certain special cases.
Please let me know why you expect this code to be refactored and how you
want it to be refactored. I might have missed something here...
+}
+
+static int xmgmt_match_slot_and_resto
Re: [PATCH V4 XRT Alveo 08/20] fpga: xrt: platform driver infrastructure
Hi Tom,
On 3/31/21 5:50 AM, Tom Rix wrote:
Several just for debugging items, consider adding a CONFIG_XRT_DEBUGGING
I'd like to clarify what "only for debugging" means here. It actually
means that the content of the msg/output only makes sense to a
developer, v.s. end user. It does not mean that only developer will ever
execute this code path which triggers the debugging code.
We have msg from print functions like this, and we have output from
sysfs node like this. We can't just disable all of them by default
because the content only makes sense to a developer. In some cases,
requiring a recompilation of the driver to enable the debugging code is
very difficult to do. E.g., when we need to debug a customer issue and
we do not have access to the system. It's a big ask for our customer to
recompile, reload the driver and reproduce the issue for us (v.s. just
collect and send us the msg/output).
On 3/23/21 10:29 PM, Lizhi Hou wrote:
Infrastructure code providing APIs for managing leaf driver instance
groups, facilitating inter-leaf driver calls and root calls.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/lib/subdev.c | 865 ++
1 file changed, 865 insertions(+)
create mode 100644 drivers/fpga/xrt/lib/subdev.c
diff --git a/drivers/fpga/xrt/lib/subdev.c b/drivers/fpga/xrt/lib/subdev.c
new file mode 100644
index ..6428b183fee3
--- /dev/null
+++ b/drivers/fpga/xrt/lib/subdev.c
@@ -0,0 +1,865 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include
+#include
+#include "xleaf.h"
+#include "subdev_pool.h"
+#include "lib-drv.h"
+#include "metadata.h"
+
+#define IS_ROOT_DEV(dev) ((dev)->bus == _bus_type)
for readablity, add a new line here
Will do.
+static inline struct device *find_root(struct platform_device *pdev)
+{
+ struct device *d = DEV(pdev);
+
+ while (!IS_ROOT_DEV(d))
+ d = d->parent;
+ return d;
+}
+
+/*
+ * It represents a holder of a subdev. One holder can repeatedly hold a subdev
+ * as long as there is a unhold corresponding to a hold.
+ */
+struct xrt_subdev_holder {
+ struct list_head xsh_holder_list;
+ struct device *xsh_holder;
+ int xsh_count;
+ struct kref xsh_kref;
+};
+
+/*
+ * It represents a specific instance of platform driver for a subdev, which
+ * provides services to its clients (another subdev driver or root driver).
+ */
+struct xrt_subdev {
+ struct list_head xs_dev_list;
+ struct list_head xs_holder_list;
+ enum xrt_subdev_id xs_id; /* type of subdev */
+ struct platform_device *xs_pdev;/* a particular subdev inst */
+ struct completion xs_holder_comp;
+};
+
+static struct xrt_subdev *xrt_subdev_alloc(void)
+{
+ struct xrt_subdev *sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
ok
+
+ if (!sdev)
+ return NULL;
+
+ INIT_LIST_HEAD(>xs_dev_list);
+ INIT_LIST_HEAD(>xs_holder_list);
+ init_completion(>xs_holder_comp);
+ return sdev;
+}
+
+static void xrt_subdev_free(struct xrt_subdev *sdev)
+{
+ kfree(sdev);
Abstraction for a single function is not needed, use kfree directly.
Will do.
+}
+
+int xrt_subdev_root_request(struct platform_device *self, u32 cmd, void *arg)
+{
+ struct device *dev = DEV(self);
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(self);
+
+ WARN_ON(!pdata->xsp_root_cb);
ok
+ return (*pdata->xsp_root_cb)(dev->parent, pdata->xsp_root_cb_arg, cmd,
arg);
+}
+
+/*
+ * Subdev common sysfs nodes.
+ */
+static ssize_t holders_show(struct device *dev, struct device_attribute *attr,
char *buf)
+{
+ ssize_t len;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct xrt_root_get_holders holders = { pdev, buf, 1024 };
Since 1024 is config, #define it somewhere so it can be tweeked later
Will do.
+
+ len = xrt_subdev_root_request(pdev, XRT_ROOT_GET_LEAF_HOLDERS, );
+ if (len >= holders.xpigh_holder_buf_len)
+ return len;
+ buf[len] = '\n';
+ return len + 1;
+}
+static DEVICE_ATTR_RO(holders);
+
+static struct attribute *xrt_subdev_attrs[] = {
+ _attr_holders.attr,
+ NULL,
+};
+
+static ssize_t metadata_output(struct file *filp, struct kobject *kobj,
+struct bin_attribute *attr, char *buf, loff_t off,
size_t count)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct platform_device *pdev = to_platform_device(dev);
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
+ unsigned char *blob;
+ unsigned long size;
+ ssize_t ret = 0;
+
+ blob = pdata->xsp_dtb;
+ size = xrt_md_size(dev, blob);
+ if (size == XRT_MD_INVALID_LENGTH) {
+ ret = -EINVAL;
+ goto failed;
+ }
+
+ i
Re: [PATCH V4 XRT Alveo 05/20] fpga: xrt: group platform driver
Hi Tom,
On 3/30/21 5:52 AM, Tom Rix wrote:
On 3/23/21 10:29 PM, Lizhi Hou wrote:
group driver that manages life cycle of a bunch of leaf driver instances
and bridges them with root.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/group.h | 25 +++
drivers/fpga/xrt/lib/group.c | 286 +++
2 files changed, 311 insertions(+)
create mode 100644 drivers/fpga/xrt/include/group.h
create mode 100644 drivers/fpga/xrt/lib/group.c
diff --git a/drivers/fpga/xrt/include/group.h b/drivers/fpga/xrt/include/group.h
new file mode 100644
index ..09e9d03f53fe
--- /dev/null
+++ b/drivers/fpga/xrt/include/group.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
ok, removed generic boilerplate
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_GROUP_H_
+#define _XRT_GROUP_H_
+
+#include "xleaf.h"
move header to another patch
Yes, the header is moved to 04/20 patch.
+
+/*
+ * Group driver leaf calls.
ok
+ */
+enum xrt_group_leaf_cmd {
+ XRT_GROUP_GET_LEAF = XRT_XLEAF_CUSTOM_BASE, /* See comments in xleaf.h */
ok
+ XRT_GROUP_PUT_LEAF,
+ XRT_GROUP_INIT_CHILDREN,
+ XRT_GROUP_FINI_CHILDREN,
+ XRT_GROUP_TRIGGER_EVENT,
+};
+
+#endif /* _XRT_GROUP_H_ */
diff --git a/drivers/fpga/xrt/lib/group.c b/drivers/fpga/xrt/lib/group.c
new file mode 100644
index ..7b8716569641
--- /dev/null
+++ b/drivers/fpga/xrt/lib/group.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo FPGA Group Driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include
+#include "xleaf.h"
+#include "subdev_pool.h"
+#include "group.h"
+#include "metadata.h"
+#include "lib-drv.h"
+
+#define XRT_GRP "xrt_group"
+
+struct xrt_group {
+ struct platform_device *pdev;
+ struct xrt_subdev_pool leaves;
+ bool leaves_created;
+ struct mutex lock; /* lock for group */
+};
+
+static int xrt_grp_root_cb(struct device *dev, void *parg,
+enum xrt_root_cmd cmd, void *arg)
ok
+{
+ int rc;
+ struct platform_device *pdev =
+ container_of(dev, struct platform_device, dev);
+ struct xrt_group *xg = (struct xrt_group *)parg;
+
+ switch (cmd) {
+ case XRT_ROOT_GET_LEAF_HOLDERS: {
+ struct xrt_root_get_holders *holders =
+ (struct xrt_root_get_holders *)arg;
+ rc = xrt_subdev_pool_get_holders(>leaves,
+ holders->xpigh_pdev,
+ holders->xpigh_holder_buf,
+ holders->xpigh_holder_buf_len);
+ break;
+ }
+ default:
+ /* Forward parent call to root. */
+ rc = xrt_subdev_root_request(pdev, cmd, arg);
+ break;
+ }
+
+ return rc;
+}
+
+/*
+ * Cut subdev's dtb from group's dtb based on passed-in endpoint descriptor.
+ * Return the subdev's dtb through dtbp, if found.
+ */
+static int xrt_grp_cut_subdev_dtb(struct xrt_group *xg, struct
xrt_subdev_endpoints *eps,
+ char *grp_dtb, char **dtbp)
+{
+ int ret, i, ep_count = 0;
+ char *dtb = NULL;
+
+ ret = xrt_md_create(DEV(xg->pdev), );
+ if (ret)
+ return ret;
+
+ for (i = 0; eps->xse_names[i].ep_name || eps->xse_names[i].regmap_name;
i++) {
+ const char *ep_name = eps->xse_names[i].ep_name;
+ const char *reg_name = eps->xse_names[i].regmap_name;
+
+ if (!ep_name)
+ xrt_md_get_compatible_endpoint(DEV(xg->pdev), grp_dtb,
reg_name, _name);
+ if (!ep_name)
+ continue;
+
+ ret = xrt_md_copy_endpoint(DEV(xg->pdev), dtb, grp_dtb, ep_name,
reg_name, NULL);
+ if (ret)
+ continue;
+ xrt_md_del_endpoint(DEV(xg->pdev), grp_dtb, ep_name, reg_name);
+ ep_count++;
+ }
+ /* Found enough endpoints, return the subdev's dtb. */
+ if (ep_count >= eps->xse_min_ep) {
+ *dtbp = dtb;
+ return 0;
+ }
+
+ /* Cleanup - Restore all endpoints that has been deleted, if any. */
+ if (ep_count > 0) {
+ xrt_md_copy_endpoint(DEV(xg->pdev), grp_dtb, dtb,
+ XRT_MD_NODE_ENDPOINTS, NULL, NULL);
+ }
+ vfree(dtb);
+ *dtbp = NULL;
+ return 0;
+}
+
+static int xrt_grp_create_leaves(struct xrt_group *xg)
+{
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(xg->pdev);
+ struct xrt_subdev_endpoints *eps = NULL;
+ int ret = 0, failed = 0;
+ enum xrt_subdev_id did;
+ char *grp_dtb = NULL;
+ unsigned long mlen;
+
+
Re: [PATCH V4 XRT Alveo 04/20] fpga: xrt: xrt-lib platform driver manager
Hi Tom,
On 3/29/21 12:44 PM, Tom Rix wrote:
bisectablity may be/is an issue.
Moritz,
building happens on the last patch, so in theory there will never be a build
break needing bisection. Do we care about the misordering of serveral of these
patches?
The general idea about ordering of patches is that global defines should
be introduced before the user.
On 3/23/21 10:29 PM, Lizhi Hou wrote:
xrt-lib kernel module infrastructure code to register and manage all
leaf driver modules.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/subdev_id.h | 38
drivers/fpga/xrt/include/xleaf.h | 264 +
drivers/fpga/xrt/lib/lib-drv.c | 277 +++
ok
drivers/fpga/xrt/lib/lib-drv.h | 17 ++
4 files changed, 596 insertions(+)
create mode 100644 drivers/fpga/xrt/include/subdev_id.h
create mode 100644 drivers/fpga/xrt/include/xleaf.h
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/xrt/include/subdev_id.h
b/drivers/fpga/xrt/include/subdev_id.h
new file mode 100644
index ..42fbd6f5e80a
--- /dev/null
+++ b/drivers/fpga/xrt/include/subdev_id.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_SUBDEV_ID_H_
+#define _XRT_SUBDEV_ID_H_
+
+/*
+ * Every subdev driver has an ID for others to refer to it. There can be
multiple number of
+ * instances of a subdev driver. A tuple is a
unique identification
+ * of a specific instance of a subdev driver.
+ */
+enum xrt_subdev_id {
+ XRT_SUBDEV_GRP = 0,
not necessary to initialize all unless there are gaps.
Yeah, just trying to avoid any issue when things are accidentally reordered.
+ XRT_SUBDEV_VSEC = 1,
+ XRT_SUBDEV_VSEC_GOLDEN = 2,
+ XRT_SUBDEV_DEVCTL = 3,
+ XRT_SUBDEV_AXIGATE = 4,
+ XRT_SUBDEV_ICAP = 5,
+ XRT_SUBDEV_TEST = 6,
+ XRT_SUBDEV_MGMT_MAIN = 7,
+ XRT_SUBDEV_QSPI = 8,
+ XRT_SUBDEV_MAILBOX = 9,
+ XRT_SUBDEV_CMC = 10,
+ XRT_SUBDEV_CALIB = 11,
+ XRT_SUBDEV_CLKFREQ = 12,
+ XRT_SUBDEV_CLOCK = 13,
+ XRT_SUBDEV_SRSR = 14,
+ XRT_SUBDEV_UCS = 15,
+ XRT_SUBDEV_NUM = 16, /* Total number of subdevs. */
+ XRT_ROOT = -1, /* Special ID for root driver. */
+};
+
+#endif /* _XRT_SUBDEV_ID_H_ */
diff --git a/drivers/fpga/xrt/include/xleaf.h b/drivers/fpga/xrt/include/xleaf.h
new file mode 100644
index ..acb500df04b0
--- /dev/null
+++ b/drivers/fpga/xrt/include/xleaf.h
@@ -0,0 +1,264 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *Cheng Zhen
+ *Sonal Santan
+ */
+
+#ifndef _XRT_XLEAF_H_
+#define _XRT_XLEAF_H_
+
+#include
+#include
+#include
+#include "subdev_id.h"
+#include "xroot.h"
+#include "events.h"
+
+/* All subdev drivers should use below common routines to print out msg. */
+#define DEV(pdev)(&(pdev)->dev)
+#define DEV_PDATA(pdev) \
+ ((struct xrt_subdev_platdata *)dev_get_platdata(DEV(pdev)))
+#define DEV_DRVDATA(pdev)\
+ ((struct xrt_subdev_drvdata *) \
+ platform_get_device_id(pdev)->driver_data)
+#define FMT_PRT(prt_fn, pdev, fmt, args...) \
+ ({typeof(pdev) (_pdev) = (pdev);\
+ prt_fn(DEV(_pdev), "%s %s: " fmt, \
+ DEV_PDATA(_pdev)->xsp_root_name, __func__, ##args); })
+#define xrt_err(pdev, fmt, args...) FMT_PRT(dev_err, pdev, fmt, ##args)
+#define xrt_warn(pdev, fmt, args...) FMT_PRT(dev_warn, pdev, fmt, ##args)
+#define xrt_info(pdev, fmt, args...) FMT_PRT(dev_info, pdev, fmt, ##args)
+#define xrt_dbg(pdev, fmt, args...) FMT_PRT(dev_dbg, pdev, fmt, ##args)
+
+enum {
+ /* Starting cmd for common leaf cmd implemented by all leaves. */
+ XRT_XLEAF_COMMON_BASE = 0,
+ /* Starting cmd for leaves' specific leaf cmds. */
+ XRT_XLEAF_CUSTOM_BASE = 64,
+};
+
+enum xrt_xleaf_common_leaf_cmd {
+ XRT_XLEAF_EVENT = XRT_XLEAF_COMMON_BASE,
+};
+
+/*
+ * If populated by subdev driver, infra will handle the mechanics of
+ * char device (un)registration.
+ */
+enum xrt_subdev_file_mode {
+ /* Infra create cdev, default file name */
+ XRT_SUBDEV_FILE_DEFAULT = 0,
+ /* Infra create cdev, need to encode inst num in file name */
+ XRT_SUBDEV_FILE_MULTI_INST,
+ /* No auto creation of cdev by infra, leaf handles it by itself */
+ XRT_SUBDEV_FILE_NO_AUTO,
+};
+
+struct xrt_subdev_file_ops {
+ const struct file_operations xsf_ops;
+ dev_t xsf_dev_t;
+ const char *xsf_dev_name;
+ enum xrt_subdev_file_mode xsf_mode;
+};
+
+/*
+ * Subdev driver callbacks populated by subdev driver.
+ */
+struct xrt_subdev_drv_ops {
+ /*
+
Re: [PATCH V4 XRT Alveo 06/20] fpga: xrt: char dev node helper functions
On 3/30/21 6:45 AM, Tom Rix wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
It is unclear from the changelog if this new patch was split from an existing
patch or new content.
the file ops seem to come from mgmnt/main.c, which call what could be file ops
here. why is this complicated redirection needed ?
This is part of infra code which is split from subdev.c, not from
mgmt/main.c. Sorry about the confusion. We further split infra code to
avoid having one huge patch for review.
On 3/23/21 10:29 PM, Lizhi Hou wrote:
Helper functions for char device node creation / removal for platform
drivers. This is part of platform driver infrastructure.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/lib/cdev.c | 232
1 file changed, 232 insertions(+)
create mode 100644 drivers/fpga/xrt/lib/cdev.c
diff --git a/drivers/fpga/xrt/lib/cdev.c b/drivers/fpga/xrt/lib/cdev.c
new file mode 100644
index ..38efd24b6e10
--- /dev/null
+++ b/drivers/fpga/xrt/lib/cdev.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo FPGA device node helper functions.
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include "xleaf.h"
+
+extern struct class *xrt_class;
+
+#define XRT_CDEV_DIR "xfpga"
maybe "xrt_fpga" or just "xrt"
Will change it to just "xrt", yes.
+#define INODE2PDATA(inode) \
+ container_of((inode)->i_cdev, struct xrt_subdev_platdata, xsp_cdev)
+#define INODE2PDEV(inode)\
+ to_platform_device(kobj_to_dev((inode)->i_cdev->kobj.parent))
+#define CDEV_NAME(sysdev)(strchr((sysdev)->kobj.name, '!') + 1)
+
+/* Allow it to be accessed from cdev. */
+static void xleaf_devnode_allowed(struct platform_device *pdev)
+{
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
+
+ /* Allow new opens. */
+ mutex_lock(>xsp_devnode_lock);
+ pdata->xsp_devnode_online = true;
+ mutex_unlock(>xsp_devnode_lock);
+}
+
+/* Turn off access from cdev and wait for all existing user to go away. */
+static int xleaf_devnode_disallowed(struct platform_device *pdev)
+{
+ int ret = 0;
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
+
+ mutex_lock(>xsp_devnode_lock);
+
+ /* Prevent new opens. */
+ pdata->xsp_devnode_online = false;
+ /* Wait for existing user to close. */
+ while (!ret && pdata->xsp_devnode_ref) {
+ int rc;
+
+ mutex_unlock(>xsp_devnode_lock);
+ rc = wait_for_completion_killable(>xsp_devnode_comp);
+ mutex_lock(>xsp_devnode_lock);
+
+ if (rc == -ERESTARTSYS) {
+ /* Restore online state. */
+ pdata->xsp_devnode_online = true;
+ xrt_err(pdev, "%s is in use, ref=%d",
+ CDEV_NAME(pdata->xsp_sysdev),
+ pdata->xsp_devnode_ref);
+ ret = -EBUSY;
+ }
+ }
+
+ mutex_unlock(>xsp_devnode_lock);
+
+ return ret;
+}
+
+static struct platform_device *
+__xleaf_devnode_open(struct inode *inode, bool excl)
+{
+ struct xrt_subdev_platdata *pdata = INODE2PDATA(inode);
+ struct platform_device *pdev = INODE2PDEV(inode);
+ bool opened = false;
+
+ mutex_lock(>xsp_devnode_lock);
+
+ if (pdata->xsp_devnode_online) {
+ if (excl && pdata->xsp_devnode_ref) {
+ xrt_err(pdev, "%s has already been opened exclusively",
+ CDEV_NAME(pdata->xsp_sysdev));
+ } else if (!excl && pdata->xsp_devnode_excl) {
+ xrt_err(pdev, "%s has been opened exclusively",
+ CDEV_NAME(pdata->xsp_sysdev));
+ } else {
+ pdata->xsp_devnode_ref++;
+ pdata->xsp_devnode_excl = excl;
+ opened = true;
+ xrt_info(pdev, "opened %s, ref=%d",
+ CDEV_NAME(pdata->xsp_sysdev),
+ pdata->xsp_devnode_ref);
+ }
+ } else {
+ xrt_err(pdev, "%s is offline", CDEV_NAME(pdata->xsp_sysdev));
+ }
+
+ mutex_unlock(>xsp_devnode_lock);
+
+ pdev = opened ? pdev : NULL;
+ return pdev;
+}
+
+struct platform_device *
+xleaf_devnode_open_excl(struct inode *inode)
+{
+ return __xleaf_devnode_open(inode, true);
+}
This function is unused, remove.
This is part of infra's API for leaf driver to call. The caller has been
removed from this initial patch set to reduce the s
Re: [PATCH V4 XRT Alveo 07/20] fpga: xrt: root driver infrastructure
Hi Tom,
On 3/30/21 8:11 AM, Tom Rix wrote:
This was split from 'fpga: xrt: platform driver infrastructure'
and fpga: xrt: managment physical function driver (root)
Yes, trying not to have huge patch for review.
On 3/23/21 10:29 PM, Lizhi Hou wrote:
Contains common code for all root drivers and handles root calls from
platform drivers. This is part of root driver infrastructure.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/events.h | 45 +++
drivers/fpga/xrt/include/xroot.h | 117 ++
drivers/fpga/xrt/lib/subdev_pool.h | 53 +++
drivers/fpga/xrt/lib/xroot.c | 589 +
4 files changed, 804 insertions(+)
create mode 100644 drivers/fpga/xrt/include/events.h
create mode 100644 drivers/fpga/xrt/include/xroot.h
create mode 100644 drivers/fpga/xrt/lib/subdev_pool.h
create mode 100644 drivers/fpga/xrt/lib/xroot.c
diff --git a/drivers/fpga/xrt/include/events.h
b/drivers/fpga/xrt/include/events.h
new file mode 100644
index ..775171a47c8e
--- /dev/null
+++ b/drivers/fpga/xrt/include/events.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
ok
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_EVENTS_H_
+#define _XRT_EVENTS_H_
ok
+
+#include "subdev_id.h"
+
+/*
+ * Event notification.
+ */
+enum xrt_events {
+ XRT_EVENT_TEST = 0, /* for testing */
+ /*
+ * Events related to specific subdev
+ * Callback arg: struct xrt_event_arg_subdev
+ */
+ XRT_EVENT_POST_CREATION,
+ XRT_EVENT_PRE_REMOVAL,
+ /*
+ * Events related to change of the whole board
+ * Callback arg:
+ */
+ XRT_EVENT_PRE_HOT_RESET,
+ XRT_EVENT_POST_HOT_RESET,
+ XRT_EVENT_PRE_GATE_CLOSE,
+ XRT_EVENT_POST_GATE_OPEN,
+};
+
+struct xrt_event_arg_subdev {
+ enum xrt_subdev_id xevt_subdev_id;
+ int xevt_subdev_instance;
+};
+
+struct xrt_event {
+ enum xrt_events xe_evt;
+ struct xrt_event_arg_subdev xe_subdev;
+};
+
+#endif /* _XRT_EVENTS_H_ */
diff --git a/drivers/fpga/xrt/include/xroot.h b/drivers/fpga/xrt/include/xroot.h
new file mode 100644
index ..91c0aeb30bf8
--- /dev/null
+++ b/drivers/fpga/xrt/include/xroot.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_ROOT_H_
+#define _XRT_ROOT_H_
+
+#include
+#include
+#include "subdev_id.h"
+#include "events.h"
+
+typedef bool (*xrt_subdev_match_t)(enum xrt_subdev_id,
+ struct platform_device *, void *);
+#define XRT_SUBDEV_MATCH_PREV((xrt_subdev_match_t)-1)
+#define XRT_SUBDEV_MATCH_NEXT((xrt_subdev_match_t)-2)
+
+/*
+ * Root calls.
+ */
+enum xrt_root_cmd {
+ /* Leaf actions. */
+ XRT_ROOT_GET_LEAF = 0,
+ XRT_ROOT_PUT_LEAF,
+ XRT_ROOT_GET_LEAF_HOLDERS,
+
+ /* Group actions. */
+ XRT_ROOT_CREATE_GROUP,
+ XRT_ROOT_REMOVE_GROUP,
+ XRT_ROOT_LOOKUP_GROUP,
+ XRT_ROOT_WAIT_GROUP_BRINGUP,
+
+ /* Event actions. */
+ XRT_ROOT_EVENT_SYNC,
+ XRT_ROOT_EVENT_ASYNC,
+
+ /* Device info. */
+ XRT_ROOT_GET_RESOURCE,
+ XRT_ROOT_GET_ID,
+
+ /* Misc. */
+ XRT_ROOT_HOT_RESET,
+ XRT_ROOT_HWMON,
+};
+
+struct xrt_root_get_leaf {
+ struct platform_device *xpigl_caller_pdev;
+ xrt_subdev_match_t xpigl_match_cb;
+ void *xpigl_match_arg;
+ struct platform_device *xpigl_tgt_pdev;
+};
+
+struct xrt_root_put_leaf {
+ struct platform_device *xpipl_caller_pdev;
+ struct platform_device *xpipl_tgt_pdev;
+};
+
+struct xrt_root_lookup_group {
+ struct platform_device *xpilp_pdev; /* caller's pdev */
+ xrt_subdev_match_t xpilp_match_cb;
+ void *xpilp_match_arg;
+ int xpilp_grp_inst;
+};
+
+struct xrt_root_get_holders {
+ struct platform_device *xpigh_pdev; /* caller's pdev */
+ char *xpigh_holder_buf;
+ size_t xpigh_holder_buf_len;
+};
+
+struct xrt_root_get_res {
+ struct resource *xpigr_res;
+};
+
+struct xrt_root_get_id {
+ unsigned short xpigi_vendor_id;
+ unsigned short xpigi_device_id;
+ unsigned short xpigi_sub_vendor_id;
+ unsigned short xpigi_sub_device_id;
+};
+
+struct xrt_root_hwmon {
+ bool xpih_register;
+ const char *xpih_name;
+ void *xpih_drvdata;
+ const struct attribute_group **xpih_groups;
+ struct device *xpih_hwmon_dev;
+};
+
+/*
+ * Callback for leaf to make a root request. Arguments are: parent device,
parent cookie, req,
+ * and arg.
+ */
+typedef int (*xrt_subdev_root_cb_t)(struct device *, void *, u32, void *);
+int xrt_subdev_root_request(struct platform_device *self, u32 cmd, void *arg);
+
+/*
+ * Defines physical function (MPF / UPF) specific operations
+ * needed in common root driver.
+ */
+struct xroot_physical_function_callback {
+ void (*xpc_hot_reset)(struct pci_dev *pdev);
Re: [PATCH V3 XRT Alveo 07/18] fpga: xrt: management physical function driver (root)
On 3/17/21 2:08 PM, Tom Rix wrote:
On 3/16/21 1:29 PM, Max Zhen wrote:
Hi Tom,
On 2/26/21 7:01 AM, Tom Rix wrote:
A question i do not know the answer to.
Seems like 'golden' is linked to a manufacturing (diagnostics?) image.
If the public will never see it, should handling it here be done ?
Moritz, do you know ?
Golden image is preloaded on the device when it is shipped to customer. Then,
customer can load other shells (from Xilinx or some other vendor). If something
goes wrong with the shell, customer can always go back to golden and start over
again. So, golden image is going to be used in public, not just internally by
Xilinx.
Thanks for the explanation.
On 2/17/21 10:40 PM, Lizhi Hou wrote:
The PCIE device driver which attaches to management function on Alveo
to the management
Sure.
devices. It instantiates one or more partition drivers which in turn
more fpga partition / group ?
Group driver.
instantiate platform drivers. The instantiation of partition and platform
drivers is completely data driven.
data driven ? everything is data driven. do you mean dtb driven ?
Data driven means not hard-coded. Here data means meta data which is presented
in device tree format, dtb.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/xroot.h | 114 +++
drivers/fpga/xrt/mgmt/root.c | 342 +++
2 files changed, 456 insertions(+)
create mode 100644 drivers/fpga/xrt/include/xroot.h
create mode 100644 drivers/fpga/xrt/mgmt/root.c
diff --git a/drivers/fpga/xrt/include/xroot.h b/drivers/fpga/xrt/include/xroot.h
new file mode 100644
index ..752e10daa85e
--- /dev/null
+++ b/drivers/fpga/xrt/include/xroot.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_ROOT_H_
+#define _XRT_ROOT_H_
+
+#include
+#include "subdev_id.h"
+#include "events.h"
+
+typedef bool (*xrt_subdev_match_t)(enum xrt_subdev_id,
+ struct platform_device *, void *);
+#define XRT_SUBDEV_MATCH_PREV((xrt_subdev_match_t)-1)
+#define XRT_SUBDEV_MATCH_NEXT((xrt_subdev_match_t)-2)
+
+/*
+ * Root IOCTL calls.
+ */
+enum xrt_root_ioctl_cmd {
+ /* Leaf actions. */
+ XRT_ROOT_GET_LEAF = 0,
+ XRT_ROOT_PUT_LEAF,
+ XRT_ROOT_GET_LEAF_HOLDERS,
+
+ /* Group actions. */
+ XRT_ROOT_CREATE_GROUP,
+ XRT_ROOT_REMOVE_GROUP,
+ XRT_ROOT_LOOKUP_GROUP,
+ XRT_ROOT_WAIT_GROUP_BRINGUP,
+
+ /* Event actions. */
+ XRT_ROOT_EVENT,
should this be XRT_ROOT_EVENT_SYNC ?
Sure.
+ XRT_ROOT_EVENT_ASYNC,
+
+ /* Device info. */
+ XRT_ROOT_GET_RESOURCE,
+ XRT_ROOT_GET_ID,
+
+ /* Misc. */
+ XRT_ROOT_HOT_RESET,
+ XRT_ROOT_HWMON,
+};
+
+struct xrt_root_ioctl_get_leaf {
+ struct platform_device *xpigl_pdev; /* caller's pdev */
xpigl_ ? unneeded suffix in element names
It's needed since the it might be included and used in > 1 .c files. I'd like
to keep it's name unique.
This is an element name, the variable name sound be unique enough to make it
clear.
This is not a critical issue, ok as-is.
+ xrt_subdev_match_t xpigl_match_cb;
+ void *xpigl_match_arg;
+ struct platform_device *xpigl_leaf; /* target leaf pdev */
+};
+
+struct xrt_root_ioctl_put_leaf {
+ struct platform_device *xpipl_pdev; /* caller's pdev */
+ struct platform_device *xpipl_leaf; /* target's pdev */
caller_pdev;
target_pdev;
Sure.
+};
+
+struct xrt_root_ioctl_lookup_group {
+ struct platform_device *xpilp_pdev; /* caller's pdev */
+ xrt_subdev_match_t xpilp_match_cb;
+ void *xpilp_match_arg;
+ int xpilp_grp_inst;
+};
+
+struct xrt_root_ioctl_get_holders {
+ struct platform_device *xpigh_pdev; /* caller's pdev */
+ char *xpigh_holder_buf;
+ size_t xpigh_holder_buf_len;
+};
+
+struct xrt_root_ioctl_get_res {
+ struct resource *xpigr_res;
+};
+
+struct xrt_root_ioctl_get_id {
+ unsigned short xpigi_vendor_id;
+ unsigned short xpigi_device_id;
+ unsigned short xpigi_sub_vendor_id;
+ unsigned short xpigi_sub_device_id;
+};
+
+struct xrt_root_ioctl_hwmon {
+ bool xpih_register;
+ const char *xpih_name;
+ void *xpih_drvdata;
+ const struct attribute_group **xpih_groups;
+ struct device *xpih_hwmon_dev;
+};
+
+typedef int (*xrt_subdev_root_cb_t)(struct device *, void *, u32, void *);
This function pointer type is important, please add a comment about its use and
expected parameters
Added.
+int xrt_subdev_root_request(struct platform_device *self, u32 cmd, void *arg);
+
+/*
+ * Defines physical function (MPF / UPF) specific operations
+ * needed in common root driver.
+ */
+struct xroot_pf_cb {
+ void (*xpc_hot_reset)(struct pci_dev *pdev);
This is only ever set to xm
Re: [PATCH V3 XRT Alveo 07/18] fpga: xrt: management physical function driver (root)
Hi Tom,
On 2/26/21 7:01 AM, Tom Rix wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
A question i do not know the answer to.
Seems like 'golden' is linked to a manufacturing (diagnostics?) image.
If the public will never see it, should handling it here be done ?
Moritz, do you know ?
Golden image is preloaded on the device when it is shipped to customer.
Then, customer can load other shells (from Xilinx or some other vendor).
If something goes wrong with the shell, customer can always go back to
golden and start over again. So, golden image is going to be used in
public, not just internally by Xilinx.
On 2/17/21 10:40 PM, Lizhi Hou wrote:
The PCIE device driver which attaches to management function on Alveo
to the management
Sure.
devices. It instantiates one or more partition drivers which in turn
more fpga partition / group ?
Group driver.
instantiate platform drivers. The instantiation of partition and platform
drivers is completely data driven.
data driven ? everything is data driven. do you mean dtb driven ?
Data driven means not hard-coded. Here data means meta data which is
presented in device tree format, dtb.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/xroot.h | 114 +++
drivers/fpga/xrt/mgmt/root.c | 342 +++
2 files changed, 456 insertions(+)
create mode 100644 drivers/fpga/xrt/include/xroot.h
create mode 100644 drivers/fpga/xrt/mgmt/root.c
diff --git a/drivers/fpga/xrt/include/xroot.h b/drivers/fpga/xrt/include/xroot.h
new file mode 100644
index ..752e10daa85e
--- /dev/null
+++ b/drivers/fpga/xrt/include/xroot.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_ROOT_H_
+#define _XRT_ROOT_H_
+
+#include
+#include "subdev_id.h"
+#include "events.h"
+
+typedef bool (*xrt_subdev_match_t)(enum xrt_subdev_id,
+ struct platform_device *, void *);
+#define XRT_SUBDEV_MATCH_PREV((xrt_subdev_match_t)-1)
+#define XRT_SUBDEV_MATCH_NEXT((xrt_subdev_match_t)-2)
+
+/*
+ * Root IOCTL calls.
+ */
+enum xrt_root_ioctl_cmd {
+ /* Leaf actions. */
+ XRT_ROOT_GET_LEAF = 0,
+ XRT_ROOT_PUT_LEAF,
+ XRT_ROOT_GET_LEAF_HOLDERS,
+
+ /* Group actions. */
+ XRT_ROOT_CREATE_GROUP,
+ XRT_ROOT_REMOVE_GROUP,
+ XRT_ROOT_LOOKUP_GROUP,
+ XRT_ROOT_WAIT_GROUP_BRINGUP,
+
+ /* Event actions. */
+ XRT_ROOT_EVENT,
should this be XRT_ROOT_EVENT_SYNC ?
Sure.
+ XRT_ROOT_EVENT_ASYNC,
+
+ /* Device info. */
+ XRT_ROOT_GET_RESOURCE,
+ XRT_ROOT_GET_ID,
+
+ /* Misc. */
+ XRT_ROOT_HOT_RESET,
+ XRT_ROOT_HWMON,
+};
+
+struct xrt_root_ioctl_get_leaf {
+ struct platform_device *xpigl_pdev; /* caller's pdev */
xpigl_ ? unneeded suffix in element names
It's needed since the it might be included and used in > 1 .c files. I'd
like to keep it's name unique.
+ xrt_subdev_match_t xpigl_match_cb;
+ void *xpigl_match_arg;
+ struct platform_device *xpigl_leaf; /* target leaf pdev */
+};
+
+struct xrt_root_ioctl_put_leaf {
+ struct platform_device *xpipl_pdev; /* caller's pdev */
+ struct platform_device *xpipl_leaf; /* target's pdev */
caller_pdev;
target_pdev;
Sure.
+};
+
+struct xrt_root_ioctl_lookup_group {
+ struct platform_device *xpilp_pdev; /* caller's pdev */
+ xrt_subdev_match_t xpilp_match_cb;
+ void *xpilp_match_arg;
+ int xpilp_grp_inst;
+};
+
+struct xrt_root_ioctl_get_holders {
+ struct platform_device *xpigh_pdev; /* caller's pdev */
+ char *xpigh_holder_buf;
+ size_t xpigh_holder_buf_len;
+};
+
+struct xrt_root_ioctl_get_res {
+ struct resource *xpigr_res;
+};
+
+struct xrt_root_ioctl_get_id {
+ unsigned short xpigi_vendor_id;
+ unsigned short xpigi_device_id;
+ unsigned short xpigi_sub_vendor_id;
+ unsigned short xpigi_sub_device_id;
+};
+
+struct xrt_root_ioctl_hwmon {
+ bool xpih_register;
+ const char *xpih_name;
+ void *xpih_drvdata;
+ const struct attribute_group **xpih_groups;
+ struct device *xpih_hwmon_dev;
+};
+
+typedef int (*xrt_subdev_root_cb_t)(struct device *, void *, u32, void *);
This function pointer type is important, please add a comment about its use and
expected parameters
Added.
+int xrt_subdev_root_request(struct platform_device *self, u32 cmd, void *arg);
+
+/*
+ * Defines physical function (MPF / UPF) specific operations
+ * needed in common root driver.
+ */
+struct xroot_pf_cb {
+ void (*xpc_hot_reset)(struct pci_dev *pdev);
This is only ever set to xmgmt_root_hot_reset, why is this abstraction needed ?
As
Re: [PATCH V3 XRT Alveo 06/18] fpga: xrt: platform driver infrastructure
Hi Tom,
On 2/25/21 1:59 PM, Tom Rix wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
On 2/17/21 10:40 PM, Lizhi Hou wrote:
infrastructure code providing APIs for managing leaf driver instance
groups, facilitating inter-leaf driver calls and root calls, managing leaf
driver device nodes.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/events.h| 48 ++
drivers/fpga/xrt/include/subdev_id.h | 43 ++
drivers/fpga/xrt/include/xleaf.h | 276 +
drivers/fpga/xrt/lib/cdev.c | 231 +++
drivers/fpga/xrt/lib/subdev.c| 871 +++
drivers/fpga/xrt/lib/subdev_pool.h | 53 ++
drivers/fpga/xrt/lib/xroot.c | 598 ++
7 files changed, 2120 insertions(+)
create mode 100644 drivers/fpga/xrt/include/events.h
create mode 100644 drivers/fpga/xrt/include/subdev_id.h
create mode 100644 drivers/fpga/xrt/include/xleaf.h
create mode 100644 drivers/fpga/xrt/lib/cdev.c
create mode 100644 drivers/fpga/xrt/lib/subdev.c
create mode 100644 drivers/fpga/xrt/lib/subdev_pool.h
create mode 100644 drivers/fpga/xrt/lib/xroot.c
diff --git a/drivers/fpga/xrt/include/events.h
b/drivers/fpga/xrt/include/events.h
new file mode 100644
index ..2a9aae8bceb4
--- /dev/null
+++ b/drivers/fpga/xrt/include/events.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
general problem with generic, low information comments
This is removed.
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_EVENTS_H_
+#define _XRT_EVENTS_H_
+
+#include
why is platform_device.h needed ?
It is not needed. Removed.
+#include "subdev_id.h"
+
+/*
+ * Event notification.
+ */
+enum xrt_events {
+ XRT_EVENT_TEST = 0, /* for testing */
+ /*
+ * Events related to specific subdev
+ * Callback arg: struct xrt_event_arg_subdev
+ */
+ XRT_EVENT_POST_CREATION,
+ XRT_EVENT_PRE_REMOVAL,
+ /*
+ * Events related to change of the whole board
+ * Callback arg:
+ */
+ XRT_EVENT_PRE_HOT_RESET,
+ XRT_EVENT_POST_HOT_RESET,
+ XRT_EVENT_PRE_GATE_CLOSE,
+ XRT_EVENT_POST_GATE_OPEN,
+};
+
+struct xrt_event_arg_subdev {
+ enum xrt_subdev_id xevt_subdev_id;
+ int xevt_subdev_instance;
+};
+
+struct xrt_event {
+ enum xrt_events xe_evt;
+ struct xrt_event_arg_subdev xe_subdev;
+};
+
+#endif /* _XRT_EVENTS_H_ */
diff --git a/drivers/fpga/xrt/include/subdev_id.h
b/drivers/fpga/xrt/include/subdev_id.h
new file mode 100644
index ..6205a9f26196
--- /dev/null
+++ b/drivers/fpga/xrt/include/subdev_id.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_SUBDEV_ID_H_
+#define _XRT_SUBDEV_ID_H_
+
+/*
+ * Every subdev driver should have an ID for others to refer to it.
driver has an ID
Sure.
+ * There can be unlimited number of instances of a subdev driver. A
unlimited? change to 'multiple'
Sure.
+ * tuple should be a unique identification of
tuple is a unique
Sure.
+ * a specific instance of a subdev driver.
+ * NOTE: PLEASE do not change the order of IDs. Sub devices in the same
+ * group are initialized by this order.
why does the order matter? the enums are all initialized
Right. Will remove this statement.
+ */
+enum xrt_subdev_id {
+ XRT_SUBDEV_GRP = 0,
+ XRT_SUBDEV_VSEC = 1,
+ XRT_SUBDEV_VSEC_GOLDEN = 2,
+ XRT_SUBDEV_DEVCTL = 3,
+ XRT_SUBDEV_AXIGATE = 4,
+ XRT_SUBDEV_ICAP = 5,
+ XRT_SUBDEV_TEST = 6,
+ XRT_SUBDEV_MGMT_MAIN = 7,
+ XRT_SUBDEV_QSPI = 8,
+ XRT_SUBDEV_MAILBOX = 9,
+ XRT_SUBDEV_CMC = 10,
+ XRT_SUBDEV_CALIB = 11,
+ XRT_SUBDEV_CLKFREQ = 12,
+ XRT_SUBDEV_CLOCK = 13,
+ XRT_SUBDEV_SRSR = 14,
+ XRT_SUBDEV_UCS = 15,
+ XRT_SUBDEV_NUM = 16, /* Total number of subdevs. */
+ XRT_ROOT = -1, /* Special ID for root driver. */
+};
+
+#endif /* _XRT_SUBDEV_ID_H_ */
diff --git a/drivers/fpga/xrt/include/xleaf.h b/drivers/fpga/xrt/include/xleaf.h
new file mode 100644
index ..10215a75d474
--- /dev/null
+++ b/drivers/fpga/xrt/include/xleaf.h
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *Cheng Zhen
+ *Sonal Santan
+ */
+
+#ifndef _XRT_XLEAF_H_
+#define _XRT_XLEAF_H_
+
+#include
not needed
Removed.
+#include
+#include
+#include
+#include
not needed
check if includes are actually needed.
Removed these headers. They are not needed. The rest are needed.
Re: [PATCH V3 XRT Alveo 04/18] fpga: xrt: xrt-lib platform driver manager
On 2/22/21 7:05 AM, Tom Rix wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
On 2/17/21 10:40 PM, Lizhi Hou wrote:
xrt-lib kernel module infrastructure code to register and manage all
leaf driver modules.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/lib/main.c | 274
drivers/fpga/xrt/lib/main.h | 17 +++
2 files changed, 291 insertions(+)
create mode 100644 drivers/fpga/xrt/lib/main.c
create mode 100644 drivers/fpga/xrt/lib/main.h
Not sure if 'main' is a good base name for something going into a lib.
These files are the main file for xrt-lib.ko. I'm not sure what name you
prefer, but I've changed them to lib-drv.[c|h]. Let me know if you don't
like them...
diff --git a/drivers/fpga/xrt/lib/main.c b/drivers/fpga/xrt/lib/main.c
new file mode 100644
index ..36fb62710843
--- /dev/null
+++ b/drivers/fpga/xrt/lib/main.c
@@ -0,0 +1,274 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Xilinx Alveo FPGA Support
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include "xleaf.h"
+#include "xroot.h"
+#include "main.h"
+
+#define XRT_IPLIB_MODULE_NAME"xrt-lib"
+#define XRT_IPLIB_MODULE_VERSION "4.0.0"
+#define XRT_MAX_DEVICE_NODES 128
+#define XRT_DRVNAME(drv) ((drv)->driver.name)
+
+/*
+ * Subdev driver is known by ID to others. We map the ID to it's
by it's ID
Sure.
+ * struct platform_driver, which contains it's binding name and driver/file
ops.
+ * We also map it to the endpoint name in DTB as well, if it's different
+ * than the driver's binding name.
+ */
+struct xrt_drv_map {
+ struct list_head list;
+ enum xrt_subdev_id id;
+ struct platform_driver *drv;
+ struct xrt_subdev_endpoints *eps;
+ struct ida ida; /* manage driver instance and char dev minor */
+};
+
+static DEFINE_MUTEX(xrt_lib_lock); /* global lock protecting xrt_drv_maps list
*/
+static LIST_HEAD(xrt_drv_maps);
+struct class *xrt_class;
+
+static inline struct xrt_subdev_drvdata *
+xrt_drv_map2drvdata(struct xrt_drv_map *map)
+{
+ return (struct xrt_subdev_drvdata *)map->drv->id_table[0].driver_data;
+}
+
+static struct xrt_drv_map *
+xrt_drv_find_map_by_id_nolock(enum xrt_subdev_id id)
name could be by convention
__xrt_drv_find_map_id
Sure.
+{
+ const struct list_head *ptr;
+
+ list_for_each(ptr, _drv_maps) {
+ struct xrt_drv_map *tmap = list_entry(ptr, struct xrt_drv_map,
list);
+
+ if (tmap->id == id)
+ return tmap;
+ }
+ return NULL;
+}
+
+static struct xrt_drv_map *
+xrt_drv_find_map_by_id(enum xrt_subdev_id id)
+{
+ struct xrt_drv_map *map;
+
+ mutex_lock(_lib_lock);
+ map = xrt_drv_find_map_by_id_nolock(id);
+ mutex_unlock(_lib_lock);
+ /*
+ * map should remain valid even after lock is dropped since a registered
even after the lock
Sure.
+ * driver should only be unregistered when driver module is being
unloaded,
+ * which means that the driver should not be used by then.
+ */
+ return map;
+}
+
+static int xrt_drv_register_driver(struct xrt_drv_map *map)
+{
+ struct xrt_subdev_drvdata *drvdata;
+ int rc = 0;
+ const char *drvname = XRT_DRVNAME(map->drv);
+
+ rc = platform_driver_register(map->drv);
+ if (rc) {
+ pr_err("register %s platform driver failed\n", drvname);
+ return rc;
+ }
+
+ drvdata = xrt_drv_map2drvdata(map);
+ if (drvdata) {
+ /* Initialize dev_t for char dev node. */
+ if (xleaf_devnode_enabled(drvdata)) {
+ rc =
alloc_chrdev_region(>xsd_file_ops.xsf_dev_t, 0,
+ XRT_MAX_DEVICE_NODES, drvname);
+ if (rc) {
+ platform_driver_unregister(map->drv);
+ pr_err("failed to alloc dev minor for %s: %d\n",
drvname, rc);
+ return rc;
+ }
+ } else {
+ drvdata->xsd_file_ops.xsf_dev_t = (dev_t)-1;
+ }
+ }
+
+ ida_init(>ida);
+
+ pr_info("%s registered successfully\n", drvname);
+
+ return 0;
+}
+
+static void xrt_drv_unregister_driver(struct xrt_drv_map *map)
+{
+ const char *drvname = XRT_DRVNAME(map->drv);
+ struct xrt_subdev_drvdata *drvdata;
+
+ ida_destroy(>ida);
+
+ drvdata = xrt_drv_map2drvdata(map);
+ if (drvdata && drvdata->xsd_file_ops.xsf_dev_t != (dev_t)-1) {
+ unregister_chrdev_region(drvdata->xsd_file_ops.xsf_dev_t,
+
Re: [PATCH V3 XRT Alveo 04/18] fpga: xrt: xrt-lib platform driver manager
Hi Moritz,
On 2/21/21 12:39 PM, Moritz Fischer wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
Lizhi,
On Wed, Feb 17, 2021 at 10:40:05PM -0800, Lizhi Hou wrote:
xrt-lib kernel module infrastructure code to register and manage all
leaf driver modules.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/lib/main.c | 274
drivers/fpga/xrt/lib/main.h | 17 +++
2 files changed, 291 insertions(+)
create mode 100644 drivers/fpga/xrt/lib/main.c
create mode 100644 drivers/fpga/xrt/lib/main.h
diff --git a/drivers/fpga/xrt/lib/main.c b/drivers/fpga/xrt/lib/main.c
new file mode 100644
index ..36fb62710843
--- /dev/null
+++ b/drivers/fpga/xrt/lib/main.c
@@ -0,0 +1,274 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Xilinx Alveo FPGA Support
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include "xleaf.h"
+#include "xroot.h"
+#include "main.h"
+
+#define XRT_IPLIB_MODULE_NAME"xrt-lib"
+#define XRT_IPLIB_MODULE_VERSION "4.0.0"
+#define XRT_MAX_DEVICE_NODES 128
+#define XRT_DRVNAME(drv) ((drv)->driver.name)
+
+/*
+ * Subdev driver is known by ID to others. We map the ID to it's
+ * struct platform_driver, which contains it's binding name and driver/file
ops.
+ * We also map it to the endpoint name in DTB as well, if it's different
+ * than the driver's binding name.
+ */
+struct xrt_drv_map {
+ struct list_head list;
+ enum xrt_subdev_id id;
+ struct platform_driver *drv;
+ struct xrt_subdev_endpoints *eps;
+ struct ida ida; /* manage driver instance and char dev minor */
+};
+
+static DEFINE_MUTEX(xrt_lib_lock); /* global lock protecting xrt_drv_maps list
*/
+static LIST_HEAD(xrt_drv_maps);
+struct class *xrt_class;
+
+static inline struct xrt_subdev_drvdata *
+xrt_drv_map2drvdata(struct xrt_drv_map *map)
+{
+ return (struct xrt_subdev_drvdata *)map->drv->id_table[0].driver_data;
+}
+
+static struct xrt_drv_map *
+xrt_drv_find_map_by_id_nolock(enum xrt_subdev_id id)
+{
+ const struct list_head *ptr;
+
+ list_for_each(ptr, _drv_maps) {
+ struct xrt_drv_map *tmap = list_entry(ptr, struct xrt_drv_map,
list);
list_for_each_entry, maybe?
Yes!
+
+ if (tmap->id == id)
+ return tmap;
+ }
+ return NULL;
+}
+
+static struct xrt_drv_map *
+xrt_drv_find_map_by_id(enum xrt_subdev_id id)
+{
+ struct xrt_drv_map *map;
+
+ mutex_lock(_lib_lock);
+ map = xrt_drv_find_map_by_id_nolock(id);
+ mutex_unlock(_lib_lock);
+ /*
+ * map should remain valid even after lock is dropped since a registered
+ * driver should only be unregistered when driver module is being
unloaded,
+ * which means that the driver should not be used by then.
+ */
+ return map;
+}
+
+static int xrt_drv_register_driver(struct xrt_drv_map *map)
+{
+ struct xrt_subdev_drvdata *drvdata;
+ int rc = 0;
+ const char *drvname = XRT_DRVNAME(map->drv);
+
+ rc = platform_driver_register(map->drv);
+ if (rc) {
+ pr_err("register %s platform driver failed\n", drvname);
+ return rc;
+ }
+
+ drvdata = xrt_drv_map2drvdata(map);
+ if (drvdata) {
+ /* Initialize dev_t for char dev node. */
+ if (xleaf_devnode_enabled(drvdata)) {
+ rc =
alloc_chrdev_region(>xsd_file_ops.xsf_dev_t, 0,
+ XRT_MAX_DEVICE_NODES, drvname);
+ if (rc) {
+ platform_driver_unregister(map->drv);
+ pr_err("failed to alloc dev minor for %s: %d\n",
drvname, rc);
+ return rc;
+ }
+ } else {
+ drvdata->xsd_file_ops.xsf_dev_t = (dev_t)-1;
+ }
+ }
+
+ ida_init(>ida);
+
+ pr_info("%s registered successfully\n", drvname);
Is this not xrt_info() then?
xrt_info() is meant for leaf driver to call where struct platform_device
is available. We are initializing the drivers here, so can't call
xrt_info().
+
+ return 0;
+}
+
+static void xrt_drv_unregister_driver(struct xrt_drv_map *map)
+{
+ const char *drvname = XRT_DRVNAME(map->drv);
+ struct xrt_subdev_drvdata *drvdata;
+
+ ida_destroy(>ida);
+
+ drvdata = xrt_drv_map2drvdata(map);
+ if (drvdata && drvdata->xsd_file_ops.xsf_dev_t != (dev_t)-1) {
+ unregister_chrdev_region(drvdata->xsd_file_ops.xsf_dev_t,
+ XRT_MAX_DEVICE_NODES);
+ }
+
+ platform_driver_un
Re: [PATCH V3 XRT Alveo 05/18] fpga: xrt: group platform driver
Hi Tom,
On 2/22/21 10:50 AM, Tom Rix wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
On 2/17/21 10:40 PM, Lizhi Hou wrote:
group driver that manages life cycle of a bunch of leaf driver instances
and bridges them with root.
Signed-off-by: Sonal Santan
Signed-off-by: Max Zhen
Signed-off-by: Lizhi Hou
---
drivers/fpga/xrt/include/group.h | 27
drivers/fpga/xrt/lib/group.c | 265 +++
2 files changed, 292 insertions(+)
create mode 100644 drivers/fpga/xrt/include/group.h
create mode 100644 drivers/fpga/xrt/lib/group.c
diff --git a/drivers/fpga/xrt/include/group.h b/drivers/fpga/xrt/include/group.h
new file mode 100644
index ..1874cdd5120d
--- /dev/null
+++ b/drivers/fpga/xrt/include/group.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
A bit too generic, please add a description or remove.
Will remove.
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#ifndef _XRT_GROUP_H_
+#define _XRT_GROUP_H_
+
+#include "xleaf.h"
This is patch 6, consider comments on patch 4.
Will move this header to other patch.
+
+/*
+ * Group driver IOCTL calls.
Are these really ioctl calls?
Seems more like messages between nodes in a tree.
Consider changing to better jagon, maybe ioctl -> msg
You're right. They are not really ioctl calls. They are just calls b/w
leaf nodes. I changed to leaf calls/commands.
+ */
+enum xrt_group_ioctl_cmd {
+ XRT_GROUP_GET_LEAF = XRT_XLEAF_CUSTOM_BASE, /* See comments in xleaf.h */
XRT_LEAF_CUSTOM_BASE is a #define, while these are enums. To be consistent, the
XRT_LEAF_CUSTOM_BASE should be an enum in xleaf, you can initialize it to 64
there.
Will fix.
+ XRT_GROUP_PUT_LEAF,
+ XRT_GROUP_INIT_CHILDREN,
+ XRT_GROUP_FINI_CHILDREN,
+ XRT_GROUP_TRIGGER_EVENT,
+};
+
+#endif /* _XRT_GROUP_H_ */
diff --git a/drivers/fpga/xrt/lib/group.c b/drivers/fpga/xrt/lib/group.c
new file mode 100644
index ..6ba56eea479b
--- /dev/null
+++ b/drivers/fpga/xrt/lib/group.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo FPGA Group Driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ * Cheng Zhen
+ */
+
+#include
+#include
+#include "xleaf.h"
+#include "subdev_pool.h"
+#include "group.h"
+#include "metadata.h"
+#include "main.h"
+
+#define XRT_GRP "xrt_group"
+
+struct xrt_group {
+ struct platform_device *pdev;
+ struct xrt_subdev_pool leaves;
+ bool leaves_created;
+ struct mutex lock; /* lock for group */
+};
+
+static int xrt_grp_root_cb(struct device *dev, void *parg,
+u32 cmd, void *arg)
could 'cmd' be some enum type ?
Will fix.
+{
+ int rc;
+ struct platform_device *pdev =
+ container_of(dev, struct platform_device, dev);
+ struct xrt_group *xg = (struct xrt_group *)parg;
+
+ switch (cmd) {
+ case XRT_ROOT_GET_LEAF_HOLDERS: {
+ struct xrt_root_ioctl_get_holders *holders =
+ (struct xrt_root_ioctl_get_holders *)arg;
+ rc = xrt_subdev_pool_get_holders(>leaves,
+ holders->xpigh_pdev,
+ holders->xpigh_holder_buf,
+ holders->xpigh_holder_buf_len);
+ break;
+ }
+ default:
+ /* Forward parent call to root. */
+ rc = xrt_subdev_root_request(pdev, cmd, arg);
+ break;
+ }
+
+ return rc;
+}
+
+static int xrt_grp_create_leaves(struct xrt_group *xg)
+{
+ struct xrt_subdev_platdata *pdata = DEV_PDATA(xg->pdev);
+ enum xrt_subdev_id did;
+ struct xrt_subdev_endpoints *eps = NULL;
+ int ep_count = 0, i, ret = 0, failed = 0;
+ unsigned long mlen;
+ char *dtb, *grp_dtb = NULL;
+ const char *ep_name;
+
+ mutex_lock(>lock);
+
+ if (xg->leaves_created) {
+ mutex_unlock(>lock);
This happens should be programming error, so print out some error message
The root driver does not remember which group has created leaves or not.
It'll just blindly make the call. The group driver itself should
remember and tell the root that it's done already or not. So, this is
not considered as an error.
+ return -EEXIST;
+ }
+
+ xrt_info(xg->pdev, "bringing up leaves...");
+
+ /* Create all leaves based on dtb. */
+ if (!pdata)
+ goto bail;
move to above the lock and fail with something like -EINVAL
Will fix.
+
+ mlen = xrt_md_size(DEV(xg->pdev), pdata->xsp_dtb);
+ if (mlen == XRT_MD_INVALID_LENGT
Re: [PATCH Xilinx Alveo 7/8] fpga: xrt: Alveo management physical function driver
Hi Yilun,
On 12/1/20 7:00 PM, Xu Yilun wrote:
+static int xmgmt_main_event_cb(struct platform_device *pdev,
+ enum xrt_events evt, void *arg)
+{
+ struct xmgmt_main *xmm = platform_get_drvdata(pdev);
+ struct xrt_event_arg_subdev *esd = (struct xrt_event_arg_subdev *)arg;
+ enum xrt_subdev_id id;
+ int instance;
+ size_t fwlen;
+
+ switch (evt) {
+ case XRT_EVENT_POST_CREATION: {
+ id = esd->xevt_subdev_id;
+ instance = esd->xevt_subdev_instance;
+ xrt_info(pdev, "processing event %d for (%d, %d)",
+ evt, id, instance);
+
+ if (id == XRT_SUBDEV_GPIO)
+ xmm->gpio_ready = true;
+ else if (id == XRT_SUBDEV_QSPI)
+ xmm->flash_ready = true;
+ else
+ BUG_ON(1);
+
+ if (xmm->gpio_ready && xmm->flash_ready) {
+ int rc;
+
+ rc = load_firmware_from_disk(pdev, >firmware_blp,
+ );
+ if (rc != 0) {
+ rc = load_firmware_from_flash(pdev,
+ >firmware_blp, );
I'm curious that before the shell metadata is loaded, how the QSPI
subdev is enumerated and get to work? The QSPI DT info itself is
stored in metadata, is it?
No, it is not from the shell metadata. The QSPI subdev info is
discovered from a rom located on the PCIE BAR pointed to by VSEC cap
found in config space.
I didn't find the creation of leaf platform devices, maybe I can find
the answer in the missing Patch #5?
Leaf driver is children of partition driver. They are created in
xrt_part_create_leaves() in xrt-partition.c.
Thanks,
Max
Thanks,
Yilun
+ }
+ if (rc == 0 && is_valid_firmware(pdev,
+ xmm->firmware_blp, fwlen))
+ (void) xmgmt_create_blp(xmm);
+ else
+ xrt_err(pdev,
+ "failed to find firmware, giving up");
+ xmm->evt_hdl = NULL;
+ }
+ break;
+ }
RE: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a document describing Alveo XRT drivers
Hi Moritz, I manually fixed some line breaks. Not sure why outlook is not doing it properly. Let me know if it still looks bad to you. Please see my reply below. > > > Max, > > On Thu, Dec 03, 2020 at 03:38:26AM +, Max Zhen wrote: > > [...cut...] > > > > > > > > +xclbin over the User partition as part of DFX. When a user > > > > > > +requests loading of a specific xclbin the xmgmt management > > > > > > +driver reads the parent interface UUID specified in the xclbin > > > > > > +and matches it with child interface UUID exported by Shell to > > > > > > +determine if xclbin is compatible with the Shell. If match fails > > > > > > loading of xclbin is denied. > > > > > > + > > > > > > +xclbin loading is requested using ICAP_DOWNLOAD_AXLF ioctl command. > > > > > > +When loading xclbin xmgmt driver performs the following operations: > > > > > > + > > > > > > +1. Sanity check the xclbin contents 2. Isolate the User > > > > > > +partition 3. Download the bitstream using the FPGA config engine > > > > > > (ICAP) 4. > > > > > > +De-isolate the User partition > > > > > Is this modelled as bridges and regions? > > > > > > > > Alveo drivers as written today do not use fpga bridge and region > > > > framework. It seems that if we add support for that framework, it’s > > > > possible to receive PR program request from kernel outside of xmgmt > > > > driver? > > > > Currently, we can’t support this and PR program can only be initiated > > > > using XRT’s runtime API in user space. > > > > > > I'm not 100% sure I understand the concern here, let me reply to what > > > I think I understand: > > > > > > You're worried that if you use FPGA region as interface to accept PR > > > requests something else could attempt to reconfigure the region from > > > within the kernel using the FPGA Region API? > > > > > > Assuming I got this right, I don't think this is a big deal. When you > > > create the regions you control who gets the references to it. > > > > Thanks for explaining. Yes, I think you got my point :-). > > We can add code to make a region 'static' or 'one-time' or 'fixed'. > > > > > > > > From what I've seen so far Regions seem to be roughly equivalent to > > > Partitions, hence my surprise to see a new structure bypassing them. > > > > I see where the gap is. > > > > Regions in Linux is very different than "partitions" we have defined in > > xmgmt. Regions seem to be a software data structure > > representing an area on the FPGA that can be reprogrammed. This area is > > protected by the concept of "bridge" which can be disabled > > before program and reenabled after it. And you go through region when you > > need to reprogram this area. > > Your central management driver can create / destroy regions at will. It > can keep them in a list, array or tree. > > Regions can but don't have to have bridges. > > If you need to go through the central driver to reprogram a region, > you can use that to figure out which region to program. That sounds fine. I can create a region and call into it from xmgmt for PR programing. The region will, then, call the xmgmt's fpga manager to program it. > > > > The "partition" is part of the main infrastructure of xmgmt driver, which > > represents a group of subdev drivers for each individual IP > > (HW subcomponents). Basically, xmgmt root driver is parent of several > > partitions who is, in turn, the parent of several subdev drivers. > > The parent manages the life cycle of its children here. > > I don't see how this is conceptually different from what DFL does, and > they managed to use Regions and Bridges. > > If things are missing in the framework, please add them instead of > rewriting an entire parallel framework. > > > > > We do have a partition to represent the group of subdevs/IPs in the > > reprogrammable area. And we also have partitions > > representing other areas which cannot be reprogrammed. So, it is difficult > > to use "Region" to implement "partition". > > You implement your regions callbacks, you can return -EINVAL / -ENOTTY > if you want to fail a reprogramming request to a static partion / > region. > > > From what you have explained, it seems that even if I use region / bridge &g
RE: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a document describing Alveo XRT drivers
Hi Moritz, Please see my reply below. Thanks, -Max > -Original Message- > From: Moritz Fischer > Sent: Wednesday, December 2, 2020 15:10 > To: Max Zhen > Cc: Moritz Fischer ; Sonal Santan ; > linux-kernel@vger.kernel.org; linux-f...@vger.kernel.org; Lizhi Hou > ; Michal Simek ; Stefano > Stabellini ; devicet...@vger.kernel.org > Subject: Re: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a > document describing Alveo XRT drivers > > > Hi Max, > > On Wed, Dec 02, 2020 at 09:24:29PM +, Max Zhen wrote: > > Hi Moritz, > > > > Thanks for your feedback. Please see my reply inline. > > > > Thanks, > > -Max > > > > > -Original Message- > > > From: Moritz Fischer > > > Sent: Monday, November 30, 2020 20:55 > > > To: Sonal Santan > > > Cc: linux-kernel@vger.kernel.org; linux-f...@vger.kernel.org; Max > > > Zhen ; Lizhi Hou ; Michal > > > Simek ; Stefano Stabellini > > > ; devicet...@vger.kernel.org > > > Subject: Re: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a > > > document describing Alveo XRT drivers > > > > > > > > > On Sat, Nov 28, 2020 at 04:00:33PM -0800, Sonal Santan wrote: > > > > From: Sonal Santan > > > > > > > > Describe Alveo XRT driver architecture and provide basic > > > > overview of Xilinx Alveo platform. > > > > > > > > Signed-off-by: Sonal Santan > > > > --- > > > > Documentation/fpga/index.rst | 1 + > > > > Documentation/fpga/xrt.rst | 588 > > > +++ > > > > 2 files changed, 589 insertions(+) create mode 100644 > > > > Documentation/fpga/xrt.rst > > > > [...cut...] > > > > +xclbin over the User partition as part of DFX. When a user > > > > +requests loading of a specific xclbin the xmgmt management > > > > +driver reads the parent interface UUID specified in the xclbin > > > > +and matches it with child interface UUID exported by Shell to > > > > +determine if xclbin is compatible > > > with the Shell. If match fails loading of xclbin is denied. > > > > + > > > > +xclbin loading is requested using ICAP_DOWNLOAD_AXLF ioctl > command. > > > > +When loading xclbin xmgmt driver performs the following operations: > > > > + > > > > +1. Sanity check the xclbin contents 2. Isolate the User > > > > +partition 3. Download the bitstream using the FPGA config engine > > > > (ICAP) 4. > > > > +De-isolate the User partition > > > Is this modelled as bridges and regions? > > > > Alveo drivers as written today do not use fpga bridge and region > framework. It seems that if we add support for that framework, it’s > possible to receive PR program request from kernel outside of xmgmt driver? > Currently, we can’t support this and PR program can only be initiated > using XRT’s runtime API in user space. > > I'm not 100% sure I understand the concern here, let me reply to what > I think I understand: > > You're worried that if you use FPGA region as interface to accept PR > requests something else could attempt to reconfigure the region from > within the kernel using the FPGA Region API? > > Assuming I got this right, I don't think this is a big deal. When you > create the regions you control who gets the references to it. Thanks for explaining. Yes, I think you got my point :-). > > From what I've seen so far Regions seem to be roughly equivalent to > Partitions, hence my surprise to see a new structure bypassing them. I see where the gap is. Regions in Linux is very different than "partitions" we have defined in xmgmt. Regions seem to be a software data structure representing an area on the FPGA that can be reprogrammed. This area is protected by the concept of "bridge" which can be disabled before program and reenabled after it. And you go through region when you need to reprogram this area. The "partition" is part of the main infrastructure of xmgmt driver, which represents a group of subdev drivers for each individual IP (HW subcomponents). Basically, xmgmt root driver is parent of several partitions who is, in turn, the parent of several subdev drivers. The parent manages the life cycle of its children here. We do have a partition to represent the group of subdevs/IPs in the reprogrammable area. And we also have partitions representing other areas which cannot be reprogrammed. So, it is difficult to use "Region" to implement "partition". From
RE: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a document describing Alveo XRT drivers
Hi Moritz, Thanks for your feedback. Please see my reply inline. Thanks, -Max > -Original Message- > From: Moritz Fischer > Sent: Monday, November 30, 2020 20:55 > To: Sonal Santan > Cc: linux-kernel@vger.kernel.org; linux-f...@vger.kernel.org; Max Zhen > ; Lizhi Hou ; Michal Simek > ; Stefano Stabellini ; > devicet...@vger.kernel.org > Subject: Re: [PATCH Xilinx Alveo 1/8] Documentation: fpga: Add a document > describing Alveo XRT drivers > > > On Sat, Nov 28, 2020 at 04:00:33PM -0800, Sonal Santan wrote: > > From: Sonal Santan > > > > Describe Alveo XRT driver architecture and provide basic overview of > > Xilinx Alveo platform. > > > > Signed-off-by: Sonal Santan > > --- > > Documentation/fpga/index.rst | 1 + > > Documentation/fpga/xrt.rst | 588 > +++ > > 2 files changed, 589 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 ..9f37d46459b0 > > --- /dev/null > > +++ b/Documentation/fpga/xrt.rst > > @@ -0,1 +1,588 @@ > > +== > > +XRTV2 Linux Kernel Driver Overview > > +== > > + > > +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 > > +driver's configuration and behavior is determined by meta data provided > by platform (in *device tree* format). > > +Primary management physical function (MPF) driver is called > > +**xmgmt**. Primary user physical function (UPF) driver is called > > +**xuser** and HW subsystem drivers are packaged into a library module > called **xrt-lib**, which is shared by **xmgmt** and **xuser** (WIP). > WIP? Working in progress. I'll expand it in the doc. > > + > > +Alveo Platform Overview > > +=== > > + > > +Alveo platforms are architected as two physical FPGA partitions: > > +*Shell* and *User*. Shell > Nit: The Shell provides ... Sure. Will fix. > > +provides basic infrastructure for the Alveo platform like PCIe > > +connectivity, board management, Dynamic Function Exchange (DFX), > > +sensors, clocking, reset, and security. User partition contains user > compiled binary which is loaded by a process called DFX also known as partial > reconfiguration. > > + > > +Physical partitions require strict HW compatibility with each other for DFX > to work properly. > > +Every physical partition has two interface UUIDs: *parent* UUID and > > +*child* UUID. For simple single stage platforms Shell → User forms > > +parent child relationship. For complex two stage platforms Base → Shell > → User forms the parent child relationship chain. > > + > > +.. note:: > > + Partition compatibility matching is key design component of Alveo > platforms and XRT. Partitions > > + have child and parent relationship. A loaded partition exposes child > partition UUID to advertise > > + its compatibility requirement for child partition. When loading a child > partition the xmgmt > > + management driver matches parent UUID of the child partition against > child UUID exported by the > > + parent. Parent and child partition UUIDs are stored in the *xclbin* (for > user) or *xsabin* (for > > + base and shell). Except for root UUID, VSEC, hardware itself does not > know about UUIDs. UUIDs are > > + stored in xsabin and xclbin. > > + > > + > > +The physical partitions and their loading is illustrated below:: > > + > > +SHELL USER > > ++---+ +---+ > > +| | | | > > +| VSEC UUID | CHILD PARENT |LOGIC UUID | > > +| o--->|<o | > > +| | UUID UUID | | > > ++-+-+
