[PATCH 0/3] Use dma_default_coherent for devicetree default coherency

[PATCH 0/3] Use dma_default_coherent for devicetree default coherency

[Jiaxun Yang]

Hi all,

This series split out second half of my previous series
"[PATCH 0/4] MIPS DMA coherence fixes".

It intends to use dma_default_coherent to determine the default coherency of
devicetree probed devices instead of hardcoding it with Kconfig options.

For some MIPS systems, dma_default_coherent is determined with either
bootloader or hardware registers in platform initilization code, and devicetree
does not explicility specify the coherency of the device, so we need the ability
to change the default coherency of devicetree probed devices.

For other platforms that supports noncoherent, dma_default_coherent is a fixed
value set by arch code. It's defaulted to false for most archs except RISC-V.

Thanks
- Jiaxun


Jiaxun Yang (3):
  dma-mapping: Provide a fallback dma_default_coherent
  riscv: Set dma_default_coherent to true
  of: address: Use dma_default_coherent to determine default coherency

 arch/powerpc/Kconfig        | 1 -
 arch/riscv/Kconfig          | 1 -
 arch/riscv/kernel/setup.c   | 3 +++
 drivers/of/Kconfig          | 4 ----
 drivers/of/address.c        | 2 +-
 include/linux/dma-map-ops.h | 1 +
 kernel/dma/mapping.c        | 4 ++++
 7 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.37.1 (Apple Git-137.1)

[PATCH 1/3] dma-mapping: Provide a fallback dma_default_coherent

[Jiaxun Yang]

dma_default_coherent was decleared unconditionally at kernel/dma/mapping.c
but only decleared when any of non-coherent options is enabled in dma-map-ops.h.

Guard the declaration in mapping.c with non-coherent options and provide
a fallback definition.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 include/linux/dma-map-ops.h | 1 +
 kernel/dma/mapping.c        | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index 41bf4bdb117a..0a1aaf7163b3 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -269,6 +269,7 @@ static inline bool dev_is_dma_coherent(struct device *dev)
 	return dev->dma_coherent;
 }
 #else
+#define dma_default_coherent true
 static inline bool dev_is_dma_coherent(struct device *dev)
 {
 	return true;
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 68106e3791f6..80f9663ffe26 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -17,7 +17,11 @@
 #include "debug.h"
 #include "direct.h"
 
+#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;
+#endif
 
 /*
  * Managed DMA API
-- 
2.37.1 (Apple Git-137.1)

[PATCH 2/3] riscv: Set dma_default_coherent to true

[Jiaxun Yang]

For riscv our assumption is unless a device states it is non-coherent,
we take it to be DMA coherent.

For devicetree probed devices that have been true since very begining
with OF_DMA_DEFAULT_COHERENT selected.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/riscv/kernel/setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 376d2827e736..34b371180976 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
 	riscv_init_cbom_blocksize();
 	riscv_fill_hwcap();
 	apply_boot_alternatives();
+#ifdef CONFIG_RISCV_DMA_NONCOHERENT
+	dma_default_coherent = true;
+#endif
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
 	    riscv_isa_extension_available(NULL, ZICBOM))
 		riscv_noncoherent_supported();
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Wed, Feb 22, 2023 at 01:37:11PM +0000, Jiaxun Yang wrote:
> For riscv our assumption is unless a device states it is non-coherent,
> we take it to be DMA coherent.
> 
> For devicetree probed devices that have been true since very begining
> with OF_DMA_DEFAULT_COHERENT selected.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  arch/riscv/kernel/setup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 376d2827e736..34b371180976 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
>  	riscv_init_cbom_blocksize();
>  	riscv_fill_hwcap();
>  	apply_boot_alternatives();
> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> +	dma_default_coherent = true;
> +#endif

Do we really need to add ifdeffery for this here?
It's always coherent by default, so why do we need to say set it in
setup_arch() when we know that, regardless of options, it is true?

Cheers,
Conor.

>  	if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
>  	    riscv_isa_extension_available(NULL, ZICBOM))
>  		riscv_noncoherent_supported();
> -- 
> 2.37.1 (Apple Git-137.1)
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Jiaxun Yang]

> 2023年2月22日 14:50，Conor Dooley <conor@kernel.org> 写道：
> 
> On Wed, Feb 22, 2023 at 01:37:11PM +0000, Jiaxun Yang wrote:
>> For riscv our assumption is unless a device states it is non-coherent,
>> we take it to be DMA coherent.
>> 
>> For devicetree probed devices that have been true since very begining
>> with OF_DMA_DEFAULT_COHERENT selected.
>> 
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>> arch/riscv/kernel/setup.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 376d2827e736..34b371180976 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
>> riscv_init_cbom_blocksize();
>> riscv_fill_hwcap();
>> apply_boot_alternatives();
>> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
>> + dma_default_coherent = true;
>> +#endif
> 
> Do we really need to add ifdeffery for this here?
> It's always coherent by default, so why do we need to say set it in
> setup_arch() when we know that, regardless of options, it is true?

Because this symbol is only a variable when:

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)

Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.

Otherwise this symbol is defined to true and we can’t make a assignment to it.

Thanks
- Jiaxun


> 
> Cheers,
> Conor.
> 
>> if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
>>     riscv_isa_extension_available(NULL, ZICBOM))
>> riscv_noncoherent_supported();
>> -- 
>> 2.37.1 (Apple Git-137.1)

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

On Wed, Feb 22, 2023 at 03:55:19PM +0000, Jiaxun Yang wrote:
> 
> 
> > 2023年2月22日 14:50，Conor Dooley <conor@kernel.org> 写道：
> > 
> > On Wed, Feb 22, 2023 at 01:37:11PM +0000, Jiaxun Yang wrote:
> >> For riscv our assumption is unless a device states it is non-coherent,
> >> we take it to be DMA coherent.
> >> 
> >> For devicetree probed devices that have been true since very begining
> >> with OF_DMA_DEFAULT_COHERENT selected.
> >> 
> >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> >> ---
> >> arch/riscv/kernel/setup.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> index 376d2827e736..34b371180976 100644
> >> --- a/arch/riscv/kernel/setup.c
> >> +++ b/arch/riscv/kernel/setup.c
> >> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
> >> riscv_init_cbom_blocksize();
> >> riscv_fill_hwcap();
> >> apply_boot_alternatives();
> >> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> >> + dma_default_coherent = true;
> >> +#endif
> > 
> > Do we really need to add ifdeffery for this here?
> > It's always coherent by default, so why do we need to say set it in
> > setup_arch() when we know that, regardless of options, it is true?
> 
> Because this symbol is only a variable when:
> 
> 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)
> 
> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
> 
> Otherwise this symbol is defined to true and we can’t make a assignment to it.

Maybe I am just slow today, but I don't get why you need to add
ifdeffery to setup_arch() to do something that is always true.
Why can't you just set this in riscv/mm/dma-noncoherent.c? What am I
missing?

Cheers,
Conor.

> >> if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
> >>     riscv_isa_extension_available(NULL, ZICBOM))
> >> riscv_noncoherent_supported();
> >> -- 
> >> 2.37.1 (Apple Git-137.1)
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Jiaxun Yang]

> 2023年2月22日 16:02，Conor Dooley <conor@kernel.org> 写道：
> 
> On Wed, Feb 22, 2023 at 03:55:19PM +0000, Jiaxun Yang wrote:
>> 
>> 
>>> 2023年2月22日 14:50，Conor Dooley <conor@kernel.org> 写道：
>>> 
>>> On Wed, Feb 22, 2023 at 01:37:11PM +0000, Jiaxun Yang wrote:
>>>> For riscv our assumption is unless a device states it is non-coherent,
>>>> we take it to be DMA coherent.
>>>> 
>>>> For devicetree probed devices that have been true since very begining
>>>> with OF_DMA_DEFAULT_COHERENT selected.
>>>> 
>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>> ---
>>>> arch/riscv/kernel/setup.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>> index 376d2827e736..34b371180976 100644
>>>> --- a/arch/riscv/kernel/setup.c
>>>> +++ b/arch/riscv/kernel/setup.c
>>>> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
>>>> riscv_init_cbom_blocksize();
>>>> riscv_fill_hwcap();
>>>> apply_boot_alternatives();
>>>> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
>>>> + dma_default_coherent = true;
>>>> +#endif
>>> 
>>> Do we really need to add ifdeffery for this here?
>>> It's always coherent by default, so why do we need to say set it in
>>> setup_arch() when we know that, regardless of options, it is true?
>> 
>> Because this symbol is only a variable when:
>> 
>> 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)
>> 
>> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
>> 
>> Otherwise this symbol is defined to true and we can’t make a assignment to it.
> 
> Maybe I am just slow today, but I don't get why you need to add
> ifdeffery to setup_arch() to do something that is always true.
> Why can't you just set this in riscv/mm/dma-noncoherent.c? What am I
> missing?

Hmm that sounds like a good idea but I was unable to find a place for this.

riscv/mm/dma-noncoherent.c are just a bunch of callbacks, there is no initialisation
function that will always run on every boot. riscv_noncoherent_supported is only
called conditionally.

Perhaps I can add a initcall in dma-noncoherent.c but it seems to be a little bit
overkilling.

Actually I’d prefer to have a config option for the default value but it seems like
it’s not in Christoph’s flavour.

Thanks
- Jiaxun

Re: [PATCH 2/3] riscv: Set dma_default_coherent to true

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2952 bytes --]

On Wed, Feb 22, 2023 at 04:20:16PM +0000, Jiaxun Yang wrote:
> > 2023年2月22日 16:02，Conor Dooley <conor@kernel.org> 写道：
> > On Wed, Feb 22, 2023 at 03:55:19PM +0000, Jiaxun Yang wrote:
> >>> 2023年2月22日 14:50，Conor Dooley <conor@kernel.org> 写道：
> >>> On Wed, Feb 22, 2023 at 01:37:11PM +0000, Jiaxun Yang wrote:
> >>>> For riscv our assumption is unless a device states it is non-coherent,
> >>>> we take it to be DMA coherent.
> >>>> 
> >>>> For devicetree probed devices that have been true since very begining
> >>>> with OF_DMA_DEFAULT_COHERENT selected.
> >>>> 
> >>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> >>>> ---
> >>>> arch/riscv/kernel/setup.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>> 
> >>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >>>> index 376d2827e736..34b371180976 100644
> >>>> --- a/arch/riscv/kernel/setup.c
> >>>> +++ b/arch/riscv/kernel/setup.c
> >>>> @@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
> >>>> riscv_init_cbom_blocksize();
> >>>> riscv_fill_hwcap();
> >>>> apply_boot_alternatives();
> >>>> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> >>>> + dma_default_coherent = true;
> >>>> +#endif
> >>> 
> >>> Do we really need to add ifdeffery for this here?
> >>> It's always coherent by default, so why do we need to say set it in
> >>> setup_arch() when we know that, regardless of options, it is true?
> >> 
> >> Because this symbol is only a variable when:
> >> 
> >> 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)
> >> 
> >> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
> >> 
> >> Otherwise this symbol is defined to true and we can’t make a assignment to it.

> > Maybe I am just slow today, but I don't get why you need to add
> > ifdeffery to setup_arch() to do something that is always true.
> > Why can't you just set this in riscv/mm/dma-noncoherent.c? What am I
> > missing?
> 
> Hmm that sounds like a good idea but I was unable to find a place for this.
> 
> riscv/mm/dma-noncoherent.c are just a bunch of callbacks, there is no initialisation
> function that will always run on every boot. riscv_noncoherent_supported is only
> called conditionally.

Right, that's fair. riscv_noncoherent_supported() is for when we know we
can support non-coherent DMA, not to detect whether we can, hence the
conditional nature.
Apologies for sending you on a wild goose chase there.

> Perhaps I can add a initcall in dma-noncoherent.c but it seems to be a little bit
> overkilling.

Yah, probably would be.

> Actually I’d prefer to have a config option for the default value but it seems like
> it’s not in Christoph’s flavour.

We already have a config option that sets things up nicely for RISC-V
that you're getting rid of! ;)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/3] of: address: Use dma_default_coherent to determine default coherency

[Jiaxun Yang]

As for now all arches have dma_default_coherent matched with default
DMA coherency for of devices, so there is no need to have a standalone
config option.

This also fixes a case that for some MIPS platforms, coherency information
is not carried in devicetree and kernel will override dma_default_coherent
at early boot.

Note for PowerPC: CONFIG_OF_DMA_DEFUALT_COHERENT was only selected when
CONFIG_NOT_COHERENT_CACHE is false, in this case dma_default_coherent will
be true, so it still matches present behavior.

Note for RISC-V: dma_default_coherent is set to true at init code in this
series.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/powerpc/Kconfig | 1 -
 arch/riscv/Kconfig   | 1 -
 drivers/of/Kconfig   | 4 ----
 drivers/of/address.c | 2 +-
 4 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2c9cdf1d8761..c67e5da714f7 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -272,7 +272,6 @@ config PPC
 	select NEED_PER_CPU_PAGE_FIRST_CHUNK	if PPC64
 	select NEED_SG_DMA_LENGTH
 	select OF
-	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
 	select OF_EARLY_FLATTREE
 	select OLD_SIGACTION			if PPC32
 	select OLD_SIGSUSPEND
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 1d46a268ce16..406c6816d289 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -119,7 +119,6 @@ config RISCV
 	select MODULES_USE_ELF_RELA if MODULES
 	select MODULE_SECTIONS if MODULES
 	select OF
-	select OF_DMA_DEFAULT_COHERENT
 	select OF_EARLY_FLATTREE
 	select OF_IRQ
 	select PCI_DOMAINS_GENERIC if PCI
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 644386833a7b..e40f10bf2ba4 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -102,8 +102,4 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
-config OF_DMA_DEFAULT_COHERENT
-	# arches should select this if DMA is coherent by default for OF devices
-	bool
-
 endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 4c0b169ef9bf..23ade4919853 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -1103,7 +1103,7 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
 bool of_dma_is_coherent(struct device_node *np)
 {
 	struct device_node *node;
-	bool is_coherent = IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT);
+	bool is_coherent = dma_default_coherent;
 
 	node = of_node_get(np);
 
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH 3/3] of: address: Use dma_default_coherent to determine default coherency

[Robin Murphy]

On 2023-02-22 13:37, Jiaxun Yang wrote:
> As for now all arches have dma_default_coherent matched with default
> DMA coherency for of devices, so there is no need to have a standalone
> config option.
> 
> This also fixes a case that for some MIPS platforms, coherency information
> is not carried in devicetree and kernel will override dma_default_coherent
> at early boot.
> 
> Note for PowerPC: CONFIG_OF_DMA_DEFUALT_COHERENT was only selected when
> CONFIG_NOT_COHERENT_CACHE is false, in this case dma_default_coherent will
> be true, so it still matches present behavior.
> 
> Note for RISC-V: dma_default_coherent is set to true at init code in this
> series.

OK, so the fundamental problem here is that we have two slightly 
different conflicting mechanisms, the ex-PowerPC config option, and the 
ex-MIPS dma_default_coherent for which of_dma_is_coherent() has 
apparently been broken forever.

I'd agree that it's worth consolidating the two, but please separate out 
the fix as below, so it's feasible to backport without having to muck 
about in arch code.

> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   arch/powerpc/Kconfig | 1 -
>   arch/riscv/Kconfig   | 1 -
>   drivers/of/Kconfig   | 4 ----
>   drivers/of/address.c | 2 +-
>   4 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2c9cdf1d8761..c67e5da714f7 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -272,7 +272,6 @@ config PPC
>   	select NEED_PER_CPU_PAGE_FIRST_CHUNK	if PPC64
>   	select NEED_SG_DMA_LENGTH
>   	select OF
> -	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
>   	select OF_EARLY_FLATTREE
>   	select OLD_SIGACTION			if PPC32
>   	select OLD_SIGSUSPEND
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 1d46a268ce16..406c6816d289 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -119,7 +119,6 @@ config RISCV
>   	select MODULES_USE_ELF_RELA if MODULES
>   	select MODULE_SECTIONS if MODULES
>   	select OF
> -	select OF_DMA_DEFAULT_COHERENT
>   	select OF_EARLY_FLATTREE
>   	select OF_IRQ
>   	select PCI_DOMAINS_GENERIC if PCI
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 644386833a7b..e40f10bf2ba4 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -102,8 +102,4 @@ config OF_OVERLAY
>   config OF_NUMA
>   	bool
>   
> -config OF_DMA_DEFAULT_COHERENT
> -	# arches should select this if DMA is coherent by default for OF devices
> -	bool
> -
>   endif # OF
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 4c0b169ef9bf..23ade4919853 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -1103,7 +1103,7 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
>   bool of_dma_is_coherent(struct device_node *np)
>   {
>   	struct device_node *node;
> -	bool is_coherent = IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT);
> +	bool is_coherent = dma_default_coherent;

AFAICS, all you should actually need is a single self-contained addition 
here, something like:

+	/*
+	 * DT-based MIPS doesn't use OF_DMA_DEFAULT_COHERENT, but
+	 * might override the system-wide default at runtime.
+	 */
+#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)
+	is_coherent = dma_default_coherent;
+#endif

>   
>   	node = of_node_get(np);
>   

Then *after* that's fixed, we can do a more comprehensive refactoring to 
merge the two mechanisms properly. FWIW I think I'd prefer an approach 
closer to the first one, where config options control the initial value 
of dma_default_coherent rather than architectures having to override it 
unconditionally (and TBH I'd also like to have a generic config symbol 
for whether an arch supports per-device coherency or not).

Thanks,
Robin.

Re: [PATCH 3/3] of: address: Use dma_default_coherent to determine default coherency

[Jiaxun Yang]

> 2023年2月22日 17:24，Robin Murphy <robin.murphy@arm.com> 写道：

[...]

> 
> AFAICS, all you should actually need is a single self-contained addition here, something like:
> 
> + /*
> +  * DT-based MIPS doesn't use OF_DMA_DEFAULT_COHERENT, but
> +  * might override the system-wide default at runtime.
> +  */
> +#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)
> + is_coherent = dma_default_coherent;
> +#endif

That makes more sense, thanks.
I’ll append CONFIG_MIPS as a condition here as well because it may break RISC-V whose dma_default_coherent
is not set to true from very beginning.

> 
>>     node = of_node_get(np);
>>  
> 
> Then *after* that's fixed, we can do a more comprehensive refactoring to merge the two mechanisms properly. FWIW I think I'd prefer an approach closer to the first one, where config options control the initial value of dma_default_coherent rather than architectures having to override it unconditionally (and TBH I'd also like to have a generic config symbol for whether an arch supports per-device coherency or not).

Ok I’ll try to revert to the initial way.
Is there any reason that an arch can’t support per-device coherency?

Thanks
- Jiaxun


> 
> Thanks,
> Robin.