Re: [PATCH 5/6] wifi: ath11k: add multipd support for QCN6122

On 29/10/2025 15:26, George Moussalem via B4 Relay wrote:
> From: George Moussalem <[email protected]>
> 
> IPQ5018/QCN6122 platforms use multi PD (protection domains) to avoid
> having one instance of the running Q6 firmware crashing resulting in
> crashing the others.
> 
> The IPQ5018 platform can have up to two QCN6122 wifi chips.
> To differentiate the two, the PD instance number (1 or 2) is added to
> the QMI service instance ID, which the QCN6122 firmware also expects.
> IPQ5018 internal wifi is always the first PD while QCN6122 cards must be
> second or third.
> 
> Signed-off-by: George Moussalem <[email protected]>
> ---
> See below patch for more info:
> https://lore.kernel.org/all/[email protected]/

I don't see any common part with that. Your bindings are completely
different and while PD was justified there, there is no such
justification here. Neither in the bindings.

> ---
>  drivers/net/wireless/ath/ath11k/ahb.c  | 31 +++++++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath11k/core.h |  4 ++++
>  drivers/net/wireless/ath/ath11k/pci.c  |  1 +
>  3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
> index 7b267dd62e964b2c4d6c3bbe016abd1ad0297219..820a383e88caf125892176e421b0121fed7e7055 100644
> --- a/drivers/net/wireless/ath/ath11k/ahb.c
> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
> @@ -429,6 +429,7 @@ static void ath11k_ahb_init_qmi_ce_config(struct ath11k_base *ab)
>  	cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
>  	cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
>  	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> +	ab->qmi.service_ins_id += ab->userpd_id;
>  }
>  
>  static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab)
> @@ -1101,6 +1102,28 @@ static int ath11k_ahb_fw_resources_init(struct ath11k_base *ab)
>  	return ret;
>  }
>  
> +static int ath11k_get_userpd_id(struct device *dev, int *userpd)
> +{
> +	int ret, userpd_id;
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,userpd", &userpd_id);
> +
> +	if (ret)
> +		return ret;
> +
> +	switch (userpd_id) {
> +	case 2:
> +		*userpd = ATH11K_QCN6122_USERPD_2;
> +		break;
> +	case 3:
> +		*userpd = ATH11K_QCN6122_USERPD_3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int ath11k_ahb_fw_resource_deinit(struct ath11k_base *ab)
>  {
>  	struct ath11k_ahb *ab_ahb = ath11k_ahb_priv(ab);
> @@ -1142,6 +1165,7 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
>  	const struct ath11k_hif_ops *hif_ops;
>  	const struct ath11k_pci_ops *pci_ops;
>  	enum ath11k_hw_rev hw_rev;
> +	int userpd_id = 0;
>  	int ret;
>  
>  	hw_rev = (uintptr_t)device_get_match_data(&pdev->dev);
> @@ -1160,6 +1184,12 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
>  	case ATH11K_HW_QCN6122_HW10:
>  		hif_ops = &ath11k_ahb_hif_ops_qcn6122;
>  		pci_ops = &ath11k_ahb_pci_ops_wcn6750;
> +		ret = ath11k_get_userpd_id(&pdev->dev, &userpd_id);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to get userpd: %d\n", ret);
> +			return ret;
> +		}
> +		dev_info(&pdev->dev, "multi-pd architecture - userpd: %d\n", userpd_id);

This does not look like useful printk message. Drivers should be silent
on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

>  		break;
>  	default:

Best regards,
Krzysztof