Re: [PATCH v1 02/16] net-next/yunsilicon: Enable CMDQ
flat view
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: 2024-12-18 14:47:10
On 12/18/24 11:50, Xin Tian wrote:
Enable cmd queue to support driver-firmware communication. Hardware control will be performed through cmdq mostly. Co-developed-by: Honggang Wei <redacted> Co-developed-by: Lei Yan <redacted> Signed-off-by: Xin Tian <redacted>
quoted hunk ↗ jump to hunk
+ +#ifndef XSC_CMD_H +#define XSC_CMD_H + +#define CMDQ_VERSION 0x32 + +#define MAX_MBOX_OUT_LEN 2048 + +#define QOS_PRIO_MAX 7 +#define QOS_DSCP_MAX 63
weird formatting
quoted hunk ↗ jump to hunk
+#define MAC_PORT_DSCP_SHIFT 6 +#define QOS_PCP_MAX 7 +#define DSCP_PCP_UNSET 255 +#define MAC_PORT_PCP_SHIFT 3 +#define XSC_MAX_MAC_NUM 8 +#define XSC_BOARD_SN_LEN 32 +#define MAX_PKT_LEN 9800
you don't want to collide with future defines in core kernel
quoted hunk ↗ jump to hunk
+#define XSC_RTT_CFG_QPN_MAX 32 + +#define XSC_PCIE_LAT_CFG_INTERVAL_MAX 8 +#define XSC_PCIE_LAT_CFG_HISTOGRAM_MAX 9 +#define XSC_PCIE_LAT_EN_DISABLE 0 +#define XSC_PCIE_LAT_EN_ENABLE 1 +#define XSC_PCIE_LAT_PERIOD_MIN 1 +#define XSC_PCIE_LAT_PERIOD_MAX 20 +#define DPU_PORT_WGHT_CFG_MAX 1 + +enum { + XSC_CMD_STAT_OK = 0x0, + XSC_CMD_STAT_INT_ERR = 0x1, + XSC_CMD_STAT_BAD_OP_ERR = 0x2, + XSC_CMD_STAT_BAD_PARAM_ERR = 0x3, + XSC_CMD_STAT_BAD_SYS_STATE_ERR = 0x4, + XSC_CMD_STAT_BAD_RES_ERR = 0x5, + XSC_CMD_STAT_RES_BUSY = 0x6, + XSC_CMD_STAT_LIM_ERR = 0x8, + XSC_CMD_STAT_BAD_RES_STATE_ERR = 0x9, + XSC_CMD_STAT_IX_ERR = 0xa, + XSC_CMD_STAT_NO_RES_ERR = 0xf, + XSC_CMD_STAT_BAD_INP_LEN_ERR = 0x50, + XSC_CMD_STAT_BAD_OUTP_LEN_ERR = 0x51, + XSC_CMD_STAT_BAD_QP_STATE_ERR = 0x10,
I would keep the list numerically sorted
quoted hunk ↗ jump to hunk
+ XSC_CMD_STAT_BAD_PKT_ERR = 0x30, + XSC_CMD_STAT_BAD_SIZE_OUTS_CQES_ERR = 0x40, +}; + +enum { + DPU_PORT_WGHT_TARGET_HOST, + DPU_PORT_WGHT_TARGET_SOC, + DPU_PORT_WGHT_TARGET_NUM, +}; + +enum { + DPU_PRIO_WGHT_TARGET_HOST2SOC, + DPU_PRIO_WGHT_TARGET_SOC2HOST, + DPU_PRIO_WGHT_TARGET_HOSTSOC2LAG, + DPU_PRIO_WGHT_TARGET_NUM, +}; + +#define XSC_AP_FEAT_UDP_SPORT_MIN 1024 +#define XSC_AP_FEAT_UDP_SPORT_MAX 65535
there is really nothing to reuse from code networking?
quoted hunk ↗ jump to hunk
+ +enum { + XSC_CMD_OP_QUERY_HCA_CAP = 0x100, + XSC_CMD_OP_QUERY_ADAPTER = 0x101, + XSC_CMD_OP_INIT_HCA = 0x102, + XSC_CMD_OP_TEARDOWN_HCA = 0x103, + XSC_CMD_OP_ENABLE_HCA = 0x104, + XSC_CMD_OP_DISABLE_HCA = 0x105, + XSC_CMD_OP_MODIFY_HCA = 0x106, + XSC_CMD_OP_QUERY_PAGES = 0x107, + XSC_CMD_OP_MANAGE_PAGES = 0x108, + XSC_CMD_OP_SET_HCA_CAP = 0x109, + XSC_CMD_OP_QUERY_CMDQ_VERSION = 0x10a, + XSC_CMD_OP_QUERY_MSIX_TBL_INFO = 0x10b, + XSC_CMD_OP_FUNCTION_RESET = 0x10c, + XSC_CMD_OP_DUMMY = 0x10d,
I didn't checked, but please limit the scope of added defines to cover only what this series uses (IOW: no dead code) [...]
quoted hunk ↗ jump to hunk
+ +enum xsc_eth_qp_num_sel { + XSC_ETH_QP_NUM_8K_SEL = 0, + XSC_ETH_QP_NUM_8K_8TC_SEL, + XSC_ETH_QP_NUM_SEL_MAX,
no comma after items that you don't expect addition after
quoted hunk ↗ jump to hunk
+}; + +enum xsc_eth_vf_num_sel { + XSC_ETH_VF_NUM_SEL_8 = 0, + XSC_ETH_VF_NUM_SEL_16, + XSC_ETH_VF_NUM_SEL_32, + XSC_ETH_VF_NUM_SEL_64, + XSC_ETH_VF_NUM_SEL_128, + XSC_ETH_VF_NUM_SEL_256, + XSC_ETH_VF_NUM_SEL_512, + XSC_ETH_VF_NUM_SEL_1024, + XSC_ETH_VF_NUM_SEL_MAX +}; + +enum { + LINKSPEED_MODE_UNKNOWN = -1, + LINKSPEED_MODE_10G = 10000, + LINKSPEED_MODE_25G = 25000, + LINKSPEED_MODE_40G = 40000, + LINKSPEED_MODE_50G = 50000, + LINKSPEED_MODE_100G = 100000, + LINKSPEED_MODE_200G = 200000, + LINKSPEED_MODE_400G = 400000,
reminder about prefixing your enums by XSC_ would be also good to add your max supported speed into cover letter (to get some more atteniton :))
quoted hunk ↗ jump to hunk
+}; + +enum { + MODULE_SPEED_UNKNOWN, + MODULE_SPEED_10G, + MODULE_SPEED_25G, + MODULE_SPEED_40G_R4, + MODULE_SPEED_50G_R, + MODULE_SPEED_50G_R2, + MODULE_SPEED_100G_R2, + MODULE_SPEED_100G_R4, + MODULE_SPEED_200G_R4, + MODULE_SPEED_200G_R8, + MODULE_SPEED_400G_R8, +}; + +enum xsc_dma_direct { + DMA_DIR_TO_MAC, + DMA_DIR_READ, + DMA_DIR_WRITE, + DMA_DIR_LOOPBACK, + DMA_DIR_MAX, +}; + +/* hw feature bitmap, 32bit */ +enum xsc_hw_feature_flag { + XSC_HW_RDMA_SUPPORT = 0x1,
= BIT(0)
+ XSC_HW_PFC_PRIO_STATISTIC_SUPPORT = 0x2,
= BIT(1), etc
quoted hunk ↗ jump to hunk
+ XSC_HW_THIRD_FEATURE = 0x4, + XSC_HW_PFC_STALL_STATS_SUPPORT = 0x8, + XSC_HW_RDMA_CM_SUPPORT = 0x20, + + XSC_HW_LAST_FEATURE = 0x80000000, +}; + +enum xsc_lldp_dcbx_sub_cmd { + XSC_OS_HANDLE_LLDP_STATUS = 0x1, + XSC_DCBX_STATUS +}; + +struct xsc_inbox_hdr { + __be16 opcode; + u8 rsvd[4]; + __be16 ver;
this is command version? (vs API as a whole, or HW ver, etc)
quoted hunk ↗ jump to hunk
+}; + +struct xsc_outbox_hdr { + u8 status; + u8 rsvd[5]; + __be16 ver; +}; + +struct xsc_alloc_ia_lock_mbox_in { + struct xsc_inbox_hdr hdr; + u8 lock_num; + u8 rsvd[7]; +}; + +#define XSC_RES_NUM_IAE_GRP 16 + +struct xsc_alloc_ia_lock_mbox_out { + struct xsc_outbox_hdr hdr; + u8 lock_idx[XSC_RES_NUM_IAE_GRP]; +}; + +struct xsc_release_ia_lock_mbox_in { + struct xsc_inbox_hdr hdr; + u8 lock_idx[XSC_RES_NUM_IAE_GRP]; +}; + +struct xsc_release_ia_lock_mbox_out { + struct xsc_outbox_hdr hdr; + u8 rsvd[8]; +}; + +struct xsc_pci_driver_init_params_in { + struct xsc_inbox_hdr hdr; + __be32 s_wqe_mode; + __be32 r_wqe_mode; + __be32 local_timeout_retrans; + u8 mac_lossless_prio[XSC_MAX_MAC_NUM];
please look up your formatting (through the series)
quoted hunk ↗ jump to hunk
+ __be32 group_mod; +}; + +struct xsc_pci_driver_init_params_out { + struct xsc_outbox_hdr hdr; + u8 rsvd[8]; +}; + +/*CQ mbox*/ +struct xsc_cq_context { + __be16 eqn; + __be16 pa_num; + __be16 glb_func_id; + u8 log_cq_sz; + u8 cq_type; +}; + +struct xsc_create_cq_mbox_in { + struct xsc_inbox_hdr hdr; + struct xsc_cq_context ctx; + __be64 pas[];
would be great to explain your shortcuts: eqn, pas, and all other non obvious ones [...]
quoted hunk ↗ jump to hunk
+/*PD mbox*/ +struct xsc_alloc_pd_request { + u8 rsvd[8]; +};
really? what this struct is for?
quoted hunk ↗ jump to hunk
+ +struct xsc_alloc_pd_mbox_in { + struct xsc_inbox_hdr hdr; + struct xsc_alloc_pd_request req;
this is the only use of the above [...]
quoted hunk ↗ jump to hunk
+/* vport mbox */ +struct xsc_nic_vport_context { + __be32 min_wqe_inline_mode:3; + __be32 disable_mc_local_lb:1; + __be32 disable_uc_local_lb:1; + __be32 roce_en:1; + + __be32 arm_change_event:1; + __be32 event_on_mtu:1; + __be32 event_on_promisc_change:1; + __be32 event_on_vlan_change:1; + __be32 event_on_mc_address_change:1; + __be32 event_on_uc_address_change:1; + __be32 affiliation_criteria:4;
I guess you will have hard time reading those bitfields out look at FIELD_GET() and similar
quoted hunk ↗ jump to hunk
+ __be32 affiliated_vhca_id; + + __be16 mtu; + + __be64 system_image_guid; + __be64 port_guid; + __be64 node_guid; + + __be32 qkey_violation_counter; + + __be16 spoofchk:1; + __be16 trust:1; + __be16 promisc:1; + __be16 allmcast:1; + __be16 vlan_allowed:1; + __be16 allowed_list_type:3; + __be16 allowed_list_size:10; + + __be16 vlan_proto; + __be16 vlan; + u8 qos; + u8 permanent_address[6]; + u8 current_address[6]; + u8 current_uc_mac_address[0][2]; +}; + +enum { + XSC_HCA_VPORT_SEL_PORT_GUID = 1 << 0, + XSC_HCA_VPORT_SEL_NODE_GUID = 1 << 1, + XSC_HCA_VPORT_SEL_STATE_POLICY = 1 << 2,
BIT(0), BIT(1), BIT(2) [...]
quoted hunk ↗ jump to hunk
+ +struct xsc_array128 { + u8 array128[16]; +};
both struct name and member name are wrong, this is an u128 type basically
quoted hunk ↗ jump to hunk
+ +struct xsc_query_hca_vport_gid_out { + struct xsc_outbox_hdr hdr; + u16 gids_num; + struct xsc_array128 gid[];
__counted_by(gids_num)
quoted hunk ↗ jump to hunk
+}; + +struct xsc_query_hca_vport_gid_in { + struct xsc_inbox_hdr hdr; + u32 other_vport:1;
most other things you have in Big Endian, and now CPU endiannes, it's intentional?
quoted hunk ↗ jump to hunk
+ u32 port_num:4; + u32 vport_number:16; + u32 rsvd0:11; + u16 gid_index; +}; + +struct xsc_pkey { + u16 pkey; +}; + +struct xsc_query_hca_vport_pkey_out { + struct xsc_outbox_hdr hdr; + struct xsc_pkey pkey[]; +}; + +struct xsc_query_hca_vport_pkey_in { + struct xsc_inbox_hdr hdr; + u32 other_vport:1; + u32 port_num:4; + u32 vport_number:16; + u32 rsvd0:11; + u16 pkey_index; +}; + +struct xsc_query_vport_state_out { + struct xsc_outbox_hdr hdr; + u8 admin_state:4; + u8 state:4; +}; + +struct xsc_query_vport_state_in { + struct xsc_inbox_hdr hdr; + u32 other_vport:1; + u32 vport_number:16; + u32 rsvd0:15; +}; + +struct xsc_modify_vport_state_out { + struct xsc_outbox_hdr hdr; +}; + +struct xsc_modify_vport_state_in { + struct xsc_inbox_hdr hdr; + u32 other_vport:1; + u32 vport_number:16; + u32 rsvd0:15; + u8 admin_state:4; + u8 rsvd1:4; +}; + +struct xsc_traffic_counter { + u64 packets; + u64 bytes; +}; + +struct xsc_query_vport_counter_out { + struct xsc_outbox_hdr hdr; + struct xsc_traffic_counter received_errors; + struct xsc_traffic_counter transmit_errors; + struct xsc_traffic_counter received_ib_unicast; + struct xsc_traffic_counter transmitted_ib_unicast; + struct xsc_traffic_counter received_ib_multicast; + struct xsc_traffic_counter transmitted_ib_multicast; + struct xsc_traffic_counter received_eth_broadcast; + struct xsc_traffic_counter transmitted_eth_broadcast; + struct xsc_traffic_counter received_eth_unicast; + struct xsc_traffic_counter transmitted_eth_unicast; + struct xsc_traffic_counter received_eth_multicast; + struct xsc_traffic_counter transmitted_eth_multicast; +}; +
not related to "getting counters from the vport", but please make sure to be familiar with struct rtnl_link_stats64 for actual reporting needs
+#define ETH_ALEN 6
please remove [...]
quoted hunk ↗ jump to hunk
+ +struct xsc_event_linkstatus_resp { + u8 linkstatus; /*0:down, 1:up*/ +}; + +struct xsc_event_linkinfo { + u8 linkstatus; /*0:down, 1:up*/ + u8 port; + u8 duplex; + u8 autoneg; + u32 linkspeed; + u64 supported; + u64 advertising; + u64 supported_fec; /* reserved, not support currently */ + u64 advertised_fec; /* reserved, not support currently */ + u64 supported_speed[2]; + u64 advertising_speed[2]; +};
heads up: please make our link speed, pause, fec code commits clearly marked, so reviewers interested in that topic would recognize and in general: please elaborate a bit in the commit message what it does in terms of standard networking/kernel (not limiting to your 3-4 letter acronyms used for name of channel between your driver and HW) this patch is so long that I will end here [...]
+
+struct hwc_set_t {add prefix xsc_ in the name don't name structs _t, as that would be confused for typedefed one [...]
quoted hunk ↗ jump to hunk
+ + u8 dword_0[0x20]; + u8 dword_1[0x20]; + u8 dword_2[0x20]; + u8 dword_3[0x20]; + u8 dword_4[0x20]; + u8 dword_5[0x20]; + u8 dword_6[0x20]; + u8 dword_7[0x20]; + u8 dword_8[0x20]; + u8 dword_9[0x20]; + u8 dword_10[0x20]; + u8 dword_11[0x20];
sizeof(dword) is neither sizeof(u8) nor 0x20 (bad name)
quoted hunk ↗ jump to hunk
+++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_cmdq.h@@ -0,0 +1,218 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd. + * All rights reserved. + */ + +#ifndef XSC_CMDQ_H +#define XSC_CMDQ_H + +#include "common/xsc_cmd.h" + +enum { + /* one minute for the sake of bringup. Generally, commands must always
outdated comment
quoted hunk ↗ jump to hunk
+ * complete and we may need to increase this timeout value + */ + XSC_CMD_TIMEOUT_MSEC = 10 * 1000, + XSC_CMD_WQ_MAX_NAME = 32,
take a look at the abundant kernel provided work queues, not need to spawn your own most of the time