From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 0D3CB2C52 for ; Wed, 6 Mar 2019 13:27:40 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2019 04:27:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,447,1544515200"; d="scan'208";a="280218225" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.14.113]) ([10.252.14.113]) by orsmga004.jf.intel.com with ESMTP; 06 Mar 2019 04:27:36 -0800 To: Rosen Xu , dev@dpdk.org Cc: tianfei.zhang@intel.com, dan.wei@intel.com, andy.pei@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, santos.chen@intel.com, zhang.zhang@intel.com References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1551338000-120348-4-git-send-email-rosen.xu@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: <07a297cd-79b2-db7b-9029-471e8be60a67@intel.com> Date: Wed, 6 Mar 2019 12:27:36 +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: <1551338000-120348-4-git-send-email-rosen.xu@intel.com> 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 12:27:41 -0000 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 > --- > 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, ^~~