From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tianfei.zhang@intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
 by dpdk.org (Postfix) with ESMTP id 31CDA23C
 for <dev@dpdk.org>; Sun,  6 May 2018 02:28:42 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
 by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 05 May 2018 17:28:41 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.49,367,1520924400"; d="scan'208";a="47038982"
Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205])
 by FMSMGA003.fm.intel.com with ESMTP; 05 May 2018 17:28:40 -0700
Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by
 fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS)
 id 14.3.319.2; Sat, 5 May 2018 17:28:40 -0700
Received: from cdsmsx151.ccr.corp.intel.com (172.17.4.38) by
 FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS)
 id 14.3.319.2; Sat, 5 May 2018 17:28:40 -0700
Received: from cdsmsx104.ccr.corp.intel.com ([169.254.4.183]) by
 CDSMSX151.ccr.corp.intel.com ([169.254.3.38]) with mapi id 14.03.0319.002;
 Sun, 6 May 2018 08:28:37 +0800
From: "Zhang, Tianfei" <tianfei.zhang@intel.com>
To: Shreyansh Jain <shreyansh.jain@nxp.com>, "Xu, Rosen" <rosen.xu@intel.com>, 
 "dev@dpdk.org" <dev@dpdk.org>
CC: "Doherty, Declan" <declan.doherty@intel.com>, "Richardson, Bruce"
 <bruce.richardson@intel.com>, "Yigit, Ferruh" <ferruh.yigit@intel.com>,
 "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Liu, Song"
 <song.liu@intel.com>, "Wu, Hao" <hao.wu@intel.com>, "gaetan.rivet@6wind.com"
 <gaetan.rivet@6wind.com>, "Wu, Yanglong" <yanglong.wu@intel.com>
Thread-Topic: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver
Thread-Index: AQHT47GZ8yCs6dWYTUuyByDOF3NdzKQg9DmAgADmiXA=
Date: Sun, 6 May 2018 00:28:37 +0000
Message-ID: <BA6F50564D52C24884F9840E07E32DEC4DD75A7C@CDSMSX104.ccr.corp.intel.com>
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>
 <HE1PR0402MB27800B8745D97FAC720467EC90850@HE1PR0402MB2780.eurprd04.prod.outlook.com>
In-Reply-To: <HE1PR0402MB27800B8745D97FAC720467EC90850@HE1PR0402MB2780.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTg5NGU1NjAtOGUwZC00MjQzLTk1ZGItYTM3ZWUwMWU1ZWNhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJVNkU1YUhHMTh2UEcrOUhqOEpmSmFFeXArcUg2T3JsXC93azFKUlZ2RWVKM0R3emkrTGhUR0JSMEo1MElkOHJMbyJ9
x-ctpclassification: CTP_NT
x-originating-ip: [172.17.6.105]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev
	Driver
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Sun, 06 May 2018 00:28:42 -0000



> -----Original Message-----
> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> Sent: Sunday, May 6, 2018 2:43 AM
> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>; Liu, Song <song.liu@intel.com>; Wu, Hao
> <hao.wu@intel.com>; gaetan.rivet@6wind.com; Wu, Yanglong
> <yanglong.wu@intel.com>
> Subject: RE: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver
>=20
> Hello Rosen,
>=20
> > -----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 <shreyansh.jain@nxp.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 <yanglong.wu@intel.com>
> > Subject: [PATCH v7 3/5] iFPGA: Add Intel FPGA BUS Rawdev Driver
> >
> > From: Rosen Xu <rosen.xu@intel.com>
> >
> > Add Intel FPGA BUS Rawdev Driver which is based on librte_rawdev
> > library.
> >
> > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > Signed-off-by: Yanglong Wu <yanglong.wu@intel.com>
> > Signed-off-by: Figo zhang <tianfei.zhang@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
> >
>=20
> [...]
>=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 <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[] =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)
>=20
> Sorry, for nitpicking, but the function definitions should follow [1] mod=
el
> where return types are different lines than name.
>=20
> Something like:
>=20
> static int
> ifpga_fill_afu_dev(...)
>=20
>=20
> [1]
> http://dpdk.org/doc/guides/contributing/coding_style.html#c-function-defi
> nition-declaration-and-use
>=20
> 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).
>=20
> > +{
> > +	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, &region_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();
> > +
>=20
> [...]
>=20
> > +
> > +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__);
>=20
> Actually, this is not INFO - this is debug. And, this seems like place fo=
r
> FUNC_TRACE call you have already defined in your log file.
>=20
> > +	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,
> > +};
> > +
>=20
> [...]
>=20
> > 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)
>=20
> 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 t=
hose
> logs.
>=20
> > +
> > +#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_ */
>=20
> [...]
>=20
> 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:
>=20
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>=20
> Thanks for the work and patience.

Thanks Shreyansh great support, we will send the v8 patches today.