Message-ID: <243cd21d-6a35-4ff0-9b38-ec519a804670@oss.qualcomm.com>
Date: Fri, 18 Oct 2024 22:29:33 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
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>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Balamurugan S <quic_bselvara@...cinc.com>,
        P Praneesh <quic_ppranees@...cinc.com>
Subject: Re: [PATCH v2 17/22] wifi: ath12k: add AHB driver support for IPQ5332

On 15.10.2024 8:26 PM, Raj Kumar Bhagat wrote:
> From: Balamurugan S <quic_bselvara@...cinc.com>
> 
> Add Initial Ath12k AHB driver support for IPQ5332. IPQ5332 is AHB
> based IEEE802.11be 2 GHz 2x2 WiFi device.
> 
> Tested-on: IPQ5332 hw1.0 AHB WLAN.WBE.1.3.1-00130-QCAHKSWPL_SILICONZ-1
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00210-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Balamurugan S <quic_bselvara@...cinc.com>
> Co-developed-by: P Praneesh <quic_ppranees@...cinc.com>
> Signed-off-by: P Praneesh <quic_ppranees@...cinc.com>
> Co-developed-by: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
> ---

[...]

> +enum ext_irq_num {
> +	host2wbm_desc_feed = 16,
> +	host2reo_re_injection,

Why?


> +static u32 ath12k_ahb_cmem_read32(struct ath12k_base *ab, u32 offset)
> +{
> +	offset = offset - HAL_IPQ5332_CMEM_BASE;
> +	return ioread32(ab->mem_cmem + offset);

return ioread32(ab->mem_cmem + offset - HAL_IPQ5332_CMEM_BASE)?

Or maybe the mem_cmem base should be moved?

> +static int ath12k_ahb_start(struct ath12k_base *ab)
> +{
> +	ath12k_ahb_ce_irqs_enable(ab);
> +	ath12k_ce_rx_post_buf(ab);
> +
> +	return 0;
> +}

Neither this nor ath12k_pci returns anything useful - perhaps make this void?

> +static void ath12k_ahb_free_ext_irq(struct ath12k_base *ab)

Any reason we're not using devm APIs?

> +static int ath12k_ahb_config_ext_irq(struct ath12k_base *ab)
> +{
> +	struct ath12k_ext_irq_grp *irq_grp;
> +	const struct hal_ops *hal_ops;
> +	int i, j, irq, irq_idx, ret;
> +	u32 num_irq;
> +
> +	hal_ops = ab->hw_params->hal_ops;
> +	for (i = 0; i < ATH12K_EXT_IRQ_GRP_NUM_MAX; i++) {
> +		irq_grp = &ab->ext_irq_grp[i];
> +		num_irq = 0;
> +
> +		irq_grp->ab = ab;
> +		irq_grp->grp_id = i;
> +
> +		irq_grp->napi_ndev = alloc_netdev_dummy(0);
> +		if (!irq_grp->napi_ndev)
> +			return -ENOMEM;
> +
> +		netif_napi_add(irq_grp->napi_ndev, &irq_grp->napi,
> +			       ath12k_ahb_ext_grp_napi_poll);
> +
> +		for (j = 0; j < ATH12K_EXT_IRQ_NUM_MAX; j++) {
> +			if (ab->hw_params->ring_mask->tx[i] &&
> +			    j <= ATH12K_MAX_TCL_RING_NUM &&
> +			    (ab->hw_params->ring_mask->tx[i] &
> +			     BIT(hal_ops->tcl_to_wbm_rbm_map[j].wbm_ring_num))) {
> +				irq_grp->irqs[num_irq++] =
> +					wbm2host_tx_completions_ring1 - j;
> +			}

This is unreadable

> +
> +			if (ab->hw_params->ring_mask->rx[i] & BIT(j)) {

Consider taking a pointer to ring_mask so that the lines are shorter

> +				irq_grp->irqs[num_irq++] =
> +					reo2host_destination_ring1 - j;
> +			}
> +
> +			if (ab->hw_params->ring_mask->rx_err[i] & BIT(j))
> +				irq_grp->irqs[num_irq++] = reo2host_exception;
> +
> +			if (ab->hw_params->ring_mask->rx_wbm_rel[i] & BIT(j))
> +				irq_grp->irqs[num_irq++] = wbm2host_rx_release;
> +
> +			if (ab->hw_params->ring_mask->reo_status[i] & BIT(j))
> +				irq_grp->irqs[num_irq++] = reo2host_status;
> +
> +			if (ab->hw_params->ring_mask->rx_mon_dest[i] & BIT(j))
> +				irq_grp->irqs[num_irq++] =
> +					rxdma2host_monitor_destination_mac1;
> +		}
> +
> +		irq_grp->num_irq = num_irq;
> +
> +		for (j = 0; j < irq_grp->num_irq; j++) {
> +			irq_idx = irq_grp->irqs[j];
> +
> +			irq = platform_get_irq_byname(ab->pdev,
> +						      irq_name[irq_idx]);
> +			ab->irq_num[irq_idx] = irq;
> +			irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_DISABLE_UNLAZY);
> +			ret = request_irq(irq, ath12k_ahb_ext_interrupt_handler,
> +					  IRQF_TRIGGER_RISING,
> +					  irq_name[irq_idx], irq_grp);
> +			if (ret) {
> +				ath12k_err(ab, "failed request_irq for %d\n",
> +					   irq);
> +			}
> +		}

Instead of doing all this magic, can we request the IRQs manually, as we
have interrupt-names in dt?

> +	}
> +
> +	return 0;
> +}
> +
> +static int ath12k_ahb_config_irq(struct ath12k_base *ab)
> +{
> +	int irq, irq_idx, i;
> +	int ret;
> +
> +	/* Configure CE irqs */
> +	for (i = 0; i < ab->hw_params->ce_count; i++) {
> +		struct ath12k_ce_pipe *ce_pipe = &ab->ce.ce_pipe[i];
> +
> +		if (ath12k_ce_get_attr_flags(ab, i) & CE_ATTR_DIS_INTR)
> +			continue;
> +
> +		irq_idx = ATH12K_IRQ_CE0_OFFSET + i;
> +
> +		INIT_WORK(&ce_pipe->intr_wq, ath12k_ahb_ce_workqueue);
> +		irq = platform_get_irq_byname(ab->pdev, irq_name[irq_idx]);
> +		ret = request_irq(irq, ath12k_ahb_ce_interrupt_handler,
> +				  IRQF_TRIGGER_RISING, irq_name[irq_idx],
> +				  ce_pipe);
> +		if (ret)
> +			return ret;
> +
> +		ab->irq_num[irq_idx] = irq;
> +	}
> +
> +	/* Configure external interrupts */
> +	ret = ath12k_ahb_config_ext_irq(ab);
> +
> +	return ret;
> +}
> +
> +static int ath12k_ahb_map_service_to_pipe(struct ath12k_base *ab, u16 service_id,
> +					  u8 *ul_pipe, u8 *dl_pipe)
> +{
> +	const struct service_to_pipe *entry;
> +	bool ul_set = false, dl_set = false;
> +	int i;
> +
> +	for (i = 0; i < ab->hw_params->svc_to_ce_map_len; i++) {
> +		entry = &ab->hw_params->svc_to_ce_map[i];
> +
> +		if (__le32_to_cpu(entry->service_id) != service_id)
> +			continue;
> +
> +		switch (__le32_to_cpu(entry->pipedir)) {
> +		case PIPEDIR_NONE:
> +			break;
> +		case PIPEDIR_IN:
> +			WARN_ON(dl_set);
> +			*dl_pipe = __le32_to_cpu(entry->pipenum);
> +			dl_set = true;
> +			break;
> +		case PIPEDIR_OUT:
> +			WARN_ON(ul_set);
> +			*ul_pipe = __le32_to_cpu(entry->pipenum);
> +			ul_set = true;
> +			break;
> +		case PIPEDIR_INOUT:
> +			WARN_ON(dl_set);
> +			WARN_ON(ul_set);
> +			*dl_pipe = __le32_to_cpu(entry->pipenum);
> +			*ul_pipe = __le32_to_cpu(entry->pipenum);
> +			dl_set = true;
> +			ul_set = true;
> +			break;
> +		}

if pipedir == PIPEDIR_IN || pipedir == PIPEDIR_INOUT
if pipedir == PIPEDIR_OUT || pipedir == PIPE_INOUT

?

> +	}
> +
> +	if (WARN_ON(!ul_set || !dl_set))
> +		return -ENOENT;
> +
> +	return 0;
> +}
> +
> +static const struct ath12k_hif_ops ath12k_ahb_hif_ops_ipq5332 = {
> +	.start = ath12k_ahb_start,
> +	.stop = ath12k_ahb_stop,
> +	.read32 = ath12k_ahb_read32,
> +	.write32 = ath12k_ahb_write32,
> +	.cmem_read32 = ath12k_ahb_cmem_read32,
> +	.cmem_write32 = ath12k_ahb_cmem_write32,
> +	.irq_enable = ath12k_ahb_ext_irq_enable,
> +	.irq_disable = ath12k_ahb_ext_irq_disable,
> +	.map_service_to_pipe = ath12k_ahb_map_service_to_pipe,
> +};
> +
> +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, "gcc_xo_clk");
> +	if (IS_ERR_OR_NULL(ab_ahb->xo_clk)) {
> +		ath12k_err(ab, "failed to get gcc_xo_clk: %d\n",
> +			   PTR_ERR_OR_ZERO(ab_ahb->xo_clk));
> +		ret = ab_ahb->xo_clk ? PTR_ERR(ab_ahb->xo_clk) : -ENODEV;
> +		ab_ahb->xo_clk = NULL;
> +		return ret;
> +	}

Won't clk core print errors in both of these cases?

> +
> +	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");
> +		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);
> +}
> +
> +static int ath12k_ahb_resource_init(struct ath12k_base *ab)
> +{
> +	struct platform_device *pdev = ab->pdev;
> +	struct resource *mem_res;
> +	void __iomem *mem;
> +	int ret;
> +
> +	mem = devm_platform_get_and_ioremap_resource(pdev, 0, &mem_res);
> +	if (IS_ERR(mem)) {
> +		dev_err(&pdev->dev, "ioremap error\n");
> +		ret = PTR_ERR(mem);
> +		goto out;

If you assign ab->mem directly, you can get rid of the line below
and return the error here

> +	}
> +
> +	ab->mem = mem;
> +	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.
> +		 */

Please capitalize words consistently

> +		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;
> +	}
> +
> +	if (ab->hw_params->cmem_remap) {
> +		const struct cmem_remap *cmem_remap = ab->hw_params->cmem_remap;
> +		/* For device like IPQ5332 CMEM region is outside WCSS block.

IPQ5332 is not a 'device'

> +		 * Allocate separate I/O remap to access CMEM address.
> +		 */
> +		ab->mem_cmem = ioremap(cmem_remap->base, cmem_remap->size);
> +		if (IS_ERR(ab->mem_cmem)) {
> +			dev_err(&pdev->dev, "cmem ioremap error\n");
> +			ret = -ENOMEM;
> +			goto err_mem_ce_unmap;
> +		}
> +	}

[...]

> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to set 32-bit consistent dma\n");

s/consistent/coherent

You're setting the mask, not the DMA itself

[...]

> +	/* 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);
> +		mem_node = of_find_node_by_name(NULL, "mlo_global_mem_0");

This is not mentioned or documented anywhere.

Konrad

Powered by blists - more mailing lists