Re: [PATCH v5 08/13] wifi: ath12k: add AHB driver support for IPQ5332

[Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>]

On 1/30/2025 1:27 PM, Krzysztof Kozlowski wrote:
> On 30/01/2025 05:35, Raj Kumar Bhagat wrote:
>> +static int ath12k_ahb_clock_init(struct ath12k_base *ab)
>> +{
>> +	struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +	int ret;
>> +
>> +	ab_ahb->xo_clk = devm_clk_get(ab->dev, "xo");
>> +	if (IS_ERR_OR_NULL(ab_ahb->xo_clk)) {
> 
> No, you are not supposed to use IS_ERR_OR_NULL(). That's indication of bug.
> 
>> +		ret = ab_ahb->xo_clk ? PTR_ERR(ab_ahb->xo_clk) : -ENODEV;
> 
> I don't understand this. It's the third time you are reimplementing
> standard code in some odd way, different than all other drivers.
> 
> Read the description of this function. Can clk_get return NULL? Of
> course not. This is so overcomplicated for no reason, I wonder if it is
> actually buggy here.
> 

My bad, will update in next version.

> 
>> +		return dev_err_probe(&ab->pdev->dev, ret, "failed to get xo clock\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ath12k_ahb_clock_deinit(struct ath12k_base *ab)
>> +{
>> +	struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +
>> +	devm_clk_put(ab->dev, ab_ahb->xo_clk);
>> +	ab_ahb->xo_clk = NULL;
>> +}
>> +
>> +static int ath12k_ahb_clock_enable(struct ath12k_base *ab)
>> +{
>> +	struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +	int ret;
>> +
>> +	if (IS_ERR_OR_NULL(ab_ahb->xo_clk)) {
>> +		ath12k_err(ab, "clock is not initialized\n");
> 
> NAK.
> 
> Sorry, this code makes no sense. This is some random code. This code
> cannot be executed before probe. If it can: your driver is buggy, so fix
> your driver.
> 
> After the probe(), this is never NULL as an error. Either you have here
> valid pointer or you failed the probe.
> 
> This driver fails on basics of driver probing.
> 

Will remove the above code in next version.

>> +		return -EIO;
>> +	}
>> +
>> +	ret = clk_prepare_enable(ab_ahb->xo_clk);
>> +	if (ret) {
>> +		ath12k_err(ab, "failed to enable gcc_xo_clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ath12k_ahb_clock_disable(struct ath12k_base *ab)
>> +{
>> +	struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>> +
>> +	clk_disable_unprepare(ab_ahb->xo_clk);
> 
> Don't create such wrappers for single clock. Does not help.
> 

Sure, will remove the wrapper.

>> +}
>> +
>> +static int ath12k_ahb_resource_init(struct ath12k_base *ab)
>> +{
>> +	struct platform_device *pdev = ab->pdev;
>> +	struct resource *mem_res;
>> +	int ret;
>> +
>> +	ab->mem = devm_platform_get_and_ioremap_resource(pdev, 0, &mem_res);
>> +	if (IS_ERR(ab->mem)) {
>> +		ret = dev_err_probe(&pdev->dev, PTR_ERR(ab->mem), "ioremap error\n");
>> +		goto out;
>> +	}
>> +
>> +	ab->mem_len = resource_size(mem_res);
>> +
>> +	if (ab->hw_params->ce_remap) {
>> +		const struct ce_remap *ce_remap = ab->hw_params->ce_remap;
>> +		/* CE register space is moved out of WCSS and the space is not
>> +		 * contiguous, hence remapping the CE registers to a new space
>> +		 * for accessing them.
>> +		 */
>> +		ab->mem_ce = ioremap(ce_remap->base, ce_remap->size);
>> +		if (IS_ERR(ab->mem_ce)) {
>> +			dev_err(&pdev->dev, "ce ioremap error\n");
>> +			ret = -ENOMEM;
>> +			goto err_mem_unmap;
>> +		}
>> +		ab->ce_remap = true;
>> +		ab->ce_remap_base_addr = HAL_IPQ5332_CE_WFSS_REG_BASE;
>> +	}
>> +
>> +	ret = ath12k_ahb_clock_init(ab);
>> +	if (ret)
>> +		goto err_mem_ce_unmap;
>> +
>> +	ret =  ath12k_ahb_clock_enable(ab);
>> +	if (ret)
>> +		goto err_clock_deinit;
>> +
>> +	return 0;
>> +
>> +err_clock_deinit:
>> +	ath12k_ahb_clock_deinit(ab);
>> +
>> +err_mem_ce_unmap:
>> +	if (ab->hw_params->ce_remap)
>> +		iounmap(ab->mem_ce);
>> +
>> +err_mem_unmap:
>> +	ab->mem_ce = NULL;
>> +	devm_iounmap(ab->dev, ab->mem);
>> +
>> +out:
>> +	ab->mem = NULL;
>> +	return ret;
>> +}
>> +
>> +static void ath12k_ahb_resource_deinit(struct ath12k_base *ab)
>> +{
>> +	if (ab->mem)
>> +		devm_iounmap(ab->dev, ab->mem);
>> +
>> +	if (ab->mem_ce)
>> +		iounmap(ab->mem_ce);
>> +
>> +	ab->mem = NULL;
>> +	ab->mem_ce = NULL;
>> +
>> +	ath12k_ahb_clock_disable(ab);
>> +	ath12k_ahb_clock_deinit(ab);
>> +}
>> +
>> +static enum ath12k_hw_rev ath12k_ahb_get_hw_rev(struct platform_device *pdev)
>> +{
>> +	const struct of_device_id *of_id;
>> +
>> +	of_id = of_match_device(ath12k_ahb_of_match, &pdev->dev);
>> +	if (!of_id) {
>> +		dev_err(&pdev->dev, "Failed to find matching device tree id\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return (enum ath12k_hw_rev)of_id->data;
> 
> You just open-coded of_device_get_match_data().
> 

Thanks, will use of_device_get_match_data() in next version.

>> +}
>> +
>> +static int ath12k_ahb_probe(struct platform_device *pdev)
>> +{
>> +	struct ath12k_base *ab;
>> +	const struct ath12k_hif_ops *hif_ops;
>> +	struct device_node *mem_node;
>> +	enum ath12k_hw_rev hw_rev;
>> +	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;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	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");
>> +		return ret;
>> +	}
>> +
>> +	ab = ath12k_core_alloc(&pdev->dev, sizeof(struct ath12k_ahb),
>> +			       ATH12K_BUS_AHB);
>> +	if (!ab) {
>> +		dev_err(&pdev->dev, "failed to allocate ath12k base\n");
> 
> No, driver never prints allocation errors. You are duplicating existing
> core printk.
> 

Thanks, will remove this error print.

> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ab->hif.ops = hif_ops;
>> +	ab->pdev = pdev;
>> +	ab->hw_rev = hw_rev;
>> +	platform_set_drvdata(pdev, ab);
>> +
>> +	/* Set fixed_mem_region to true for platforms that support fixed memory
>> +	 * reservation from DT. If memory is reserved from DT for FW, ath12k driver
>> +	 * need not to allocate memory.
>> +	 */
>> +	if (!of_property_read_u32(ab->dev->of_node, "memory-region", &addr)) {
>> +		set_bit(ATH12K_FLAG_FIXED_MEM_REGION, &ab->dev_flags);
>> +
>> +		/* If the platform supports fixed memory, then it should define/
>> +		 * reserve MLO global memory in DT to support Multi Link Operation
>> +		 * (IEEE 802.11be).
>> +		 * If MLO global memory is not reserved in fixed memory mode, then
>> +		 * MLO cannot be supported.
>> +		 */
>> +		mem_node = ath12k_core_get_reserved_mem_by_name(ab, "mlo-global-mem");
>> +		if (!mem_node)
>> +			ab->single_chip_mlo_supp = false;
>> +		else
>> +			of_node_put(mem_node);
>> +	}
>> +
>> +	ret = ath12k_core_pre_init(ab);
>> +	if (ret)
>> +		goto err_core_free;
>> +
>> +	ret = ath12k_ahb_resource_init(ab);
>> +	if (ret)
>> +		goto err_core_free;
>> +
>> +	ret = ath12k_hal_srng_init(ab);
>> +	if (ret)
>> +		goto err_resource_deinit;
>> +
>> +	ret = ath12k_ce_alloc_pipes(ab);
>> +	if (ret) {
>> +		ath12k_err(ab, "failed to allocate ce pipes: %d\n", ret);
>> +		goto err_hal_srng_deinit;
>> +	}
>> +
>> +	ath12k_ahb_init_qmi_ce_config(ab);
>> +
>> +	ret = ath12k_ahb_config_irq(ab);
>> +	if (ret) {
>> +		ath12k_err(ab, "failed to configure irq: %d\n", ret);
>> +		goto err_ce_free;
>> +	}
>> +
>> +	ret = ath12k_core_init(ab);
>> +	if (ret) {
>> +		ath12k_err(ab, "failed to init core: %d\n", ret);
>> +		goto err_ce_free;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_ce_free:
>> +	ath12k_ce_free_pipes(ab);
>> +
>> +err_hal_srng_deinit:
>> +	ath12k_hal_srng_deinit(ab);
>> +
>> +err_resource_deinit:
>> +	ath12k_ahb_resource_deinit(ab);
>> +
>> +err_core_free:
>> +	ath12k_core_free(ab);
>> +	platform_set_drvdata(pdev, NULL);
>> +
>> +	return ret;
>> +}
>> +
>> +static void ath12k_ahb_remove_prepare(struct ath12k_base *ab)
>> +{
>> +	unsigned long left;
>> +
>> +	if (test_bit(ATH12K_FLAG_RECOVERY, &ab->dev_flags)) {
>> +		left = wait_for_completion_timeout(&ab->driver_recovery,
>> +						   ATH12K_AHB_RECOVERY_TIMEOUT);
>> +		if (!left)
>> +			ath12k_warn(ab, "failed to receive recovery response completion\n");
>> +	}
>> +
>> +	set_bit(ATH12K_FLAG_UNREGISTERING, &ab->dev_flags);
>> +	cancel_work_sync(&ab->restart_work);
>> +	cancel_work_sync(&ab->qmi.event_work);
>> +}
>> +
>> +static void ath12k_ahb_free_resources(struct ath12k_base *ab)
>> +{
>> +	struct platform_device *pdev = ab->pdev;
>> +
>> +	ath12k_hal_srng_deinit(ab);
>> +	ath12k_ce_free_pipes(ab);
>> +	ath12k_ahb_resource_deinit(ab);
>> +	ath12k_core_free(ab);
>> +	platform_set_drvdata(pdev, NULL);
>> +}
>> +
>> +static void ath12k_ahb_remove(struct platform_device *pdev)
>> +{
>> +	struct ath12k_base *ab = platform_get_drvdata(pdev);
>> +
>> +	if (test_bit(ATH12K_FLAG_QMI_FAIL, &ab->dev_flags)) {
>> +		ath12k_qmi_deinit_service(ab);
>> +		goto qmi_fail;
>> +	}
>> +
>> +	ath12k_ahb_remove_prepare(ab);
>> +	ath12k_core_deinit(ab);
>> +
>> +qmi_fail:
>> +	ath12k_ahb_free_resources(ab);
>> +}
>> +
>> +static void ath12k_ahb_shutdown(struct platform_device *pdev)
>> +{
>> +	struct ath12k_base *ab = platform_get_drvdata(pdev);
>> +
>> +	/* platform shutdown() & remove() are mutually exclusive.
>> +	 * remove() is invoked during rmmod & shutdown() during
>> +	 * system reboot/shutdown.
>> +	 */
> 
> You should rather explain why you cannot use one callback for both. Why
> this has to be duplicated?
> 
>> +	ath12k_ahb_remove_prepare(ab);
>> +
>> +	if (!(test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)))
>> +		goto free_resources;
>> +
>> +	ath12k_core_deinit(ab);
> 
> And why this is actually different order than remove().
> 

The .shutdown() is not required for IPQ5332 wifi. In the next version
will remove the shutdown callback.