On 15.10.2024 8:26 PM, Raj Kumar Bhagat wrote:
> From: Balamurugan S <quic_bselvara@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Co-developed-by: P Praneesh <quic_ppranees@xxxxxxxxxxx>
> Signed-off-by: P Praneesh <quic_ppranees@xxxxxxxxxxx>
> Co-developed-by: Raj Kumar Bhagat <quic_rajkbhag@xxxxxxxxxxx>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@xxxxxxxxxxx>
> ---
[...]
> +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
