From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 520BC2BA3 for ; Wed, 6 Mar 2019 18:54:53 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2019 09:54:52 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,448,1544515200"; d="scan'208";a="149186262" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.14.113]) ([10.252.14.113]) by fmsmga002.fm.intel.com with ESMTP; 06 Mar 2019 09:54:49 -0800 To: "Zhang, Tianfei" , "Xu, Rosen" , "dev@dpdk.org" Cc: "Wei, Dan" , "Pei, Andy" , "Yang, Qiming" , "Wang, Haiyue" , "Chen, Santos" , "Zhang, Zhang" References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1551338000-120348-4-git-send-email-rosen.xu@intel.com> <07a297cd-79b2-db7b-9029-471e8be60a67@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLuQINBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABiQI8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: <543b6067-9056-635c-5ac3-f96a88fb0c95@intel.com> Date: Wed, 6 Mar 2019 17:54:49 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v1 03/11] drivers/raw/ifpga_rawdev: add OPAE share code for IPN3KE X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Mar 2019 17:54:54 -0000 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 ; dev@dpdk.org >> Cc: Zhang, Tianfei ; Wei, Dan >> ; Pei, Andy ; Yang, Qiming >> ; Wang, Haiyue ; Chen, >> Santos ; Zhang, Zhang >> 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 >>> --- >>> 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.