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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.