* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
[not found] ` <20220415142055.30873-7-sven@svenpeter.dev>
@ 2022-04-19 5:31 ` Christoph Hellwig
2022-04-19 5:59 ` Hector Martin
2022-04-19 9:47 ` Sven Peter
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Hellwig @ 2022-04-19 5:31 UTC (permalink / raw)
To: Sven Peter
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Hector Martin, Alyssa Rosenzweig, Rob Herring,
Krzysztof Kozlowski, Arnd Bergmann, Marc Zyngier, devicetree,
linux-arm-kernel, linux-kernel, linux-nvme, linux-spdx
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
2022-04-19 5:31 ` [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver Christoph Hellwig
@ 2022-04-19 5:59 ` Hector Martin
2022-04-19 9:47 ` Sven Peter
1 sibling, 0 replies; 6+ messages in thread
From: Hector Martin @ 2022-04-19 5:59 UTC (permalink / raw)
To: Christoph Hellwig, Sven Peter
Cc: Keith Busch, Jens Axboe, Sagi Grimberg, Alyssa Rosenzweig,
Rob Herring, Krzysztof Kozlowski, Arnd Bergmann, Marc Zyngier,
devicetree, linux-arm-kernel, linux-kernel, linux-nvme,
linux-spdx
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 :-) )
--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
2022-04-19 5:31 ` [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver Christoph Hellwig
2022-04-19 5:59 ` Hector Martin
@ 2022-04-19 9:47 ` Sven Peter
2022-04-19 9:52 ` Arnd Bergmann
1 sibling, 1 reply; 6+ messages in thread
From: Sven Peter @ 2022-04-19 9:47 UTC (permalink / raw)
To: hch@lst.de
Cc: Keith Busch, axboe@fb.com, sagi@grimberg.me, Hector Martin,
Alyssa Rosenzweig, Rob Herring, Krzysztof Kozlowski,
Arnd Bergmann, Marc Zyngier, devicetree, linux-arm-kernel,
linux-kernel, linux-nvme, linux-spdx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
2022-04-19 9:47 ` Sven Peter
@ 2022-04-19 9:52 ` Arnd Bergmann
2022-04-20 4:34 ` hch
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2022-04-19 9:52 UTC (permalink / raw)
To: Sven Peter
Cc: hch@lst.de, Keith Busch, axboe@fb.com, sagi@grimberg.me,
Hector Martin, Alyssa Rosenzweig, Rob Herring,
Krzysztof Kozlowski, Arnd Bergmann, Marc Zyngier, DTML, Linux ARM,
Linux Kernel Mailing List, linux-nvme, linux-spdx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
2022-04-19 9:52 ` Arnd Bergmann
@ 2022-04-20 4:34 ` hch
2022-04-20 9:53 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: hch @ 2022-04-20 4:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Sven Peter, hch@lst.de, Keith Busch, axboe@fb.com,
sagi@grimberg.me, Hector Martin, Alyssa Rosenzweig, Rob Herring,
Krzysztof Kozlowski, Marc Zyngier, DTML, Linux ARM,
Linux Kernel Mailing List, linux-nvme, linux-spdx
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---
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver
2022-04-20 4:34 ` hch
@ 2022-04-20 9:53 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2022-04-20 9:53 UTC (permalink / raw)
To: hch@lst.de
Cc: Arnd Bergmann, Sven Peter, Keith Busch, axboe@fb.com,
sagi@grimberg.me, Hector Martin, Alyssa Rosenzweig, Rob Herring,
Krzysztof Kozlowski, Marc Zyngier, DTML, Linux ARM,
Linux Kernel Mailing List, linux-nvme, linux-spdx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-20 9:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220415142055.30873-1-sven@svenpeter.dev>
[not found] ` <20220415142055.30873-7-sven@svenpeter.dev>
2022-04-19 5:31 ` [PATCH v2 6/6] nvme-apple: Add initial Apple SoC NVMe driver Christoph Hellwig
2022-04-19 5:59 ` Hector Martin
2022-04-19 9:47 ` Sven Peter
2022-04-19 9:52 ` Arnd Bergmann
2022-04-20 4:34 ` hch
2022-04-20 9:53 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox