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...
Cheers
Vladimir
>
> alex> 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
>>> Vladimir
>>>
>>>>
>>>> Robin.
>>>
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
