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 > >>>>> 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? > > 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 <[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/ > > Thanks, would you mind putting these links into driver documentation as > well? ACK > > > > >>>> > >>>> <....> > >>>> > >>> > >>> -- > >>> - Tomasz Duszyński > >>> > >> > > > > -- > > - Tomasz Duszyński > > > -- - Tomasz Duszyński
