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

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

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?

And if possible can you please split this patch into logical parts?

> 
> 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?

<...>

> +	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=]

<...>

> @@ -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'?

<...>

> +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));
                                                                     ^

<...>

> +/**
> + * 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.

<...>

> @@ -0,0 +1,490 @@
> +
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation

Do you prefer to update date to 2019?

<...>

> +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()

<...>

> +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]

<...>

> +
> +#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,
    ^~~