Re: [PATCH 5/9] raw/dpaa2_cmdif: introduce DPAA2 command interface driver

[Nipun Gupta <nipun.gupta@nxp.com>]

> -----Original Message-----
> From: Shreyansh Jain
> Sent: Thursday, February 22, 2018 18:42
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: thomas@monjalon.net; Hemant Agrawal <hemant.agrawal@nxp.com>;
> dev@dpdk.org
> Subject: Re: [PATCH 5/9] raw/dpaa2_cmdif: introduce DPAA2 command
> interface driver
> 
> Hi Nipun,
> 
> Some trivial inputs:
> 
> On Thursday 22 February 2018 03:04 PM, Nipun Gupta wrote:
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> >   MAINTAINERS                                        |   6 +
> >   config/common_armv8a_linuxapp                      |   5 +
> >   config/common_base                                 |   5 +
> >   config/defconfig_arm64-dpaa2-linuxapp-gcc          |   6 +
> >   drivers/raw/Makefile                               |   1 +
> >   drivers/raw/dpaa2_cmdif/Makefile                   |  33 +++++
> >   drivers/raw/dpaa2_cmdif/dpaa2_cmdif.c              | 139
> +++++++++++++++++++++
> >   drivers/raw/dpaa2_cmdif/dpaa2_cmdif_logs.h         |  35 ++++++
> >   .../dpaa2_cmdif/rte_pmd_dpaa2_cmdif_version.map    |   4 +
> >   mk/rte.app.mk                                      |   1 +
> >   10 files changed, 235 insertions(+)
> >   create mode 100644 drivers/raw/dpaa2_cmdif/Makefile
> >   create mode 100644 drivers/raw/dpaa2_cmdif/dpaa2_cmdif.c
> >   create mode 100644 drivers/raw/dpaa2_cmdif/dpaa2_cmdif_logs.h
> >   create mode 100644
> drivers/raw/dpaa2_cmdif/rte_pmd_dpaa2_cmdif_version.map
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a646ca3..e06df3b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -725,6 +725,12 @@ F: drivers/crypto/zuc/
> >   F: doc/guides/cryptodevs/zuc.rst
> >   F: doc/guides/cryptodevs/features/zuc.ini
> >
> > +Rawdev Drivers
> > +--------------
> > +
> > +DPAA2 CMDIF
> > +M: Nipun Gupta <nipun.gupta@nxp.com>
> > +F: drivers/raw/dpaa2_cmdif/
> 
> I think you should put one more new line here. For all the sub-section
> for Maintainers, there are 2 new lines before new sub-section starts.

Agree.. I'll take care in the next version.

> 
> >
> >   Eventdev Drivers
> >   ----------------
> > diff --git a/config/common_armv8a_linuxapp
> b/config/common_armv8a_linuxapp
> > index 507b28a..32e3cfe 100644
> > --- a/config/common_armv8a_linuxapp
> > +++ b/config/common_armv8a_linuxapp
> > @@ -94,3 +94,8 @@
> CONFIG_RTE_DPAA_SEC_PMD_MAX_NB_SESSIONS=2048
> >   #
> >   CONFIG_RTE_LIBRTE_PMD_DPAA2_SEC=y
> >   CONFIG_RTE_DPAA2_SEC_PMD_MAX_NB_SESSIONS=2048
> > +
> > +#
> > +# Compile DPAA2 Command Intefrace driver
>                             ^^^^^^^^^
>                       Interface - spell mistake

Agree. I will update.

> 
> > +#
> > +CONFIG_RTE_LIBRTE_PMD_DPAA2_CMDIF=y
> > diff --git a/config/common_base b/config/common_base
> > index ad03cf4..a549b54 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -587,6 +587,11 @@ CONFIG_RTE_RAWDEV_MAX_DEVS=10
> >   CONFIG_RTE_LIBRTE_PMD_SKELETON_RAWDEV=y
> >
> >   #
> > +# Compile DPAA2 CMDIF support
> > +#
> > +CONFIG_RTE_LIBRTE_PMD_DPAA2_CMDIF=n
> > +
> > +#
> >   # Compile librte_ring
> >   #
> >   CONFIG_RTE_LIBRTE_RING=y
> > diff --git a/config/defconfig_arm64-dpaa2-linuxapp-gcc
> b/config/defconfig_arm64-dpaa2-linuxapp-gcc
> > index afdbc34..ce1ffc5 100644
> > --- a/config/defconfig_arm64-dpaa2-linuxapp-gcc
> > +++ b/config/defconfig_arm64-dpaa2-linuxapp-gcc
> > @@ -42,3 +42,9 @@ CONFIG_RTE_LIBRTE_DPAA2_DEBUG_TX_FREE=n
> >   CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_INIT=n
> >   CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_DRIVER=n
> >   CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_RX=n
> > +
> > +#
> > +# Compile DPAA2 Command Intefrace driver
> > +#
> > +CONFIG_RTE_LIBRTE_PMD_DPAA2_CMDIF=y
> 
> If already added in armv8a, why is it required in arm64-dpaa2 as well?
> Rawdevices are intended to be custom and I think they should be added
> only to the SoC specific configuration files.

As I understand you suggest this to be part of only arm64-dpaa2 and not in armv8a?

Thanks,
Nipun

> 
> -
> Shreyansh