Message-ID: <e6ed0d0d-7ac1-cde9-7ed5-ae2bf8991a49@xilinx.com>
Date:   Fri, 5 Mar 2021 11:58:25 -0800
From:   Lizhi Hou <lizhi.hou@...inx.com>
To:     Tom Rix <trix@...hat.com>, Lizhi Hou <lizhi.hou@...inx.com>,
        <linux-kernel@...r.kernel.org>
CC:     <linux-fpga@...r.kernel.org>, <maxz@...inx.com>,
        <sonal.santan@...inx.com>, <michal.simek@...inx.com>,
        <stefanos@...inx.com>, <devicetree@...r.kernel.org>,
        <mdf@...nel.org>, <robh@...nel.org>, Max Zhen <max.zhen@...inx.com>
Subject: Re: [PATCH V3 XRT Alveo 10/18] fpga: xrt: VSEC platform driver

Hi Tom,


On 03/01/2021 11:01 AM, Tom Rix wrote:
> On 2/17/21 10:40 PM, Lizhi Hou wrote:
>> Add VSEC driver. VSEC is a hardware function discovered by walking
>> PCI Express configure space. A platform device node will be created
>> for it. VSEC provides board logic UUID and few offset of other hardware
>> functions.
> Is this vsec walking infra or is a general find a list of mmio regions that need to be mapped in and do the mapping in as a set of platform drivers ?
vsec is pointed by PCIe vender-specific capability. And vsec itself 
locates on PCI BAR. vsec has a list of minimum IPs (mmio regions) 
required for driver to load firmware and communicate with the other pcie 
function. After firmware is loaded, xrt will look into the fireware 
metadata to get the information of rest IPs.
vsec  driver notifies the root driver for the list of minimum IPs been 
discovered. Then the root driver will create platform device nodes and 
bring up drivers based on vsec's notification.
>> Signed-off-by: Sonal Santan <sonal.santan@...inx.com>
>> Signed-off-by: Max Zhen <max.zhen@...inx.com>
>> Signed-off-by: Lizhi Hou <lizhih@...inx.com>
>> ---
>>   drivers/fpga/xrt/lib/xleaf/vsec.c | 359 ++++++++++++++++++++++++++++++
>>   1 file changed, 359 insertions(+)
>>   create mode 100644 drivers/fpga/xrt/lib/xleaf/vsec.c
>>
>> diff --git a/drivers/fpga/xrt/lib/xleaf/vsec.c b/drivers/fpga/xrt/lib/xleaf/vsec.c
>> new file mode 100644
>> index 000000000000..8e5cb22522ec
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/lib/xleaf/vsec.c
>> @@ -0,0 +1,359 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx Alveo FPGA VSEC Driver
>> + *
>> + * Copyright (C) 2020-2021 Xilinx, Inc.
>> + *
>> + * Authors:
>> + *      Lizhi Hou<Lizhi.Hou@...inx.com>
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include "metadata.h"
>> +#include "xleaf.h"
>> +
>> +#define XRT_VSEC "xrt_vsec"
>> +
>> +#define VSEC_TYPE_UUID               0x50
>> +#define VSEC_TYPE_FLASH              0x51
>> +#define VSEC_TYPE_PLATINFO   0x52
>> +#define VSEC_TYPE_MAILBOX    0x53
>> +#define VSEC_TYPE_END                0xff
> Type of devices, this list can not grow much.
Because vsec only contains minimum required IPs for loading firmware and 
communication. The list will only change when there is major hardware 
change.
>> +
>> +#define VSEC_UUID_LEN                16
>> +
>> +struct xrt_vsec_header {
>> +     u32             format;
>> +     u32             length;
>> +     u32             entry_sz;
>> +     u32             rsvd;
>> +} __packed;
>> +
>> +#define head_rd(g, r)                        \
>> +     ioread32((void *)(g)->base + offsetof(struct xrt_vsec_header, r))
>> +
>> +#define GET_BAR(entry)       (((entry)->bar_rev >> 4) & 0xf)
>> +#define GET_BAR_OFF(_entry)                          \
>> +     ({ typeof(_entry) entry = (_entry);             \
>> +      ((entry)->off_lo | ((u64)(entry)->off_hi << 16)); })
> A 48 bit value stored in xrt_md_endpoint.bar_off (long)
>
> bar_off should be u64
Will fix this.
>
>> +#define GET_REV(entry)       ((entry)->bar_rev & 0xf)
>> +
> I prefer functions over macros.
Will change to inline function.
>> +struct xrt_vsec_entry {
>> +     u8              type;
>> +     u8              bar_rev;
>> +     u16             off_lo;
>> +     u32             off_hi;
>> +     u8              ver_type;
>> +     u8              minor;
>> +     u8              major;
>> +     u8              rsvd0;
>> +     u32             rsvd1;
>> +} __packed;
>> +
>> +#define read_entry(g, i, e)                                  \
>> +     do {                                                    \
>> +             u32 *p = (u32 *)((g)->base +                    \
>> +                     sizeof(struct xrt_vsec_header) +        \
>> +                     (i) * sizeof(struct xrt_vsec_entry));   \
>> +             u32 off;                                        \
>> +             for (off = 0;                                   \
>> +                 off < sizeof(struct xrt_vsec_entry) / 4;    \
>> +                 off++)                                      \
>> +                     *((u32 *)(e) + off) = ioread32(p + off);\
>> +     } while (0)
> This could be a static inline func.
Will change to inline function.
>> +
>> +struct vsec_device {
>> +     u8              type;
>> +     char            *ep_name;
>> +     ulong           size;
>> +     char            *regmap;
>> +};
>> +
>> +static struct vsec_device vsec_devs[] = {
>> +     {
>> +             .type = VSEC_TYPE_UUID,
>> +             .ep_name = XRT_MD_NODE_BLP_ROM,
>> +             .size = VSEC_UUID_LEN,
>> +             .regmap = "vsec-uuid",
>> +     },
>> +     {
>> +             .type = VSEC_TYPE_FLASH,
>> +             .ep_name = XRT_MD_NODE_FLASH_VSEC,
>> +             .size = 4096,
>> +             .regmap = "vsec-flash",
>> +     },
>> +     {
>> +             .type = VSEC_TYPE_PLATINFO,
>> +             .ep_name = XRT_MD_NODE_PLAT_INFO,
>> +             .size = 4,
>> +             .regmap = "vsec-platinfo",
>> +     },
>> +     {
>> +             .type = VSEC_TYPE_MAILBOX,
>> +             .ep_name = XRT_MD_NODE_MAILBOX_VSEC,
>> +             .size = 48,
>> +             .regmap = "vsec-mbx",
>> +     },
> This is a static list, how would a new type be added to this ?
Because the list will only change when there is major hardware change, 
the list will be update manually if hardware introduces a new type.
>> +};
>> +
>> +struct xrt_vsec {
>> +     struct platform_device  *pdev;
>> +     void                    *base;
>> +     ulong                   length;
>> +
>> +     char                    *metadata;
>> +     char                    uuid[VSEC_UUID_LEN];
>> +};
>> +
>> +static char *type2epname(u32 type)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(vsec_devs); i++) {
>> +             if (vsec_devs[i].type == type)
>> +                     return (vsec_devs[i].ep_name);
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static ulong type2size(u32 type)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(vsec_devs); i++) {
>> +             if (vsec_devs[i].type == type)
>> +                     return (vsec_devs[i].size);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static char *type2regmap(u32 type)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(vsec_devs); i++) {
>> +             if (vsec_devs[i].type == type)
>> +                     return (vsec_devs[i].regmap);
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static int xrt_vsec_add_node(struct xrt_vsec *vsec,
>> +                          void *md_blob, struct xrt_vsec_entry *p_entry)
>> +{
>> +     struct xrt_md_endpoint ep;
>> +     char regmap_ver[64];
>> +     int ret;
>> +
>> +     if (!type2epname(p_entry->type))
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * VSEC may have more than 1 mailbox instance for the card
>> +      * which has more than 1 physical function.
>> +      * This is not supported for now. Assuming only one mailbox
>> +      */
> are multiple uuid types allowed ?
No. And there will be only one uuid in vsec list.
>
> this says assume 1, but logic will recreate 1+
>
> can you check if a mbx ep exists before creating ?
Maybe the comment is confusing. All current Alveo boards only have one 
mailbox in vsec list. In theory, there could be more than 1 mailboxes in 
the future. And how it will present in vsec list is undetermined.
>
>> +
>> +     snprintf(regmap_ver, sizeof(regmap_ver) - 1, "%d-%d.%d.%d",
>> +              p_entry->ver_type, p_entry->major, p_entry->minor,
>> +              GET_REV(p_entry));
>> +     ep.ep_name = type2epname(p_entry->type);
>> +     ep.bar = GET_BAR(p_entry);
>> +     ep.bar_off = GET_BAR_OFF(p_entry);
> here is the bar_off type overlow
Will fix it.
>> +     ep.size = type2size(p_entry->type);
>> +     ep.regmap = type2regmap(p_entry->type);
>> +     ep.regmap_ver = regmap_ver;
>> +     ret = xrt_md_add_endpoint(DEV(vsec->pdev), vsec->metadata, &ep);
>> +     if (ret) {
>> +             xrt_err(vsec->pdev, "add ep failed, ret %d", ret);
>> +             goto failed;
>> +     }
>> +
>> +failed:
>> +     return ret;
>> +}
>> +
>> +static int xrt_vsec_create_metadata(struct xrt_vsec *vsec)
>> +{
>> +     struct xrt_vsec_entry entry;
>> +     int i, ret;
>> +
>> +     ret = xrt_md_create(&vsec->pdev->dev, &vsec->metadata);
>> +     if (ret) {
>> +             xrt_err(vsec->pdev, "create metadata failed");
>> +             return ret;
>> +     }
>> +
>> +     for (i = 0; i * sizeof(entry) < vsec->length -
>> +         sizeof(struct xrt_vsec_header); i++) {
>> +             read_entry(vsec, i, &entry);
>> +             xrt_vsec_add_node(vsec, vsec->metadata, &entry);
> This can fail.
Will add check.
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int xrt_vsec_ioctl(struct platform_device *pdev, u32 cmd, void *arg)
>> +{
>> +     int ret = 0;
>> +
>> +     switch (cmd) {
>> +     case XRT_XLEAF_EVENT:
>> +             /* Does not handle any event. */
>> +             break;
>> +     default:
>> +             ret = -EINVAL;
>> +             xrt_err(pdev, "should never been called");
>> +             break;
>> +     }
> This function looks like a noop.  Is anything going to be added to this later ?
It could be. And there are broadcast events can reach to this handler. I 
think it is harmless to ignore and return.
>> +
>> +     return ret;
>> +}
>> +
>> +static int xrt_vsec_mapio(struct xrt_vsec *vsec)
>> +{
>> +     struct xrt_subdev_platdata *pdata = DEV_PDATA(vsec->pdev);
>> +     const u32 *bar;
>> +     const u64 *bar_off;
>> +     struct resource *res = NULL;
>> +     ulong addr;
>> +     int ret;
>> +
>> +     if (!pdata || xrt_md_size(DEV(vsec->pdev), pdata->xsp_dtb) == XRT_MD_INVALID_LENGTH) {
>> +             xrt_err(vsec->pdev, "empty metadata");
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = xrt_md_get_prop(DEV(vsec->pdev), pdata->xsp_dtb, XRT_MD_NODE_VSEC,
>> +                           NULL, XRT_MD_PROP_BAR_IDX, (const void **)&bar, NULL);
>> +     if (ret) {
>> +             xrt_err(vsec->pdev, "failed to get bar idx, ret %d", ret);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = xrt_md_get_prop(DEV(vsec->pdev), pdata->xsp_dtb, XRT_MD_NODE_VSEC,
>> +                           NULL, XRT_MD_PROP_OFFSET, (const void **)&bar_off, NULL);
>> +     if (ret) {
>> +             xrt_err(vsec->pdev, "failed to get bar off, ret %d", ret);
>> +             return -EINVAL;
>> +     }
>> +
>> +     xrt_info(vsec->pdev, "Map vsec at bar %d, offset 0x%llx",
>> +              be32_to_cpu(*bar), be64_to_cpu(*bar_off));
>> +
>> +     xleaf_get_barres(vsec->pdev, &res, be32_to_cpu(*bar));
>> +     if (!res) {
>> +             xrt_err(vsec->pdev, "failed to get bar addr");
>> +             return -EINVAL;
>> +     }
>> +
>> +     addr = res->start + (ulong)be64_to_cpu(*bar_off);
> review this type, addr is ulong and bar_off is not.
Will use u64.
>> +
>> +     vsec->base = ioremap(addr, sizeof(struct xrt_vsec_header));
>> +     if (!vsec->base) {
>> +             xrt_err(vsec->pdev, "Map header failed");
>> +             return -EIO;
>> +     }
> why the double call on ioremap ?
>
> just do the last one.
The first ioremap only maps in the header and read out the length of the 
body (mmio list).
Then the next ioremap maps in the body based on the length.
>
>> +
>> +     vsec->length = head_rd(vsec, length);
>> +     iounmap(vsec->base);
>> +     vsec->base = ioremap(addr, vsec->length);
>> +     if (!vsec->base) {
>> +             xrt_err(vsec->pdev, "map failed");
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int xrt_vsec_remove(struct platform_device *pdev)
>> +{
>> +     struct xrt_vsec *vsec;
>> +
>> +     vsec = platform_get_drvdata(pdev);
>> +
>> +     if (vsec->base) {
>> +             iounmap(vsec->base);
>> +             vsec->base = NULL;
>> +     }
>> +
>> +     vfree(vsec->metadata);
>> +
>> +     return 0;
>> +}
>> +
>> +static int xrt_vsec_probe(struct platform_device *pdev)
>> +{
>> +     struct xrt_vsec *vsec;
>> +     int                     ret = 0;
>> +
>> +     vsec = devm_kzalloc(&pdev->dev, sizeof(*vsec), GFP_KERNEL);
>> +     if (!vsec)
>> +             return -ENOMEM;
>> +
>> +     vsec->pdev = pdev;
>> +     platform_set_drvdata(pdev, vsec);
>> +
>> +     ret = xrt_vsec_mapio(vsec);
>> +     if (ret)
>> +             goto failed;
>> +
>> +     ret = xrt_vsec_create_metadata(vsec);
>> +     if (ret) {
>> +             xrt_err(pdev, "create metadata failed, ret %d", ret);
>> +             goto failed;
>> +     }
>> +     ret = xleaf_create_group(pdev, vsec->metadata);
>> +     if (ret < 0)
>> +             xrt_err(pdev, "create group failed, ret %d", ret);
>> +     else
>> +             ret = 0;
> why is it just
>
> if (ret)
>
>    fail ?
xleaf_create_group() returns 0 or positive id on success. I will change to

if (ret < 0)
     goto fail;
  return 0

fail:

Thanks,
Lizhi
>
> Tom
>
>> +
>> +failed:
>> +     if (ret)
>> +             xrt_vsec_remove(pdev);
>> +
>> +     return ret;
>> +}
>> +
>> +static struct xrt_subdev_endpoints xrt_vsec_endpoints[] = {
>> +     {
>> +             .xse_names = (struct xrt_subdev_ep_names []){
>> +                     { .ep_name = XRT_MD_NODE_VSEC },
>> +                     { NULL },
>> +             },
>> +             .xse_min_ep = 1,
>> +     },
>> +     { 0 },
>> +};
>> +
>> +static struct xrt_subdev_drvdata xrt_vsec_data = {
>> +     .xsd_dev_ops = {
>> +             .xsd_ioctl = xrt_vsec_ioctl,
>> +     },
>> +};
>> +
>> +static const struct platform_device_id xrt_vsec_table[] = {
>> +     { XRT_VSEC, (kernel_ulong_t)&xrt_vsec_data },
>> +     { },
>> +};
>> +
>> +static struct platform_driver xrt_vsec_driver = {
>> +     .driver = {
>> +             .name = XRT_VSEC,
>> +     },
>> +     .probe = xrt_vsec_probe,
>> +     .remove = xrt_vsec_remove,
>> +     .id_table = xrt_vsec_table,
>> +};
>> +
>> +void vsec_leaf_init_fini(bool init)
>> +{
>> +     if (init)
>> +             xleaf_register_driver(XRT_SUBDEV_VSEC, &xrt_vsec_driver, xrt_vsec_endpoints);
>> +     else
>> +             xleaf_unregister_driver(XRT_SUBDEV_VSEC);
>> +}

Powered by blists - more mailing lists