Re: [PATCH 3/5] kvx: Implement dma handling primitives

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Yann,

On 03.03.21 10:33, Yann Sionneau wrote:
> On 03/03/2021 10:14, Lucas Stach wrote:
>>> Right now we aren't using any driver that require dma_alloc_coherent,
>>> but we use drivers that requires dma_alloc and dma_map_single instead.
>> I would vote for a BUILD_BUG_ON_MSG in this function, so you get a
>> compile time error and you can state what needs to be done in order to
>> get rid of the failure.
> 
> If we define the function and put a BUILD_BUG_ON_MSG() inside, I am guessing that all builds will fail, right?
> 
> But we only want the builds that actually call this function to fail.
> 
> Maybe we can just define dma_alloc_coherent() as being a macro, to BUILD_BUG_ON_MSG.
> 
> Like:
> 
> #define dma_alloc_coherent(a, b) BUILD_BUG_ON_MSG(1, "dma_alloc_coherent is not supported yet on KVX. You would need to add MMU support to be able to map uncached pages")

If the macro is expanded, it will fail. Even if the code is ultimately unused because
of linker section garbage collection. You could define dma_alloc_coherent with following
body:

{
	extern void *coherent_allocation_not_implemented_on_kvx(void);
	/* You would need to add MMU support to be able to map uncached pages */
	return coherent_allocation_not_implemented_on_kvx();
}

If after linker GC, a reference to dma_alloc_coherent remains, you will get a linker
error explaining why.

Cheers,
Ahmad

> 
> What do you think?
> 
>>
>>>>>> +/*
>>>>>> + * The implementation of arch should follow the following rules:
>>>>>> + *        map        for_cpu        for_device    unmap
>>>>>> + * TO_DEV    writeback    none        writeback    none
>>>>>> + * FROM_DEV    invalidate    invalidate(*)    invalidate    invalidate(*)
>>>>>> + * BIDIR    writeback    invalidate    writeback    invalidate
>>>>>> + *
>>>>>> + * (*) - only necessary if the CPU speculatively prefetches.
>>>>>> + *
>>>>>> + * (see https://lkml.org/lkml/2018/5/18/979)
>>>>>> + */
>>>>>> +
>>>>>> +void dma_sync_single_for_device(dma_addr_t addr, size_t size,
>>>>>> +                enum dma_data_direction dir)
>>>>>> +{
>>>>>> +    switch (dir) {
>>>>>> +    case DMA_FROM_DEVICE:
>>>>>> +        kvx_dcache_invalidate_mem_area(addr, size);
>>>> Why do you need to explicitly invalidate, but not flush? Even if the
>>>> CPU speculatively prefetches, the coherency protocol should make sure
>>>> to invalidate the speculatively loaded lines, right?
>>> Since we don't have a coherent memory, here we need to invalidate L1
>>> dcache to let the CPU see deivce's writes in memory.
>>> Also every write goes through the cache, flush is not required.
>> Ah, if all your caches are write-through that makes sense. Can you add
>> a comment somewhere stating that this implementation assumes WT caches
>> on KVX? This way we can avoid the confusion Ahamd and myself fell into
>> when glancing over the code.
>>
>> Regards,
>> Lucas
>>
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox