Re: [RFC PATCH 5/5] wifi: ath12k: Enable IPQ5424 WiFi device support

[Krzysztof Kozlowski <krzk@kernel.org>]

On Thu, Jan 30, 2025 at 10:48:38AM +0530, Raj Kumar Bhagat wrote:
> From: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
> 
> Currently, ath12k AHB (in IPQ5332) uses SCM calls to authenticate the
> firmware image to bring up userpd. From IPQ5424 onwards, Q6 firmware can
> directly communicate with the Trusted Management Engine - Lite (TME-L),
> eliminating the need for SCM calls for userpd bring-up.
> 
> Hence, to enable IPQ5424 device support, use qcom_mdt_load_no_init() and
> skip the SCM call as Q6 will directly authenticate the userpd firmware.
> 
> Tested-on: IPQ5424 hw1.0 AHB WLAN.WBE.1.5-01053-QCAHKSWPL_SILICONZ-1
> Tested-on: IPQ5332 hw1.0 AHB WLAN.WBE.1.3.1-00130-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
> Co-developed-by: Saravanakumar Duraisamy <quic_saradura@quicinc.com>
> Signed-off-by: Saravanakumar Duraisamy <quic_saradura@quicinc.com>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/ahb.c | 80 +++++++++++++++++----------
>  drivers/net/wireless/ath/ath12k/ahb.h |  9 +++
>  2 files changed, 61 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
> index d502b59a78d8..75767915dec3 100644
> --- a/drivers/net/wireless/ath/ath12k/ahb.c
> +++ b/drivers/net/wireless/ath/ath12k/ahb.c
> @@ -21,6 +21,9 @@ static const struct of_device_id ath12k_ahb_of_match[] = {
>  	{ .compatible = "qcom,ipq5332-wifi",
>  	  .data = (void *)ATH12K_HW_IPQ5332_HW10,
>  	},
> +	{ .compatible = "qcom,ipq5424-wifi",
> +	  .data = (void *)ATH12K_HW_IPQ5424_HW10,
> +	},
>  	{ }
>  };
>  
> @@ -398,8 +401,8 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  		ATH12K_AHB_UPD_SWID;
>  
>  	/* Load FW image to a reserved memory location */
> -	ret = qcom_mdt_load(dev, fw, fw_name, pasid, mem_region, mem_phys, mem_size,
> -			    &mem_phys);
> +	ret = ab_ahb->ahb_ops->mdt_load(dev, fw, fw_name, pasid, mem_region, mem_phys,
> +					mem_size, &mem_phys);
>  	if (ret) {
>  		ath12k_err(ab, "Failed to load MDT segments: %d\n", ret);
>  		goto err_fw;
> @@ -430,11 +433,13 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  		goto err_fw2;
>  	}
>  
> -	/* Authenticate FW image using peripheral ID */
> -	ret = qcom_scm_pas_auth_and_reset(pasid);
> -	if (ret) {
> -		ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
> -		goto err_fw2;
> +	if (ab_ahb->scm_auth_enabled) {
> +		/* Authenticate FW image using peripheral ID */
> +		ret = qcom_scm_pas_auth_and_reset(pasid);
> +		if (ret) {
> +			ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
> +			goto err_fw2;
> +		}
>  	}
>  
>  	/* Instruct Q6 to spawn userPD thread */
> @@ -491,13 +496,15 @@ static void ath12k_ahb_power_down(struct ath12k_base *ab, bool is_suspend)
>  
>  	qcom_smem_state_update_bits(ab_ahb->stop_state, BIT(ab_ahb->stop_bit), 0);
>  
> -	pasid = (u32_encode_bits(ab_ahb->userpd_id, ATH12K_USERPD_ID_MASK)) |
> -		ATH12K_AHB_UPD_SWID;
> -	/* Release the firmware */
> -	ret = qcom_scm_pas_shutdown(pasid);
> -	if (ret)
> -		ath12k_err(ab, "scm pas shutdown failed for userPD%d: %d\n",
> -			   ab_ahb->userpd_id, ret);
> +	if (ab_ahb->scm_auth_enabled) {
> +		pasid = (u32_encode_bits(ab_ahb->userpd_id, ATH12K_USERPD_ID_MASK)) |
> +			 ATH12K_AHB_UPD_SWID;
> +		/* Release the firmware */
> +		ret = qcom_scm_pas_shutdown(pasid);
> +		if (ret)
> +			ath12k_err(ab, "scm pas shutdown failed for userPD%d\n",
> +				   ab_ahb->userpd_id);
> +	}
>  }
>  
>  static void ath12k_ahb_init_qmi_ce_config(struct ath12k_base *ab)
> @@ -707,6 +714,14 @@ static int ath12k_ahb_map_service_to_pipe(struct ath12k_base *ab, u16 service_id
>  	return 0;
>  }
>  
> +static const struct ath12k_ahb_ops ahb_ops_ipq5332 = {
> +	.mdt_load = qcom_mdt_load,
> +};
> +
> +static const struct ath12k_ahb_ops ahb_ops_ipq5424 = {
> +	.mdt_load = qcom_mdt_load_no_init,
> +};
> +
>  static const struct ath12k_hif_ops ath12k_ahb_hif_ops_ipq5332 = {
>  	.start = ath12k_ahb_start,
>  	.stop = ath12k_ahb_stop,
> @@ -1041,19 +1056,9 @@ static int ath12k_ahb_probe(struct platform_device *pdev)
>  	struct device_node *mem_node;
>  	struct ath12k_ahb *ab_ahb;
>  	enum ath12k_hw_rev hw_rev;
> -	u32 addr, userpd_id;
> +	u32 addr;
>  	int ret;
>  
> -	hw_rev = ath12k_ahb_get_hw_rev(pdev);
> -	switch (hw_rev) {
> -	case ATH12K_HW_IPQ5332_HW10:
> -		hif_ops = &ath12k_ahb_hif_ops_ipq5332;
> -		userpd_id = ATH12K_IPQ5332_USERPD_ID;
> -		break;
> -	default:
> -		return -EOPNOTSUPP;
> -	}

You just added this code in previous patchset, why are you moving it?

> -
>  	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to set 32-bit coherent dma\n");
> @@ -1067,13 +1072,32 @@ static int ath12k_ahb_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	ab_ahb = ath12k_ab_to_ahb(ab);
> +	ab_ahb->ab = ab;
> +
> +	hw_rev = ath12k_ahb_get_hw_rev(pdev);
> +	switch (hw_rev) {
> +	case ATH12K_HW_IPQ5332_HW10:
> +		hif_ops = &ath12k_ahb_hif_ops_ipq5332;
> +		ab_ahb->userpd_id = ATH12K_IPQ5332_USERPD_ID;
> +		ab_ahb->scm_auth_enabled = true;
> +		ab_ahb->ahb_ops = &ahb_ops_ipq5332;
> +		break;
> +	case ATH12K_HW_IPQ5424_HW10:
> +		hif_ops = &ath12k_ahb_hif_ops_ipq5332;
> +		ab_ahb->userpd_id = ATH12K_IPQ5332_USERPD_ID;
> +		ab_ahb->scm_auth_enabled = false;
> +		ab_ahb->ahb_ops = &ahb_ops_ipq5424;

Why you cannot store just proper driver data structure in match data?
This entire switch is redundant.

Best regards,
Krzysztof