Hello Rosen,
> -----Original Message-----
> From: Xu, Rosen [mailto:rosen...@intel.com]
> Sent: Friday, May 4, 2018 7:41 PM
> To: dev@dpdk.org
> Cc: rosen...@intel.com; declan.dohe...@intel.com;
> bruce.richard...@intel.com; Shreyansh Jain <shreyansh.j...@nxp.com>;
> ferruh.yi...@intel.com; konstantin.anan...@intel.com;
> tianfei.zh...@intel.com; song....@intel.com; hao...@intel.com;
> gaetan.ri...@6wind.com; Yanglong Wu <yanglong...@intel.com>
> Subject: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver
>
> From: Rosen Xu <rosen...@intel.com>
>
> Add Intel FPGA BUS Rawdev Driver which is based on
> librte_rawdev library.
>
> Signed-off-by: Rosen Xu <rosen...@intel.com>
> Signed-off-by: Yanglong Wu <yanglong...@intel.com>
> Signed-off-by: Figo zhang <tianfei.zh...@intel.com>
> ---
> config/common_base | 1 +
> drivers/raw/Makefile | 1 +
> drivers/raw/ifpga_rawdev/Makefile | 36 ++
> drivers/raw/ifpga_rawdev/ifpga_rawdev.c | 599
> +++++++++++++++++++++
> drivers/raw/ifpga_rawdev/ifpga_rawdev.h | 37 ++
> .../raw/ifpga_rawdev/rte_ifpga_rawdev_version.map | 4 +
> mk/rte.app.mk | 1 +
> 7 files changed, 679 insertions(+)
> create mode 100644 drivers/raw/ifpga_rawdev/Makefile
> create mode 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.c
> create mode 100644 drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> create mode 100644
> drivers/raw/ifpga_rawdev/rte_ifpga_rawdev_version.map
>
[...]
> --- /dev/null
> +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
> @@ -0,0 +1,599 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +
> +#include <string.h>
> +#include <dirent.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <rte_log.h>
> +#include <rte_bus.h>
> +#include <rte_eal_memconfig.h>
> +#include <rte_malloc.h>
> +#include <rte_devargs.h>
> +#include <rte_memcpy.h>
> +#include <rte_pci.h>
> +#include <rte_bus_pci.h>
> +#include <rte_kvargs.h>
> +#include <rte_alarm.h>
> +
> +#include <rte_errno.h>
> +#include <rte_per_lcore.h>
> +#include <rte_memory.h>
> +#include <rte_memzone.h>
> +#include <rte_eal.h>
> +#include <rte_common.h>
> +#include <rte_bus_vdev.h>
> +
> +#include "base/opae_hw_api.h"
> +#include "rte_rawdev.h"
> +#include "rte_rawdev_pmd.h"
> +#include "rte_bus_ifpga.h"
> +#include "ifpga_common.h"
> +#include "ifpga_logs.h"
> +#include "ifpga_rawdev.h"
> +
> +int ifpga_rawdev_logtype;
> +
> +#define PCI_VENDOR_ID_INTEL 0x8086
> +/* PCI Device ID */
> +#define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
> +#define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
> +#define PCIE_DEVICE_ID_PF_DSC_1_X 0x09C4
> +/* VF Device */
> +#define PCIE_DEVICE_ID_VF_INT_5_X 0xBCBF
> +#define PCIE_DEVICE_ID_VF_INT_6_X 0xBCC1
> +#define PCIE_DEVICE_ID_VF_DSC_1_X 0x09C5
> +#define RTE_MAX_RAW_DEVICE 10
> +
> +static const struct rte_pci_id pci_ifpga_map[] = {
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X)
> },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_INT_5_X)
> },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_6_X)
> },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_INT_6_X)
> },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_DSC_1_X)
> },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_DSC_1_X)
> },
> + { .vendor_id = 0, /* sentinel */ },
> +};
> +
> +static int ifpga_fill_afu_dev(struct opae_accelerator *acc,
> + struct rte_afu_device *afu_dev)
Sorry, for nitpicking, but the function definitions should follow [1] model
where return types are different lines than name.
Something like:
static int
ifpga_fill_afu_dev(...)
[1]
http://dpdk.org/doc/guides/contributing/coding_style.html#c-function-definition-declaration-and-use
This is valid for multiple function definitions. Can you please fix when you
send v8 (which you might have to send for rebasing over master).
> +{
> + struct rte_mem_resource *res = afu_dev->mem_resource;
> + struct opae_acc_region_info region_info;
> + struct opae_acc_info info;
> + unsigned long i;
> + int ret;
> +
> + ret = opae_acc_get_info(acc, &info);
> + if (ret)
> + return ret;
> +
> + if (info.num_regions > PCI_MAX_RESOURCE)
> + return -EFAULT;
> +
> + afu_dev->num_region = info.num_regions;
> +
> + for (i = 0; i < info.num_regions; i++) {
> + region_info.index = i;
> + ret = opae_acc_get_region_info(acc, ®ion_info);
> + if (ret)
> + return ret;
> +
> + if ((region_info.flags & ACC_REGION_MMIO) &&
> + (region_info.flags & ACC_REGION_READ) &&
> + (region_info.flags & ACC_REGION_WRITE)) {
> + res[i].phys_addr = region_info.phys_addr;
> + res[i].len = region_info.len;
> + res[i].addr = region_info.addr;
> + } else
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static void ifpga_rawdev_info_get(struct rte_rawdev *dev,
> + rte_rawdev_obj_t dev_info)
> +{
> + struct opae_adapter *adapter;
> + struct opae_accelerator *acc;
> + struct rte_afu_device *afu_dev;
> +
> + IFPGA_RAWDEV_PMD_FUNC_TRACE();
> +
[...]
> +
> +static int
> +ifpga_rawdev_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> + struct rte_pci_device *pci_dev)
> +{
> +
> + IFPGA_RAWDEV_PMD_INFO("## %s\n", __func__);
Actually, this is not INFO - this is debug. And, this seems like place for
FUNC_TRACE call you have already defined in your log file.
> + return ifpga_rawdev_create(pci_dev, rte_socket_id());
> +}
> +
> +static int ifpga_rawdev_pci_remove(struct rte_pci_device *pci_dev)
> +{
> + return ifpga_rawdev_destroy(pci_dev);
> +}
> +
> +static struct rte_pci_driver rte_ifpga_rawdev_pmd = {
> + .id_table = pci_ifpga_map,
> + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> + .probe = ifpga_rawdev_pci_probe,
> + .remove = ifpga_rawdev_pci_remove,
> +};
> +
[...]
> diff --git a/drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> new file mode 100644
> index 0000000..9a09561
> --- /dev/null
> +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
> + */
> +
> +#ifndef _IFPGA_RAWDEV_H_
> +#define _IFPGA_RAWDEV_H_
> +
> +extern int ifpga_rawdev_logtype;
> +
> +#define IFPGA_RAWDEV_PMD_LOG(level, fmt, args...) \
> + rte_log(RTE_LOG_ ## level, ifpga_rawdev_logtype, "ifgpa: " fmt
> "\n", \
> + ##args)
Another one.
In many of your logs, you are adding '\n' in the end. Your PMD_LOG definition
also has a '\n' - that is double new lines being printed for those logs.
> +
> +#define IFPGA_RAWDEV_PMD_FUNC_TRACE() IFPGA_RAWDEV_PMD_LOG(DEBUG,
> ">>")
> +
> +#define IFPGA_RAWDEV_PMD_DEBUG(fmt, args...) \
> + IFPGA_RAWDEV_PMD_LOG(DEBUG, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_INFO(fmt, args...) \
> + IFPGA_RAWDEV_PMD_LOG(INFO, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_ERR(fmt, args...) \
> + IFPGA_RAWDEV_PMD_LOG(ERR, fmt, ## args)
> +#define IFPGA_RAWDEV_PMD_WARN(fmt, args...) \
> + IFPGA_RAWDEV_PMD_LOG(WARNING, fmt, ## args)
> +
> +enum ifpga_rawdev_device_state {
> + IFPGA_IDLE,
> + IFPGA_READY,
> + IFPGA_ERROR
> +};
> +
> +static inline struct opae_adapter *
> +ifpga_rawdev_get_priv(const struct rte_rawdev *rawdev)
> +{
> + return rawdev->dev_private;
> +}
> +
> +#endif /* _IFPGA_RAWDEV_H_ */
[...]
There are some nitpicks highlighted in mail above - but nothing major. I would
have no more comments (as well as review) for this.
In principle, I am OK with this. For v8, please feel free to use for this patch:
Acked-by: Shreyansh Jain <shreyansh.j...@nxp.com>
Thanks for the work and patience.
