Re: [dpdk-dev] [PATCH v1 3/5] net/fm10k: add ffu and statistics and config file functions

["Wang, Xiao W" <xiao.w.wang@intel.com>]

Hi,

Comments inline.

Best Regards,
Xiao

> -----Original Message-----
> From: Xiaojun Liu <xiaojun.liu@silicom.co.il>
> Sent: Friday, February 28, 2020 4:38 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Kwan, Ngai-mint <ngai-mint.kwan@intel.com>; Keller,
> Jacob E <jacob.e.keller@intel.com>
> Cc: dev@dpdk.org; Xiaojun Liu <xiaojun.liu@silicom.co.il>
> Subject: [PATCH v1 3/5] net/fm10k: add ffu and statistics and config file
> functions
> 
> Add ffu to support offload flow into HW.
> It supports forward, mirror, push VLAN, pop VLAN.
> It also supports flowset for a group flow definition.
> The config file can configure debug log, port speed,
> epl port mapping dpdk port, flowset. All these configuration
> will be used by switch management.
> Statistics includes epl port, ffu rule, dpdk port, and error.
> All these statistics data are read from HW.
> Modify switch header file to support getting logical port
> and glort and device info.
> 

If new features are added, please doc them in doc/guides/nics/features/fm10k.ini

> To enable the switch management, you need add
> CONFIG_RTE_FM10K_MANAGEMENT=y in
> config/common_linux when building.

[...]

> +
> +	/* GLORT_DEST_TABLE
> +	 * Field Name		Bit(s)	Type	Default
> +	 * DestMask			47:0	RW		0x0
> +	 * IP_MulticastIndex59:48	RW		0x0
> +	 * Reserved			63:60	RSV		0x0
> +	 */

Please fix the alignment issue around here.

> +	temp64 = sw->mcast_dest_table_idx;
> +	temp64 = temp64 << 48 | 1 << lport1 | 1 << lport2;
> +	fm10k_write_switch_reg64(sw,
> +			FM10K_SW_GLORT_DEST_TABLE
> +			(sw->glort_dest_table_idx), temp64);
> +	sw->glort_dest_table_idx++;
> +
> +	/* MCAST_DEST_TABLE
> +	 * Field Name		Bit(s)	Type	Default
> +	 * PortMask			47:0	RW		0x0
> +	 * LenTableIdx		61:48	RW		0x0
> +	 * Reserved			63:62	RSV		00b
> +	 */
> +	temp64 = sw->mcast_len_table_idx;
> +	temp64 = 1 << lport1 | 1 << lport2 | temp64 << 48;
> +	fm10k_write_switch_reg64(sw,
> +			FM10K_SW_SCHED_MCAST_DEST_TABLE
> +			(sw->mcast_dest_table_idx++), temp64);
> +
> +	/* MCAST_LEN_TABLE
> +	 * Field Name		Bit(s)	Type	Default
> +	 * L3_McastIdx		14:0	RW		0x0
> +	 * L3_Repcnt		26:15	RW		0x0
> +	 * Reserved			31:27	RSV		0x0
> +	 */
> +	temp64 =
> +		sw->mcast_vlan_table_idx | 1 << 15;
> +	fm10k_write_switch_reg64(sw,
> +			FM10K_SW_SCHED_MCAST_LEN_TABLE
> +			(sw->mcast_len_table_idx++), temp64);
> +
> +	/* MCAST_VLAN_TABLE
> +	 * Field Name		Bit(s)	Type	Default
> +	 * VID				11:0	RW		0x0
> +	 * DGLORT			27:12	RW		0x0
> +	 * ReplaceVID		28		RW		0b
> +	 * ReplaceDGLORT	29		RW		0b
> +	 * Reserved			31:30	RSV		00b
> +	 */
> +	temp64 = vlan1 |
> +			fm10k_switch_pf_glort_get
> +			(lport1) << 12 | 1 << 28 | 1 << 29;
> +	fm10k_write_switch_reg64(sw,
> +			FM10K_SW_MOD_MCAST_VLAN_TABLE
> +			(sw->mcast_vlan_table_idx++), temp64);
> +	temp64 = vlan2 |
> +			fm10k_switch_pf_glort_get
> +			(lport2) << 12 | 1 << 28 | 1 << 29;
> +	fm10k_write_switch_reg64(sw,
> +			FM10K_SW_MOD_MCAST_VLAN_TABLE
> +			(sw->mcast_vlan_table_idx++), temp64);
> +
> +	return dglort;

[...]

> +	for (i = 0;
> +			i < sizeof(rx_port_cnt_map_table) /
> +				sizeof(rx_port_cnt_map_table[0]);
> +			i++) {

The style looks strange. No need to switch to a new line as " for (i = 0;" is so short.
[...]