Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <b0ac4a51-9a24-443a-8155-6100d5e6b00f@quicinc.com>
Date:   Mon, 30 Oct 2023 17:53:22 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/13] wifi: ath12k: subscribe required word mask from rx
 tlv
Content-Language: en-US
To:     Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>,
        <ath12k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>,
        Karthikeyan Kathirvel <quic_kathirve@quicinc.com>
References: <20231030222700.18914-1-quic_rajkbhag@quicinc.com>
 <20231030222700.18914-11-quic_rajkbhag@quicinc.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20231030222700.18914-11-quic_rajkbhag@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/30/2023 3:26 PM, Raj Kumar Bhagat wrote:
> From: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>
> 
> Most of the RX descriptors fields are currently not used in the
> ath12k driver. Hence add support to selectively subscribe to the
> required quad words (64 bits) within msdu_end and mpdu_start of
> rx_desc.
> 
> Add compact rx_desc structures and configure the bit mask for Rx TLVs
> (msdu_end, mpdu_start, mpdu_end) via registers. With these registers
> SW can configure to DMA the partial TLV struct to Rx buffer.
> 
> Each TLV type has its own register to configure the mask value.
> The mask value configured in register will indicate if a particular
> QWORD has to be written to rx buffer or not i.e., if Nth bit is enabled
> in the mask Nth QWORD will be written and it will not be written if the
> bit is disabled in mask. While 0th bit indicates whether TLV tag will be
> written or not.
> 
> Advantages of Qword subscription of TLVs
> - Avoid multiple cache-line misses as the all the required fields
> of the TLV are within 128 bytes.
> - Memory optimization as TLVs + DATA + SHINFO can fit in 2k buffer
> even for 64 bit kernel.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>
> Co-developed-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
> ---
>   drivers/net/wireless/ath/ath12k/dp.c      |   9 +
>   drivers/net/wireless/ath/ath12k/dp.h      |  13 +
>   drivers/net/wireless/ath/ath12k/dp_rx.c   |  16 +-
>   drivers/net/wireless/ath/ath12k/dp_tx.c   |  20 ++
>   drivers/net/wireless/ath/ath12k/hal.c     | 352 ++++++++++++++++++++++
>   drivers/net/wireless/ath/ath12k/hal.h     |   3 +
>   drivers/net/wireless/ath/ath12k/rx_desc.h | 112 ++++++-
>   drivers/net/wireless/ath/ath12k/wmi.h     |   2 +
>   8 files changed, 519 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
> index 80d7ce44d..faeef965e 100644
> --- a/drivers/net/wireless/ath/ath12k/dp.c
> +++ b/drivers/net/wireless/ath/ath12k/dp.c
> @@ -1001,6 +1001,15 @@ void ath12k_dp_pdev_pre_alloc(struct ath12k_base *ab)
>   
>   void ath12k_dp_hal_rx_desc_init(struct ath12k_base *ab)
>   {
> +	if (test_bit(WMI_TLV_SERVICE_WMSK_COMPACTION_RX_TLVS, ab->wmi_ab.svc_map) &&
> +	    ab->hw_params->hal_ops->rxdma_ring_wmask_rx_mpdu_start &&
> +	    ab->hw_params->hal_ops->rxdma_ring_wmask_rx_msdu_end) {
> +		/* RX TLVS compaction is supported, hence change the hal_rx_ops
> +		 * based on device.
> +		 */
> +		if (ab->hal_rx_ops == &hal_rx_qcn9274_ops)
> +			ab->hal_rx_ops = &hal_rx_qcn9274_compact_ops;

I only have one comment on this patch.

in order to avoid chipset-specific logic here suggest that there should 
be an abstraction.

several ideas come to mind:
1) have a hal_ops callback to retrieve it
2) have a pointer to the compact ops in the hw_params

since we are already using hal_ops to get the masks, suggest that is 
where we should get the pointer to the compact ops

> +	}
>   	ab->hal.hal_desc_sz =
>   		ab->hal_rx_ops->rx_desc_get_desc_size();
>   }