Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver

From: Lizhi Hou
Date: Tue Oct 04 2022 - 12:23:44 EST

Next message: Geert Uytterhoeven: "[PATCH] net: pse-pd: PSE_REGULATOR should depend on REGULATOR"
Previous message: Geert Uytterhoeven: "[PATCH] irqchip: IMX_MU_MSI should depend on ARCH_MXC"
In reply to: Ilpo Järvinen: "Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver"
Next in thread: Ilpo Järvinen: "Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx>
Signed-off-by: Sonal Santan <sonal.santan@xxxxxxx>
Signed-off-by: Max Zhen <max.zhen@xxxxxxx>
Signed-off-by: Brian Xu <brian.xu@xxxxxxx>
---
+/* 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

Next message: Geert Uytterhoeven: "[PATCH] net: pse-pd: PSE_REGULATOR should depend on REGULATOR"
Previous message: Geert Uytterhoeven: "[PATCH] irqchip: IMX_MU_MSI should depend on ARCH_MXC"
In reply to: Ilpo Järvinen: "Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver"
Next in thread: Ilpo Järvinen: "Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]