> -----Original Message----- > From: Shreyansh Jain [mailto:shreyansh.j...@nxp.com] > Sent: Sunday, May 6, 2018 2:43 AM > To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org > Cc: Doherty, Declan <declan.dohe...@intel.com>; Richardson, Bruce > <bruce.richard...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>; > Ananyev, Konstantin <konstantin.anan...@intel.com>; Zhang, Tianfei > <tianfei.zh...@intel.com>; Liu, Song <song....@intel.com>; Wu, Hao > <hao...@intel.com>; gaetan.ri...@6wind.com; Wu, Yanglong > <yanglong...@intel.com> > Subject: RE: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver > > 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-defi > nition-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.
Thanks Shreyansh great support, we will send the v8 patches today.
