Re: [PATCH v2 5/5] of: address: Always use dma_default_coherent for default coherency

Michael Ellerman
2023-03-03T00:12:08Z

Jiaxun Yang <jiaxun.yang@flygoat.com> writes:
>> 2023年3月1日 13:06，Christoph Hellwig <hch@lst.de> 写道：
>>
>>> - select OF_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
>>
>> Doesn't powerpc need to select CONFIG_ARCH_DMA_DEFAULT_COHERENT now,
>> or even better should be doing that in the patch adding that
>> symbol?
>
> If I read the code correctly for powerpc OF_DMA_DEFAULT_COHERENT is only selected
> with !NOT_COHERENT_CACHE, which means non-coherent dma support is disabled….

I think you're right, but it's not easy to understand.

powerpc's NOT_COHERENT_CACHE selects:

select ARCH_HAS_DMA_PREP_COHERENT
select ARCH_HAS_SYNC_DMA_FOR_DEVICE
select ARCH_HAS_SYNC_DMA_FOR_CPU

Then in your patch 3 you do:

#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
-bool dma_default_coherent;
+bool dma_default_coherent = IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
#endif

So for powerpc if NOT_COHERENT_CACHE=n, then none of those ARCH_HAS
symbols are defined, and so CONFIG_ARCH_DMA_DEFAULT_COHERENT is never used.

But like I said it's not very obvious, and it also seems fragile to
future changes.

So it seems it would be more future proof, and self documenting for
powerpc to just have:

select ARCH_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE

cheers