Messages in this thread | | | Date | Tue, 4 Oct 2022 09:23:31 -0700 | Subject | Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver | From | Lizhi Hou <> |
| |
On 10/4/22 01:18, Ilpo Järvinen wrote: > On Wed, 28 Sep 2022, Lizhi Hou wrote: > >> Add driver to enable PCIe board which uses XDMA (the DMA/Bridge Subsystem >> for PCI Express). For example, Xilinx Alveo PCIe devices. >> https://www.xilinx.com/products/boards-and-kits/alveo.html >> >> The XDMA engine support up to 4 Host to Card (H2C) and 4 Card to Host (C2H) >> channels. Memory transfers are specified on a per-channel basis in >> descriptor linked lists, which the DMA fetches from host memory and >> processes. Events such as descriptor completion and errors are signaled >> using interrupts. The hardware detail is provided by >> https://docs.xilinx.com/r/en-US/pg195-pcie-dma/Introduction >> >> This driver implements dmaengine APIs. >> - probe the available DMA channels >> - use dma_slave_map for channel lookup >> - use virtual channel to manage dmaengine tx descriptors >> - implement device_prep_slave_sg callback to handle host scatter gather >> list >> - implement device_config to config device address for DMA transfer >> >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> >> Signed-off-by: Sonal Santan <sonal.santan@amd.com> >> Signed-off-by: Max Zhen <max.zhen@amd.com> >> Signed-off-by: Brian Xu <brian.xu@amd.com> >> --- >> +/* bits of the channel control register */ >> +#define CHAN_CTRL_RUN_STOP BIT(0) >> +#define CHAN_CTRL_IE_DESC_STOPPED BIT(1) >> +#define CHAN_CTRL_IE_DESC_COMPLETED BIT(2) >> +#define CHAN_CTRL_IE_DESC_ALIGN_MISMATCH BIT(3) >> +#define CHAN_CTRL_IE_MAGIC_STOPPED BIT(4) >> +#define CHAN_CTRL_IE_IDLE_STOPPED BIT(6) >> +#define CHAN_CTRL_IE_READ_ERROR (0x1FUL << 9) >> +#define CHAN_CTRL_IE_DESC_ERROR (0x1FUL << 19) > Looks GENMASK() Sure. Thanks for pointing it out. > >> +/* bits of the SG DMA control register */ >> +#define XDMA_CTRL_RUN_STOP BIT(0) >> +#define XDMA_CTRL_IE_DESC_STOPPED BIT(1) >> +#define XDMA_CTRL_IE_DESC_COMPLETED BIT(2) >> +#define XDMA_CTRL_IE_DESC_ALIGN_MISMATCH BIT(3) >> +#define XDMA_CTRL_IE_MAGIC_STOPPED BIT(4) >> +#define XDMA_CTRL_IE_IDLE_STOPPED BIT(6) >> +#define XDMA_CTRL_IE_READ_ERROR (0x1FUL << 9) >> +#define XDMA_CTRL_IE_DESC_ERROR (0x1FUL << 19) > Ditto. > >> +/** >> + * xdma_config_channels - Detect and config DMA channels >> + * @xdev: DMA device pointer >> + * @dir: Channel direction >> + */ >> +static int xdma_config_channels(struct xdma_device *xdev, >> + enum dma_transfer_direction dir) >> +{ >> + struct xdma_platdata *pdata = dev_get_platdata(&xdev->pdev->dev); >> + u32 base, identifier, target; >> + struct xdma_chan **chans; >> + u32 *chan_num; >> + int i, j, ret; >> + >> + if (dir == DMA_MEM_TO_DEV) { >> + base = XDMA_CHAN_H2C_OFFSET; >> + target = XDMA_CHAN_H2C_TARGET; >> + chans = &xdev->h2c_chans; >> + chan_num = &xdev->h2c_chan_num; >> + } else if (dir == DMA_DEV_TO_MEM) { >> + base = XDMA_CHAN_C2H_OFFSET; >> + target = XDMA_CHAN_C2H_TARGET; >> + chans = &xdev->c2h_chans; >> + chan_num = &xdev->c2h_chan_num; >> + } else { >> + xdma_err(xdev, "invalid direction specified"); >> + return -EINVAL; >> + } >> + >> + /* detect number of available DMA channels */ >> + for (i = 0, *chan_num = 0; i < pdata->max_dma_channels; i++) { >> + ret = xdma_read_reg(xdev, base + i * XDMA_CHAN_STRIDE, >> + XDMA_CHAN_IDENTIFIER, &identifier); >> + if (ret) { >> + xdma_err(xdev, "failed to read channel id: %d", ret); >> + return ret; >> + } >> + >> + /* check if it is available DMA channel */ >> + if (XDMA_CHAN_CHECK_TARGET(identifier, target)) >> + (*chan_num)++; >> + } >> + >> + if (!*chan_num) { >> + xdma_err(xdev, "does not probe any channel"); >> + return -EINVAL; >> + } >> + >> + *chans = devm_kzalloc(&xdev->pdev->dev, sizeof(**chans) * (*chan_num), >> + GFP_KERNEL); >> + if (!*chans) >> + return -ENOMEM; >> + >> + for (i = 0, j = 0; i < pdata->max_dma_channels; i++) { >> + ret = xdma_read_reg(xdev, base + i * XDMA_CHAN_STRIDE, >> + XDMA_CHAN_IDENTIFIER, &identifier); >> + if (ret) { >> + xdma_err(xdev, "failed to read channel id: %d", ret); >> + return ret; >> + } > Is it ok to not rollback the allocation in case of an error occurs?
In this loop, the failures are returned by read/write registers. The read/write register failure indicates serious hardware issue and the hardware may not be rollback in this situation.
> >> + >> + if (!XDMA_CHAN_CHECK_TARGET(identifier, target)) >> + continue; >> + >> + if (j == *chan_num) { >> + xdma_err(xdev, "invalid channel number"); >> + return -EIO; >> + } > Same here? > >> + >> + /* init channel structure and hardware */ >> + (*chans)[j].xdev_hdl = xdev; >> + (*chans)[j].base = base + i * XDMA_CHAN_STRIDE; >> + (*chans)[j].dir = dir; >> + >> + ret = xdma_channel_init(&(*chans)[j]); >> + if (ret) >> + return ret; > And here. > >> + (*chans)[j].vchan.desc_free = xdma_free_desc; >> + vchan_init(&(*chans)[j].vchan, &xdev->dma_dev); >> + >> + j++; >> + } >> + >> + dev_info(&xdev->pdev->dev, "configured %d %s channels", j, >> + (dir == DMA_MEM_TO_DEV) ? "H2C" : "C2H"); >> + >> + return 0; >> +} > >> +static int xdma_irq_init(struct xdma_device *xdev) >> +{ >> + u32 irq = xdev->irq_start; >> + int i, j, ret; >> + >> + /* return failure if there are not enough IRQs */ >> + if (xdev->irq_num < xdev->h2c_chan_num + xdev->c2h_chan_num) { >> + xdma_err(xdev, "not enough irq"); >> + return -EINVAL; >> + } >> + >> + /* setup H2C interrupt handler */ >> + for (i = 0; i < xdev->h2c_chan_num; i++) { >> + ret = request_irq(irq, xdma_channel_isr, 0, >> + "xdma-h2c-channel", &xdev->h2c_chans[i]); >> + if (ret) { >> + xdma_err(xdev, "H2C channel%d request irq%d failed: %d", >> + i, irq, ret); >> + for (j = 0; j < i; j++) { >> + free_irq(xdev->h2c_chans[j].irq, >> + &xdev->h2c_chans[j]); >> + } >> + return ret; > Remove freeing for loop and just do > goto failed_init_c2h; > And reverse the iteration order down on the error path (from i to zero) by > using > while (i--) Thanks. This looks much cleaner than before. :) > >> + } >> + xdev->h2c_chans[i].irq = irq; >> + irq++; >> + } >> + >> + /* setup C2H interrupt handler */ >> + for (i = 0; i < xdev->c2h_chan_num; i++) { >> + ret = request_irq(irq, xdma_channel_isr, 0, >> + "xdma-c2h-channel", &xdev->c2h_chans[i]); >> + if (ret) { >> + xdma_err(xdev, "H2C channel%d request irq%d failed: %d", >> + i, irq, ret); >> + for (j = 0; j < i; j++) { >> + free_irq(xdev->c2h_chans[j].irq, >> + &xdev->c2h_chans[j]); >> + } >> + goto failed_init_c2h; > Ditto. But use j for this whole for loop so you can just do while (j--) on > the error path as the index variable won't clash with the previous loop. > >> + } >> + xdev->c2h_chans[i].irq = irq; >> + irq++; >> + } >> + >> + /* config hardware IRQ registers */ >> + ret = xdma_set_vector_reg(xdev, XDMA_IRQ_CHAN_VEC_NUM, 0, >> + xdev->h2c_chan_num + xdev->c2h_chan_num); >> + if (ret) { >> + xdma_err(xdev, "failed to set channel vectors: %d", ret); >> + goto failed; >> + } >> + >> + /* enable interrupt */ >> + ret = xdma_enable_intr(xdev); >> + if (ret) { >> + xdma_err(xdev, "failed to enable interrupts: %d", ret); >> + goto failed; >> + } >> + >> + return 0; >> + >> +failed: >> + for (i = 0; i < xdev->c2h_chan_num; i++) > As mentioned above: > > while (j--) > >> + free_irq(xdev->c2h_chans[i].irq, &xdev->c2h_chans[i]); >> +failed_init_c2h: >> + for (i = 0; i < xdev->h2c_chan_num; i++) > while (i--) > >> + free_irq(xdev->h2c_chans[i].irq, &xdev->h2c_chans[i]); >> + >> + return ret; >> +} >> + >> +static bool xdma_filter_fn(struct dma_chan *chan, void *param) >> +{ >> + struct xdma_chan *xdma_chan = to_xdma_chan(chan); >> + struct xdma_chan_info *chan_info = param; >> + >> + if (chan_info->dir != xdma_chan->dir) >> + return false; >> + >> + return true; > return chan_info->dir == xdma_chan->dir;
Sure. I will change this.
Thanks,
Lizhi
> >
| |