Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

[Brian Norris]

On Tue, Jul 19, 2016 at 03:43:17AM +0800, kbuild test robot wrote:
> tree:   git://git.infradead.org/linux-mtd-next.git master
> head:   f78921b9020c510ed222a6c2402e2aa126432415
> commit: 140623410536905fa6ab737b625decfde6c64a72 [30/33] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
> reproduce:
>         git checkout 140623410536905fa6ab737b625decfde6c64a72
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_read_execute':
> >> drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl' [-Werror=implicit-function-declaration]
>        readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
>        ^~~~~~
>    drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_write_execute':
> >> drivers/mtd/spi-nor/cadence-quadspi.c:613:3: error: implicit declaration of function 'writesl' [-Werror=implicit-function-declaration]
>       writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
>       ^~~~~~~
>    cc1: some warnings being treated as errors

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).

Brian

Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

[Stefan Roese]

On 18.07.2016 22:20, Brian Norris wrote:
> On Tue, Jul 19, 2016 at 03:43:17AM +0800, kbuild test robot wrote:
>> tree:   git://git.infradead.org/linux-mtd-next.git master
>> head:   f78921b9020c510ed222a6c2402e2aa126432415
>> commit: 140623410536905fa6ab737b625decfde6c64a72 [30/33] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller
>> config: x86_64-allmodconfig (attached as .config)
>> compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
>> reproduce:
>>          git checkout 140623410536905fa6ab737b625decfde6c64a72
>>          # save the attached .config to linux build tree
>>          make ARCH=x86_64
>>
>> All errors (new ones prefixed by >>):
>>
>>     drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_read_execute':
>>>> drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl' [-Werror=implicit-function-declaration]
>>         readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
>>         ^~~~~~
>>     drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_write_execute':
>>>> drivers/mtd/spi-nor/cadence-quadspi.c:613:3: error: implicit declaration of function 'writesl' [-Werror=implicit-function-declaration]
>>        writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
>>        ^~~~~~~
>>     cc1: some warnings being treated as errors
>
> 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.

Thanks,
Stefan

[PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Brian Norris]

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.

If I could get an ack on something like this, I'll apply it soon:

---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.
 
-- 

Re: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Marek Vasut]

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.
>  
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Brian Norris]

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

Re: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Brian Norris]

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

Re: [PATCH] mtd: spi-nor: don't build Cadence QuadSPI on non-ARM

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut

Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

[Marek Vasut]

On 07/19/2016 08:03 AM, Stefan Roese wrote:
> On 18.07.2016 22:20, Brian Norris wrote:
>> On Tue, Jul 19, 2016 at 03:43:17AM +0800, kbuild test robot wrote:
>>> tree:   git://git.infradead.org/linux-mtd-next.git master
>>> head:   f78921b9020c510ed222a6c2402e2aa126432415
>>> commit: 140623410536905fa6ab737b625decfde6c64a72 [30/33] mtd:
>>> spi-nor: Add driver for Cadence Quad SPI Flash Controller
>>> config: x86_64-allmodconfig (attached as .config)
>>> compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
>>> reproduce:
>>>          git checkout 140623410536905fa6ab737b625decfde6c64a72
>>>          # save the attached .config to linux build tree
>>>          make ARCH=x86_64
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>     drivers/mtd/spi-nor/cadence-quadspi.c: In function
>>> 'cqspi_indirect_read_execute':
>>>>> drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit
>>>>> declaration of function 'readsl'
>>>>> [-Werror=implicit-function-declaration]
>>>         readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
>>>         ^~~~~~
>>>     drivers/mtd/spi-nor/cadence-quadspi.c: In function
>>> 'cqspi_indirect_write_execute':
>>>>> drivers/mtd/spi-nor/cadence-quadspi.c:613:3: error: implicit
>>>>> declaration of function 'writesl'
>>>>> [-Werror=implicit-function-declaration]
>>>        writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
>>>        ^~~~~~~
>>>     cc1: some warnings being treated as errors
>>
>> 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.

Indeed, they should and they were used to fix other drivers too.
I'll send a patch in a bit.

-- 
Best regards,
Marek Vasut