| Message ID | 20220415142055.30873-1-sven@svenpeter.dev |
|---|---|
| Headers | show |
| Series | Apple M1 (Pro/Max) NVMe driver | expand |
On Fri, Apr 15, 2022 at 04:20:55PM +0200, Sven Peter wrote: > +++ b/drivers/nvme/host/apple.c > @@ -0,0 +1,1597 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Apple ANS NVM Express device driver > + * Copyright The Asahi Linux Contributors Is that actually a valid legal entity? > +#include <linux/io-64-nonatomic-lo-hi.h> Does this controller still not support 64-bit MMIO accesses like the old Apple PCIe controllers or is this just a leftover? The rest of the code looks fine to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 19/04/2022 14.31, Christoph Hellwig wrote: > On Fri, Apr 15, 2022 at 04:20:55PM +0200, Sven Peter wrote: >> +++ b/drivers/nvme/host/apple.c >> @@ -0,0 +1,1597 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Apple ANS NVM Express device driver >> + * Copyright The Asahi Linux Contributors > > Is that actually a valid legal entity? It does not have to be. See here for the rationale behind this style of copyright line: https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ TL;DR name- and year-ful copyright lines are basically useless, as they become out of date almost immediately after they are applied. This way we acknowledge that the files have multiple contributors (and that the copyright line isn't trying to be an exhaustive list thereof). This style is so far rare, but not unheard of, in the kernel; there is prior art (e.g. grep for 'Chromium OS Authors'). (I get to re-tell this story every time we upstream to a new subsystem; I think it's the sixth time or so :-) )
On Tue, Apr 19, 2022, at 07:31, Christoph Hellwig wrote: > On Fri, Apr 15, 2022 at 04:20:55PM +0200, Sven Peter wrote: >> +++ b/drivers/nvme/host/apple.c >> @@ -0,0 +1,1597 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Apple ANS NVM Express device driver >> + * Copyright The Asahi Linux Contributors > > Is that actually a valid legal entity? > >> +#include <linux/io-64-nonatomic-lo-hi.h> > > Does this controller still not support 64-bit MMIO accesses like > the old Apple PCIe controllers or is this just a leftover? I just checked again and 64-bit accesses seem to work fine. I'll remove the lo_hi_* calls and this include. > > The rest of the code looks fine to me: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! Sven
On Tue, Apr 19, 2022 at 11:47 AM Sven Peter <sven@svenpeter.dev> wrote: > On Tue, Apr 19, 2022, at 07:31, Christoph Hellwig wrote: > > On Fri, Apr 15, 2022 at 04:20:55PM +0200, Sven Peter wrote: > >> +++ b/drivers/nvme/host/apple.c > >> @@ -0,0 +1,1597 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Apple ANS NVM Express device driver > >> + * Copyright The Asahi Linux Contributors > > > > Is that actually a valid legal entity? > > > >> +#include <linux/io-64-nonatomic-lo-hi.h> > > > > Does this controller still not support 64-bit MMIO accesses like > > the old Apple PCIe controllers or is this just a leftover? > > I just checked again and 64-bit accesses seem to work fine. > I'll remove the lo_hi_* calls and this include. If you remove the #include, it is no longer possible to compile-test this on all 32-bit architectures, though that is probably fine as long as the Kconfig file has the right dependencies, like depends on ARCH_APPLE || (COMPILE_TEST && 64BIT) I'd prefer to keep the #include here, but I don't mind the dependency if Christoph prefers it that way. Arnd
On Tue, Apr 19, 2022 at 11:52:15AM +0200, Arnd Bergmann wrote: > > I just checked again and 64-bit accesses seem to work fine. > > I'll remove the lo_hi_* calls and this include. > > If you remove the #include, it is no longer possible to compile-test > this on all 32-bit architectures, though that is probably fine as long > as the Kconfig file has the right dependencies, like > > depends on ARCH_APPLE || (COMPILE_TEST && 64BIT) > > I'd prefer to keep the #include here, but I don't mind the dependency > if Christoph prefers it that way. So thre's really two steps here: 1) stop uing lo_hi_readq diretly which forces 32-bit access even on 64-bit platforms 2) stop using the io-64-nonatomic headers entirely I definitively want 1) done if the hardware does not require it. Trying to cater to 32-bit build tests on hardware that has no chance of ever being used there by including the header seems a bit silly, but if it makes folks happy I can live with it. > > Arnd ---end quoted text---
On Wed, Apr 20, 2022 at 6:34 AM hch@lst.de <hch@lst.de> wrote: > On Tue, Apr 19, 2022 at 11:52:15AM +0200, Arnd Bergmann wrote: > > > I just checked again and 64-bit accesses seem to work fine. > > > I'll remove the lo_hi_* calls and this include. > > > > If you remove the #include, it is no longer possible to compile-test > > this on all 32-bit architectures, though that is probably fine as long > > as the Kconfig file has the right dependencies, like > > > > depends on ARCH_APPLE || (COMPILE_TEST && 64BIT) > > > > I'd prefer to keep the #include here, but I don't mind the dependency > > if Christoph prefers it that way. > > So thre's really two steps here: > > 1) stop uing lo_hi_readq diretly which forces 32-bit access even on > 64-bit platforms > 2) stop using the io-64-nonatomic headers entirely > > I definitively want 1) done if the hardware does not require it. Yes, of cours.e > Trying to cater to 32-bit build tests on hardware that has no chance of > ever being used there by including the header seems a bit silly, but if > it makes folks happy I can live with it. As I said, I don't have a strong opinion either, it's either a trivial change in Kconfig or a trivial header inclusion and I'd pick the header one because it's more obvious what this is for without adding a comment. Arnd
Hi, v1: https://lore.kernel.org/linux-nvme/20220321165049.35985-1-sven@svenpeter.dev/T/ Thanks everyone for the reviews of v1! I've listed the changes in the individual commits to keep better track of them. And here's the v1 cover letter as a reference as well: This series includes everything[*] required to get NVMe up and running on Apple's M1, M1 Pro and M1 Max SoCs. The NVMe controller on these machines is not attached to a PCI bus and this driver originally started out when Arnd added platform support to pci.c and I added the required Apple quirks. Christoph Hellwig stumbled upon an early version and suggested to instead rewrite it as a stand-alone driver since some of these quirks are rather awkward to implement and would affect the hot path otherwise [1]. Here's the first version that creates apple.c to handle these weird NVMe controllers. The following parts are included: - Device tree bindings: Since this is the first and probably only SoC that has NVMe outside of a PCIe bus I've put them into soc/apple. The same bindings are also used by u-boot and OpenBSD already. - SART address filter: Some of the buffers required by the NVMe controller sit behind a simple DMA address filter Apple calls SART. It allows to specify up to 16 physical address ranges that are allowed and will reject access to anything else. Unlike a real IOMMU there's no way to setup pagetables and also not all buffers are subject to this filtering. Most buffers used by the NVMe commands themselves use an integrated IOMMU instead. - RTKit IPC protocol: The NVMe controller is running a proprietary RTOS Apple calls RTKit and we need to communicate with it in order to bring up and use the NVMe controller. This communication happens over a mailbox interface that is already upstream. This protocol is also used for various other drivers present on these SoCs (SMC, display controller, Thunderbolt/USB4). - And finally the NVMe driver itself: The driver registers a platform device and is mainly based on pci.c with a few special Apple quirks. The biggest difference to normal NVMe (except for the missing PCI bus) is that command submission works differently: The SQ is replaced with a simple array and a command is triggered by writing its tag to a MMIO register. Additionally, the command must also be setup in the proprietary NVMMU before it can be submitted. There is some code duplication with pci.c for the setup of the PRPs. Depending on what you prefer this could be moved to a common file shared between pci.c and apple.c. The hardware here is the same hardware that's already used in T2 Macs. The only difference is that the T2 chip itself initializes the controller, disable some quirks (the NVMMU and the weird submission array) and then exposes it over a PCIe interface. The driver itself has been successfully used by multiple people as their daily driver for weeks at this point and no major issues have been reported. A smaller issue is that flushes on the devices take *much* longer than expected. Jens Axboe has a workaround where the flushes are delayed but that one isn't ready for submission yet. Best, Sven [1] https://lore.kernel.org/linux-nvme/YRE7vCyn9d1ClhRm@infradead.org/ [*] The only missing part in this series are the device tree updates but since these will go through arm-soc anyway I haven't included them here but will instead submit them once this series is in a shape where it can be merged. Sven Peter (6): dt-bindings: iommu: Add Apple SART DMA address filter dt-bindings: nvme: Add Apple ANS NVMe soc: apple: Always include Makefile soc: apple: Add SART driver soc: apple: Add RTKit IPC library nvme-apple: Add initial Apple SoC NVMe driver .../devicetree/bindings/iommu/apple,sart.yaml | 52 + .../bindings/nvme/apple,nvme-ans.yaml | 111 ++ MAINTAINERS | 4 + drivers/nvme/host/Kconfig | 13 + drivers/nvme/host/Makefile | 3 + drivers/nvme/host/apple.c | 1597 +++++++++++++++++ drivers/soc/Makefile | 2 +- drivers/soc/apple/Kconfig | 24 + drivers/soc/apple/Makefile | 6 + drivers/soc/apple/rtkit-crashlog.c | 154 ++ drivers/soc/apple/rtkit-internal.h | 62 + drivers/soc/apple/rtkit.c | 958 ++++++++++ drivers/soc/apple/sart.c | 327 ++++ include/linux/soc/apple/rtkit.h | 159 ++ include/linux/soc/apple/sart.h | 57 + 15 files changed, 3528 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/iommu/apple,sart.yaml create mode 100644 Documentation/devicetree/bindings/nvme/apple,nvme-ans.yaml create mode 100644 drivers/nvme/host/apple.c create mode 100644 drivers/soc/apple/rtkit-crashlog.c create mode 100644 drivers/soc/apple/rtkit-internal.h create mode 100644 drivers/soc/apple/rtkit.c create mode 100644 drivers/soc/apple/sart.c create mode 100644 include/linux/soc/apple/rtkit.h create mode 100644 include/linux/soc/apple/sart.h