RE: [PATCH Xilinx Alveo 7/8] fpga: xrt: Alveo management physical function driver
Hello Moritz,
> -Original Message-
> From: Moritz Fischer
> Sent: Tuesday, December 1, 2020 12:52
> 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 7/8] fpga: xrt: Alveo management physical
function driver
>
>
> Hi Sonal,
>
> On Sat, Nov 28, 2020 at 04:00:39PM -0800, Sonal Santan wrote:
> > From: Sonal Santan
> >
> > Add management physical function driver core. The driver attaches
> > to management physical function of Alveo devices. It instantiates
> > the root driver and one or more partition drivers which in turn
> > instantiate platform drivers. The instantiation of partition and
> > platform drivers is completely data driven. The driver integrates
> > with FPGA manager and provides xclbin download service.
> >
> > Signed-off-by: Sonal Santan
> > ---
> > drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c | 194
> > drivers/fpga/alveo/mgmt/xmgmt-fmgr.h | 29 +
> > drivers/fpga/alveo/mgmt/xmgmt-main-impl.h| 36 +
> > drivers/fpga/alveo/mgmt/xmgmt-main-mailbox.c | 930
+++
> > drivers/fpga/alveo/mgmt/xmgmt-main-ulp.c | 190
> > drivers/fpga/alveo/mgmt/xmgmt-main.c | 843 +
> > drivers/fpga/alveo/mgmt/xmgmt-root.c | 375
> > 7 files changed, 2597 insertions(+)
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-fmgr.h
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-impl.h
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-mailbox.c
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-ulp.c
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main.c
> > create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-root.c
> >
> > diff --git a/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
b/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> > new file mode 100644
> > index ..d451b5a2c291
> > --- /dev/null
> > +++ b/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Xilinx Alveo Management Function Driver
> > + *
> > + * Copyright (C) 2019-2020 Xilinx, Inc.
> > + * Bulk of the code borrowed from XRT mgmt driver file, fmgr.c
> > + *
> > + * Authors: sonal.san...@xilinx.com
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "xrt-subdev.h"
> > +#include "xmgmt-fmgr.h"
> > +#include "xrt-axigate.h"
> > +#include "xmgmt-main-impl.h"
> > +
> > +/*
> > + * Container to capture and cache full xclbin as it is passed in blocks by
FPGA
> > + * Manager. Driver needs access to full xclbin to walk through xclbin
sections.
> > + * FPGA Manager's .write() backend sends incremental blocks without any
> > + * knowledge of xclbin format forcing us to collect the blocks and stitch
them
> > + * together here.
> > + */
> > +
> > +struct xfpga_klass {
> Nit: xfpga_priv or xfpga_drvdata?
> > + const struct platform_device *pdev;
> > + struct axlf *blob;
> > + char name[64];
> Nit: 64 could be a named constant ?
> > + size_t count;
> > + size_t total_count;
> > + struct mutex axlf_lock;
> > + int reader_ref;
> > + enum fpga_mgr_states state;
> > + enum xfpga_sec_level sec_level;
> This appears unused, do you want to add this with the code that uses it?
This hook is for validating signature of FPGA image. Will look into adding it
into the next version of the patch.
> > +};
>
> Maybe add some kerneldoc markup?
Will do in the next version
> > +
> > +struct key *xfpga_keys;
> Appears unused, can you introduce this together with the code using it?
> > +
> > +static int xmgmt_pr_write_init(struct fpga_manager *mgr,
> > + struct fpga_image_info *info, const char *buf, size_t count)
> > +{
> > + struct xfpga_klass *obj = mgr->priv;
> > + const struct axlf *bin = (const struct axlf *)buf;
> Nit: Reverse x-mas tree please.
>
> xx
>
> xxx
> x
Will update in the next version
> > +
> > + if (count < sizeof(struct axlf)) {
> > + obj->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> > + return -E
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, &xmm->firmware_blp,
+ &fwlen);
+ if (rc != 0) {
+ rc = load_firmware_from_flash(pdev,
+ &xmm->firmware_blp, &fwlen);
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 7/8] fpga: xrt: Alveo management physical function driver
> +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, &xmm->firmware_blp,
> + &fwlen);
> + if (rc != 0) {
> + rc = load_firmware_from_flash(pdev,
> + &xmm->firmware_blp, &fwlen);
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?
I didn't find the creation of leaf platform devices, maybe I can find
the answer in the missing Patch #5?
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 7/8] fpga: xrt: Alveo management physical function driver
Hi Sonal,
On Sat, Nov 28, 2020 at 04:00:39PM -0800, Sonal Santan wrote:
> From: Sonal Santan
>
> Add management physical function driver core. The driver attaches
> to management physical function of Alveo devices. It instantiates
> the root driver and one or more partition drivers which in turn
> instantiate platform drivers. The instantiation of partition and
> platform drivers is completely data driven. The driver integrates
> with FPGA manager and provides xclbin download service.
>
> Signed-off-by: Sonal Santan
> ---
> drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c | 194
> drivers/fpga/alveo/mgmt/xmgmt-fmgr.h | 29 +
> drivers/fpga/alveo/mgmt/xmgmt-main-impl.h| 36 +
> drivers/fpga/alveo/mgmt/xmgmt-main-mailbox.c | 930 +++
> drivers/fpga/alveo/mgmt/xmgmt-main-ulp.c | 190
> drivers/fpga/alveo/mgmt/xmgmt-main.c | 843 +
> drivers/fpga/alveo/mgmt/xmgmt-root.c | 375
> 7 files changed, 2597 insertions(+)
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-fmgr.h
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-impl.h
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-mailbox.c
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main-ulp.c
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-main.c
> create mode 100644 drivers/fpga/alveo/mgmt/xmgmt-root.c
>
> diff --git a/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> b/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> new file mode 100644
> index ..d451b5a2c291
> --- /dev/null
> +++ b/drivers/fpga/alveo/mgmt/xmgmt-fmgr-drv.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx Alveo Management Function Driver
> + *
> + * Copyright (C) 2019-2020 Xilinx, Inc.
> + * Bulk of the code borrowed from XRT mgmt driver file, fmgr.c
> + *
> + * Authors: sonal.san...@xilinx.com
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "xrt-subdev.h"
> +#include "xmgmt-fmgr.h"
> +#include "xrt-axigate.h"
> +#include "xmgmt-main-impl.h"
> +
> +/*
> + * Container to capture and cache full xclbin as it is passed in blocks by
> FPGA
> + * Manager. Driver needs access to full xclbin to walk through xclbin
> sections.
> + * FPGA Manager's .write() backend sends incremental blocks without any
> + * knowledge of xclbin format forcing us to collect the blocks and stitch
> them
> + * together here.
> + */
> +
> +struct xfpga_klass {
Nit: xfpga_priv or xfpga_drvdata?
> + const struct platform_device *pdev;
> + struct axlf *blob;
> + char name[64];
Nit: 64 could be a named constant ?
> + size_t count;
> + size_t total_count;
> + struct mutex axlf_lock;
> + int reader_ref;
> + enum fpga_mgr_states state;
> + enum xfpga_sec_level sec_level;
This appears unused, do you want to add this with the code that uses it?
> +};
Maybe add some kerneldoc markup?
> +
> +struct key *xfpga_keys;
Appears unused, can you introduce this together with the code using it?
> +
> +static int xmgmt_pr_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info, const char *buf, size_t count)
> +{
> + struct xfpga_klass *obj = mgr->priv;
> + const struct axlf *bin = (const struct axlf *)buf;
Nit: Reverse x-mas tree please.
xx
xxx
x
> +
> + if (count < sizeof(struct axlf)) {
> + obj->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> + return -EINVAL;
> + }
> +
> + if (count > bin->m_header.m_length) {
> + obj->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> + return -EINVAL;
> + }
> +
> + /* Free up the previous blob */
> + vfree(obj->blob);
> + obj->blob = vmalloc(bin->m_header.m_length);
> + if (!obj->blob) {
> + obj->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> + return -ENOMEM;
> + }
> +
> + xrt_info(obj->pdev, "Begin download of xclbin %pUb of length %lld B",
> + &bin->m_header.uuid, bin->m_header.m_length);
We already have framework level prints for that (admittedly somewhat
less verbose). Please remove.
> +
> + obj->count = 0;
> + obj->total_count = bin->m_header.m_length;
> + obj->state = FPGA_MGR_STATE_WRITE_INIT;
Does the framework state tracking not work for you?
> + return 0;
> +}
> +
> +static int xmgmt_pr_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct xfpga_klass *obj = mgr->priv;
> + char *curr = (char *)obj->blob;
> +
> + if ((obj->state != FPGA_MGR_STATE_WRITE_INIT) &&
> + (obj->state != FPGA_MGR_STATE_WRITE)) {
> + obj->state = FPGA_MGR_STATE_WRITE_ERR;
> + return -EINVAL;
> + }
> +
> + curr += obj->count;
> + obj->count += count;
> +
> + /*
> + * The xclbin buffer should not be
