From: Hector Martin <firstname.lastname@example.org>
To: Rob Herring <email@example.com>, Arnd Bergmann <firstname.lastname@example.org>
Cc: Sven Peter <email@example.com>,
Alyssa Rosenzweig <firstname.lastname@example.org>,
Keith Busch <email@example.com>, "firstname.lastname@example.org" <email@example.com>,
"firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>,
Marc Zyngier <firstname.lastname@example.org>, DTML <email@example.com>,
Linux ARM <firstname.lastname@example.org>,
Linux Kernel Mailing List <email@example.com>,
Subject: Re: [PATCH 4/9] soc: apple: Add SART driver
Date: Tue, 5 Apr 2022 00:01:09 +0900 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 04/04/2022 23.58, Rob Herring wrote:
> On Sat, Apr 02, 2022 at 09:07:17PM +0200, Arnd Bergmann wrote:
>> On Sat, Apr 2, 2022 at 2:38 PM Sven Peter <email@example.com> wrote:
>>> On Mon, Mar 21, 2022, at 18:07, Arnd Bergmann wrote:
>>>> On Mon, Mar 21, 2022 at 5:50 PM Sven Peter <firstname.lastname@example.org> wrote:
>>>>> The NVMe co-processor on the Apple M1 uses a DMA address filter called
>>>>> SART for some DMA transactions. This adds a simple driver used to
>>>>> configure the memory regions from which DMA transactions are allowed.
>>>>> Co-developed-by: Hector Martin <email@example.com>
>>>>> Signed-off-by: Hector Martin <firstname.lastname@example.org>
>>>>> Signed-off-by: Sven Peter <email@example.com>
>>>> Can you add some explanation about why this uses a custom interface
>>>> instead of hooking into the dma_map_ops?
>>> In a perfect world this would just be an IOMMU implementation but since
>>> SART can't create any real IOVA space using pagetables it doesn't fit
>>> inside that subsytem.
>>> In a slightly less perfect world I could just implement dma_map_ops here
>>> but that won't work either because not all DMA buffers of the NVMe
>>> device have to go through SART and those allocations happen
>>> inside the same device and would use the same dma_map_ops.
>>> The NVMe controller has two separate DMA filters:
>>> - NVMMU, which must be set up for any command that uses PRPs and
>>> ensures that the DMA transactions only touch the pages listed
>>> inside the PRP structure. NVMMU itself is tightly coupled
>>> to the NVMe controller: The list of allowed pages is configured
>>> based on command's tag id and even commands that require no DMA
>>> transactions must be listed inside NVMMU before they are started.
>>> - SART, which must be set up for some shared memory buffers (e.g.
>>> log messages from the NVMe firmware) and for some NVMe debug
>>> commands that don't use PRPs.
>>> SART is only loosely coupled to the NVMe controller and could
>>> also be used together with other devices. It's also the only
>>> thing that changed between M1 and M1 Pro/Max/Ultra and that's
>>> why I decided to separate it from the NVMe driver.
>>> I'll add this explanation to the commit message.
>> Ok, thanks.
>>>>> +static void sart2_get_entry(struct apple_sart *sart, int index, u8 *flags,
>>>>> + phys_addr_t *paddr, size_t *size)
>>>>> + u32 cfg = readl_relaxed(sart->regs + APPLE_SART2_CONFIG(index));
>>>>> + u32 paddr_ = readl_relaxed(sart->regs + APPLE_SART2_PADDR(index));
>>>> Why do you use the _relaxed() accessors here and elsewhere in the driver?
>>> This device itself doesn't do any DMA transactions so it needs no memory
>>> synchronization barriers. Only the consumer (i.e. rtkit and nvme) read/write
>>> from/to these buffers (multiple times) and they have the required barriers
>>> in place whenever they are used.
>>> These buffers so far are only allocated at probe time though so even using
>>> the normal writel/readl here won't hurt performance at all. I can just use
>>> those if you prefer or alternatively add a comment why _relaxed is fine here.
>>> This is a bit similar to the discussion for the pinctrl series last year .
>> I think it's better to only use the _relaxed version where it actually helps,
>> with a comment about it, and use the normal version elsewhere, in
>> particular in functions that you have copied from the normal nvme driver.
>> I had tried to compare some of your code with the other version and
>> was rather confused by that.
> Oh good, I tell folks the opposite (and others do too). We don't accept
> random explicit barriers without explanation, but implicit ones are
> okay? The resulting code on arm32 is also pretty horrible with the L2x0
> and OMAP sync hooks not that that matters here.
> I don't really care too much which way we go, but we should document one
> rule and follow that.
I'm sure maz@ has an opinion here too :-)
(3... 2... 1... fight!)
Hector Martin (firstname.lastname@example.org)
Public Key: https://mrcn.st/pub
next prev parent reply other threads:[~2022-04-04 15:01 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-21 16:50 [PATCH 0/9] Apple M1 (Pro/Max) NVMe driver Sven Peter
2022-03-21 16:50 ` [PATCH 1/9] dt-bindings: soc: apple: Add Apple SART Sven Peter
2022-03-31 21:23 ` Rob Herring
2022-04-02 12:58 ` Sven Peter
2022-03-21 16:50 ` [PATCH 2/9] dt-bindings: soc: apple: Add ANS NVMe Sven Peter
2022-03-23 11:14 ` Krzysztof Kozlowski
2022-04-02 13:05 ` Sven Peter
2022-04-02 16:06 ` Krzysztof Kozlowski
2022-03-21 16:50 ` [PATCH 3/9] soc: apple: Always include Makefile Sven Peter
2022-03-21 16:50 ` [PATCH 4/9] soc: apple: Add SART driver Sven Peter
2022-03-21 17:07 ` Arnd Bergmann
2022-04-02 12:38 ` Sven Peter
2022-04-02 19:07 ` Arnd Bergmann
2022-04-04 14:58 ` Rob Herring
2022-04-04 15:01 ` Hector Martin [this message]
2022-04-05 15:37 ` Sven Peter
2022-03-21 17:17 ` Alyssa Rosenzweig
2022-04-02 12:40 ` Sven Peter
2022-03-21 16:50 ` [PATCH 5/9] soc: apple: Add RTKit IPC library Sven Peter
2022-03-22 13:13 ` Arnd Bergmann
2022-03-22 17:41 ` Robin Murphy
2022-04-02 12:56 ` Sven Peter
2022-04-02 18:30 ` Arnd Bergmann
2022-04-03 10:45 ` Sven Peter
2022-03-22 17:23 ` Alyssa Rosenzweig
2022-04-02 12:50 ` Sven Peter
2022-03-23 11:19 ` Krzysztof Kozlowski
2022-04-02 13:51 ` Sven Peter
2022-04-02 16:08 ` Krzysztof Kozlowski
2022-04-04 15:02 ` Rob Herring
2022-04-04 15:47 ` Hector Martin
2022-03-21 16:50 ` [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver Sven Peter
2022-03-21 17:01 ` Keith Busch
2022-04-02 13:10 ` Sven Peter
2022-03-22 13:38 ` Arnd Bergmann
2022-04-02 13:34 ` Sven Peter
2022-04-02 13:58 ` Janne Grunau
2022-04-02 14:02 ` Sven Peter
2022-04-02 21:26 ` Arnd Bergmann
2022-03-24 6:16 ` Christoph Hellwig
2022-04-02 12:47 ` Sven Peter
2022-04-04 15:57 ` Hector Martin
2022-04-04 15:59 ` Christoph Hellwig
2022-04-04 16:03 ` Sven Peter
2022-04-04 16:05 ` hch
2022-04-04 16:05 ` Hector Martin
2022-04-04 18:29 ` Jens Axboe
2022-04-05 6:24 ` Christoph Hellwig
2022-03-21 16:50 ` [PATCH 7/9] nvme-apple: Serialize command issue Sven Peter
2022-03-24 6:16 ` Christoph Hellwig
2022-04-02 12:42 ` Sven Peter
2022-03-21 16:50 ` [PATCH 8/9] nvme-apple: Add support for multiple power domains Sven Peter
2022-03-21 16:50 ` [PATCH 9/9] nvme-apple: Add support for suspend/resume Sven Peter
2022-03-24 6:17 ` Christoph Hellwig
2022-03-22 17:26 ` [PATCH 0/9] Apple M1 (Pro/Max) NVMe driver Alyssa Rosenzweig
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* 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).