Received-SPF: pass (google.com: domain of linux-wireless+bounces-181-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <b7ac484a-4b60-4743-bb77-7e3a38180e7d@quicinc.com>
Date: Wed, 29 Nov 2023 09:52:13 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] wifi: ath12k: add support for BA1024
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20231128025440.46988-1-quic_bqiang@quicinc.com>
 <20231128025440.46988-3-quic_bqiang@quicinc.com>
 <803affe3-c332-457a-b0ca-92450664cb6e@quicinc.com>
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <803affe3-c332-457a-b0ca-92450664cb6e@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 11/28/2023 11:47 PM, Jeff Johnson wrote:
> On 11/27/2023 6:54 PM, Baochen Qiang wrote:
>> Currently the maximum block ACK window size supported is 256.
>> This results in that, when connected to an AP which supports
>> larger BA sizes like BA512 or BA1024, only BA256 is
>> established, leading to a lower peak throughput.
>>
>> So add support for BA1024, this is doen by allocating a larger
> 
> nit: s/doen/done/
> 
>> REO queue and advertising IEEE80211_MAX_AMPDU_BUF_EHT support
>> to MAC80211.
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/dp.h       |  2 +-
>>   drivers/net/wireless/ath/ath12k/hal_desc.h | 28 ++++++++++++++++++++++
>>   drivers/net/wireless/ath/ath12k/hal_rx.c   | 11 ++++++---
>>   drivers/net/wireless/ath/ath12k/mac.c      |  2 +-
>>   4 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
>> index 61f765432516..50db1403ebce 100644
>> --- a/drivers/net/wireless/ath/ath12k/dp.h
>> +++ b/drivers/net/wireless/ath/ath12k/dp.h
>> @@ -145,7 +145,7 @@ struct ath12k_pdev_dp {
>>   
>>   #define DP_RX_HASH_ENABLE	1 /* Enable hash based Rx steering */
>>   
>> -#define DP_BA_WIN_SZ_MAX	256
>> +#define DP_BA_WIN_SZ_MAX	1024
>>   
>>   #define DP_TCL_NUM_RING_MAX	4
>>   
>> diff --git a/drivers/net/wireless/ath/ath12k/hal_desc.h b/drivers/net/wireless/ath/ath12k/hal_desc.h
>> index ec204939e50c..f12977aa6afe 100644
>> --- a/drivers/net/wireless/ath/ath12k/hal_desc.h
>> +++ b/drivers/net/wireless/ath/ath12k/hal_desc.h
>> @@ -2517,6 +2517,34 @@ struct hal_reo_update_rx_queue {
>>   	__le32 pn[4];
>>   } __packed;
>>   
>> +struct hal_rx_reo_queue_1k {
>> +	struct hal_desc_header desc_hdr;
>> +	__le32 rx_bitmap_319_288;
> 
> are these individual bitmap members ever directly referenced? seems it
> would make more sense to have simply:
> 	__le32 rx_bitmap_1023_288[23]
> 
> this would align with struct hal_rx_reo_queue which defines:
> 	__le32 rx_bitmap[9]
> 
>> +	__le32 rx_bitmap_351_320;
>> +	__le32 rx_bitmap_383_352;
>> +	__le32 rx_bitmap_415_384;
>> +	__le32 rx_bitmap_447_416;
>> +	__le32 rx_bitmap_479_448;
>> +	__le32 rx_bitmap_511_480;
>> +	__le32 rx_bitmap_543_512;
>> +	__le32 rx_bitmap_575_544;
>> +	__le32 rx_bitmap_607_576;
>> +	__le32 rx_bitmap_639_608;
>> +	__le32 rx_bitmap_671_640;
>> +	__le32 rx_bitmap_703_672;
>> +	__le32 rx_bitmap_735_704;
>> +	__le32 rx_bitmap_767_736;
>> +	__le32 rx_bitmap_799_768;
>> +	__le32 rx_bitmap_831_800;
>> +	__le32 rx_bitmap_863_832;
>> +	__le32 rx_bitmap_895_864;
>> +	__le32 rx_bitmap_927_896;
>> +	__le32 rx_bitmap_959_928;
>> +	__le32 rx_bitmap_991_960;
>> +	__le32 rx_bitmap_1023_992;
>> +	__le32 reserved[8];
>> +} __packed;
>> +
>>   #define HAL_REO_UNBLOCK_CACHE_INFO0_UNBLK_CACHE		BIT(0)
>>   #define HAL_REO_UNBLOCK_CACHE_INFO0_RESOURCE_IDX	GENMASK(2, 1)
>>   
>> diff --git a/drivers/net/wireless/ath/ath12k/hal_rx.c b/drivers/net/wireless/ath/ath12k/hal_rx.c
>> index f6afbd8196bf..6fa874a93d3a 100644
>> --- a/drivers/net/wireless/ath/ath12k/hal_rx.c
>> +++ b/drivers/net/wireless/ath/ath12k/hal_rx.c
>> @@ -688,23 +688,28 @@ void ath12k_hal_reo_update_rx_reo_queue_status(struct ath12k_base *ab,
>>   
>>   u32 ath12k_hal_reo_qdesc_size(u32 ba_window_size, u8 tid)
>>   {
>> -	u32 num_ext_desc;
>> +	u32 num_ext_desc, num_1k_desc = 0;
>>   
>>   	if (ba_window_size <= 1) {
>>   		if (tid != HAL_DESC_REO_NON_QOS_TID)
>>   			num_ext_desc = 1;
>>   		else
>>   			num_ext_desc = 0;
>> +
>>   	} else if (ba_window_size <= 105) {
>>   		num_ext_desc = 1;
>>   	} else if (ba_window_size <= 210) {
>>   		num_ext_desc = 2;
>> -	} else {
>> +	} else if (ba_window_size <= 256) {
>>   		num_ext_desc = 3;
>> +	} else {
>> +		num_ext_desc = 10;
>> +		num_1k_desc = 1;
>>   	}
>>   
>>   	return sizeof(struct hal_rx_reo_queue) +
>> -		(num_ext_desc * sizeof(struct hal_rx_reo_queue_ext));
>> +		(num_ext_desc * sizeof(struct hal_rx_reo_queue_ext)) +
>> +		(num_1k_desc * sizeof(struct hal_rx_reo_queue_1k));
>>   }
>>   
>>   void ath12k_hal_reo_qdesc_setup(struct hal_rx_reo_queue *qdesc,
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index fc0d14ea328e..3cfb17f71aa6 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -7474,7 +7474,7 @@ static int __ath12k_mac_register(struct ath12k *ar)
>>   	ar->hw->queues = ATH12K_HW_MAX_QUEUES;
>>   	ar->hw->wiphy->tx_queue_len = ATH12K_QUEUE_LEN;
>>   	ar->hw->offchannel_tx_hw_queue = ATH12K_HW_MAX_QUEUES - 1;
>> -	ar->hw->max_rx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF_HE;
>> +	ar->hw->max_rx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF_EHT;
>>   
>>   	ar->hw->vif_data_size = sizeof(struct ath12k_vif);
>>   	ar->hw->sta_data_size = sizeof(struct ath12k_sta);
> 
> Are any related changes needed to struct hal_reo_get_queue_stats_status?
> Or to ath12k_hal_reo_status_queue_stats()?
> There I see the 256-bit bitmap being dumped -- do you need to dump the
> 1024-bit bitmap? Is there a mechanism which allows that?
I don't think it's possible. As you can see, struct 
hal_reo_get_queue_stats_status is directly extracted from REO_STATUS 
ring, which is queued by firmware/HW. That is to say firmware/HW doesn't 
upload other bitmap info, except for the first 256 bits, to host, so 
host has no way to dump them.
> 
>