Hi Vipin,
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, December 6, 2018 2:03 PM
> To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 01/20] net/ice: add base code
>
> snipped
> > > > >
> > > > > > +Intel® ICE driver
> > > > > > +==================
> > > > > > +
> > > > > > +This directory contains source code of FreeBSD ice driver of
> > > > > > +version
> > > > > > +2018.10.30 released by the team which develops basic drivers
> > > > > > +for any ice NIC. The directory of base/ contains the original
> > > > > > +source
> > > package.
> > > > > > +This driver is valid for the product(s) listed below
> > > > > > +
> > > > > > +* Intel® Ethernet Network Adapters E810
> > > > > > +
> > > > > > +Updating the driver
> > > > > > +===================
> > > > > > +
> > > > > > +NOTE: The source code in this directory should not be
> > > > > > +modified apart from the following file(s):
> > > > > > +
> > > > > > + ice_osdep.h
> > > > >
> > > > > I this README persistent in upcoming releases of 'driver/net/ice'?
> > > > Yes.
> > > If Linux driver is enabled in 4.20.1 or higher, then will the
> > > wording 'This directory contains source code of FreeBSD ice driver of'
> > > still
> hold true?
> > Although I don't understand why we talk about the Linux driver
> > version, but I think the answer is yes.
> Ok, reason for linux driver is because
> 1. you would be planning to push the default kernel driver to linux for ICE.
> 2. the documentation states FreeBSD 2018.10.30, so if there is future
> enhancement pulled from linux driver this would added here too?
Sure, we'll keep updating this code.
>
> >
> > >
> > > > >
> > > snipped
> > > > > > +#define ICE_AQC_MAN_MAC_UPDATE_LAA 0
> > > > > > +#define ICE_AQC_MAN_MAC_UPDATE_LAA_WOL (BIT(0) <<
> > > > > > ICE_AQC_MAN_MAC_WR_S)
> > > > >
> > > > > Can the code be rearranged for?
> > > > We don’t want to change the base code for the sake of maintenance.
> > > I do not follow this, is not your team or individual maintaining the same?
> > > because there should be maintainer for this PMD.
> > This code is not implemented by us. You can take us as a
> > representative of the develop team.
> > If any bug, we'll hande it.
> Ok, currently the team which maintains the code do not want to change the
> order of code for sake of maintenance. Confusing approach, but I leave this
> to other members to comment.
>
> >
> > >
> > > snipped
> > > > > > +struct ice_aqc_get_phy_caps_data {
> > > > > > + __le64 phy_type_low; /* Use values from
> > ICE_PHY_TYPE_LOW_* */
> > > > > > + __le64 reserved;
> > > > > > + u8 caps;
> > > > > > +#define ICE_AQC_PHY_EN_TX_LINK_PAUSE BIT(0)
> > > > > > +#define ICE_AQC_PHY_EN_RX_LINK_PAUSE BIT(1)
> > > > > > +#define ICE_AQC_PHY_LOW_POWER_MODE BIT(2)
> > > > > > +#define ICE_AQC_PHY_EN_LINK BIT(3)
> > > > > > +#define ICE_AQC_PHY_AN_MODE BIT(4)
> > > > > > +#define ICE_AQC_PHY_EN_MOD_QUAL
> BIT(5)
> > > > > > +#define ICE_AQC_PHY_EN_LESM BIT(6)
> > > > > > +#define ICE_AQC_PHY_EN_AUTO_FEC
> BIT(7)
> > > > > > +#define ICE_AQC_PHY_CAPS_MASK
> > > > > > MAKEMASK(0xff, 0)
> > > > > > + u8 low_power_ctrl;
> > > > > > +#define ICE_AQC_PHY_EN_D3COLD_LOW_POWER_AUTONEG
> > > > > BIT(0)
> > > > > > + __le16 eee_cap;
> > > > > > +#define ICE_AQC_PHY_EEE_EN_100BASE_TX BIT(0)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_1000BASE_T BIT(1)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_10GBASE_T BIT(2)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_1000BASE_KX
> BIT(3)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_10GBASE_KR BIT(4)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_25GBASE_KR BIT(5)
> > > > > > +#define ICE_AQC_PHY_EEE_EN_40GBASE_KR4
> BIT(6)
> > > > > > + __le16 eeer_value;
> > > > > > + u8 phy_id_oui[4]; /* PHY/Module ID connected on the port
> */
> > > > > > + u8 phy_fw_ver[8];
> > > > > > + u8 link_fec_options;
> > > > > > +#define ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN BIT(0)
> > > > > > +#define ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ BIT(1)
> > > > > > +#define ICE_AQC_PHY_FEC_25G_RS_528_REQ
> BIT(2)
> > > > > > +#define ICE_AQC_PHY_FEC_25G_KR_REQ BIT(3)
> > > > > > +#define ICE_AQC_PHY_FEC_25G_RS_544_REQ
> BIT(4)
> > > > > > +#define ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN BIT(6)
> > > > > > +#define ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN BIT(7)
> > > > > > +#define ICE_AQC_PHY_FEC_MASK
> > > > > > MAKEMASK(0xdf, 0)
> > > > > > + u8 extended_compliance_code;
> > > > > > +#define ICE_MODULE_TYPE_TOTAL_BYTE 3
> > > > > > + u8 module_type[ICE_MODULE_TYPE_TOTAL_BYTE];
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE0_SFP_PLUS
> 0xA0
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE0_QSFP_PLUS 0x80
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_SFP_PLUS_CU_PASSIVE BIT(0)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_SFP_PLUS_CU_ACTIVE BIT(1)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_10G_BASE_SR BIT(4)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_10G_BASE_LR BIT(5)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_10G_BASE_LRM
> BIT(6)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE1_10G_BASE_ER BIT(7)
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE2_SFP_PLUS
> 0xA0
> > > > > > +#define ICE_AQC_MOD_TYPE_BYTE2_QSFP_PLUS 0x86
> > > > > > + u8 qualified_module_count;
> > > > > > +#define ICE_AQC_QUAL_MOD_COUNT_MAX 16
> > > > > > + struct {
> > > > > > + u8 v_oui[3];
> > > > > > + u8 rsvd3;
> > > > > > + u8 v_part[16];
> > > > > > + __le32 v_rev;
> > > > > > + __le64 rsvd8;
> > > > > > + } qual_modules[ICE_AQC_QUAL_MOD_COUNT_MAX];
> > > > > > +};
> > > > > > +
> > > > >
> > > > > Does the NIC support physical loopback? I am not able to find here.
> > > > Not sure about it. But no plan for this at this stage.
> > > Please add this in release note and PMD documentation the same.
> > No, we list all the things done. It doesn't make sense to list
> > everything not supported or not implemented.
> I think it is necessary, because application 'testpmd' has option to 'set tx
> loopback (port_id) (on|off)'. So If ICE DSI PMD does not support it and in
> testpmd it fails both DTS team and DPDK user should be made aware via
> documentation for limitation.
If the feature is not supported, the not supported failure is returned. That's
a RTE layer design and common solution for all the devices.
>
> >
> > >
> > > >
> > > > >
> > > > > > +#define ICE_AQ_PHY_ENA_LOW_POWER BIT(2)
> > > > >
> > > > > Does Low Power PMD is exposed to DPDK? If yes, can you mention
> > > > > the performance numbers or variance in Release documents?
> > > > No plan for it at this release.
> > > Would not it be better to not add here or update as comment and
> > > release note about the same. Right now it is like dead code.
> > >
> > > >
> > > > >
> > > > > Snipped
> > > > >
> > > > > > +
> > > > > > +/* Memory types */
> > > > > > +enum ice_memset_type {
> > > > > > + ICE_NONDMA_MEM = 0,
> > > > > > + ICE_DMA_MEM
> > > > > > +};
> > > > > > +
> > > > > > +/* Memcpy types */
> > > > > > +enum ice_memcpy_type {
> > > > > > + ICE_NONDMA_TO_NONDMA = 0,
> > > > > > + ICE_NONDMA_TO_DMA,
> > > > > > + ICE_DMA_TO_DMA,
> > > > > > + ICE_DMA_TO_NONDMA
> > > > > > +};
> > > > > > +
> > > > >
> > > > > Is this exposed to user (rte_eth_dev) API? If yes, can you
> > > > > please let know the performance impact in RX|TX in release notes too.
> > > > No plan for it at this release.
> > > Please update
> > > a. what is difference least as comments.
> > > b. in release notes about the same.
> > >
> > > snipped
