| Message ID | 20160719200541.GA854@google.com |
|---|---|
| State | Accepted |
| Commit | 1dcff2e4ae728a36876bdb108173f4cbcae128bf |
| Headers | show |
On 07/19/2016 10:05 PM, Brian Norris wrote: > On Tue, Jul 19, 2016 at 08:03:00AM +0200, Stefan Roese wrote: >> On 18.07.2016 22:20, Brian Norris wrote: >>> Hmm, does x86 not define readsl()/writesl()? I can never tell what >>> accessors are supposed to be "standard" across architectures. >>> >>> Either we need to drop the COMPILE_TEST or maybe make it (!X86 && >>> COMPILE_TEST). >> >> iowrite32_rep() etc should work for x86 as well. > > Looks like it might. I'm not sure the original submitter can retest > right now (travel), so I'd probably rather just take the easy fix for > now, and we can widen to COMPILE_TEST later if desired. Isn't there a generic readsl() and writesl() implementation in include/asm-generic/io.h ? > If I could get an ack on something like this, I'll apply it soon: This is fine, I am making a note to revisit this. > ---8<--- > From: Brian Norris <computersforpeace@gmail.com> > Date: Tue, 19 Jul 2016 13:02:40 -0700 > Subject: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM > > This controller driver is used only on ARM but is mostly written > portably so it can build on other arch'es. Unfortunately, at least x86 > doesn't provibe readsl()/writesl() accessors. We could possibly fix this > issue in the future by using io{read,write}32_rep() instead, but let's > just drop the architectures we aren't using for now. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/spi-nor/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig > index 1e6f037923d9..4a682ee0f632 100644 > --- a/drivers/mtd/spi-nor/Kconfig > +++ b/drivers/mtd/spi-nor/Kconfig > @@ -40,7 +40,7 @@ config SPI_ATMEL_QUADSPI > > config SPI_CADENCE_QUADSPI > tristate "Cadence Quad SPI controller" > - depends on OF && (ARM || COMPILE_TEST) > + depends on OF && ARM > help > Enable support for the Cadence Quad SPI Flash controller. > >
On Wed, Jul 20, 2016 at 03:50:27AM +0200, Marek Vasut wrote: > On 07/19/2016 10:05 PM, Brian Norris wrote: > > On Tue, Jul 19, 2016 at 08:03:00AM +0200, Stefan Roese wrote: > >> On 18.07.2016 22:20, Brian Norris wrote: > >>> Hmm, does x86 not define readsl()/writesl()? I can never tell what > >>> accessors are supposed to be "standard" across architectures. > >>> > >>> Either we need to drop the COMPILE_TEST or maybe make it (!X86 && > >>> COMPILE_TEST). > >> > >> iowrite32_rep() etc should work for x86 as well. > > > > Looks like it might. I'm not sure the original submitter can retest > > right now (travel), so I'd probably rather just take the easy fix for > > now, and we can widen to COMPILE_TEST later if desired. > > Isn't there a generic readsl() and writesl() implementation in > include/asm-generic/io.h ? Yes, but somehow x86 has managed to avoid that. I guess it's optional for arch/<foo>/include/asm/io.h to include <asm-generic/io.h>? At any rate, I double-checked myself by adding '#error "blah"' to include/asm-generic/io.h, and x86 still seemed to build fine (at least for the modules I was checking, like cadence-quadspi.o). > > If I could get an ack on something like this, I'll apply it soon: > > This is fine, I am making a note to revisit this. Cool. In that case... > > ---8<--- > > From: Brian Norris <computersforpeace@gmail.com> > > Date: Tue, 19 Jul 2016 13:02:40 -0700 > > Subject: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM > > > > This controller driver is used only on ARM but is mostly written > > portably so it can build on other arch'es. Unfortunately, at least x86 > > doesn't provibe readsl()/writesl() accessors. We could possibly fix this > > issue in the future by using io{read,write}32_rep() instead, but let's > > just drop the architectures we aren't using for now. > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Applied. Brian
On 07/20/2016 04:50 AM, Brian Norris wrote: > On Wed, Jul 20, 2016 at 03:50:27AM +0200, Marek Vasut wrote: >> On 07/19/2016 10:05 PM, Brian Norris wrote: >>> On Tue, Jul 19, 2016 at 08:03:00AM +0200, Stefan Roese wrote: >>>> On 18.07.2016 22:20, Brian Norris wrote: >>>>> Hmm, does x86 not define readsl()/writesl()? I can never tell what >>>>> accessors are supposed to be "standard" across architectures. >>>>> >>>>> Either we need to drop the COMPILE_TEST or maybe make it (!X86 && >>>>> COMPILE_TEST). >>>> >>>> iowrite32_rep() etc should work for x86 as well. >>> >>> Looks like it might. I'm not sure the original submitter can retest >>> right now (travel), so I'd probably rather just take the easy fix for >>> now, and we can widen to COMPILE_TEST later if desired. >> >> Isn't there a generic readsl() and writesl() implementation in >> include/asm-generic/io.h ? > > Yes, but somehow x86 has managed to avoid that. I guess it's optional > for arch/<foo>/include/asm/io.h to include <asm-generic/io.h>? At any > rate, I double-checked myself by adding '#error "blah"' to > include/asm-generic/io.h, and x86 still seemed to build fine (at least > for the modules I was checking, like cadence-quadspi.o). Yep, I just checked the same and it's not included from arch/x86/include/asm/io.h for whatever reason. Maybe this needs to be fixed on x86 level? >>> If I could get an ack on something like this, I'll apply it soon: >> >> This is fine, I am making a note to revisit this. > > Cool. In that case... > >>> ---8<--- >>> From: Brian Norris <computersforpeace@gmail.com> >>> Date: Tue, 19 Jul 2016 13:02:40 -0700 >>> Subject: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM >>> >>> This controller driver is used only on ARM but is mostly written >>> portably so it can build on other arch'es. Unfortunately, at least x86 >>> doesn't provibe readsl()/writesl() accessors. We could possibly fix this >>> issue in the future by using io{read,write}32_rep() instead, but let's >>> just drop the architectures we aren't using for now. >>> >>> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > Applied. Thanks > Brian >
On Wed, Jul 20, 2016 at 04:58:08AM +0200, Marek Vasut wrote: > On 07/20/2016 04:50 AM, Brian Norris wrote: > > On Wed, Jul 20, 2016 at 03:50:27AM +0200, Marek Vasut wrote: > >> On 07/19/2016 10:05 PM, Brian Norris wrote: > >>> On Tue, Jul 19, 2016 at 08:03:00AM +0200, Stefan Roese wrote: > >>>> On 18.07.2016 22:20, Brian Norris wrote: > >>>>> Hmm, does x86 not define readsl()/writesl()? I can never tell what > >>>>> accessors are supposed to be "standard" across architectures. > >>>>> > >>>>> Either we need to drop the COMPILE_TEST or maybe make it (!X86 && > >>>>> COMPILE_TEST). > >>>> > >>>> iowrite32_rep() etc should work for x86 as well. > >>> > >>> Looks like it might. I'm not sure the original submitter can retest > >>> right now (travel), so I'd probably rather just take the easy fix for > >>> now, and we can widen to COMPILE_TEST later if desired. > >> > >> Isn't there a generic readsl() and writesl() implementation in > >> include/asm-generic/io.h ? > > > > Yes, but somehow x86 has managed to avoid that. I guess it's optional > > for arch/<foo>/include/asm/io.h to include <asm-generic/io.h>? At any > > rate, I double-checked myself by adding '#error "blah"' to > > include/asm-generic/io.h, and x86 still seemed to build fine (at least > > for the modules I was checking, like cadence-quadspi.o). > > Yep, I just checked the same and it's not included from > arch/x86/include/asm/io.h for whatever reason. Maybe this needs to be > fixed on x86 level? Maybe. That's why I added the x86 maintainers. Maybe they'd respond better^Wmore loudly if I just sent a patch to do that :) But seriously, doing the above really breaks things, even if I stick the include at the end of asm/io.h. There's plenty of stuff that the asm-generic version includes based on #ifndef some_accessor, except x86 uses a static inline for their definition. So it seems it's not trivial to get an architecture to fall back gracefully to asm-generic; you have to put in some work. It also may not be all that desirable to have some allegedly generic version generate something that may not be safe on a given architecture (and I don't purport to understand the x86 memory model). Additionally, it looks like asm-generic/io.h is actually only included in 14 of 33 arch'es, so it seems like that's really not a designated goal. It does make it awfully difficult to figure out what I/O accessors are *actually* portable though... Brian
On 07/20/2016 05:25 AM, Brian Norris wrote: > On Wed, Jul 20, 2016 at 04:58:08AM +0200, Marek Vasut wrote: >> On 07/20/2016 04:50 AM, Brian Norris wrote: >>> On Wed, Jul 20, 2016 at 03:50:27AM +0200, Marek Vasut wrote: >>>> On 07/19/2016 10:05 PM, Brian Norris wrote: >>>>> On Tue, Jul 19, 2016 at 08:03:00AM +0200, Stefan Roese wrote: >>>>>> On 18.07.2016 22:20, Brian Norris wrote: >>>>>>> Hmm, does x86 not define readsl()/writesl()? I can never tell what >>>>>>> accessors are supposed to be "standard" across architectures. >>>>>>> >>>>>>> Either we need to drop the COMPILE_TEST or maybe make it (!X86 && >>>>>>> COMPILE_TEST). >>>>>> >>>>>> iowrite32_rep() etc should work for x86 as well. >>>>> >>>>> Looks like it might. I'm not sure the original submitter can retest >>>>> right now (travel), so I'd probably rather just take the easy fix for >>>>> now, and we can widen to COMPILE_TEST later if desired. >>>> >>>> Isn't there a generic readsl() and writesl() implementation in >>>> include/asm-generic/io.h ? >>> >>> Yes, but somehow x86 has managed to avoid that. I guess it's optional >>> for arch/<foo>/include/asm/io.h to include <asm-generic/io.h>? At any >>> rate, I double-checked myself by adding '#error "blah"' to >>> include/asm-generic/io.h, and x86 still seemed to build fine (at least >>> for the modules I was checking, like cadence-quadspi.o). >> >> Yep, I just checked the same and it's not included from >> arch/x86/include/asm/io.h for whatever reason. Maybe this needs to be >> fixed on x86 level? > > Maybe. That's why I added the x86 maintainers. Maybe they'd respond > better^Wmore loudly if I just sent a patch to do that :) > > But seriously, doing the above really breaks things, even if I stick the > include at the end of asm/io.h. There's plenty of stuff that the > asm-generic version includes based on #ifndef some_accessor, except x86 > uses a static inline for their definition. So it seems it's not trivial > to get an architecture to fall back gracefully to asm-generic; you have > to put in some work. It also may not be all that desirable to have some > allegedly generic version generate something that may not be safe on a > given architecture (and I don't purport to understand the x86 memory > model). > > Additionally, it looks like asm-generic/io.h is actually only included > in 14 of 33 arch'es, so it seems like that's really not a designated > goal. It does make it awfully difficult to figure out what I/O accessors > are *actually* portable though... Ouch :-( Maybe this is an opportunity for cleanup then ? I think disabling the compile test for now is good, but we should revisit this once I'm back and capable of digging in properly. Thus far, I am mostly handling my mails on the bullet trains (which are awesome here in Japan, I tell you that ;-) ), so I'm really not able to give this as much attention as it requires. > Brian >
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig index 1e6f037923d9..4a682ee0f632 100644 --- a/drivers/mtd/spi-nor/Kconfig +++ b/drivers/mtd/spi-nor/Kconfig @@ -40,7 +40,7 @@ config SPI_ATMEL_QUADSPI config SPI_CADENCE_QUADSPI tristate "Cadence Quad SPI controller" - depends on OF && (ARM || COMPILE_TEST) + depends on OF && ARM help Enable support for the Cadence Quad SPI Flash controller.