Message-ID: <0f3efa0c-b5e0-44e4-850a-d63b0beeb0b8@kernel.org>
Date: Thu, 30 Jan 2025 08:57:21 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>, ath12k@...ts.infradead.org
Cc: linux-wireless@...r.kernel.org, Kalle Valo <kvalo@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Balamurugan S <quic_bselvara@...cinc.com>,
 P Praneesh <quic_ppranees@...cinc.com>
Subject: Re: [PATCH v5 08/13] wifi: ath12k: add AHB driver support for IPQ5332

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.


> +		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.

> +		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.

> +}
> +
> +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().

> +}
> +
> +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.


> +		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().

You
Best regards,
Krzysztof

Powered by blists - more mailing lists