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