Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 8D8A8C4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Jouni Malinen <jouni@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        Seevalamuthu Mariappan <seevalam@codeaurora.org>,
        Anilkumar Kolli <akolli@codeaurora.org>
Subject: Re: [PATCH 12/12] ath11k: Change QCN9074 firmware to operate in mode-2
References: <20210721212029.142388-1-jouni@codeaurora.org>
        <20210721212029.142388-12-jouni@codeaurora.org>
Date:   Fri, 12 Nov 2021 10:19:23 +0200
In-Reply-To: <20210721212029.142388-12-jouni@codeaurora.org> (Jouni Malinen's
        message of "Thu, 22 Jul 2021 00:20:29 +0300")
Message-ID: <871r3lkcqc.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Jouni Malinen <jouni@codeaurora.org> writes:

> From: Seevalamuthu Mariappan <seevalam@codeaurora.org>
>
> In mode-2 QCN9074 firmware uses 15 MB of host memory and firmware
> request 1 MB size segments in QMI, whereas in mode-0 firmware uses 45 MB
> of host memory and each segment is of 2 MB size. In mode-2 firmware
> operates with reduced number of vdevs and peers.
>
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.4.0.1-01838-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Seevalamuthu Mariappan <seevalam@codeaurora.org>
> Co-developed-by: Anilkumar Kolli <akolli@codeaurora.org>
> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
> Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath11k/core.c | 23 ++++++++++++++++++++++-
>  drivers/net/wireless/ath/ath11k/core.h |  6 ++++++
>  drivers/net/wireless/ath/ath11k/hw.c   | 14 +++++++-------
>  drivers/net/wireless/ath/ath11k/hw.h   | 20 +++++++++++---------
>  drivers/net/wireless/ath/ath11k/mac.c  | 10 +++++-----
>  drivers/net/wireless/ath/ath11k/qmi.c  |  2 +-
>  6 files changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
> index d2ab3b134632..9f2c9795767e 100644
> --- a/drivers/net/wireless/ath/ath11k/core.c
> +++ b/drivers/net/wireless/ath/ath11k/core.c
> @@ -77,6 +77,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
>  		.supports_shadow_regs = false,
>  		.idle_ps = false,
>  		.cold_boot_calib = true,
> +		.fw_mem_mode = 0,
>  		.supports_suspend = false,
>  		.hal_desc_sz = sizeof(struct hal_rx_desc_ipq8074),
>  		.fix_l1ss = true,

[...]

> +static const struct ath11k_num_vdevs_peers ath11k_vdevs_peers[] = {
> +	{
> +		.num_vdevs = (16 + 1),
> +		.num_peers = 512,
> +	},
> +	{
> +		.num_vdevs = (8 + 1),
> +		.num_peers = 128,
> +	},
> +	{
> +		.num_vdevs = 8,
> +		.num_peers = 128,
> +	},
> +};

I am worried about this array. It implies that _all_ hardware support
these modes but is that really the case? I would guess that these are
very much hardware and firmware specific values.

So because of that I would feel clearer to have num_vdevs and num_peers
in ath11k_hw_params, to make it clear that the values are hardware
specific. And then have fw_mem_mode, num_vdevs and num_peers in their
own struct within struct ath11k_hw_params, just like spectral has
grouped valued together.

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

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