Feedback-ID: ifd894703:Fastmail
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\))
Subject: Re: [PATCH 2/3] riscv: Set dma_default_coherent to true
From:   Jiaxun Yang <jiaxun.yang@flygoat.com>
In-Reply-To: <Y/Y8lEndecMfD+Ur@spud>
Date:   Wed, 22 Feb 2023 16:20:16 +0000
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>,
        mpe@ellerman.id.au, paul.walmsley@sifive.com, palmer@dabbelt.com,
        Rob Herring <robh+dt@kernel.org>,
        Christoph Hellwig <hch@lst.de>, m.szyprowski@samsung.com,
        robin.murphy@arm.com, linux-riscv@lists.infradead.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <E46E0161-24E3-4185-B408-9357C49F1B51@flygoat.com>
References: <20230222133712.8079-1-jiaxun.yang@flygoat.com>
 <20230222133712.8079-3-jiaxun.yang@flygoat.com> <Y/YrvDBJcYUQt4WC@spud>
 <6BBA7BEA-8595-436D-B4BF-D7DB95069C53@flygoat.com> <Y/Y8lEndecMfD+Ur@spud>
To:     Conor Dooley <conor@kernel.org>
Precedence: bulk


> 2023=E5=B9=B42=E6=9C=8822=E6=97=A5 16:02=EF=BC=8CConor Dooley =
<conor@kernel.org> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> On Wed, Feb 22, 2023 at 03:55:19PM +0000, Jiaxun Yang wrote:
>>=20
>>=20
>>> 2023=E5=B9=B42=E6=9C=8822=E6=97=A5 14:50=EF=BC=8CConor Dooley =
<conor@kernel.org> =E5=86=99=E9=81=93=EF=BC=9A
>>>=20
>>> 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.
>>>>=20
>>>> For devicetree probed devices that have been true since very =
begining
>>>> with OF_DMA_DEFAULT_COHERENT selected.
>>>>=20
>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>> ---
>>>> arch/riscv/kernel/setup.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>=20
>>>> 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 =3D true;
>>>> +#endif
>>>=20
>>> 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?
>>=20
>> Because this symbol is only a variable when:
>>=20
>> 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)
>>=20
>> Which is only true if  CONFIG_RISCV_DMA_NONCOHERENT is selected.
>>=20
>> Otherwise this symbol is defined to true and we can=E2=80=99t make a =
assignment to it.
>=20
> 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=E2=80=99d prefer to have a config option for the default =
value but it seems like
it=E2=80=99s not in Christoph=E2=80=99s flavour.

Thanks
- Jiaxun