On 10/17/19 12:16 PM, Vladimir Murzin wrote:
On 10/17/19 11:03 AM, Alexandre Torgue wrote:Hi Vlad On 10/17/19 11:46 AM, Vladimir Murzin wrote:On 10/14/19 4:01 PM, Vladimir Murzin wrote:On 10/14/19 2:54 PM, Robin Murphy wrote:On 13/10/2019 15:28, Daniele Alessandrelli wrote:Hi, It looks like dma_alloc_coherent() is setting the dma_handle output parameter to the memory physical address and not the device bus address when the device is using reserved memory regions for DMA allocation. This is despite using 'dma_ranges' in the device tree to describe the DMA memory mapping. Is this expected behavior or a bug?That does sound like a bug :(Here is a reduced version of the device tree I'm using: \ { reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; mydev_rsvd: rsvd_mem@494800000 { compatible = "shared-dma-pool"; reg = <0x4 0x94800000 0x0 0x200000>; no-map; }; }; soc { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ranges; dma_ranges; mybus { ranges = <>; dma-ranges = <>; compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ranges = <0x0 0x0 0x0 0x0 0x0 0x80000000>; dma-ranges = <0x0 0x80000000 0x4 0x80000000 0x0 0x80000000>; mydevice { compatible = "my-compatible-string"; memory-region = <&mydev_rsvd>; } } } }; It looks like this issue was previously fixed by commit c41f9ea998f3 ("drivers: dma-coherent: Account dma_pfn_offset when used with device tree") which introduced a new function ('dma_get_device_base()') to return the reserved memory address as seen by the device. However, such a function, even if still there, is not used anymore in latest code (as of v5.4-rc2). Was that done for a specific reason? Or is it just a mistake?Hmm, it looks like 43fc509c3efb ("dma-coherent: introduce interface for default DMA pool") removed the caller of dma_get_device_base() in the alloc path shortly after it was introduced, which certainly appears as if it may have been unintentional - Vladimir?I do not remember it was intentional. Looking at history, default DMA pool was a response to another report. However, now I'm wondering why it was not caught by STM32 - most of that work was required to support "dma-ranges" with NOMMU+caches (Cortex-M7). Alex (or anybody else from ST), maybe you have some input?Seem they do not care :)Concerning, the default dma pool: - It was introduced because dma on cortexM7 couldn't use cache-able memory region. So we configured MPU in bootloader (I think it can be done now by Kernel) a small "no cache" memory region and then declare this same region in kernel device tree for the dma pool. Without that we can't execute any dma trasferts. Sorry for the late answer :(Still better then no answer ;) Yes, I remember why we have default DMA pool. IIRC, STM used dma-ranges, it is how c41f9ea998f3 ("drivers: dma-coherent: Account dma_pfn_offset when used with device tree") was discovered. It also means that STM is broken after 878ec36 (ARM: NOMMU: Wire-up default DMA interface) with combination of dma-ranges and default DMA pool. Hoverer, I've never seen report from STM, so either I'm missing something or such combination is not in use nowdays...
IIRC, I didn't added this support by default in device tree because I got some troubles with XIP setup (it would be nice that I check the status with a latest kernel). SO I don't verify this configuration when I execute my no-regression tests for pull STM32 pull request.
Nevertheless, I think I tested it since kernel 4.13 without issue but I didn't use "dma-ranges" property.
Cheers Vladimiralex> I'm wondering if I've missed something with diff bellow (it was a long time ago when I touched DMA)?diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c index db92478..287ef89 100644 --- a/arch/arm/mm/dma-mapping-nommu.c +++ b/arch/arm/mm/dma-mapping-nommu.c @@ -35,7 +35,7 @@ static void *arm_nommu_dma_alloc(struct device *dev, size_t size, unsigned long attrs) { - void *ret = dma_alloc_from_global_coherent(size, dma_handle); + void *ret = dma_alloc_from_global_coherent(dev, size, dma_handle); /* * dma_alloc_from_global_coherent() may fail because: diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 4a1c4fc..10918c5 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -162,7 +162,7 @@ int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr); int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, size_t size, int *ret); -void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle); +void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle); int dma_release_from_global_coherent(int order, void *vaddr); int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, size_t size, int *ret); @@ -172,7 +172,7 @@ int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, #define dma_release_from_dev_coherent(dev, order, vaddr) (0) #define dma_mmap_from_dev_coherent(dev, vma, vaddr, order, ret) (0) -static inline void *dma_alloc_from_global_coherent(ssize_t size, +static inline void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle) { return NULL; diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c index 545e386..551b0eb 100644 --- a/kernel/dma/coherent.c +++ b/kernel/dma/coherent.c @@ -123,8 +123,9 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, return ret; } -static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem, - ssize_t size, dma_addr_t *dma_handle) +static void *__dma_alloc_from_coherent(struct device *dev, + struct dma_coherent_mem *mem, + ssize_t size, dma_addr_t *dma_handle) { int order = get_order(size); unsigned long flags; @@ -143,7 +144,7 @@ static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem, /* * Memory was found in the coherent area. */ - *dma_handle = mem->device_base + (pageno << PAGE_SHIFT); + *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT); ret = mem->virt_base + (pageno << PAGE_SHIFT); spin_unlock_irqrestore(&mem->spinlock, flags); memset(ret, 0, size); @@ -175,17 +176,18 @@ int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size, if (!mem) return 0; - *ret = __dma_alloc_from_coherent(mem, size, dma_handle); + *ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle); return 1; } -void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle) +void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, + dma_addr_t *dma_handle) { if (!dma_coherent_default_memory) return NULL; - return __dma_alloc_from_coherent(dma_coherent_default_memory, size, - dma_handle); + return __dma_alloc_from_coherent(dev, dma_coherent_default_memory, size, + dma_handle); } static int __dma_release_from_coherent(struct dma_coherent_mem *mem,Cheers VladimirRobin.
_______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
