Re: [PATCH V3 XRT Alveo 17/18] fpga: xrt: partition isolation platform driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Moritz,


On 02/21/2021 12:36 PM, Moritz Fischer wrote:

On Wed, Feb 17, 2021 at 10:40:18PM -0800, Lizhi Hou wrote:
Add partition isolation platform driver. partition isolation is
a hardware function discovered by walking firmware metadata.
A platform device node will be created for it. Partition isolation
function isolate the different fpga regions

Signed-off-by: Sonal Santan <sonal.santan@xxxxxxxxxx>
Signed-off-by: Max Zhen <max.zhen@xxxxxxxxxx>
Signed-off-by: Lizhi Hou <lizhih@xxxxxxxxxx>
---
  drivers/fpga/xrt/include/xleaf/axigate.h |  25 ++
  drivers/fpga/xrt/lib/xleaf/axigate.c     | 298 +++++++++++++++++++++++
  2 files changed, 323 insertions(+)
  create mode 100644 drivers/fpga/xrt/include/xleaf/axigate.h
  create mode 100644 drivers/fpga/xrt/lib/xleaf/axigate.c

diff --git a/drivers/fpga/xrt/include/xleaf/axigate.h b/drivers/fpga/xrt/include/xleaf/axigate.h
new file mode 100644
index 000000000000..2cef71e13b30
--- /dev/null
+++ b/drivers/fpga/xrt/include/xleaf/axigate.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for XRT Axigate Leaf Driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *   Lizhi Hou <Lizhi.Hou@xxxxxxxxxx>
+ */
+
+#ifndef _XRT_AXIGATE_H_
+#define _XRT_AXIGATE_H_
+
+#include "xleaf.h"
+#include "metadata.h"
+
+/*
+ * AXIGATE driver IOCTL calls.
+ */
+enum xrt_axigate_ioctl_cmd {
+     XRT_AXIGATE_FREEZE = XRT_XLEAF_CUSTOM_BASE, /* See comments in xleaf.h */
+     XRT_AXIGATE_FREE,
+};
+
+#endif       /* _XRT_AXIGATE_H_ */
diff --git a/drivers/fpga/xrt/lib/xleaf/axigate.c b/drivers/fpga/xrt/lib/xleaf/axigate.c
new file mode 100644
index 000000000000..382969f9925f
--- /dev/null
+++ b/drivers/fpga/xrt/lib/xleaf/axigate.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo FPGA AXI Gate Driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *      Lizhi Hou<Lizhi.Hou@xxxxxxxxxx>
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include "metadata.h"
+#include "xleaf.h"
+#include "xleaf/axigate.h"
+
+#define XRT_AXIGATE "xrt_axigate"
+
+struct axigate_regs {
+     u32             iag_wr;
+     u32             iag_rvsd;
+     u32             iag_rd;
+} __packed;
Just make them #defines, even more so if there are only 3 of them.
We will use #define and regmap.
+
+struct xrt_axigate {
+     struct platform_device  *pdev;
+     void                    *base;
+     struct mutex            gate_lock; /* gate dev lock */
+
+     void                    *evt_hdl;
+     const char              *ep_name;
+
+     bool                    gate_freezed;
+};
+
+/* the ep names are in the order of hardware layers */
+static const char * const xrt_axigate_epnames[] = {
+     XRT_MD_NODE_GATE_PLP,
+     XRT_MD_NODE_GATE_ULP,
+     NULL
+};
+
+#define reg_rd(g, r)                                         \
+     ioread32((void *)(g)->base + offsetof(struct axigate_regs, r))
+#define reg_wr(g, v, r)                                              \
+     iowrite32(v, (void *)(g)->base + offsetof(struct axigate_regs, r))
+
+static inline void freeze_gate(struct xrt_axigate *gate)
+{
+     reg_wr(gate, 0, iag_wr);
+     ndelay(500);
+     reg_rd(gate, iag_rd);
+}
+
+static inline void free_gate(struct xrt_axigate *gate)
+{
+     reg_wr(gate, 0x2, iag_wr);
+     ndelay(500);
Magic constants?
Will use #define for 500
+     (void)reg_rd(gate, iag_rd);
At the very least add a comment on why? Is this for PCI synchronization
reasons?
Will add comment. Some old board requires this extra read and it will not hurt performance.

+     reg_wr(gate, 0x3, iag_wr);
+     ndelay(500);
Magic constants?
+     reg_rd(gate, iag_rd);
Does it nead a (void) or not? Be consistent, again, why do we read here
at all?
+}
+
+static int xrt_axigate_epname_idx(struct platform_device *pdev)
+{
+     int                     i;
+     int                     ret;
+     struct resource         *res;
Nope. Indents:

struct resource *res;
int, i, ret;
Will change this.

+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!res) {
+             xrt_err(pdev, "Empty Resource!");
+             return -EINVAL;
+     }
+
+     for (i = 0; xrt_axigate_epnames[i]; i++) {
+             ret = strncmp(xrt_axigate_epnames[i], res->name,
+                           strlen(xrt_axigate_epnames[i]) + 1);
+             if (!ret)
+                     break;
+     }
+
+     ret = (xrt_axigate_epnames[i]) ? i : -EINVAL;
Why not just:

         if (xrt_axigate_epnames[i])
                 return i;

         return -EINVAL;
Will change this.
+     return ret;
+}
+
+static void xrt_axigate_freeze(struct platform_device *pdev)
+{
+     struct xrt_axigate      *gate;
+     u32                     freeze = 0;
Indents. Fix everywhere.
Will fix this.
+
+     gate = platform_get_drvdata(pdev);
+
+     mutex_lock(&gate->gate_lock);
+     freeze = reg_rd(gate, iag_rd);
+     if (freeze) {           /* gate is opened */
+             xleaf_broadcast_event(pdev, XRT_EVENT_PRE_GATE_CLOSE, false);
+             freeze_gate(gate);
+     }
+
+     gate->gate_freezed = true;
s/freezed/frozen
Will change terms to open / close.
+     mutex_unlock(&gate->gate_lock);
+
+     xrt_info(pdev, "freeze gate %s", gate->ep_name);
debug?
axigate is a very critical part for programming FPGA. We hope to have an explicit printk to indicate the axigate open/close.
+}
+
+static void xrt_axigate_free(struct platform_device *pdev)
+{
+     struct xrt_axigate      *gate;
+     u32                     freeze;
+
+     gate = platform_get_drvdata(pdev);
+
+     mutex_lock(&gate->gate_lock);
+     freeze = reg_rd(gate, iag_rd);
+     if (!freeze) {          /* gate is closed */
+             free_gate(gate);
+             xleaf_broadcast_event(pdev, XRT_EVENT_POST_GATE_OPEN, true);
+             /* xrt_axigate_free() could be called in event cb, thus
+              * we can not wait for the completes
+              */
+     }
+
+     gate->gate_freezed = false;
+     mutex_unlock(&gate->gate_lock);
+
+     xrt_info(pdev, "free gate %s", gate->ep_name);
+}
+
+static void xrt_axigate_event_cb(struct platform_device *pdev, void *arg)
+{
+     struct platform_device *leaf;
+     struct xrt_event *evt = (struct xrt_event *)arg;
+     enum xrt_events e = evt->xe_evt;
+     enum xrt_subdev_id id = evt->xe_subdev.xevt_subdev_id;
+     int instance = evt->xe_subdev.xevt_subdev_instance;
+     struct xrt_axigate *gate = platform_get_drvdata(pdev);
+     struct resource *res;
Reverse x-mas tree;
xxxxxxxxxx
xxxxxxxxx
xxxxxxxx
xxxxxx
Will fix this.
+
+     switch (e) {
+     case XRT_EVENT_POST_CREATION:
+             break;
+     default:
+             return;
+     }
+
+     if (id != XRT_SUBDEV_AXIGATE)
+             return;
+
+     leaf = xleaf_get_leaf_by_id(pdev, id, instance);
+     if (!leaf)
+             return;
+
+     res = platform_get_resource(leaf, IORESOURCE_MEM, 0);
+     if (!res || !strncmp(res->name, gate->ep_name, strlen(res->name) + 1)) {
+             (void)xleaf_put_leaf(pdev, leaf);
+             return;
+     }
+
+     /*
+      * higher level axigate instance created,
+      * make sure the gate is openned. This covers 1RP flow which
+      * has plp gate as well.
+      */
+     if (xrt_axigate_epname_idx(leaf) > xrt_axigate_epname_idx(pdev))
+             xrt_axigate_free(pdev);
+     else
+             xleaf_ioctl(leaf, XRT_AXIGATE_FREE, NULL);
+
+     (void)xleaf_put_leaf(pdev, leaf);
+}
+
+static int
+xrt_axigate_leaf_ioctl(struct platform_device *pdev, u32 cmd, void *arg)
+{
+     switch (cmd) {
+     case XRT_XLEAF_EVENT:
+             xrt_axigate_event_cb(pdev, arg);
+             break;
+     case XRT_AXIGATE_FREEZE:
+             xrt_axigate_freeze(pdev);
+             break;
+     case XRT_AXIGATE_FREE:
+             xrt_axigate_free(pdev);
+             break;
+     default:
+             xrt_err(pdev, "unsupported cmd %d", cmd);
+             return -EINVAL;
+     }
+
+     return 0;
+}
+
+static int xrt_axigate_remove(struct platform_device *pdev)
+{
+     struct xrt_axigate      *gate;
+
+     gate = platform_get_drvdata(pdev);
+
+     if (gate->base)
+             iounmap(gate->base);
+
+     platform_set_drvdata(pdev, NULL);
+     devm_kfree(&pdev->dev, gate);
No! The point of using devres is so cleanup happens on removal.
While you're at it, if you move the ioremap to a devres version, this
function can basically go away entirely.
Will fix this.
+
+     return 0;
+}
+
+static int xrt_axigate_probe(struct platform_device *pdev)
+{
+     struct xrt_axigate      *gate;
+     struct resource         *res;
+     int                     ret;
+
+     gate = devm_kzalloc(&pdev->dev, sizeof(*gate), GFP_KERNEL);
+     if (!gate)
+             return -ENOMEM;
+
+     gate->pdev = pdev;
+     platform_set_drvdata(pdev, gate);
+
+     xrt_info(pdev, "probing...");
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!res) {
+             xrt_err(pdev, "Empty resource 0");
+             ret = -EINVAL;
+             goto failed;
+     }
+
+     gate->base = ioremap(res->start, res->end - res->start + 1);
+     if (!gate->base) {
+             xrt_err(pdev, "map base iomem failed");
+             ret = -EFAULT;
+             goto failed;
+     }
+
+     gate->ep_name = res->name;
+
+     mutex_init(&gate->gate_lock);
+
+     return 0;
+
+failed:
+     xrt_axigate_remove(pdev);
+     return ret;
+}
+
+static struct xrt_subdev_endpoints xrt_axigate_endpoints[] = {
+     {
+             .xse_names = (struct xrt_subdev_ep_names[]) {
+                     { .ep_name = "ep_pr_isolate_ulp_00" },
+                     { NULL },
+             },
+             .xse_min_ep = 1,
+     },
+     {
+             .xse_names = (struct xrt_subdev_ep_names[]) {
+                     { .ep_name = "ep_pr_isolate_plp_00" },
+                     { NULL },
+             },
+             .xse_min_ep = 1,
+     },
+     { 0 },
+};
+
+static struct xrt_subdev_drvdata xrt_axigate_data = {
+     .xsd_dev_ops = {
+             .xsd_ioctl = xrt_axigate_leaf_ioctl,
+     },
+};
+
+static const struct platform_device_id xrt_axigate_table[] = {
+     { XRT_AXIGATE, (kernel_ulong_t)&xrt_axigate_data },
+     { },
+};
+
+static struct platform_driver xrt_axigate_driver = {
+     .driver = {
+             .name = XRT_AXIGATE,
+     },
+     .probe = xrt_axigate_probe,
+     .remove = xrt_axigate_remove,
+     .id_table = xrt_axigate_table,
+};
+
+void axigate_leaf_init_fini(bool init)
+{
+     if (init) {
+             xleaf_register_driver(XRT_SUBDEV_AXIGATE,
+                                   &xrt_axigate_driver, xrt_axigate_endpoints);
+     } else {
+             xleaf_unregister_driver(XRT_SUBDEV_AXIGATE);
+     }
+}
This thing is duplicated in every file, maybe a macro would be an idea.
Will define a macro.

Thanks,
Lizhi
--
2.18.4

- Moritz




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux