| Message ID | 20230222133712.8079-4-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use dma_default_coherent for devicetree default coherency | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 13 and now 13 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | fail | Failed to build the tree with this patch. |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | fail | Failed to build the tree with this patch. |
| conchuod/alphanumeric_selects | success | Out of order selects before the patch: 729 and now 728 |
| conchuod/build_rv32_defconfig | fail | Build failed |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 2 this patch: 2 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 30 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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.
> 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.
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);
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(-)