Date:   Tue, 4 Oct 2022 09:23:31 -0700
From:   Lizhi Hou <lizhi.hou@....com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     <vkoul@...nel.org>, <dmaengine@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, <trix@...hat.com>,
        <tumic@...see.org>, <max.zhen@....com>, <sonal.santan@....com>,
        <larry.liu@....com>, <brian.xu@....com>
Subject: Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma
 driver


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

>
>

Powered by blists - more mailing lists