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@xxxxxxxxxxx>
> ---
> 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
