From: Robin Murphy <robin.murphy@arm.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>, Will Deacon <will@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Vinod Koul <vkoul@kernel.org>, Akhil R <akhilrajeev@nvidia.com>,
christian.koenig@amd.com, devicetree@vger.kernel.org,
digetx@gmail.com, jonathanh@nvidia.com, ldewangan@nvidia.com,
linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-tegra@vger.kernel.org, sumit.semwal@linaro.org,
wsa@kernel.org
Subject: Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C
Date: Fri, 7 Oct 2022 18:40:12 +0100 [thread overview]
Message-ID: <acdbf49c-1a73-a0b9-a10d-42d544be3117@arm.com> (raw)
In-Reply-To: <Y0BDB2lz4PHUwE8L@orome>
On 2022-10-07 16:17, Thierry Reding wrote:
> On Fri, Oct 07, 2022 at 03:53:21PM +0100, Robin Murphy wrote:
>> On 2022-10-07 15:34, Thierry Reding wrote:
>>> On Fri, Oct 07, 2022 at 03:20:37PM +0200, Thierry Reding wrote:
>>>> On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote:
>>>>> Add dma properties to support GPCDMA for I2C in Tegra 186 and later
>>>>> chips
>>>>>
>>>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>>>> ---
>>>>> arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++
>>>>> arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++
>>>>> arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++
>>>>> 3 files changed, 96 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> index 59a10fb184f8..3580fbf99091 100644
>>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> @@ -672,6 +672,10 @@
>>>>> clock-names = "div-clk";
>>>>> resets = <&bpmp TEGRA186_RESET_I2C1>;
>>>>> reset-names = "i2c";
>>>>> + iommus = <&smmu TEGRA186_SID_GPCDMA_0>;
>>>>> + dma-coherent;
>>>>
>>>> I wonder: why do we need the iommus and dma-coherent properties here?
>>>> The I2C controllers are not directly accessing memory, instead it's the
>>>> GPCDMA via the dmas/dma-names properties. The GPCDMA already has these
>>>> properties set, so they seem to be useless here.
>>>
>>> Looking at this some more, the reason why we need these is so that the
>>> struct device backing these I2C controllers is attached to an IOMMU and
>>> the DMA ops are set up correspondingly. Without these, the DMA memory
>>> allocated by the I2C controllers will not be mapped through the IOMMU
>>> and cause faults because the GPCDMA is the one that needs to access
>>> those.
>>>
>>> I do recall that we have a similar case for audio where the "sound card"
>>> needs to have an iommus property to make sure it allocates memory
>>> through the same IOMMU domain as the ADMA, which is the device that ends
>>> up doing the actual memory accesses.
>>>
>>> Rob, Robin, Will, do you know of a good way other than the DT workaround
>>> here to address this? I think ideally we would want to obtain the "DMA
>>> parent" of these devices so that we allocate memory for that parent
>>> instead of the child. We do have some existing infrastructure for this
>>> type of relationship with the __of_get_dma_parent() function as well as
>>> the interconnects property, but I wonder if that's really the right way
>>> to represent this.
>>>
>>> Adding "interconnects" properties would also duplicate the "dmas"
>>> properties we already use to obtain the TX and RX DMA channels. One
>>> simple way to more accurately do this would be to reach into the DMA
>>> engine internals (dma_chan->device->dev) and pass that to dma_alloc_*()
>>> to make sure we allocate for the correct device. For audio that could be
>>> a bit complicated because most of that code is shared across multiple
>>> vendors. I couldn't find any examples where a driver would reach into
>>> DMA channels to allocate for the parent, so I'm wondering what other
>>> people do to solve this issue. Or if anyone else even has the same
>>> issue.
>>
>> As far as I'm aware that's the correct approach, i.e. if a driver is using
>> an external dmaengine then it's responsible for making DMA mappings for the
>> correct DMA channel device. We ended up being a bit asymmetrical in that the
>> dmaengine driver itself has to take care of its own mapping for the
>> non-memory end of a transfer when an IOMMU is involved - that's what
>> dma_map_resource() was created for, see pl330 and rcar-dmac for examples.
>>
>> The only driver I have first-hand experience with in this context is
>> amba-pl011, using pl330 through an SMMU on the Arm Juno board, but that
>> definitely works fine without DT hacks.
>
> Oh yeah, I see. That's exactly what I had in mind. And now that I'm
> using the right regex I can also find more occurrences of this. Thanks
> for those pointers. Looks like that's the right approach then. I'll try
> to make corresponding changes and see if there's any fallout.
Eww, from a quick look the DMA API usage in i2c-tegra is worse than just
this. Calling dma_sync_* on a buffer from dma_alloc_coherent() is both
egregiously wrong and semantically pointless - the fundamental purpose
of a coherent allocation is that both CPU and device can access it at
any time without any explicit synchronisation. If the driver's doing its
own bounce-buffering for reasons of data alignment then the syncs should
just go; otherwise get rid of the coherent buffer and replace the syncs
with proper map/unmap. Either way, be sure to throw CONFIG_DMA_API_DEBUG
at it, hard.
Cheers,
Robin.
next prev parent reply other threads:[~2022-10-07 17:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-06 14:47 [PATCH v2 0/3] Add GPCDMA support to Tegra I2C Akhil R
2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R
2022-09-10 9:03 ` Dmitry Osipenko
2022-09-16 19:47 ` Wolfram Sang
2022-09-06 14:47 ` [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C Akhil R
2022-10-07 13:20 ` Thierry Reding
2022-10-07 14:34 ` Thierry Reding
2022-10-07 14:53 ` Robin Murphy
2022-10-07 15:17 ` Thierry Reding
2022-10-07 17:40 ` Robin Murphy [this message]
2022-09-06 14:47 ` [PATCH v2 3/3] arm64: defconfig: Make TEGRA186_GPC_DMA built-in Akhil R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=acdbf49c-1a73-a0b9-a10d-42d544be3117@arm.com \
--to=robin.murphy@arm.com \
--cc=akhilrajeev@nvidia.com \
--cc=christian.koenig@amd.com \
--cc=devicetree@vger.kernel.org \
--cc=digetx@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=lars@metafoo.de \
--cc=ldewangan@nvidia.com \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=sumit.semwal@linaro.org \
--cc=thierry.reding@gmail.com \
--cc=vkoul@kernel.org \
--cc=will@kernel.org \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).