[PATCH 1/3] dma: Use dma-mapping for cache ops and sync after write

The DMA'd memory area needs cleaned and invalidated after the DMA write so that any stale cache lines do not mask new data.
Signed-off-by: Andrew Davis afd@ti.com --- drivers/dma/dma-uclass.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/dma-uclass.c b/drivers/dma/dma-uclass.c index 012609bb53..70f06f1f09 100644 --- a/drivers/dma/dma-uclass.c +++ b/drivers/dma/dma-uclass.c @@ -19,6 +19,7 @@ #include <asm/cache.h> #include <dm/read.h> #include <dma-uclass.h> +#include <linux/dma-mapping.h> #include <dt-structs.h> #include <errno.h>
@@ -235,6 +236,8 @@ int dma_memcpy(void *dst, void *src, size_t len) { struct udevice *dev; const struct dma_ops *ops; + dma_addr_t destination; + dma_addr_t source; int ret;
ret = dma_get_device(DMA_SUPPORTS_MEM_TO_MEM, &dev); @@ -245,11 +248,17 @@ int dma_memcpy(void *dst, void *src, size_t len) if (!ops->transfer) return -ENOSYS;
- /* Invalidate the area, so no writeback into the RAM races with DMA */ - invalidate_dcache_range((unsigned long)dst, (unsigned long)dst + - roundup(len, ARCH_DMA_MINALIGN)); + /* Clean the areas, so no writeback into the RAM races with DMA */ + destination = dma_map_single(dst, len, DMA_FROM_DEVICE); + source = dma_map_single(src, len, DMA_TO_DEVICE);
- return ops->transfer(dev, DMA_MEM_TO_MEM, dst, src, len); + ret = ops->transfer(dev, DMA_MEM_TO_MEM, dst, src, len); + + /* Clean+Invalidate the areas after, so we can see DMA'd data */ + dma_unmap_single(destination, len, DMA_FROM_DEVICE); + dma_unmap_single(source, len, DMA_TO_DEVICE); + + return ret; }
UCLASS_DRIVER(dma) = {

We should clean the caches before any DMA operation and clean+invalidate after. This matches what the DMA framework does for us already but adds it to the two functions here in this driver that don't yet go through the new DMA framework.
Signed-off-by: Andrew Davis afd@ti.com --- drivers/dma/ti-edma3.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/dma/ti-edma3.c b/drivers/dma/ti-edma3.c index ec3dc62d2f..53dc4e8ce5 100644 --- a/drivers/dma/ti-edma3.c +++ b/drivers/dma/ti-edma3.c @@ -13,6 +13,7 @@ #include <common.h> #include <dm.h> #include <dma-uclass.h> +#include <linux/dma-mapping.h> #include <asm/omap_common.h> #include <asm/ti-common/ti-edma3.h>
@@ -487,8 +488,10 @@ void __edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, { int xfer_len; int max_xfer = EDMA_FILL_BUFFER_SIZE * 65535; + dma_addr_t source;
memset((void *)edma_fill_buffer, val, sizeof(edma_fill_buffer)); + source = dma_map_single(edma_fill_buffer, len, DMA_TO_DEVICE);
while (len) { xfer_len = len; @@ -501,6 +504,8 @@ void __edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, len -= xfer_len; dst += xfer_len; } + + dma_unmap_single(source, len, DMA_FROM_DEVICE); }
#ifndef CONFIG_DMA @@ -508,13 +513,27 @@ void __edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, void edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num, void *dst, void *src, size_t len) { + /* Clean the areas, so no writeback into the RAM races with DMA */ + dma_addr_t destination = dma_map_single(dst, len, DMA_FROM_DEVICE); + dma_addr_t source = dma_map_single(src, len, DMA_TO_DEVICE); + __edma3_transfer(edma3_base_addr, edma_slot_num, dst, src, len, len); + + /* Clean+Invalidate the areas after, so we can see DMA'd data */ + dma_unmap_single(destination, len, DMA_FROM_DEVICE); + dma_unmap_single(source, len, DMA_TO_DEVICE); }
void edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, void *dst, u8 val, size_t len) { + /* Clean the area, so no writeback into the RAM races with DMA */ + dma_addr_t destination = dma_map_single(dst, len, DMA_FROM_DEVICE); + __edma3_fill(edma3_base_addr, edma_slot_num, dst, val, len); + + /* Clean+Invalidate the area after, so we can see DMA'd data */ + dma_unmap_single(destination, len, DMA_FROM_DEVICE); }
#else

On Fri, Oct 07, 2022 at 12:11:12PM -0500, Andrew Davis wrote:
We should clean the caches before any DMA operation and clean+invalidate after. This matches what the DMA framework does for us already but adds it to the two functions here in this driver that don't yet go through the new DMA framework.
Signed-off-by: Andrew Davis afd@ti.com
Applied to u-boot/master, thanks!

DMA operations should function on DMA addresses, not virtual addresses. Although these are usually the same in U-Boot, it is more correct to be explicit with our types here.
Signed-off-by: Andrew Davis afd@ti.com --- drivers/dma/dma-uclass.c | 2 +- drivers/dma/sandbox-dma-test.c | 4 ++-- drivers/dma/ti-edma3.c | 14 +++++++------- drivers/dma/ti/k3-udma.c | 4 ++-- include/dma-uclass.h | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dma-uclass.c b/drivers/dma/dma-uclass.c index 70f06f1f09..81dbb4da10 100644 --- a/drivers/dma/dma-uclass.c +++ b/drivers/dma/dma-uclass.c @@ -252,7 +252,7 @@ int dma_memcpy(void *dst, void *src, size_t len) destination = dma_map_single(dst, len, DMA_FROM_DEVICE); source = dma_map_single(src, len, DMA_TO_DEVICE);
- ret = ops->transfer(dev, DMA_MEM_TO_MEM, dst, src, len); + ret = ops->transfer(dev, DMA_MEM_TO_MEM, destination, source, len);
/* Clean+Invalidate the areas after, so we can see DMA'd data */ dma_unmap_single(destination, len, DMA_FROM_DEVICE); diff --git a/drivers/dma/sandbox-dma-test.c b/drivers/dma/sandbox-dma-test.c index aebf3eef96..2b8259a35b 100644 --- a/drivers/dma/sandbox-dma-test.c +++ b/drivers/dma/sandbox-dma-test.c @@ -39,9 +39,9 @@ struct sandbox_dma_dev { };
static int sandbox_dma_transfer(struct udevice *dev, int direction, - void *dst, void *src, size_t len) + dma_addr_t dst, dma_addr_t src, size_t len) { - memcpy(dst, src, len); + memcpy((void *)dst, (void *)src, len);
return 0; } diff --git a/drivers/dma/ti-edma3.c b/drivers/dma/ti-edma3.c index 53dc4e8ce5..1ad3b92dbf 100644 --- a/drivers/dma/ti-edma3.c +++ b/drivers/dma/ti-edma3.c @@ -396,7 +396,7 @@ void qedma3_stop(u32 base, struct edma3_channel_config *cfg) }
void __edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num, - void *dst, void *src, size_t len, size_t s_len) + dma_addr_t dst, dma_addr_t src, size_t len, size_t s_len) { struct edma3_slot_config slot; struct edma3_channel_config edma_channel; @@ -484,7 +484,7 @@ void __edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num, }
void __edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, - void *dst, u8 val, size_t len) + dma_addr_t dst, u8 val, size_t len) { int xfer_len; int max_xfer = EDMA_FILL_BUFFER_SIZE * 65535; @@ -499,7 +499,7 @@ void __edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, xfer_len = max_xfer;
__edma3_transfer(edma3_base_addr, edma_slot_num, dst, - edma_fill_buffer, xfer_len, + source, xfer_len, EDMA_FILL_BUFFER_SIZE); len -= xfer_len; dst += xfer_len; @@ -517,7 +517,7 @@ void edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num, dma_addr_t destination = dma_map_single(dst, len, DMA_FROM_DEVICE); dma_addr_t source = dma_map_single(src, len, DMA_TO_DEVICE);
- __edma3_transfer(edma3_base_addr, edma_slot_num, dst, src, len, len); + __edma3_transfer(edma3_base_addr, edma_slot_num, destination, source, len, len);
/* Clean+Invalidate the areas after, so we can see DMA'd data */ dma_unmap_single(destination, len, DMA_FROM_DEVICE); @@ -530,7 +530,7 @@ void edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num, /* Clean the area, so no writeback into the RAM races with DMA */ dma_addr_t destination = dma_map_single(dst, len, DMA_FROM_DEVICE);
- __edma3_fill(edma3_base_addr, edma_slot_num, dst, val, len); + __edma3_fill(edma3_base_addr, edma_slot_num, destination, val, len);
/* Clean+Invalidate the area after, so we can see DMA'd data */ dma_unmap_single(destination, len, DMA_FROM_DEVICE); @@ -538,8 +538,8 @@ void edma3_fill(unsigned long edma3_base_addr, unsigned int edma_slot_num,
#else
-static int ti_edma3_transfer(struct udevice *dev, int direction, void *dst, - void *src, size_t len) +static int ti_edma3_transfer(struct udevice *dev, int direction, + dma_addr_t dst, dma_addr_t src, size_t len) { struct ti_edma3_priv *priv = dev_get_priv(dev);
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c index 86603d43f1..ab6639b4a1 100644 --- a/drivers/dma/ti/k3-udma.c +++ b/drivers/dma/ti/k3-udma.c @@ -2304,7 +2304,7 @@ err_res_free: }
static int udma_transfer(struct udevice *dev, int direction, - void *dst, void *src, size_t len) + dma_addr_t dst, dma_addr_t src, size_t len) { struct udma_dev *ud = dev_get_priv(dev); /* Channel0 is reserved for memcpy */ @@ -2325,7 +2325,7 @@ static int udma_transfer(struct udevice *dev, int direction, if (ret) return ret;
- udma_prep_dma_memcpy(uc, (dma_addr_t)dst, (dma_addr_t)src, len); + udma_prep_dma_memcpy(uc, dst, src, len); udma_start(uc); udma_poll_completion(uc, &paddr); udma_stop(uc); diff --git a/include/dma-uclass.h b/include/dma-uclass.h index 340437acc1..ea721baae6 100644 --- a/include/dma-uclass.h +++ b/include/dma-uclass.h @@ -132,8 +132,8 @@ struct dma_ops { * @len: Length of the data to be copied (number of bytes). * @return zero on success, or -ve error code. */ - int (*transfer)(struct udevice *dev, int direction, void *dst, - void *src, size_t len); + int (*transfer)(struct udevice *dev, int direction, dma_addr_t dst, + dma_addr_t src, size_t len); };
#endif /* _DMA_UCLASS_H */

On Fri, Oct 07, 2022 at 12:11:13PM -0500, Andrew Davis wrote:
DMA operations should function on DMA addresses, not virtual addresses. Although these are usually the same in U-Boot, it is more correct to be explicit with our types here.
Signed-off-by: Andrew Davis afd@ti.com
Applied to u-boot/master, thanks!

On Fri, Oct 07, 2022 at 12:11:11PM -0500, Andrew Davis wrote:
The DMA'd memory area needs cleaned and invalidated after the DMA write so that any stale cache lines do not mask new data.
Signed-off-by: Andrew Davis afd@ti.com
Applied to u-boot/master, thanks!
participants (2)
-
Andrew Davis
-
Tom Rini