[dpdk-dev] [PATCH v2 01/20] net/ice: add base code

[Varghese, Vipin]

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?

> 
> >
> > > >
> > 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.

> 
> >
> > >
> > > >
> > > > > +#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