From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Christoph Hellwig <hch@lst.de>
Cc:     "linux-mips@vger.kernel.org" <linux-mips@vger.kernel.org>,
        linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        paul.walmsley@sifive.com, palmer@dabbelt.com,
        Rob Herring <robh+dt@kernel.org>, m.szyprowski@samsung.com,
        robin.murphy@arm.com, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v2 5/5] of: address: Always use dma_default_coherent for
 default coherency
In-Reply-To: <B43602D1-89D4-465F-83B1-CD106E07CB29@flygoat.com>
References: <20230223113644.23356-1-jiaxun.yang@flygoat.com>
 <20230223113644.23356-6-jiaxun.yang@flygoat.com>
 <20230301130613.GC467@lst.de>
 <B43602D1-89D4-465F-83B1-CD106E07CB29@flygoat.com>
Date:   Fri, 03 Mar 2023 11:11:56 +1100
Message-ID: <87356mlmlv.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Jiaxun Yang <jiaxun.yang@flygoat.com> writes:
>> 2023=E5=B9=B43=E6=9C=881=E6=97=A5 13:06=EF=BC=8CChristoph Hellwig <hch@l=
st.de> =E5=86=99=E9=81=93=EF=BC=9A
>>=20
>>> - select OF_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
>>=20
>> 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 disable=
d=E2=80=A6.

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 =3D IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
 #endif

So for powerpc if NOT_COHERENT_CACHE=3Dn, 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