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 adapter. > >>> 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 <[email protected]> > >>> Tomasz Duszynski <[email protected]> > >>> > >>> Marvell: > >>> Dmitri Epshtein <[email protected]> > >>> Natalie Samsonov <[email protected]> > >>> > >>> Signed-off-by: Jacek Siuda <[email protected]> > >>> Signed-off-by: Tomasz Duszynski <[email protected]> > >> > >> <...> > >> > >>> +static struct rte_vdev_driver pmd_mrvl_drv = { > >>> + .probe = rte_pmd_mrvl_probe, > >>> + .remove = 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 projects. > > 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ński > > > -- - Tomasz Duszyński
