[PATCH v2 2/2] wifi: ath11k: support board-specific firmware overrides

[Dmitry Baryshkov]

On Thu, Oct 24, 2024 at 08:25:14AM +0800, Miaoqing Pan wrote:
> QCA6698AQ IP core is the same as WCN6855 hw2.1, but it has different RF,
> IPA, thermal, RAM size and etc, so new firmware files used. This change
> allows board DT files to override the subdir of the firmware directory
> used to lookup the amss.bin and m3.bin.

I have slight concerns regarding the _board_ DT files overriding the
subdir. This opens a can of worms, allowing per-board firmware sets,
which (as far as I understand) is far from being what driver maintainers
would like to see. This was required for ath10k-snoc devices, since
firmware for those platforms is signed by the vendor keys and it is
limited to a particular SoC or SoC family. For ath11k-pci there is no
such limitation.

Would it be possible to use PCI subvendor / subdev to identify affected
cards? PCI Revision? Any other way to identify the device?  Please
provide lspci -nnvv for the affected device kind. Is there a way to
identify the RF part somehow?

Could you possibly clarify, how this situation is handled in Windows
world?

> 
> For example:
> 
> - ath11k/WCN6855/hw2.1/amss.bin,
>   ath11k/WCN6855/hw2.1/m3.bin: main firmware files, used by default
> 
> - ath11k/WCN6855/hw2.1/qca6698aq/amss.bin,
>   ath11k/WCN6855/hw2.1/qca6698aq/m3.bin

This approach looks good to me, thank you.

> 
> Tested-on: QCA6698AQ hw2.1 PCI WLAN.HSP.1.1-04402-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1
> 
> Signed-off-by: Miaoqing Pan <quic_miaoqing at quicinc.com>
> ---
>  drivers/net/wireless/ath/ath11k/core.c | 16 ++++++++++++++++
>  drivers/net/wireless/ath/ath11k/core.h | 11 +++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
> index be67382c00f6..775e48551522 100644
> --- a/drivers/net/wireless/ath/ath11k/core.c
> +++ b/drivers/net/wireless/ath/ath11k/core.c
> @@ -1178,6 +1178,22 @@ static int ath11k_core_create_chip_id_board_name(struct ath11k_base *ab, char *n
>  					       ATH11K_BDF_NAME_CHIP_ID);
>  }
>  
> +void ath11k_core_create_firmware_path(struct ath11k_base *ab,
> +				      const char *filename,
> +				      void *buf, size_t buf_len)
> +{	const char *board_name = NULL;
> +
> +	of_property_read_string(ab->dev->of_node, "firmware-name", &board_name);

soc_name rather than board_name, please. Or just fw_name.

> +
> +	if (board_name)
> +		snprintf(buf, buf_len, "%s/%s/%s/%s", ATH11K_FW_DIR,
> +			 ab->hw_params.fw.dir, board_name, filename);
> +	else
> +		snprintf(buf, buf_len, "%s/%s/%s", ATH11K_FW_DIR,
> +			 ab->hw_params.fw.dir, filename);
> +}
> +EXPORT_SYMBOL(ath11k_core_create_firmware_path);
> +
>  const struct firmware *ath11k_core_firmware_request(struct ath11k_base *ab,
>  						    const char *file)
>  {
> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
> index 09c37e19a168..ce4102cfed4d 100644
> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -1249,6 +1249,9 @@ bool ath11k_core_coldboot_cal_support(struct ath11k_base *ab);
>  
>  const struct firmware *ath11k_core_firmware_request(struct ath11k_base *ab,
>  						    const char *filename);
> +void ath11k_core_create_firmware_path(struct ath11k_base *ab,
> +				      const char *filename,
> +				      void *buf, size_t buf_len);
>  
>  static inline const char *ath11k_scan_state_str(enum ath11k_scan_state state)
>  {
> @@ -1295,14 +1298,6 @@ static inline struct ath11k *ath11k_ab_to_ar(struct ath11k_base *ab,
>  	return ab->pdevs[ath11k_hw_mac_id_to_pdev_id(&ab->hw_params, mac_id)].ar;
>  }
>  
> -static inline void ath11k_core_create_firmware_path(struct ath11k_base *ab,
> -						    const char *filename,
> -						    void *buf, size_t buf_len)
> -{
> -	snprintf(buf, buf_len, "%s/%s/%s", ATH11K_FW_DIR,
> -		 ab->hw_params.fw.dir, filename);

It could have perfectly lived here. Is there any reason to move the
function?

> -}
> -
>  static inline const char *ath11k_bus_str(enum ath11k_bus bus)
>  {
>  	switch (bus) {
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry