| Message ID | 20240329012358.2242354-2-quic_periyasa@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: refactor the link capable flag | expand |
On 3/28/2024 6:23 PM, Karthikeyan Periyasamy wrote: > Link capability categorized as Single Link Operation (SLO) and Multi Link > Operation (MLO). > > * Intra-chip SLO/MLO refers to links present within a chip > * Inter-chip SLO/MLO refers to links present across multiple chips Is "chip" the correct term? I'm thinking that this should be called "device" since that is the unit of hardware that is detected by a bus probe function and which is handled by a *device* driver. Doesn't this make more sense if the references to chip and SoC are changed to device?
On 4/1/2024 10:24 PM, Jeff Johnson wrote: > On 3/28/2024 6:23 PM, Karthikeyan Periyasamy wrote: >> Link capability categorized as Single Link Operation (SLO) and Multi Link >> Operation (MLO). >> >> * Intra-chip SLO/MLO refers to links present within a chip >> * Inter-chip SLO/MLO refers to links present across multiple chips > > Is "chip" the correct term? > > I'm thinking that this should be called "device" since that is the unit of > hardware that is detected by a bus probe function and which is handled by a > *device* driver. > > Doesn't this make more sense if the references to chip and SoC are changed to > device? > In the QMI, SLO/MLO parameter exposed as chip only not device. So followed the same terminology to avoid confusion for code readability. struct wlfw_host_mlo_chip_info_s_v01 { u8 chip_id; u8 num_local_links; u8 hw_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; u8 valid_mlo_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; }; struct qmi_wlanfw_host_cap_req_msg_v01 { ... u8 mlo_num_chips; u8 mlo_chip_info_valid; struct wlfw_host_mlo_chip_info_s_v01 mlo_chip_info[QMI_WLFW_MAX_NUM_MLO_CHIPS_V01]; ... }
On 4/2/2024 2:32 AM, Karthikeyan Periyasamy wrote: > > > On 4/1/2024 10:24 PM, Jeff Johnson wrote: >> On 3/28/2024 6:23 PM, Karthikeyan Periyasamy wrote: >>> Link capability categorized as Single Link Operation (SLO) and Multi Link >>> Operation (MLO). >>> >>> * Intra-chip SLO/MLO refers to links present within a chip >>> * Inter-chip SLO/MLO refers to links present across multiple chips >> >> Is "chip" the correct term? >> >> I'm thinking that this should be called "device" since that is the unit of >> hardware that is detected by a bus probe function and which is handled by a >> *device* driver. >> >> Doesn't this make more sense if the references to chip and SoC are changed to >> device? >> > > In the QMI, SLO/MLO parameter exposed as chip only not device. So > followed the same terminology to avoid confusion for code readability. > > struct wlfw_host_mlo_chip_info_s_v01 { > u8 chip_id; > u8 num_local_links; > u8 hw_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; > u8 valid_mlo_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; > }; > > struct qmi_wlanfw_host_cap_req_msg_v01 { > > ... > > u8 mlo_num_chips; > > u8 mlo_chip_info_valid; > > struct wlfw_host_mlo_chip_info_s_v01 > mlo_chip_info[QMI_WLFW_MAX_NUM_MLO_CHIPS_V01]; > > ... > > } > Please don't let firmware interface naming drive host driver code naming. And push back on the firmware team when they've introduced poor naming. As many Software Engineering experts stress, naming is probably the single most important thing we do. So we need to make sure we are using the correct names for all of the software objects that comprise the driver, especially with this multi-device MLO feature where we now have to represent a multitude of individual devices as a single logical wiphy. Lack of a single common term for each object in the architecture makes the code far less maintainable. /jeff
On 4/2/2024 11:11 PM, Jeff Johnson wrote: > On 4/2/2024 2:32 AM, Karthikeyan Periyasamy wrote: >> >> >> On 4/1/2024 10:24 PM, Jeff Johnson wrote: >>> On 3/28/2024 6:23 PM, Karthikeyan Periyasamy wrote: >>>> Link capability categorized as Single Link Operation (SLO) and Multi Link >>>> Operation (MLO). >>>> >>>> * Intra-chip SLO/MLO refers to links present within a chip >>>> * Inter-chip SLO/MLO refers to links present across multiple chips >>> >>> Is "chip" the correct term? >>> >>> I'm thinking that this should be called "device" since that is the unit of >>> hardware that is detected by a bus probe function and which is handled by a >>> *device* driver. >>> >>> Doesn't this make more sense if the references to chip and SoC are changed to >>> device? >>> >> >> In the QMI, SLO/MLO parameter exposed as chip only not device. So >> followed the same terminology to avoid confusion for code readability. >> >> struct wlfw_host_mlo_chip_info_s_v01 { >> u8 chip_id; >> u8 num_local_links; >> u8 hw_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; >> u8 valid_mlo_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; >> }; >> >> struct qmi_wlanfw_host_cap_req_msg_v01 { >> >> ... >> >> u8 mlo_num_chips; >> >> u8 mlo_chip_info_valid; >> >> struct wlfw_host_mlo_chip_info_s_v01 >> mlo_chip_info[QMI_WLFW_MAX_NUM_MLO_CHIPS_V01]; >> >> ... >> >> } >> > > Please don't let firmware interface naming drive host driver code naming. > And push back on the firmware team when they've introduced poor naming. > > As many Software Engineering experts stress, naming is probably the single > most important thing we do. So we need to make sure we are using the correct > names for all of the software objects that comprise the driver, especially > with this multi-device MLO feature where we now have to represent a multitude > of individual devices as a single logical wiphy. > > Lack of a single common term for each object in the architecture makes the > code far less maintainable. > Sure, will address this comment in the next version of the patch.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 4/2/2024 2:32 AM, Karthikeyan Periyasamy wrote: > >> >> >> On 4/1/2024 10:24 PM, Jeff Johnson wrote: >>> On 3/28/2024 6:23 PM, Karthikeyan Periyasamy wrote: >>>> Link capability categorized as Single Link Operation (SLO) and Multi Link >>>> Operation (MLO). >>>> >>>> * Intra-chip SLO/MLO refers to links present within a chip >>>> * Inter-chip SLO/MLO refers to links present across multiple chips >>> >>> Is "chip" the correct term? >>> >>> I'm thinking that this should be called "device" since that is the unit of >>> hardware that is detected by a bus probe function and which is handled by a >>> *device* driver. >>> >>> Doesn't this make more sense if the references to chip and SoC are changed to >>> device? >>> >> >> In the QMI, SLO/MLO parameter exposed as chip only not device. So >> followed the same terminology to avoid confusion for code readability. >> >> struct wlfw_host_mlo_chip_info_s_v01 { >> u8 chip_id; >> u8 num_local_links; >> u8 hw_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; >> u8 valid_mlo_link_id[QMI_WLFW_MAX_NUM_MLO_LINKS_PER_CHIP_V01]; >> }; >> >> struct qmi_wlanfw_host_cap_req_msg_v01 { >> >> ... >> >> u8 mlo_num_chips; >> >> u8 mlo_chip_info_valid; >> >> struct wlfw_host_mlo_chip_info_s_v01 >> mlo_chip_info[QMI_WLFW_MAX_NUM_MLO_CHIPS_V01]; >> >> ... >> >> } >> > > Please don't let firmware interface naming drive host driver code naming. > And push back on the firmware team when they've introduced poor naming. > > As many Software Engineering experts stress, naming is probably the single > most important thing we do. So we need to make sure we are using the correct > names for all of the software objects that comprise the driver, especially > with this multi-device MLO feature where we now have to represent a multitude > of individual devices as a single logical wiphy. > > Lack of a single common term for each object in the architecture makes the > code far less maintainable. Amen to that. I think we should come up with a terminology list or something, otherwise it's hard to keep up with all teams having their own terminology.
diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c index 391b6fb2bd42..231bdcd4b415 100644 --- a/drivers/net/wireless/ath/ath12k/core.c +++ b/drivers/net/wireless/ath/ath12k/core.c @@ -1227,7 +1227,7 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size, ab->dev = dev; ab->hif.bus = bus; ab->qmi.num_radios = U8_MAX; - ab->slo_capable = true; + ab->mlo_capable_flags = ATH12k_INTRA_CHIP_MLO_SUPPORT; return ab; diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h index 97e5a0ccd233..ef58f6b07b79 100644 --- a/drivers/net/wireless/ath/ath12k/core.h +++ b/drivers/net/wireless/ath/ath12k/core.h @@ -688,6 +688,21 @@ struct ath12k_soc_dp_stats { struct ath12k_soc_dp_tx_err_stats tx_err; }; +/** + * enum ath12k_link_capable_flags - link capable flags + * + * Single/Multi link capability information + * + * @ATH12k_INTRA_CHIP_MLO_SUPPORT: SLO/MLO form between the radio, where all + * the radio present within a chip. + * @ATH12k_INTER_CHIP_MLO_SUPPORT: SLO_MLO form between the radio, where all + * the radio present across the chips + */ +enum ath12k_link_capable_flags { + ATH12k_INTRA_CHIP_MLO_SUPPORT = BIT(0), + ATH12k_INTER_CHIP_MLO_SUPPORT = BIT(1), +}; + /* Master structure to hold the hw data which may be used in core module */ struct ath12k_base { enum ath12k_hw_rev hw_rev; @@ -843,10 +858,12 @@ struct ath12k_base { const struct hal_rx_ops *hal_rx_ops; - /* slo_capable denotes if the single/multi link operation - * is supported within the same chip (SoC). + /* mlo_capable_flags denotes the single/multi link operation + * capabilities of the chip (SoC). + * + * See enum ath12k_link_capable_flags */ - bool slo_capable; + u8 mlo_capable_flags; /* must be last */ u8 drv_priv[] __aligned(sizeof(void *)); diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c index adb8c3ec1950..fd519c87ae24 100644 --- a/drivers/net/wireless/ath/ath12k/mhi.c +++ b/drivers/net/wireless/ath/ath12k/mhi.c @@ -385,7 +385,7 @@ int ath12k_mhi_register(struct ath12k_pci *ab_pci) "failed to read board id\n"); } else if (board_id & OTP_VALID_DUALMAC_BOARD_ID_MASK) { dualmac = true; - ab->slo_capable = false; + ab->mlo_capable_flags = 0; ath12k_dbg(ab, ATH12K_DBG_BOOT, "dualmac fw selected for board id: %x\n", board_id); } diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c index 92845ffff44a..3f0d2b99127a 100644 --- a/drivers/net/wireless/ath/ath12k/qmi.c +++ b/drivers/net/wireless/ath/ath12k/qmi.c @@ -2124,7 +2124,7 @@ static void ath12k_qmi_phy_cap_send(struct ath12k_base *ab) struct qmi_txn txn; int ret; - if (!ab->slo_capable) + if (!ab->mlo_capable_flags) goto out; ret = qmi_txn_init(&ab->qmi.handle, &txn,
Link capability categorized as Single Link Operation (SLO) and Multi Link Operation (MLO). * Intra-chip SLO/MLO refers to links present within a chip * Inter-chip SLO/MLO refers to links present across multiple chips Currently, driver uses a boolean variable to represent intra-chip SLO/MLO capability. To accommodate inter-chip SLO/MLO capabilities within the same variable, modify the existing variable name and type. Define a new enumeration for the link capabilities to accommodate both intra-chip and inter-chip scenarios. Populate the enum based on the supported capabilities. Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> --- drivers/net/wireless/ath/ath12k/core.c | 2 +- drivers/net/wireless/ath/ath12k/core.h | 23 ++++++++++++++++++++--- drivers/net/wireless/ath/ath12k/mhi.c | 2 +- drivers/net/wireless/ath/ath12k/qmi.c | 2 +- 4 files changed, 23 insertions(+), 6 deletions(-)