From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f48.google.com (mail-lf0-f48.google.com [209.85.215.48]) by dpdk.org (Postfix) with ESMTP id 0A5651B24C for ; Fri, 6 Oct 2017 08:41:08 +0200 (CEST) Received: by mail-lf0-f48.google.com with SMTP id q132so19269313lfe.5 for ; Thu, 05 Oct 2017 23:41:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=7ZfUl/OJG61TrKjPvfLZhUhPu258T5jXPK8nxeOKn3A=; b=S4ICF7f9Q607jkSTy7fxWGocswFXkIPshSgzEqGnlihzpvcUIIc0WKBNAPRkHSuS3/ 7YRDtPFrCDm3hyj9mTUy2IZW1LN/qU34w0wtp0EYjCE33mw7mkU83ZoicmOpsUZ8MB0n Wxo3qxCKXBMfwV4vpgFF8AoyYNoeYaSDEdiXml+U1a+8sfIrYHrjhIjn0cpWTJ7CYc1n Z4FErFpUfP79qNt6kTYOqBX8Mb3o8u5Vk7nu1CJyH9TtGBLv0CC83qt02Lh6dZT1RbXi 3lLu98iLe2CLC8UaQ1f4jqmW8hauC5vErQdpRId+rk9hUtxObc0b+SZeieWr/FJwYHhe FiLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7ZfUl/OJG61TrKjPvfLZhUhPu258T5jXPK8nxeOKn3A=; b=H8zrE193Kh8lW62RTOy/D/T2w3ZaqqNLrD8QCp/Oa1cP9nikK8tmCdiL0UzQLsG3sX OYAAj82/M2CEPKZXJfFM1PgenihWNhOfGzlunEstexas2tn1mmf9mOWdF5SJhJRSng+t oi1emkNBk/9l5QJD+B0OFRmm3vk9F/q369uhVurLHF8+t3FwvoN44eddSO1hOqxFlFt0 w5asHMKw64Xtybxi88UFDHf+iXbytF9B8RcVnicxtSJJ44NECAl+MAhpefOeBN1vtlUs g/T6E6ElyHXk5WkH3JEK6kY4vX9CETR4pPBgABQGssQrJq/VqxgMX1LjBsNc4yHc9e5V vwdQ== X-Gm-Message-State: AMCzsaUUJ9RYl3yEg3Wt7WSoiNEsxyFNCW0kaS9lsqDXXfncRmqkAuYp CpG9/v4iCU3nzxz0dOq9+pUQxw== X-Google-Smtp-Source: AOwi7QAiAhfFEKl2eY4sPhbBjIreI0PsgdYz+DJTDNUwxWUlMqD6V5g5vSguD+axein9/crK321PZQ== X-Received: by 10.25.162.134 with SMTP id l128mr420580lfe.198.1507272067042; Thu, 05 Oct 2017 23:41:07 -0700 (PDT) Received: from localhost (31-172-191-173.noc.fibertech.net.pl. [31.172.191.173]) by smtp.gmail.com with ESMTPSA id o41sm136558lfi.93.2017.10.05.23.41.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Oct 2017 23:41:06 -0700 (PDT) Date: Fri, 6 Oct 2017 08:41:05 +0200 From: Tomasz Duszynski To: Ferruh Yigit Cc: Tomasz Duszynski , dev@dpdk.org, mw@semihalf.com, dima@marvell.com, nsamsono@marvell.com, Jianbo.liu@linaro.org, Jacek Siuda Message-ID: <20171006064105.GA11828@tdu> References: <1506594158-15721-2-git-send-email-tdu@semihalf.com> <1507031500-11473-1-git-send-email-tdu@semihalf.com> <1507031500-11473-3-git-send-email-tdu@semihalf.com> <20171004085959.GB5668@tdu> <20171005084333.GA20961@tdu> <14bc8ee0-d474-0041-8cc9-22c2f881eef0@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <14bc8ee0-d474-0041-8cc9-22c2f881eef0@intel.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver 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: Fri, 06 Oct 2017 06:41:09 -0000 On Thu, Oct 05, 2017 at 06:29:12PM +0100, Ferruh Yigit wrote: > On 10/5/2017 9:43 AM, Tomasz Duszynski wrote: > > On Wed, Oct 04, 2017 at 05:59:11PM +0100, Ferruh Yigit wrote: > >> On 10/4/2017 9:59 AM, Tomasz Duszynski wrote: > >>> On Wed, Oct 04, 2017 at 01:24:27AM +0100, Ferruh Yigit wrote: > >>>> On 10/3/2017 12:51 PM, Tomasz Duszynski wrote: > >>>>> Add support for the Marvell PPv2 (Packet Processor v2) 1/10 Gbps ad= apter. > >>>>> Driver is based on external, publicly available, light-weight Marve= ll > >>>>> MUSDK library that provides access to network packet processor. > >>>>> > >>>>> Driver comes with support for the following features: > >>>>> > >>>>> * Speed capabilities > >>>>> * Link status > >>>>> * Queue start/stop > >>>>> * MTU update > >>>>> * Jumbo frame > >>>>> * Promiscuous mode > >>>>> * Allmulticast mode > >>>>> * Unicast MAC filter > >>>>> * Multicast MAC filter > >>>>> * RSS hash > >>>>> * VLAN filter > >>>>> * CRC offload > >>>>> * L3 checksum offload > >>>>> * L4 checksum offload > >>>>> * Packet type parsing > >>>>> * Basic stats > >>>>> * Stats per queue > >>>> > >>>> I have more detailed comments but in high level, > >>>> what do you think splitting this patch into three patches: > >>>> - Skeleton > >>>> - Add Rx/Tx support > >>>> - Add features, like MTU update or Promiscuous etc.. support > >>> If it's how submission process works then I think you left me with no > >>> other option than splitting driver into nice patchset :). > >> > >> No, there is no defined submission process. > >> > >>> On the other > >>> hand driver is really a wrapper to MUSDK library and thus quite easy = to > >>> follow. What are the benefits of such 3-way split? > >> > >> To help others review/understand your code. Big code chunks are scary > >> and I believe most of details gets lost in big code chunks. > >> > >> When someone from community wants to understand and update/improve/fix > >> your code, to help them by logically split the code that their focus c= an > >> go into more narrow part. > >> > >> But this also means some effort in your side, so some kind of balance = is > >> required. > >> > >> I think splitting patch into smaller logical part is helpful for other= s, > >> what do you think, is it too much effort? > >> > > > > Fair enough. I'll split the driver as suggested. A few specific > > questions about functionality each patch should contain though. > > > > As for skeleton, I see others just put driver probing here. > > > > As for Rx/Tx support it seems that there's no common pattern. > > Functionality like starting/stopping device, queues configuration > > and all the other things related to Rx/Tx should be here as well? > > As you said there is no common pattern, but I think starting/stopping > device, queues configuration can go into skeleton and mainly Rx/Tx burst > functions can go into Rx/Tx patch. > But please what you think more reasonable matters here. > ACK > > > > What's left are features which go into features-patch. > > Yes. > And the .ini file, currently part of doc patch, can be part of this > features patch, it is helps more to see the code add feature and doc > documents it in same patch. > ACK > > > >>>> > >>>>> > >>>>> Driver was engineered cooperatively by Semihalf and Marvell teams. > >>>>> > >>>>> Semihalf: > >>>>> Jacek Siuda > >>>>> Tomasz Duszynski > >>>>> > >>>>> Marvell: > >>>>> Dmitri Epshtein > >>>>> Natalie Samsonov > >>>>> > >>>>> Signed-off-by: Jacek Siuda > >>>>> Signed-off-by: Tomasz Duszynski > >>>> > >>>> <...> > >>>> > >>>>> +static struct rte_vdev_driver pmd_mrvl_drv =3D { > >>>>> + .probe =3D rte_pmd_mrvl_probe, > >>>>> + .remove =3D rte_pmd_mrvl_remove, > >>>>> +}; > >>>>> + > >>>>> +RTE_PMD_REGISTER_VDEV(net_mrvl, pmd_mrvl_drv); > >>>> > >>>> Please help me understand. > >>>> > >>>> This driver implemented as virtual driver, because: > >>>> With the help of custom kernel modules, musdk library already provid= es > >>>> userspace datapath support. This PMD is an interface to musdk librar= y. > >>>> Is this correct? > >>> That is right. Another reason this NIC is not PCI device. > >> > >> We support more bus now :). Out of curiosity, which bus is device on? > > > > Bus is called Aurora2. That's proprietary SoC interconnect fabric. > > > >> > >>>> > >>>> If so, just thinking loud: > >>>> - Why not implement this PMD directly on top of kernel interface, > >>>> removing musdk layer completely? > >>>> - How big problem that this PMD depends on custom kernel code? > >>> I think the main reason is that MUSDK is already used in different pr= ojects. > >>> Keeping multiple codebases offering similar functionality would be qu= ite > >>> demanding in terms of extra work needed. > >>>> - How library and custom kernel code delivered? For which platforms? > >>> Kernel and library sources are hosted on publicly available repositor= y. > >> > >> I guess it would be nice to highlight custom kernel with external > >> patches is required. This is not mentioned in "Prerequisites" section = of > >> the document. > >> > > > > ACK > > > >>> Driver was tested on Armada 7k/8k SoCs. > >> > >> Can you please provide link to the HW mentioned in documentation? > >> > > > > You can find some info here: > > > > https://www.marvell.com/embedded-processors/armada-70xx/ > > https://www.marvell.com/embedded-processors/armada-80xx/ > > Thanks, would you mind putting these links into driver documentation as > well? ACK > > > > >>>> > >>>> <....> > >>>> > >>> > >>> -- > >>> - Tomasz Duszy=C5=84ski > >>> > >> > > > > -- > > - Tomasz Duszy=C5=84ski > > > -- - Tomasz Duszy=C5=84ski