From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f51.google.com (mail-lf0-f51.google.com [209.85.215.51]) by dpdk.org (Postfix) with ESMTP id C4DE91B199 for ; Thu, 5 Oct 2017 10:43:35 +0200 (CEST) Received: by mail-lf0-f51.google.com with SMTP id b127so16099143lfe.9 for ; Thu, 05 Oct 2017 01:43:35 -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=lSreNdKdpoAyNv+ONQQuISYdZMOig+YxhT/ttWk1DAU=; b=aqwDDZqqBQTz5uIKlBG/KVc81zDzm1So/PTwenYSQspO9Oi4JnElsDNAjWTtyZr/Q8 D/NOqsHM5l4CIVhIzTKBav9OPGfZDr6le7FnNwrT/maUMYG+PGB3B/Cd1zYlEC06GCPe K3AUVWbLiS4i3AOg/p8GrOADei9EEn0t1EaR8p6tFbqBfp7pondljdrcJhifQ9IoqOHS jbBtJX3a3ICVVNAafJXYIhbw4QS/YlNbftemA+rGYEO0+PYeDQSChX7b7RxfLep0ocB6 f9CphnPCjNdD4UjbF/1MJZxTqm6PLG5SFXwE+aT8TfYQ7tJPTkDn0DpmJv+XQTjicM9l 3wcA== 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=lSreNdKdpoAyNv+ONQQuISYdZMOig+YxhT/ttWk1DAU=; b=G2LHGF6AfyaaA+YY7oCJN1/aAeSs9LWO3sLE5mj8pDD5quaea3FwElrcGWi8KbXDea 1k4tLEHWuzOVz9MKCZD5eWF5tXLw9+9Uhh8ectkxxrRGYUALUFPUojg5GZ61IZVKf+Ua 52p1Xjlu5y9yavCeJLRiDEXuCPjzvtXuFZGtl5wQNPvwuBNdXL7plef2JcjtNtd8k5sL NceRVB8FI2yvm5TBJ8AD2BjMu5F8wP5zSXkPvlKv1CLNDEJ5ExWK3Gnz/I2IJZID+l5M rAACbLXQCGlFw4wGC4u+INuP56mn5KdiWxGGGDWIZer71mOodMgLWg+YZ3JcwbKdjC26 iZXg== X-Gm-Message-State: AMCzsaWzr8ocIQ+2d2BbMoo1DXGtBgICgMhCbuHgkDPniF3b5Vm46T5m 6MeXonZVrnkcRMDPpZouxvKqYQ== X-Google-Smtp-Source: AOwi7QD/fyayZR8crS5Rt8bIx7WbveJIS1ATDRbRRvHrk2Yr8nkSf1HquCCZfOwx9BiTI8VMK1YhQQ== X-Received: by 10.46.34.66 with SMTP id i63mr1429554lji.99.1507193015353; Thu, 05 Oct 2017 01:43:35 -0700 (PDT) Received: from localhost (31-172-191-173.noc.fibertech.net.pl. [31.172.191.173]) by smtp.gmail.com with ESMTPSA id x4sm2819724lfa.55.2017.10.05.01.43.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Oct 2017 01:43:34 -0700 (PDT) Date: Thu, 5 Oct 2017 10:43:33 +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: <20171005084333.GA20961@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: 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: Thu, 05 Oct 2017 08:43:35 -0000 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 adap= ter. > >>> Driver is based on external, publicly available, light-weight Marvell > >>> 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 can > 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 others, > 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? What's left are features which go into features-patch. > >> > >>> > >>> 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 provides > >> userspace datapath support. This PMD is an interface to musdk library. > >> 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 proj= ects. > > Keeping multiple codebases offering similar functionality would be quite > > 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 repository. > > 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/ > >> > >> <....> > >> > > > > -- > > - Tomasz Duszy=C5=84ski > > > -- - Tomasz Duszy=C5=84ski