> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, March 7, 2019 1:55 AM
> To: Zhang, Tianfei <tianfei.zh...@intel.com>; Xu, Rosen
> <rosen...@intel.com>; dev@dpdk.org
> Cc: Wei, Dan <dan....@intel.com>; Pei, Andy <andy....@intel.com>; Yang,
> Qiming <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>;
> Chen, Santos <santos.c...@intel.com>; Zhang, Zhang
> <zhang.zh...@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...@intel.com>; dev@dpdk.org
> >> Cc: Zhang, Tianfei <tianfei.zh...@intel.com>; Wei, Dan
> >> <dan....@intel.com>; Pei, Andy <andy....@intel.com>; Yang, Qiming
> >> <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>;
> Chen,
> >> Santos <santos.c...@intel.com>; Zhang, Zhang
> <zhang.zh...@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.zh...@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.
>
