Re: [dpdk-dev] [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: 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?