[PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Jon Medhurst]

This is a couple of patches I produced when trying to get dmatest to run
with the pl330 driver. The second one is actually a theoretical fix
because the FIFO on the platform I was using is too large for the tests
to hit the limit, but the datasheet says the device can lockup if all
the FIFO entries are exhausted, so it seems like a sensible precaution.

Jon Medhurst (2):
      dma: pl330: Align DMA memcpy operations to MFIFO width
      dma: pl330: Limit MFIFO usage for memcpy to avoid exhausting entries

 drivers/dma/pl330.c | 16 ++++++++++++++--

[PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Jon Medhurst]

This is a couple of patches I produced when trying to get dmatest to run
with the pl330 driver. The second one is actually a theoretical fix
because the FIFO on the platform I was using is too large for the tests
to hit the limit, but the datasheet says the device can lockup if all
the FIFO entries are exhausted, so it seems like a sensible precaution.

Jon Medhurst (2):
      dma: pl330: Align DMA memcpy operations to MFIFO width
      dma: pl330: Limit MFIFO usage for memcpy to avoid exhausting entries

 drivers/dma/pl330.c | 16 ++++++++++++++--

[PATCH 1/2] dma: pl330: Align DMA memcpy operations to MFIFO width

[Jon Medhurst]

The algorithm used for programming the DMA Controller doesn't take into
consideration the requirements of transfers that are not aligned to the
bus width. This failure may result in DMA transferring one too few MFIFO
entries (so too few bytes are copied) or the DMA trying to write one too
many MFIFO entries and hanging because this is never provided.

See "MFIFO Usage Overview" chapter in the the TRM for "CoreLink DMA
Controller DMA-330", Revision r1p1.

We work around these shortcomings by making sure we pick a burst size
and length which ensures no bursts straddle an MFIFO entry.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 drivers/dma/pl330.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa..8f869ec 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2459,8 +2459,13 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	/* Select max possible burst size */
 	burst = pl330->pcfg.data_bus_width / 8;
 
+	/*
+	 * Make sure we use a burst size that aligns with all the memcpy
+	 * parameters because our DMA programming algorithm doesn't cope with
+	 * transfers which straddle an entry in the DMA device's MFIFO.
+	 */
 	while (burst > 1) {
-		if (!(len % burst))
+		if (!((src | dst | len) % burst))
 			break;
 		burst /= 2;
 	}
@@ -2469,6 +2474,13 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	while (burst != (1 << desc->rqcfg.brst_size))
 		desc->rqcfg.brst_size++;
 
+	/*
+	 * If burst size is smaller than bus width then make sure we only
+	 * transfer one at a time to avoid a burst stradling an MFIFO entry.
+	 */
+	if (desc->rqcfg.brst_size * 8 < pl330->pcfg.data_bus_width)
+		desc->rqcfg.brst_len = 1;
+
 	desc->rqcfg.brst_len = get_burst_len(desc, len);
 
 	desc->txd.flags = flags;
-- 
2.1.1

[PATCH 1/2] dma: pl330: Align DMA memcpy operations to MFIFO width

[Jon Medhurst]

The algorithm used for programming the DMA Controller doesn't take into
consideration the requirements of transfers that are not aligned to the
bus width. This failure may result in DMA transferring one too few MFIFO
entries (so too few bytes are copied) or the DMA trying to write one too
many MFIFO entries and hanging because this is never provided.

See "MFIFO Usage Overview" chapter in the the TRM for "CoreLink DMA
Controller DMA-330", Revision r1p1.

We work around these shortcomings by making sure we pick a burst size
and length which ensures no bursts straddle an MFIFO entry.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 drivers/dma/pl330.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa..8f869ec 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2459,8 +2459,13 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	/* Select max possible burst size */
 	burst = pl330->pcfg.data_bus_width / 8;
 
+	/*
+	 * Make sure we use a burst size that aligns with all the memcpy
+	 * parameters because our DMA programming algorithm doesn't cope with
+	 * transfers which straddle an entry in the DMA device's MFIFO.
+	 */
 	while (burst > 1) {
-		if (!(len % burst))
+		if (!((src | dst | len) % burst))
 			break;
 		burst /= 2;
 	}
@@ -2469,6 +2474,13 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	while (burst != (1 << desc->rqcfg.brst_size))
 		desc->rqcfg.brst_size++;
 
+	/*
+	 * If burst size is smaller than bus width then make sure we only
+	 * transfer one at a time to avoid a burst stradling an MFIFO entry.
+	 */
+	if (desc->rqcfg.brst_size * 8 < pl330->pcfg.data_bus_width)
+		desc->rqcfg.brst_len = 1;
+
 	desc->rqcfg.brst_len = get_burst_len(desc, len);
 
 	desc->txd.flags = flags;
-- 
2.1.1

[PATCH 2/2] dma: pl330: Limit MFIFO usage for memcpy to avoid exhausting entries

[Jon Medhurst]

The MFIFO is shared by all channels so restrict each memcpy to it's fair
share. This is being over cautious, but without a global view of DMA
channel usage on a system it's not possible to come up with a more
optimum safe limit.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 8f869ec..849439a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2336,7 +2336,7 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	int burst_len;
 
 	burst_len = pl330->pcfg.data_bus_width / 8;
-	burst_len *= pl330->pcfg.data_buf_dep;
+	burst_len *= pl330->pcfg.data_buf_dep / pl330->pcfg.num_chan;
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-- 
2.1.1

[PATCH 2/2] dma: pl330: Limit MFIFO usage for memcpy to avoid exhausting entries

[Jon Medhurst]

The MFIFO is shared by all channels so restrict each memcpy to it's fair
share. This is being over cautious, but without a global view of DMA
channel usage on a system it's not possible to come up with a more
optimum safe limit.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 8f869ec..849439a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2336,7 +2336,7 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	int burst_len;
 
 	burst_len = pl330->pcfg.data_bus_width / 8;
-	burst_len *= pl330->pcfg.data_buf_dep;
+	burst_len *= pl330->pcfg.data_buf_dep / pl330->pcfg.num_chan;
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-- 
2.1.1

[PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Vinod Koul]

On Fri, Nov 07, 2014 at 06:05:16PM +0000, Jon Medhurst wrote:
> This is a couple of patches I produced when trying to get dmatest to run
> with the pl330 driver. The second one is actually a theoretical fix
> because the FIFO on the platform I was using is too large for the tests
> to hit the limit, but the datasheet says the device can lockup if all
> the FIFO entries are exhausted, so it seems like a sensible precaution.
>
Applied, thanks

Please ensure you use the right subsystem name for the patch header

-- 
~Vinod

Re: [PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Vinod Koul]

On Fri, Nov 07, 2014 at 06:05:16PM +0000, Jon Medhurst wrote:
> This is a couple of patches I produced when trying to get dmatest to run
> with the pl330 driver. The second one is actually a theoretical fix
> because the FIFO on the platform I was using is too large for the tests
> to hit the limit, but the datasheet says the device can lockup if all
> the FIFO entries are exhausted, so it seems like a sensible precaution.
>
Applied, thanks

Please ensure you use the right subsystem name for the patch header

-- 
~Vinod

[PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Jon Medhurst (Tixy)]

On Wed, 2014-11-12 at 15:01 +0530, Vinod Koul wrote:
> On Fri, Nov 07, 2014 at 06:05:16PM +0000, Jon Medhurst wrote:
> > This is a couple of patches I produced when trying to get dmatest to run
> > with the pl330 driver. The second one is actually a theoretical fix
> > because the FIFO on the platform I was using is too large for the tests
> > to hit the limit, but the datasheet says the device can lockup if all
> > the FIFO entries are exhausted, so it seems like a sensible precaution.
> >
> Applied, thanks
>
> Please ensure you use the right subsystem name for the patch header

I originally created the patch when working on Linux 3.14 and most
previous updates to the file used 'dma', I see now in recent versions
that the preferred name is 'dmaengine' (and matches the mailing list
name) so will use that in future.

BTW, though I was working on 3.14 I have retested the patches on a
3.18-rc

-- 
Tixy

Re: [PATCH 0/2] dma: pl330: Fixes for DMA memcpy

[Jon Medhurst (Tixy)]

On Wed, 2014-11-12 at 15:01 +0530, Vinod Koul wrote:
> On Fri, Nov 07, 2014 at 06:05:16PM +0000, Jon Medhurst wrote:
> > This is a couple of patches I produced when trying to get dmatest to run
> > with the pl330 driver. The second one is actually a theoretical fix
> > because the FIFO on the platform I was using is too large for the tests
> > to hit the limit, but the datasheet says the device can lockup if all
> > the FIFO entries are exhausted, so it seems like a sensible precaution.
> >
> Applied, thanks
>
> Please ensure you use the right subsystem name for the patch header

I originally created the patch when working on Linux 3.14 and most
previous updates to the file used 'dma', I see now in recent versions
that the preferred name is 'dmaengine' (and matches the mailing list
name) so will use that in future.

BTW, though I was working on 3.14 I have retested the patches on a
3.18-rc

-- 
Tixy