From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BB120A052A; Tue, 26 Jan 2021 16:29:55 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9A321140F8C; Tue, 26 Jan 2021 16:29:55 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id C7841140F88 for ; Tue, 26 Jan 2021 16:29:53 +0100 (CET) IronPort-SDR: uXXuyJQ721BBAdIzPu9gkefwS1jto74sm9jiKBdk5QViJzBERcOFdwkZHHr1+LhTzmCoz/QZPY EmilI7GmvOng== X-IronPort-AV: E=McAfee;i="6000,8403,9876"; a="198705861" X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="198705861" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 07:29:52 -0800 IronPort-SDR: e+sRlEpFPuuYmYndIwGUmc2C0uYvh/FS1GrpMvHO5t8MovM0RePAAevKOCvyuzuzA7iAJn0xLu EKSO96gtyKQw== X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="362021403" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.227.53]) ([10.213.227.53]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 07:29:51 -0800 To: Nalla Pradeep , Radha Mohan Chintakuntla , Veerasenareddy Burru Cc: jerinj@marvell.com, dev@dpdk.org, sburla@marvell.com References: <20210118093602.5449-1-pnalla@marvell.com> <20210118093602.5449-4-pnalla@marvell.com> From: Ferruh Yigit Message-ID: Date: Tue, 26 Jan 2021 15:29:49 +0000 MIME-Version: 1.0 In-Reply-To: <20210118093602.5449-4-pnalla@marvell.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 04/11] net/octeontx_ep: Added basic device setup. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/18/2021 9:35 AM, Nalla Pradeep wrote: > Functions to setup device, basic IQ and OQ registers are added. > As the patch title can you please start with lowercase after module, and do not use the final '.', if you run './devtools/check-git-log.sh' script it will highlight these kind of issues, can you please be sure all are addressed in next version. Also it would helpful to clarify all abbreviations used in the commit log, like IQ and OQ used above, it is helpul if you can write the long version for the fist time they are used like, Output Queue (OQ). > Signed-off-by: Nalla Pradeep <...> > +static void > +otx2_vf_setup_global_oq_reg(struct otx_ep_device *otx_ep, int q_no) > +{ > + volatile uint64_t reg_val = 0ull; > + > + reg_val = otx2_read64(otx_ep->hw_addr + SDP_VF_R_OUT_CONTROL(q_no)); > + > +#if defined(BUFPTR_ONLY_MODE) > + reg_val &= ~(SDP_VF_R_OUT_CTL_IMODE); > +#else > + reg_val |= (SDP_VF_R_OUT_CTL_IMODE); > +#endif Where this macro, BUFPTR_ONLY_MODE, is defined, using macros like this can lead to the dead code, can you please either remove this or rename with RTE_ prefix and document it? <...> > +int > +otx2_ep_vf_setup_device(struct otx_ep_device *otx_ep) > +{ > + uint64_t reg_val = 0ull; > + > + /* If application doesn't provide its conf, use driver default conf */ > + if (otx_ep->conf == NULL) { > + otx_ep->conf = otx2_ep_get_defconf(otx_ep); > + if (otx_ep->conf == NULL) { > + otx2_err("SDP VF default config not found"); > + return -ENOMEM; Similar comment as previous, please prefer ENOMEM error type when there is a problem with memory. There are multiple occurance of returning ENOMEM. <...> > @@ -21,10 +22,12 @@ otx_ep_chip_specific_setup(struct otx_ep_device *otx_epvf) > switch (dev_id) { > case PCI_DEVID_OCTEONTX_EP_VF: > otx_epvf->chip_id = dev_id; > + ret = otx_ep_vf_setup_device(otx_epvf); > break; > case PCI_DEVID_OCTEONTX2_EP_NET_VF: > case PCI_DEVID_CN98XX_EP_NET_VF: > otx_epvf->chip_id = dev_id; > + ret = otx2_ep_vf_setup_device(otx_epvf); > break; > default: > otx_ep_err("Unsupported device\n"); > @@ -46,6 +49,13 @@ otx_epdev_init(struct otx_ep_device *otx_epvf) > goto setup_fail; > } > > + if (otx_epvf->fn_list.setup_device_regs(otx_epvf)) { > + otx_ep_err("Failed to configure device registers\n"); > + goto setup_fail; > + } So both otx & otx2 devices are supported via function pointers, out of curiosity how different the implementations are, since both are implementing same spec as far as I understand.