From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver Date: Sat, 5 May 2018 18:42:47 +0000 Message-ID: References: <1521553556-62982-1-git-send-email-rosen.xu@intel.com> <1525443062-43231-1-git-send-email-rosen.xu@intel.com> <1525443062-43231-4-git-send-email-rosen.xu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "declan.doherty@intel.com" , "bruce.richardson@intel.com" , "ferruh.yigit@intel.com" , "konstantin.ananyev@intel.com" , "tianfei.zhang@intel.com" , "song.liu@intel.com" , "hao.wu@intel.com" , "gaetan.rivet@6wind.com" , Yanglong Wu To: "Xu, Rosen" , "dev@dpdk.org" Return-path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0088.outbound.protection.outlook.com [104.47.0.88]) by dpdk.org (Postfix) with ESMTP id 67A5DDED for ; Sat, 5 May 2018 20:43:14 +0200 (CEST) In-Reply-To: <1525443062-43231-4-git-send-email-rosen.xu@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Rosen, > -----Original Message----- > From: Xu, Rosen [mailto:rosen.xu@intel.com] > Sent: Friday, May 4, 2018 7:41 PM > To: dev@dpdk.org > Cc: rosen.xu@intel.com; declan.doherty@intel.com; > bruce.richardson@intel.com; Shreyansh Jain ; > ferruh.yigit@intel.com; konstantin.ananyev@intel.com; > tianfei.zhang@intel.com; song.liu@intel.com; hao.wu@intel.com; > gaetan.rivet@6wind.com; Yanglong Wu > Subject: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver >=20 > From: Rosen Xu >=20 > Add Intel FPGA BUS Rawdev Driver which is based on > librte_rawdev library. >=20 > Signed-off-by: Rosen Xu > Signed-off-by: Yanglong Wu > Signed-off-by: Figo zhang > --- > 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 >=20 [...] > --- /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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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[] =3D { > + { 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 =3D 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-de= finition-declaration-and-use This is valid for multiple function definitions. Can you please fix when yo= u send v8 (which you might have to send for rebasing over master). > +{ > + struct rte_mem_resource *res =3D afu_dev->mem_resource; > + struct opae_acc_region_info region_info; > + struct opae_acc_info info; > + unsigned long i; > + int ret; > + > + ret =3D opae_acc_get_info(acc, &info); > + if (ret) > + return ret; > + > + if (info.num_regions > PCI_MAX_RESOURCE) > + return -EFAULT; > + > + afu_dev->num_region =3D info.num_regions; > + > + for (i =3D 0; i < info.num_regions; i++) { > + region_info.index =3D i; > + ret =3D 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 =3D region_info.phys_addr; > + res[i].len =3D region_info.len; > + res[i].addr =3D 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 =3D { > + .id_table =3D pci_ifpga_map, > + .drv_flags =3D RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC, > + .probe =3D ifpga_rawdev_pci_probe, > + .remove =3D 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 definiti= on 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 wo= uld 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 p= atch: Acked-by: Shreyansh Jain Thanks for the work and patience.