Message-ID: <446d9abb-b1ee-b0e7-9c48-aa7e960ec1e9@xilinx.com>
Date:   Thu, 13 Jan 2022 15:41:47 -0800
From:   Lizhi Hou <lizhi.hou@...inx.com>
To:     Xu Yilun <yilun.xu@...el.com>, Lizhi Hou <lizhi.hou@...inx.com>
CC:     <linux-kernel@...r.kernel.org>, <linux-fpga@...r.kernel.org>,
        <maxz@...inx.com>, <sonal.santan@...inx.com>, <yliu@...inx.com>,
        <michal.simek@...inx.com>, <stefanos@...inx.com>,
        <devicetree@...r.kernel.org>, <trix@...hat.com>, <mdf@...nel.org>,
        <robh@...nel.org>, <dwmw2@...radead.org>,
        Max Zhen <max.zhen@...inx.com>
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 <sonal.santan@...inx.com>
>> Signed-off-by: Max Zhen <max.zhen@...inx.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@...inx.com>
>> ---
>>   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: Sonal.Santan@...inx.com
>> +#
>> +
>> +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 <lizhih@...inx.com>
>> + */
>> +
>> +#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 <maxz@...inx.com>
>> + *   Lizhi Hou <lizhih@...inx.com>
>> + */
>> +
>> +#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 <runtime@...inx.com>");
>> +MODULE_DESCRIPTION("Xilinx Alveo management function driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.27.0

Powered by blists - more mailing lists