Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch01.xlnx.xilinx.com;
Envelope-to: maxz@xilinx.com,
 max.zhen@xilinx.com,
 michal.simek@xilinx.com,
 sonal.santan@xilinx.com,
 lizhi.hou@xilinx.com,
 stefanos@xilinx.com,
 robh@kernel.org,
 trix@redhat.com,
 devicetree@vger.kernel.org,
 linux-fpga@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 mdf@kernel.org
Subject: Re: [PATCH V3 XRT Alveo 12/18] fpga: xrt: ICAP platform driver
To:     Moritz Fischer <mdf@kernel.org>, Lizhi Hou <lizhi.hou@xilinx.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-fpga@vger.kernel.org>,
        <maxz@xilinx.com>, <sonal.santan@xilinx.com>,
        <michal.simek@xilinx.com>, <stefanos@xilinx.com>,
        <devicetree@vger.kernel.org>, <trix@redhat.com>, <robh@kernel.org>,
        Max Zhen <max.zhen@xilinx.com>
References: <20210218064019.29189-1-lizhih@xilinx.com>
 <20210218064019.29189-13-lizhih@xilinx.com> <YDLBl2GFjSVcxCUe@archbook>
From:   Lizhi Hou <lizhi.hou@xilinx.com>
Message-ID: <5b305195-6515-59b5-03aa-f2714ad6b181@xilinx.com>
Date:   Tue, 2 Mar 2021 10:26:21 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <YDLBl2GFjSVcxCUe@archbook>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2021 18:26:22.1638
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 5314c191-b707-4a45-76fd-08d8dda8ae59
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch01.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: CY1NAM02FT063.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR02MB7378
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Moritz,


On 02/21/2021 12:24 PM, Moritz Fischer wrote:
> On Wed, Feb 17, 2021 at 10:40:13PM -0800, Lizhi Hou wrote:
>> Add ICAP driver. ICAP is a hardware function discovered by walking
>> firmware metadata. A platform device node will be created for it.
>> FPGA bitstream is written to hardware through ICAP.
>>
>> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>> Signed-off-by: Lizhi Hou <lizhih@xilinx.com>
>> ---
>>   drivers/fpga/xrt/include/xleaf/icap.h |  29 +++
>>   drivers/fpga/xrt/lib/xleaf/icap.c     | 317 ++++++++++++++++++++++++++
>>   2 files changed, 346 insertions(+)
>>   create mode 100644 drivers/fpga/xrt/include/xleaf/icap.h
>>   create mode 100644 drivers/fpga/xrt/lib/xleaf/icap.c
>>
>> diff --git a/drivers/fpga/xrt/include/xleaf/icap.h b/drivers/fpga/xrt/include/xleaf/icap.h
>> new file mode 100644
>> index 000000000000..a14fc0ffa78f
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/include/xleaf/icap.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Header file for XRT ICAP Leaf Driver
>> + *
>> + * Copyright (C) 2020-2021 Xilinx, Inc.
>> + *
>> + * Authors:
>> + *   Lizhi Hou <Lizhi.Hou@xilinx.com>
>> + */
>> +
>> +#ifndef _XRT_ICAP_H_
>> +#define _XRT_ICAP_H_
>> +
>> +#include "xleaf.h"
>> +
>> +/*
>> + * ICAP driver IOCTL calls.
>> + */
>> +enum xrt_icap_ioctl_cmd {
>> +     XRT_ICAP_WRITE = XRT_XLEAF_CUSTOM_BASE, /* See comments in xleaf.h */
>> +     XRT_ICAP_IDCODE,
>> +};
>> +
>> +struct xrt_icap_ioctl_wr {
>> +     void    *xiiw_bit_data;
>> +     u32     xiiw_data_len;
>> +};
>> +
>> +#endif       /* _XRT_ICAP_H_ */
>> diff --git a/drivers/fpga/xrt/lib/xleaf/icap.c b/drivers/fpga/xrt/lib/xleaf/icap.c
>> new file mode 100644
>> index 000000000000..0500a97bdef9
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/lib/xleaf/icap.c
>> @@ -0,0 +1,317 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx Alveo FPGA ICAP Driver
>> + *
>> + * Copyright (C) 2020-2021 Xilinx, Inc.
>> + *
>> + * Authors:
>> + *      Lizhi Hou<Lizhi.Hou@xilinx.com>
>> + *      Sonal Santan <sonals@xilinx.com>
>> + *      Max Zhen <maxz@xilinx.com>
>> + */
>> +
>> +#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/icap.h"
>> +#include "xclbin-helper.h"
>> +
>> +#define XRT_ICAP "xrt_icap"
>> +
>> +#define ICAP_ERR(icap, fmt, arg...)  \
>> +     xrt_err((icap)->pdev, fmt "\n", ##arg)
>> +#define ICAP_WARN(icap, fmt, arg...) \
>> +     xrt_warn((icap)->pdev, fmt "\n", ##arg)
>> +#define ICAP_INFO(icap, fmt, arg...) \
>> +     xrt_info((icap)->pdev, fmt "\n", ##arg)
>> +#define ICAP_DBG(icap, fmt, arg...)  \
>> +     xrt_dbg((icap)->pdev, fmt "\n", ##arg)
> Do we really need two layers of indirection here? What's wrong with
> dev_{info,dbg,...} ?
In case that we would change the massage 'fmt' in the future we can 
change it at one place. And it does not expose any interface or 
introduce performance issue. Could we just keep these Macros?
>> +
>> +/*
>> + * AXI-HWICAP IP register layout
>> + */
>> +struct icap_reg {
>> +     u32     ir_rsvd1[7];
>> +     u32     ir_gier;
>> +     u32     ir_isr;
>> +     u32     ir_rsvd2;
>> +     u32     ir_ier;
>> +     u32     ir_rsvd3[53];
>> +     u32     ir_wf;
>> +     u32     ir_rf;
>> +     u32     ir_sz;
>> +     u32     ir_cr;
>> +     u32     ir_sr;
>> +     u32     ir_wfv;
>> +     u32     ir_rfo;
>> +     u32     ir_asr;
>> +} __packed;
> Can we make those #define and just use writel/readl() ? If you want more
> abstraction, use regmap....
Sure, we will use #define as below and use writel/readl to access them.
       #define ICAP_REG_ISR(base)    ((base) + 0x20)
       ....
>> +
>> +struct icap {
>> +     struct platform_device  *pdev;
>> +     struct icap_reg         *icap_regs;
>> +     struct mutex            icap_lock; /* icap dev lock */
>> +
>> +     unsigned int            idcode;
>> +};
>> +
>> +static inline u32 reg_rd(void __iomem *reg)
>> +{
>> +     if (!reg)
>> +             return -1;
>> +
>> +     return ioread32(reg);
>> +}
>> +
>> +static inline void reg_wr(void __iomem *reg, u32 val)
>> +{
>> +     if (!reg)
>> +             return;
>> +
>> +     iowrite32(val, reg);
>> +}
>> +
>> +static int wait_for_done(struct icap *icap)
>> +{
>> +     u32     w;
>> +     int     i = 0;
>> +
>> +     WARN_ON(!mutex_is_locked(&icap->icap_lock));
>> +     for (i = 0; i < 10; i++) {
>> +             udelay(5);
>> +             w = reg_rd(&icap->icap_regs->ir_sr);
>> +             ICAP_INFO(icap, "XHWICAP_SR: %x", w);
>> +             if (w & 0x5)
>> +                     return 0;
>> +     }
>> +
>> +     ICAP_ERR(icap, "bitstream download timeout");
>> +     return -ETIMEDOUT;
>> +}
>> +
>> +static int icap_write(struct icap *icap, const u32 *word_buf, int size)
>> +{
>> +     int i;
>> +     u32 value = 0;
>> +
>> +     for (i = 0; i < size; i++) {
>> +             value = be32_to_cpu(word_buf[i]);
>> +             reg_wr(&icap->icap_regs->ir_wf, value);
>> +     }
>> +
>> +     reg_wr(&icap->icap_regs->ir_cr, 0x1);
>> +
>> +     for (i = 0; i < 20; i++) {
>> +             value = reg_rd(&icap->icap_regs->ir_cr);
>> +             if ((value & 0x1) == 0)
>> +                     return 0;
>> +             ndelay(50);
>> +     }
>> +
>> +     ICAP_ERR(icap, "writing %d dwords timeout", size);
>> +     return -EIO;
>> +}
>> +
>> +static int bitstream_helper(struct icap *icap, const u32 *word_buffer,
>> +                         u32 word_count)
>> +{
>> +     u32 remain_word;
>> +     u32 word_written = 0;
>> +     int wr_fifo_vacancy = 0;
>> +     int err = 0;
>> +
>> +     WARN_ON(!mutex_is_locked(&icap->icap_lock));
>> +     for (remain_word = word_count; remain_word > 0;
>> +             remain_word -= word_written, word_buffer += word_written) {
>> +             wr_fifo_vacancy = reg_rd(&icap->icap_regs->ir_wfv);
>> +             if (wr_fifo_vacancy <= 0) {
>> +                     ICAP_ERR(icap, "no vacancy: %d", wr_fifo_vacancy);
>> +                     err = -EIO;
>> +                     break;
>> +             }
>> +             word_written = (wr_fifo_vacancy < remain_word) ?
>> +                     wr_fifo_vacancy : remain_word;
>> +             if (icap_write(icap, word_buffer, word_written) != 0) {
>> +                     ICAP_ERR(icap, "write failed remain %d, written %d",
>> +                              remain_word, word_written);
>> +                     err = -EIO;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     return err;
>> +}
>> +
>> +static int icap_download(struct icap *icap, const char *buffer,
>> +                      unsigned long length)
>> +{
>> +     u32     num_chars_read = DMA_HWICAP_BITFILE_BUFFER_SIZE;
>> +     u32     byte_read;
>> +     int     err = 0;
>> +
>> +     mutex_lock(&icap->icap_lock);
>> +     for (byte_read = 0; byte_read < length; byte_read += num_chars_read) {
>> +             num_chars_read = length - byte_read;
>> +             if (num_chars_read > DMA_HWICAP_BITFILE_BUFFER_SIZE)
>> +                     num_chars_read = DMA_HWICAP_BITFILE_BUFFER_SIZE;
>> +
>> +             err = bitstream_helper(icap, (u32 *)buffer, num_chars_read / sizeof(u32));
>> +             if (err)
>> +                     goto failed;
>> +             buffer += num_chars_read;
>> +     }
>> +
>> +     err = wait_for_done(icap);
>> +
>> +failed:
>> +     mutex_unlock(&icap->icap_lock);
>> +
>> +     return err;
>> +}
>> +
>> +/*
>> + * Run the following sequence of canned commands to obtain IDCODE of the FPGA
>> + */
>> +static void icap_probe_chip(struct icap *icap)
>> +{
>> +     u32 w;
>> +
>> +     w = reg_rd(&icap->icap_regs->ir_sr);
>> +     w = reg_rd(&icap->icap_regs->ir_sr);
>> +     reg_wr(&icap->icap_regs->ir_gier, 0x0);
>> +     w = reg_rd(&icap->icap_regs->ir_wfv);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0xffffffff);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0xaa995566);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0x20000000);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0x20000000);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0x28018001);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0x20000000);
>> +     reg_wr(&icap->icap_regs->ir_wf, 0x20000000);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     reg_wr(&icap->icap_regs->ir_cr, 0x1);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     w = reg_rd(&icap->icap_regs->ir_sr);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     w = reg_rd(&icap->icap_regs->ir_sr);
>> +     reg_wr(&icap->icap_regs->ir_sz, 0x1);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     reg_wr(&icap->icap_regs->ir_cr, 0x2);
>> +     w = reg_rd(&icap->icap_regs->ir_rfo);
>> +     icap->idcode = reg_rd(&icap->icap_regs->ir_rf);
>> +     w = reg_rd(&icap->icap_regs->ir_cr);
>> +     (void)w;
> ?!
Right. 'w' does not make sense here. We will remove 'w'.

Thanks,
Lizhi
>> +}
>> +
>> +static int
>> +xrt_icap_leaf_ioctl(struct platform_device *pdev, u32 cmd, void *arg)
>> +{
>> +     struct xrt_icap_ioctl_wr        *wr_arg = arg;
>> +     struct icap                     *icap;
>> +     int                             ret = 0;
>> +
>> +     icap = platform_get_drvdata(pdev);
>> +
>> +     switch (cmd) {
>> +     case XRT_XLEAF_EVENT:
>> +             /* Does not handle any event. */
>> +             break;
>> +     case XRT_ICAP_WRITE:
>> +             ret = icap_download(icap, wr_arg->xiiw_bit_data,
>> +                                 wr_arg->xiiw_data_len);
>> +             break;
>> +     case XRT_ICAP_IDCODE:
>> +             *(u64 *)arg = icap->idcode;
>> +             break;
>> +     default:
>> +             ICAP_ERR(icap, "unknown command %d", cmd);
>> +             return -EINVAL;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int xrt_icap_remove(struct platform_device *pdev)
>> +{
>> +     struct icap     *icap;
>> +
>> +     icap = platform_get_drvdata(pdev);
>> +
>> +     platform_set_drvdata(pdev, NULL);
>> +     devm_kfree(&pdev->dev, icap);
>> +
>> +     return 0;
>> +}
>> +
>> +static int xrt_icap_probe(struct platform_device *pdev)
>> +{
>> +     struct icap     *icap;
>> +     int                     ret = 0;
>> +     struct resource         *res;
>> +
>> +     icap = devm_kzalloc(&pdev->dev, sizeof(*icap), GFP_KERNEL);
>> +     if (!icap)
>> +             return -ENOMEM;
>> +
>> +     icap->pdev = pdev;
>> +     platform_set_drvdata(pdev, icap);
>> +     mutex_init(&icap->icap_lock);
>> +
>> +     xrt_info(pdev, "probing");
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (res) {
>> +             icap->icap_regs = ioremap(res->start, res->end - res->start + 1);
>> +             if (!icap->icap_regs) {
>> +                     xrt_err(pdev, "map base failed %pR", res);
>> +                     ret = -EIO;
>> +                     goto failed;
>> +             }
>> +     }
>> +
>> +     icap_probe_chip(icap);
>> +failed:
>> +     return ret;
>> +}
>> +
>> +static struct xrt_subdev_endpoints xrt_icap_endpoints[] = {
>> +     {
>> +             .xse_names = (struct xrt_subdev_ep_names[]) {
>> +                     { .ep_name = XRT_MD_NODE_FPGA_CONFIG },
>> +                     { NULL },
>> +             },
>> +             .xse_min_ep = 1,
>> +     },
>> +     { 0 },
>> +};
>> +
>> +static struct xrt_subdev_drvdata xrt_icap_data = {
>> +     .xsd_dev_ops = {
>> +             .xsd_ioctl = xrt_icap_leaf_ioctl,
>> +     },
>> +};
>> +
>> +static const struct platform_device_id xrt_icap_table[] = {
>> +     { XRT_ICAP, (kernel_ulong_t)&xrt_icap_data },
>> +     { },
>> +};
>> +
>> +static struct platform_driver xrt_icap_driver = {
>> +     .driver = {
>> +             .name = XRT_ICAP,
>> +     },
>> +     .probe = xrt_icap_probe,
>> +     .remove = xrt_icap_remove,
>> +     .id_table = xrt_icap_table,
>> +};
>> +
>> +void icap_leaf_init_fini(bool init)
>> +{
>> +     if (init)
>> +             xleaf_register_driver(XRT_SUBDEV_ICAP, &xrt_icap_driver, xrt_icap_endpoints);
>> +     else
>> +             xleaf_unregister_driver(XRT_SUBDEV_ICAP);
>> +}
>> --
>> 2.18.4
>>
> - Moritz