Re: [PATCH v1 03/11] drivers/raw/ifpga_rawdev: add OPAE share code for IPN3KE

["Zhang, Tianfei" <tianfei.zhang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, March 7, 2019 1:55 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Xu, Rosen
> <rosen.xu@intel.com>; dev@dpdk.org
> Cc: Wei, Dan <dan.wei@intel.com>; Pei, Andy <andy.pei@intel.com>; Yang,
> Qiming <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> Chen, Santos <santos.chen@intel.com>; Zhang, Zhang
> <zhang.zhang@intel.com>
> Subject: Re: [PATCH v1 03/11] drivers/raw/ifpga_rawdev: add OPAE share
> code for IPN3KE
> 
> On 3/6/2019 1:59 PM, Zhang, Tianfei wrote:
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, March 6, 2019 8:28 PM
> >> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
> >> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; Wei, Dan
> >> <dan.wei@intel.com>; Pei, Andy <andy.pei@intel.com>; Yang, Qiming
> >> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> Chen,
> >> Santos <santos.chen@intel.com>; Zhang, Zhang
> <zhang.zhang@intel.com>
> >> Subject: Re: [PATCH v1 03/11] drivers/raw/ifpga_rawdev: add OPAE
> >> share code for IPN3KE
> >>
> >> On 2/28/2019 7:13 AM, Rosen Xu wrote:
> >>> Add OPAE share code for Intel FPGA Acceleration NIC IPN3KE.
> >>
> >> What do you think adding a file to record the version of the shared
> >> code, as it is done in Intel NIC drivers, README file?
> >>
> >> Also can you please add more details on what feautures has been added
> >> with this update?
> >
> > Thanks, good suggestion, I will add a README file in next version.
> >
> >>
> >> And if possible can you please split this patch into logical parts?
> >
> > Ok, is it possible split into multiple small patches for share code？if
> necessary, I will do it in next version.
> >
> >>
> >>>
> >>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> >>> ---
> >>>  drivers/raw/ifpga_rawdev/base/Makefile             |   7 +
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_api.c          |  69 ++-
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_api.h          |   1 +
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_defines.h      |  86 +++-
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_enumerate.c    | 342
> >> +++++--------
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_feature_dev.c  | 170 +++++--
> >>> drivers/raw/ifpga_rawdev/base/ifpga_feature_dev.h  |  62 ++-
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_fme.c          | 373
> >> ++++++++++++++
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_fme_pr.c       |   2 +-
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_hw.h           |  21 +-
> >>>  drivers/raw/ifpga_rawdev/base/ifpga_port.c         |  21 +
> >>>  drivers/raw/ifpga_rawdev/base/opae_at24_eeprom.c   |  89 ++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_at24_eeprom.h   |  14 +
> >>>  drivers/raw/ifpga_rawdev/base/opae_hw_api.c        | 189
> ++++++-
> >>>  drivers/raw/ifpga_rawdev/base/opae_hw_api.h        |  46 +-
> >>>  drivers/raw/ifpga_rawdev/base/opae_i2c.c           | 490
> >> +++++++++++++++++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_i2c.h           | 127 +++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_intel_max10.c   | 106 ++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_intel_max10.h   |  36 ++
> >>>  drivers/raw/ifpga_rawdev/base/opae_mdio.c          | 542
> >> +++++++++++++++++++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_mdio.h          |  90 ++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_osdep.h         |  11 +-
> >>>  drivers/raw/ifpga_rawdev/base/opae_phy_group.c     |  88 ++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_phy_group.h     |  53 ++
> >>>  drivers/raw/ifpga_rawdev/base/opae_spi.c           | 260
> >> ++++++++++
> >>>  drivers/raw/ifpga_rawdev/base/opae_spi.h           | 120 +++++
> >>>  .../raw/ifpga_rawdev/base/opae_spi_transaction.c   | 438
> >> +++++++++++++++++
> >>>  .../ifpga_rawdev/base/osdep_raw/osdep_generic.h    |   1 +
> >>>  .../ifpga_rawdev/base/osdep_rte/osdep_generic.h    |  10 +
> >>>  29 files changed, 3549 insertions(+), 315 deletions(-)
> >>
> >> <...>
> >>
> >>> @@ -165,37 +68,35 @@ static u64 feature_id(void __iomem *start)
> >>>
> >>>  static int
> >>>  build_info_add_sub_feature(struct build_feature_devs_info *binfo,
> >>> -			   struct feature_info *finfo, void __iomem *start)
> >>> +		void __iomem *start, u64 fid, unsigned int size,
> >>> +		unsigned int vec_start,
> >>> +		unsigned int vec_cnt)
> >>>  {
> >>>  	struct ifpga_hw *hw = binfo->hw;
> >>>  	struct feature *feature = NULL;
> >>
> >> struct names are so generic 'struct feature' & 'struct feature_ops'
> >> defined in ifpga_hw.h, they seems not added in this patch, but what
> >> do you think prefix them via "ifpga_" in a separate patch?
> >
> > Yes, I agree, add prefix name is better, maybe we forget that patch, will fix
> in next version.
> >
> >>
> >> <...>
> >>
> >>> +	feature->vec_start = vec_start;
> >>> +	feature->vec_cnt = vec_cnt;
> >>> +
> >>> +	dev_debug(binfo, "%s: id=0x%lx, phys_addr=0x%lx, size=%d\n",
> >>> +			__func__, feature->id, feature->phys_addr, size);
> >>
> >> 32bit build complains about %lx usage:
> >>
> >> .../dpdk/drivers/raw/ifpga_rawdev/base/ifpga_enumerate.c:99:51: error:
> >> format ‘%lx’ expects argument of type ‘long unsigned int’, but
> >> argument
> >> 5 has type ‘u64’ {aka ‘long long unsigned int’} [-Werror= format=]
> >
> > In this v1 patch, we forget check the 32 bit compiler, will fix in next version.
> >
> >>
> >> <...>
> >>
> >>> @@ -651,12 +539,19 @@ static int parse_feature(struct
> >> build_feature_devs_info *binfo,
> >>>  		}
> >>>
> >>>  		hdr = (struct feature_header *)start;
> >>> +		header.csr = readq(hdr);
> >>> +
> >>> +		/*debug*/
> >>
> >> I think can drop above comment, not adding extra information.
> >>
> >>> +		dev_debug(binfo, "%s: address=0x%llx, val=0x%lx,
> header.id=0x%x,
> >> header.next_offset=0x%x, header.eol=0x%x, header.type=0x%x\n",
> >>> +			__func__, (unsigned long long)(hdr), header.csr,
> >>
> >> Why not use "%p" to print the address of the variable but cast it to
> >> 'unsigned long long'?
> > Will fix in next version.
> >>
> >> <...>
> >>
> >>> +static int fme_spi_init(struct feature *feature) {
> >>> +	struct feature_fme_spi *spi;
> >>> +	struct ifpga_fme_hw *fme = (struct ifpga_fme_hw
> *)feature->parent;
> >>> +	struct altera_spi_device *spi_master;
> >>> +	struct intel_max10_device *max10;
> >>> +	int ret = 0;
> >>> +
> >>> +	spi = (struct feature_fme_spi *)feature->addr;
> >>> +
> >>> +	dev_info(fme, "FME SPI Master (Max10) Init.\n");
> >>> +	dev_debug(fme, "FME SPI base addr %llx.\n",
> >>> +		 (unsigned long long)spi);
> >>
> >> Same comment here, why not "%p", why casting the variable?
> >>
> >>> +	dev_debug(fme, "spi param=0x%lx\n", opae_readq(feature->addr
> +
> >>> +0x8));
> >>
> >> .../dpdk/drivers/raw/ifpga_rawdev/base/ifpga_fme.c:774:69: error:
> >> format ‘%lx’
> >> expects argument of type ‘long unsigned int’, but argument 4 has type
> >> ‘uint64_t’
> >> {aka ‘long long unsigned int’} [-Werror= format=]
> >>   dev_debug(fme, "spi param=0x%lx\n", opae_readq(feature->addr +
> >> 0x8));
> >
> > Will fix in next version.
> >>
> >> ^
> >>
> >> <...>
> >>
> >>> +/**
> >>> + * opae_manager_get_retimer_info - get retimer info like PKVL chip
> >>> + * @mgr: opae_manager for retimer
> >>> + * @info: info return to caller
> >>> + *
> >>> + * Return: 0 on success, otherwise error code  */ int
> >>> +opae_manager_get_retimer_info(struct opae_manager *mgr,
> >>> +	       struct opae_retimer_info *info) {
> >>> +	if (!mgr || !mgr->network_ops)
> >>> +		return -EINVAL;
> >>> +
> >>> +	//if (mgr->network_ops->get_retimer_info)
> >>> +	//	return mgr->network_ops->get_retimer_info(mgr, info);
> >>
> >>
> >> Please remove commented code, and for comments please prefer c89
> >> style comments.
> >
> > Will fix in next version.
> >
> >>
> >> <...>
> >>
> >>> @@ -0,0 +1,490 @@
> >>> +
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright(c) 2010-2018 Intel Corporation
> >>
> >> Do you prefer to update date to 2019?
> >
> > Ok.
> >>
> >> <...>
> >>
> >>> +static void phy_indirect_write(struct phy_group_device *dev, u8 entry,
> >>> +		u16 addr, u32 value)
> >>> +{
> >>> +	u64 ctrl;
> >>> +
> >>> +	ctrl = CMD_RD << CTRL_COMMAND_SHIFT |
> >>> +		(entry & CTRL_PHY_NUM_MASK) << CTRL_PHY_NUM_SHIFT |
> >>> +		(addr & CTRL_PHY_ADDR_MASK) << CTRL_PHY_ADDR_SHIFT |
> >>> +		(value & CTRL_WRITE_DATA_MASK);
> >>
> >> Is 32bit supported?
> >> If so, CMD_RD is defined as 'unsigned long' which is 4bytes long in
> >> 32bit machine. Since CTRL_COMMAND_SHIFT  is 62, the result will be
> >> different than expected. Also there is compiler warning for it:
> >>
> >> .../drivers/raw/ifpga_rawdev/base/opae_phy_group.c: In function
> >> ‘phy_indirect_write’:
> >> .../drivers/raw/ifpga_rawdev/base/opae_phy_group.c:29:16: error: left
> >> shift count >= width of type [-Werror=shift-count-overflow]
> >>   ctrl = CMD_RD << CTRL_COMMAND_SHIFT |
> >>                 ^~
> >>
> >> Same for similar usage is phy_indirect_read()
> >
> > The register is 64bit. We will fix for the 32bit compiler in next version.
> >
> >>
> >> <...>
> >>
> >>> +static unsigned int spi_write_bytes(struct altera_spi_device *dev,
> >>> +int count) {
> >>> +	unsigned int val = 0;
> >>> +	u16 *p16;
> >>> +	u32 *p32;
> >>> +
> >>> +	if (dev->txbuf) {
> >>> +		switch (dev->data_width) {
> >>> +		case 1:
> >>> +			val = dev->txbuf[count];
> >>> +			break;
> >>> +		case 2:
> >>> +			p16 = (u16 *)(dev->txbuf + 2*count);
> >>> +			val = *p16;
> >>> +			if (dev->endian == SPI_BIG_ENDIAN)
> >>> +				val = cpu_to_be16(val);
> >>> +			break;
> >>> +		case 4:
> >>> +			p32 = (u32 *)(dev->txbuf + 4*count);
> >>> +			val = *p32;
> >>> +			if (dev->endian == SPI_BIG_ENDIAN)
> >>> +				val = (val);
> >>
> >> What is the intention here? Compiler warning:
> >>
> >> .../drivers/raw/ifpga_rawdev/base/opae_spi.c:122:9: error: explicitly
> >> assigning value of variable of type 'unsigned int' to itself
> >> [-Werror,-Wself-assign]
> >
> > Ok, will fix in next version.
> >>
> >> <...>
> >>
> >>> +
> >>> +#ifdef OPAE_DEBUG
> >>
> >> Is this DEBUG macro defined anywhere?
> >>
> >> <...>
> >>
> >>> +	switch (trans_type) {
> >>> +	case SPI_TRAN_SEQ_WRITE:
> >>> +	case SPI_TRAN_NON_SEQ_WRITE:
> >>> +		for (i = 0; i < size; i++)
> >>> +			*p++ = *data++;
> >>> +
> >>> +			ret = packet_to_byte_conver(dev, size + HEADER_LEN,
> >>> +				      transaction, RESPONSE_LEN, response,
> >>> +				      &valid_len);
> >>> +			if (ret)
> >>> +				return -EBUSY;
> >>> +
> >>> +			/* check the result */
> >>> +			if (size != ((unsigned int)(response[2] & 0xff) << 8 |
> >>> +					(unsigned int)(response[3] & 0xff)))
> >>> +				ret = -EBUSY;
> >>> +
> >>> +			break;
> >>
> >> Indentation is wrong after 'for' loop. Loops seems copying from
> >> 'data' to 'transaction' buffer, which is used later, so the logic
> >> seems correct but indentation is misleading, it is causing a build warning:
> >>
> >> .../dpdk/drivers/raw/ifpga_rawdev/base/opae_spi_transaction.c: In
> >> function
> >> ‘do_transaction’:
> >> .../dpdk/drivers/raw/ifpga_rawdev/base/opae_spi_transaction.c:362:3:
> >> error: this ‘for’ clause does not guard...
> [-Werror=misleading-indentation]
> >>    for (i = 0; i < size; i++)
> >>    ^~~
> >> .../dpdk/drivers/raw/ifpga_rawdev/base/opae_spi_transaction.c:365:4:
> >> note:
> >> ...this statement, but the latter is misleadingly indented as if it
> >> were guarded by the ‘for’
> >>     ret = packet_to_byte_conver(dev, size + HEADER_LEN,
> >>     ^~~
> > Will fix in next version.
> >
> > What GCC compiler are you using?
> 
> gcc (GCC) 8.2.1 20181215 (Red Hat 8.2.1-6) clang version 7.0.1 (Fedora
> 7.0.1-2.fc29) icc (ICC) 19.0.2.187 20190117
> 
> the build errors I put are from one of the above ones, since there were many
> errors I didn't filter which error is specific to which compiler.
Thanks, we will clean our code for all of compilers in next version.
>