[PATCH 0/2] wifi: ath12k: refactor the link capable flag

[PATCH 0/2] wifi: ath12k: refactor the link capable flag

[Karthikeyan Periyasamy]

extend the link capable flag to accommodate inter-chip capability.

Karthikeyan Periyasamy (2):
  wifi: ath12k: extend the link capable flag
  wifi: ath12k: fix link capable flags

 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  |  4 +++-
 4 files changed, 25 insertions(+), 6 deletions(-)


base-commit: fe7e1b830cf3c0272aa4eaf367c4c7b29c169c3d
-- 
2.34.1

[PATCH 1/2] wifi: ath12k: extend the link capable flag

[Karthikeyan Periyasamy]

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(-)

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,
-- 
2.34.1

Re: [PATCH 1/2] wifi: ath12k: extend the link capable flag

[Jeff Johnson]

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?

Re: [PATCH 1/2] wifi: ath12k: extend the link capable flag

[Karthikeyan Periyasamy]

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];

...

}

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 1/2] wifi: ath12k: extend the link capable flag

[Jeff Johnson]

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

Re: [PATCH 1/2] wifi: ath12k: extend the link capable flag

[Karthikeyan Periyasamy]

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.


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 1/2] wifi: ath12k: extend the link capable flag

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/2] wifi: ath12k: fix link capable flags

[Karthikeyan Periyasamy]

By default driver supports intra-chip SLO/MLO, the link capability flags
reflect this. When the QMI PHY capability learning fails driver not enable
the MLO parameter in the host capability QMI request message. In this case,
reset the chip link capability flags to zero (SLO/MLO not support) to
accurately represent the capabilities.

Found in code review.

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/qmi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
index 3f0d2b99127a..db8ba5fec2ae 100644
--- a/drivers/net/wireless/ath/ath12k/qmi.c
+++ b/drivers/net/wireless/ath/ath12k/qmi.c
@@ -2006,6 +2006,8 @@ static void ath12k_host_cap_parse_mlo(struct ath12k_base *ab,
 	int i;
 
 	if (!ab->qmi.num_radios || ab->qmi.num_radios == U8_MAX) {
+		ab->mlo_capable_flags = 0;
+
 		ath12k_dbg(ab, ATH12K_DBG_QMI,
 			   "skip QMI MLO cap due to invalid num_radio %d\n",
 			   ab->qmi.num_radios);
-- 
2.34.1