[PATCH v3 0/5] Miscellaneous xdma driver enhancements

[PATCH v3 0/5] Miscellaneous xdma driver enhancements

[Jan Kuliga]

Hi,

This patch series is pretty similar to the v1 version. I've named
it v3 because I've already sent a v2 patch as a reply to the message with
[PATCH 1/5]. The problem is, that this v2 patch is broken and
should be ignored. Sorry for that.

Jan

Changes since v1:
[PATCH 1/5]: 
Complete a terminated descriptor with dma_cookie_complete()
Don't reinitialize temporary list head in xdma_terminate_all()
           
[PATCH 4/5]:
Fix incorrect text wrapping

Changes since v2:
[PATCH 1/5]:
DO NOT schedule callback from within xdma_terminate_all()

Here's the original message:

Hi,

This patch series introduces a couple of xdma driver enhancements, such
as two dmaengine callbacks, partial rework of a interrupt service
routine and loosening of dma_pool alignment requirements. I have tested
these changes with XDMA v4.1 (Rev. 20) block.

Jan
---

Jan Kuliga (5):
  dmaengine: xilinx: xdma: Add transfer termination callbacks
  dmaengine: xilinx: xdma: Get rid of duplicated macros definitions
  dmaengine: xilinx: xdma: Complete lacking register description
  dmaengine: xilinx: xdma: Rework xdma_channel_isr()
  dmaengine: xilinx: xdma: Ease dma_pool alignment requirements

 drivers/dma/xilinx/xdma-regs.h | 24 ++++------
 drivers/dma/xilinx/xdma.c      | 82 +++++++++++++++++++++++++++++-----
 2 files changed, 80 insertions(+), 26 deletions(-)


base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
-- 
2.34.1

[PATCH v3 1/5] dmaengine: xilinx: xdma: Add transfer termination callbacks

[Jan Kuliga]

The xdma driver currently doesn't implement proper transfer termination
callbacks. Therefore, there is no way to gracefully terminate the
on-going DMA transactions. That is particularly useful for cyclic DMA
transfers. Implement these callbacks.

Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
---
 drivers/dma/xilinx/xdma-regs.h |  1 +
 drivers/dma/xilinx/xdma.c      | 60 ++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
index e641a5083e14..1f17ce165f92 100644
--- a/drivers/dma/xilinx/xdma-regs.h
+++ b/drivers/dma/xilinx/xdma-regs.h
@@ -76,6 +76,7 @@ struct xdma_hw_desc {
 #define XDMA_CHAN_CONTROL_W1S		0x8
 #define XDMA_CHAN_CONTROL_W1C		0xc
 #define XDMA_CHAN_STATUS		0x40
+#define XDMA_CHAN_STATUS_RC		0x44
 #define XDMA_CHAN_COMPLETED_DESC	0x48
 #define XDMA_CHAN_ALIGNMENTS		0x4c
 #define XDMA_CHAN_INTR_ENABLE		0x90
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 84a88029226f..b8de15e3dcfc 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -632,6 +632,64 @@ xdma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t address,
 	return NULL;
 }
 
+/**
+ * xdma_terminate_all - Halt the DMA channel
+ * @chan: DMA channel
+ */
+static int xdma_terminate_all(struct dma_chan *chan)
+{
+	int ret;
+	u32 val;
+	unsigned long flags;
+	struct xdma_chan *xchan = to_xdma_chan(chan);
+	struct xdma_device *xdev = xchan->xdev_hdl;
+	struct virt_dma_desc *vd;
+	LIST_HEAD(head);
+
+	/* Clear the RUN bit to stop the transfer */
+	ret = regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_CONTROL_W1C,
+							CHAN_CTRL_RUN_STOP);
+	if (ret)
+		return ret;
+
+	/* Clear the channel status register */
+	ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val);
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&xchan->vchan.lock, flags);
+
+	/* Don't care if there were no descriptors issued */
+	vd = vchan_next_desc(&xchan->vchan);
+	if (vd) {
+		/*
+		 * No need to delete the vd from the vc.desc_issued list,
+		 * every desc list is going to be spliced into vc.desc_terminated
+		 * and initialized anyway.
+		 */
+		dma_cookie_complete(&vd->tx);
+		vchan_terminate_vdesc(vd);
+	}
+	vchan_get_all_descriptors(&xchan->vchan, &head);
+	list_splice_tail(&head, &xchan->vchan.desc_terminated);
+
+	xchan->busy = false;
+	spin_unlock_irqrestore(&xchan->vchan.lock, flags);
+
+	return 0;
+}
+
+/**
+ * xdma_synchronize - Synchronize current execution context to the DMA channel
+ * @chan: DMA channel
+ */
+static void xdma_synchronize(struct dma_chan *chan)
+{
+	struct xdma_chan *xchan = to_xdma_chan(chan);
+
+	vchan_synchronize(&xchan->vchan);
+}
+
 /**
  * xdma_device_config - Configure the DMA channel
  * @chan: DMA channel
@@ -1093,6 +1151,8 @@ static int xdma_probe(struct platform_device *pdev)
 	xdev->dma_dev.filter.mapcnt = pdata->device_map_cnt;
 	xdev->dma_dev.filter.fn = xdma_filter_fn;
 	xdev->dma_dev.device_prep_dma_cyclic = xdma_prep_dma_cyclic;
+	xdev->dma_dev.device_terminate_all = xdma_terminate_all;
+	xdev->dma_dev.device_synchronize = xdma_synchronize;
 
 	ret = dma_async_device_register(&xdev->dma_dev);
 	if (ret) {
-- 
2.34.1

[PATCH v3 2/5] dmaengine: xilinx: xdma: Get rid of duplicated macros definitions

[Jan Kuliga]

These macros are defined earlier in the file.

Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
---
 drivers/dma/xilinx/xdma-regs.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
index 1f17ce165f92..7a169377b483 100644
--- a/drivers/dma/xilinx/xdma-regs.h
+++ b/drivers/dma/xilinx/xdma-regs.h
@@ -135,18 +135,6 @@ struct xdma_hw_desc {
 #define XDMA_SGDMA_DESC_ADJ	0x4088
 #define XDMA_SGDMA_DESC_CREDIT	0x408c
 
-/* 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			GENMASK(13, 9)
-#define XDMA_CTRL_IE_DESC_ERROR			GENMASK(23, 19)
-#define XDMA_CTRL_NON_INCR_ADDR			BIT(25)
-#define XDMA_CTRL_POLL_MODE_WB			BIT(26)
-
 /*
  * interrupt registers
  */
-- 
2.34.1

[PATCH v3 3/5] dmaengine: xilinx: xdma: Complete lacking register description

[Jan Kuliga]

Complete lacking bits, that describe the status/control register values.

Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
---
 drivers/dma/xilinx/xdma-regs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
index 7a169377b483..654c5e41112d 100644
--- a/drivers/dma/xilinx/xdma-regs.h
+++ b/drivers/dma/xilinx/xdma-regs.h
@@ -102,6 +102,7 @@ struct xdma_hw_desc {
 #define CHAN_CTRL_IE_MAGIC_STOPPED		BIT(4)
 #define CHAN_CTRL_IE_IDLE_STOPPED		BIT(6)
 #define CHAN_CTRL_IE_READ_ERROR			GENMASK(13, 9)
+#define CHAN_CTRL_IE_WRITE_ERROR		GENMASK(18, 14)
 #define CHAN_CTRL_IE_DESC_ERROR			GENMASK(23, 19)
 #define CHAN_CTRL_NON_INCR_ADDR			BIT(25)
 #define CHAN_CTRL_POLL_MODE_WB			BIT(26)
@@ -112,6 +113,7 @@ struct xdma_hw_desc {
 			 CHAN_CTRL_IE_DESC_ALIGN_MISMATCH |		\
 			 CHAN_CTRL_IE_MAGIC_STOPPED |			\
 			 CHAN_CTRL_IE_READ_ERROR |			\
+			 CHAN_CTRL_IE_WRITE_ERROR |			\
 			 CHAN_CTRL_IE_DESC_ERROR)
 
 /* bits of the channel interrupt enable mask */
-- 
2.34.1

[PATCH v3 4/5] dmaengine: xilinx: xdma: Rework xdma_channel_isr()

[Jan Kuliga]

The xdma's channel status register may be read and cleared
simultaneously, by accesing it via seperate XDMA_CHAN_STATUS_RC
register. Therefore, it is possible to simplify the code
by just getting rid of a seperate redundant write.

Also, implement the actual status register checking as well.
Previously, the cyclic callback would be scheduled regardless of the
status register state. Fix it.

Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
---
 drivers/dma/xilinx/xdma-regs.h |  2 ++
 drivers/dma/xilinx/xdma.c      | 16 ++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
index 654c5e41112d..6bf7ae84e452 100644
--- a/drivers/dma/xilinx/xdma-regs.h
+++ b/drivers/dma/xilinx/xdma-regs.h
@@ -116,6 +116,8 @@ struct xdma_hw_desc {
 			 CHAN_CTRL_IE_WRITE_ERROR |			\
 			 CHAN_CTRL_IE_DESC_ERROR)
 
+#define XDMA_CHAN_STATUS_MASK CHAN_CTRL_START
+
 /* bits of the channel interrupt enable mask */
 #define CHAN_IM_DESC_ERROR			BIT(19)
 #define CHAN_IM_READ_ERROR			BIT(9)
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index b8de15e3dcfc..de4615bd4ee5 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -814,15 +814,15 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
 
 	desc->completed_desc_num += complete_desc_num;
 
-	if (desc->cyclic) {
-		ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS,
-				  &st);
-		if (ret)
-			goto out;
-
-		regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_STATUS, st);
+	/* clear-on-read the status register */
+	ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &st);
+	if (ret)
+		goto out;
 
-		vchan_cyclic_callback(vd);
+	if (desc->cyclic) {
+		st &= XDMA_CHAN_STATUS_MASK;
+		if (st & CHAN_CTRL_IE_DESC_COMPLETED)
+			vchan_cyclic_callback(vd);
 		goto out;
 	}
 
-- 
2.34.1

[PATCH v3 5/5] dmaengine: xilinx: xdma: Ease dma_pool alignment requirements

[Jan Kuliga]

According to the XDMA datasheet (PG195), the address of any descriptor
must be 32 byte aligned. The datasheet also states that a contiguous
block of descriptors must not cross a 4k address boundary. Therefore,
it is possible to ease the pressure put on the dma_pool allocator
just by requiring sufficient alignment and boundary values. Add proper
macro definition and change the values passed into the
dma_pool_create().

Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
---
 drivers/dma/xilinx/xdma-regs.h | 7 ++++---
 drivers/dma/xilinx/xdma.c      | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
index 6bf7ae84e452..d5cb12e6b8d4 100644
--- a/drivers/dma/xilinx/xdma-regs.h
+++ b/drivers/dma/xilinx/xdma-regs.h
@@ -64,9 +64,10 @@ struct xdma_hw_desc {
 	__le64		next_desc;
 };
 
-#define XDMA_DESC_SIZE		sizeof(struct xdma_hw_desc)
-#define XDMA_DESC_BLOCK_SIZE	(XDMA_DESC_SIZE * XDMA_DESC_ADJACENT)
-#define XDMA_DESC_BLOCK_ALIGN	4096
+#define XDMA_DESC_SIZE			sizeof(struct xdma_hw_desc)
+#define XDMA_DESC_BLOCK_SIZE		(XDMA_DESC_SIZE * XDMA_DESC_ADJACENT)
+#define XDMA_DESC_BLOCK_ALIGN		32
+#define XDMA_DESC_BLOCK_BOUNDARY	4096
 
 /*
  * Channel registers
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index de4615bd4ee5..d32ae93e18b6 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -735,9 +735,9 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
 		return -EINVAL;
 	}
 
-	xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
-					       dev, XDMA_DESC_BLOCK_SIZE,
-					       XDMA_DESC_BLOCK_ALIGN, 0);
+	xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
+				XDMA_DESC_BLOCK_SIZE, XDMA_DESC_BLOCK_ALIGN,
+						XDMA_DESC_BLOCK_BOUNDARY);
 	if (!xdma_chan->desc_pool) {
 		xdma_err(xdev, "unable to allocate descriptor pool");
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH v3 5/5] dmaengine: xilinx: xdma: Ease dma_pool alignment requirements

[Lizhi Hou]

On 11/24/23 11:25, Jan Kuliga wrote:
> According to the XDMA datasheet (PG195), the address of any descriptor
> must be 32 byte aligned. The datasheet also states that a contiguous
> block of descriptors must not cross a 4k address boundary. Therefore,
> it is possible to ease the pressure put on the dma_pool allocator
> just by requiring sufficient alignment and boundary values. Add proper
> macro definition and change the values passed into the
> dma_pool_create().
>
> Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
> ---
>   drivers/dma/xilinx/xdma-regs.h | 7 ++++---
>   drivers/dma/xilinx/xdma.c      | 6 +++---
>   2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
> index 6bf7ae84e452..d5cb12e6b8d4 100644
> --- a/drivers/dma/xilinx/xdma-regs.h
> +++ b/drivers/dma/xilinx/xdma-regs.h
> @@ -64,9 +64,10 @@ struct xdma_hw_desc {
>   	__le64		next_desc;
>   };
>   
> -#define XDMA_DESC_SIZE		sizeof(struct xdma_hw_desc)
> -#define XDMA_DESC_BLOCK_SIZE	(XDMA_DESC_SIZE * XDMA_DESC_ADJACENT)
> -#define XDMA_DESC_BLOCK_ALIGN	4096
> +#define XDMA_DESC_SIZE			sizeof(struct xdma_hw_desc)
> +#define XDMA_DESC_BLOCK_SIZE		(XDMA_DESC_SIZE * XDMA_DESC_ADJACENT)
> +#define XDMA_DESC_BLOCK_ALIGN		32
> +#define XDMA_DESC_BLOCK_BOUNDARY	4096
>   
>   /*
>    * Channel registers
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index de4615bd4ee5..d32ae93e18b6 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -735,9 +735,9 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
>   		return -EINVAL;
>   	}
>   
> -	xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
> -					       dev, XDMA_DESC_BLOCK_SIZE,
> -					       XDMA_DESC_BLOCK_ALIGN, 0);
> +	xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
> +				XDMA_DESC_BLOCK_SIZE, XDMA_DESC_BLOCK_ALIGN,
> +						XDMA_DESC_BLOCK_BOUNDARY);
>   	if (!xdma_chan->desc_pool) {
>   		xdma_err(xdev, "unable to allocate descriptor pool");
>   		return -ENOMEM;

This is probably not needed. The 32 adjacent descriptors here is 1024 
bytes. Defining 4k alignment should be good enough.

Thanks,

Lizhi

Re: [PATCH v3 5/5] dmaengine: xilinx: xdma: Ease dma_pool alignment requirements

[Lizhi Hou]

On 11/27/23 08:43, Lizhi Hou wrote:
>
> On 11/24/23 11:25, Jan Kuliga wrote:
>> According to the XDMA datasheet (PG195), the address of any descriptor
>> must be 32 byte aligned. The datasheet also states that a contiguous
>> block of descriptors must not cross a 4k address boundary. Therefore,
>> it is possible to ease the pressure put on the dma_pool allocator
>> just by requiring sufficient alignment and boundary values. Add proper
>> macro definition and change the values passed into the
>> dma_pool_create().
>>
>> Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl>
>> ---
>>   drivers/dma/xilinx/xdma-regs.h | 7 ++++---
>>   drivers/dma/xilinx/xdma.c      | 6 +++---
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xdma-regs.h 
>> b/drivers/dma/xilinx/xdma-regs.h
>> index 6bf7ae84e452..d5cb12e6b8d4 100644
>> --- a/drivers/dma/xilinx/xdma-regs.h
>> +++ b/drivers/dma/xilinx/xdma-regs.h
>> @@ -64,9 +64,10 @@ struct xdma_hw_desc {
>>       __le64        next_desc;
>>   };
>>   -#define XDMA_DESC_SIZE        sizeof(struct xdma_hw_desc)
>> -#define XDMA_DESC_BLOCK_SIZE    (XDMA_DESC_SIZE * XDMA_DESC_ADJACENT)
>> -#define XDMA_DESC_BLOCK_ALIGN    4096
>> +#define XDMA_DESC_SIZE            sizeof(struct xdma_hw_desc)
>> +#define XDMA_DESC_BLOCK_SIZE        (XDMA_DESC_SIZE * 
>> XDMA_DESC_ADJACENT)
>> +#define XDMA_DESC_BLOCK_ALIGN        32
>> +#define XDMA_DESC_BLOCK_BOUNDARY    4096
>>     /*
>>    * Channel registers
>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>> index de4615bd4ee5..d32ae93e18b6 100644
>> --- a/drivers/dma/xilinx/xdma.c
>> +++ b/drivers/dma/xilinx/xdma.c
>> @@ -735,9 +735,9 @@ static int xdma_alloc_chan_resources(struct 
>> dma_chan *chan)
>>           return -EINVAL;
>>       }
>>   -    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
>> -                           dev, XDMA_DESC_BLOCK_SIZE,
>> -                           XDMA_DESC_BLOCK_ALIGN, 0);
>> +    xdma_chan->desc_pool = dma_pool_create(dma_chan_name(chan), dev,
>> +                XDMA_DESC_BLOCK_SIZE, XDMA_DESC_BLOCK_ALIGN,
>> +                        XDMA_DESC_BLOCK_BOUNDARY);
>>       if (!xdma_chan->desc_pool) {
>>           xdma_err(xdev, "unable to allocate descriptor pool");
>>           return -ENOMEM;
>
> This is probably not needed. The 32 adjacent descriptors here is 1024 
> bytes. Defining 4k alignment should be good enough.
Oh, Just noticed the you have changed the alignment to 32. Sorry, I just 
hit send too quick.
>
> Thanks,
>
> Lizhi
>
>