| Message ID | 20170721222204.3402340-1-arnd@arndb.de |
|---|---|
| State | Rejected |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
Hi Arnd, Le 22/07/2017 à 00:21, Arnd Bergmann a écrit : > I ran into a link-time error with the atmel-quadspi driver on the > EBSA110 platform: > > drivers/mtd/built-in.o: In function `atmel_qspi_run_command': > :(.text+0x1ee3c): undefined reference to `_memcpy_toio' > :(.text+0x1ee48): undefined reference to `_memcpy_fromio' > > The problem is that _memcpy_toio/_memcpy_fromio are not available > on that platform, and we have to prevent building the driver there. > > A related problem is that the functions are not portable APIs > and should not be called directly from a device driver. On > little-endian machines, the regular memcpy_toio/memcpy_fromio > functions are defined as optimized versions using multi-byte > transfers that are much faster. > > Cyrille mentioned that initially using memcpy_toio/memcpy_fromio > did not work, but I suspect that this was the result of a bug > that has since been fixed. With that change, we can also > compile-test on other architectures. > > Link: http://lists.infradead.org/pipermail/linux-mtd/2016-July/068583.html > Fixes: 161aaab8a067 ("mtd: atmel-quadspi: add driver for Atmel QSPI controller") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mtd/spi-nor/Kconfig | 2 +- > drivers/mtd/spi-nor/atmel-quadspi.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig > index 293c8a4d1e49..22e5fc4080f8 100644 > --- a/drivers/mtd/spi-nor/Kconfig > +++ b/drivers/mtd/spi-nor/Kconfig > @@ -41,7 +41,7 @@ config SPI_ASPEED_SMC > > config SPI_ATMEL_QUADSPI > tristate "Atmel Quad SPI Controller" > - depends on ARCH_AT91 || (ARM && COMPILE_TEST) > + depends on ARCH_AT91 || (COMPILE_TEST && !ARCH_EBSA110) > depends on OF && HAS_IOMEM > help > This enables support for the Quad SPI controller in master mode. > diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c > index ba76fa8f2031..ff3849106e77 100644 > --- a/drivers/mtd/spi-nor/atmel-quadspi.c > +++ b/drivers/mtd/spi-nor/atmel-quadspi.c > @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, > if (cmd->enable.bits.address) > ahb_mem += cmd->address; > if (cmd->tx_buf) > - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); > + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); > else > - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); > + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); > At least on AT91 platforms and likely on most ARM boards, memcpy_fromio == memcpy_toio == memcpy. I got some sama5d2 hardware last week, so I'll try to test your patch within few days because as you said maybe memcpy() was broken when I developed this driver at first but now memcpy() is likely to have been fixed so it might be interesting to get rid of _memcpy_fromio() and _memcpy_toio() because this is not the first time those 2 functions have created issues when building the Atmel Quad SPI driver on other platforms. So thanks for you're patch, I'll give it a try :) Best regards, Cyrille > return 0; > } >
On Tue, Aug 1, 2017 at 9:41 PM, Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote: > Le 22/07/2017 à 00:21, Arnd Bergmann a écrit : >> --- a/drivers/mtd/spi-nor/atmel-quadspi.c >> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >> @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, >> if (cmd->enable.bits.address) >> ahb_mem += cmd->address; >> if (cmd->tx_buf) >> - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >> + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >> else >> - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >> + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >> > > At least on AT91 platforms and likely on most ARM boards, > memcpy_fromio == memcpy_toio == memcpy. Just to give more background, it's a bit more complicated than this: On big-endian kernels, memcpy_fromio/memcpy_toio are always defined as _memcpy_fromio/_memcpy_toio so we do byte load/store operations in the correct order, while little-endian kernels have the optimized mmiocpy() that redirects to memcpy(). This is true for all ARM platforms other than EBSA110 IIRC. Also, mmiocpy is an exported symbol that aliases to the external memcpy definition, but we can't call memcpy directly, because gcc knows how to inline calls to memcpy() and replace them by direct load/store instructions that might be unaligned and trap on uncached mmio areas. > I got some sama5d2 hardware last week, so I'll try to test your patch > within few days because as you said maybe memcpy() was broken when I > developed this driver at first but now memcpy() is likely to have been > fixed so it might be interesting to get rid of _memcpy_fromio() and > _memcpy_toio() because this is not the first time those 2 functions have > created issues when building the Atmel Quad SPI driver on other platforms. > > So thanks for you're patch, I'll give it a try :) Ok, thanks. Arnd
Hi Arnd, + Nicolas Ferre Le 01/08/2017 à 22:10, Arnd Bergmann a écrit : > On Tue, Aug 1, 2017 at 9:41 PM, Cyrille Pitchen > <cyrille.pitchen@wedev4u.fr> wrote: >> Le 22/07/2017 à 00:21, Arnd Bergmann a écrit : > >>> --- a/drivers/mtd/spi-nor/atmel-quadspi.c >>> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >>> @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, >>> if (cmd->enable.bits.address) >>> ahb_mem += cmd->address; >>> if (cmd->tx_buf) >>> - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>> + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>> else >>> - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>> + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>> >> >> At least on AT91 platforms and likely on most ARM boards, >> memcpy_fromio == memcpy_toio == memcpy. > > Just to give more background, it's a bit more complicated than this: > > On big-endian kernels, memcpy_fromio/memcpy_toio are > always defined as _memcpy_fromio/_memcpy_toio so we > do byte load/store operations in the correct order, while > little-endian kernels have the optimized mmiocpy() that redirects > to memcpy(). This is true for all ARM platforms other than EBSA110 > IIRC. > > Also, mmiocpy is an exported symbol that aliases to the external > memcpy definition, but we can't call memcpy directly, because gcc > knows how to inline calls to memcpy() and replace them by direct > load/store instructions that might be unaligned and trap on uncached > mmio areas. > >> I got some sama5d2 hardware last week, so I'll try to test your patch >> within few days because as you said maybe memcpy() was broken when I >> developed this driver at first but now memcpy() is likely to have been >> fixed so it might be interesting to get rid of _memcpy_fromio() and >> _memcpy_toio() because this is not the first time those 2 functions have >> created issues when building the Atmel Quad SPI driver on other platforms. >> >> So thanks for you're patch, I'll give it a try :) > Bad news: I've just tested this patch, applied onto the spi-nor/next branch of l2-mtd (based on 4.14.0-rc4), with a sama5d2 xplained board + Macronix MX25L25673G Quad SPI memory but it fails on the very first SPI command, Read JEDEC ID (9Fh): atmel_qspi f0020000.spi: unrecognized JEDEC id bytes: c2, 20, 20 atmel_qspi: probe of f0020000.spi failed with error -2 Best regards, Cyrille > Ok, thanks. > > Arnd >
On Fri, Oct 13, 2017 at 10:28 PM, Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote: > Hi Arnd, > > + Nicolas Ferre > > Le 01/08/2017 à 22:10, Arnd Bergmann a écrit : >> On Tue, Aug 1, 2017 at 9:41 PM, Cyrille Pitchen >> <cyrille.pitchen@wedev4u.fr> wrote: >>> Le 22/07/2017 à 00:21, Arnd Bergmann a écrit : >> >>>> --- a/drivers/mtd/spi-nor/atmel-quadspi.c >>>> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >>>> @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, >>>> if (cmd->enable.bits.address) >>>> ahb_mem += cmd->address; >>>> if (cmd->tx_buf) >>>> - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>>> + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>>> else >>>> - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>>> + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>>> >>> >>> At least on AT91 platforms and likely on most ARM boards, >>> memcpy_fromio == memcpy_toio == memcpy. >> >> Just to give more background, it's a bit more complicated than this: >> >> On big-endian kernels, memcpy_fromio/memcpy_toio are >> always defined as _memcpy_fromio/_memcpy_toio so we >> do byte load/store operations in the correct order, while >> little-endian kernels have the optimized mmiocpy() that redirects >> to memcpy(). This is true for all ARM platforms other than EBSA110 >> IIRC. >> >> Also, mmiocpy is an exported symbol that aliases to the external >> memcpy definition, but we can't call memcpy directly, because gcc >> knows how to inline calls to memcpy() and replace them by direct >> load/store instructions that might be unaligned and trap on uncached >> mmio areas. >> >>> I got some sama5d2 hardware last week, so I'll try to test your patch >>> within few days because as you said maybe memcpy() was broken when I >>> developed this driver at first but now memcpy() is likely to have been >>> fixed so it might be interesting to get rid of _memcpy_fromio() and >>> _memcpy_toio() because this is not the first time those 2 functions have >>> created issues when building the Atmel Quad SPI driver on other platforms. >>> >>> So thanks for you're patch, I'll give it a try :) >> > > Bad news: > > I've just tested this patch, applied onto the spi-nor/next branch of > l2-mtd (based on 4.14.0-rc4), with a sama5d2 xplained board + Macronix > MX25L25673G Quad SPI memory but it fails on the very first SPI command, > Read JEDEC ID (9Fh): > > atmel_qspi f0020000.spi: unrecognized JEDEC id bytes: c2, 20, 20 > atmel_qspi: probe of f0020000.spi failed with error -2 Ah, too bad. Should we just add the !ARCH_EBSA110 dependency then? Arnd
Hi Arnd, Le 16/10/2017 à 13:53, Arnd Bergmann a écrit : > On Fri, Oct 13, 2017 at 10:28 PM, Cyrille Pitchen > <cyrille.pitchen@wedev4u.fr> wrote: >> Hi Arnd, >> >> + Nicolas Ferre >> >> Le 01/08/2017 à 22:10, Arnd Bergmann a écrit : >>> On Tue, Aug 1, 2017 at 9:41 PM, Cyrille Pitchen >>> <cyrille.pitchen@wedev4u.fr> wrote: >>>> Le 22/07/2017 à 00:21, Arnd Bergmann a écrit : >>> >>>>> --- a/drivers/mtd/spi-nor/atmel-quadspi.c >>>>> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >>>>> @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, >>>>> if (cmd->enable.bits.address) >>>>> ahb_mem += cmd->address; >>>>> if (cmd->tx_buf) >>>>> - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>>>> + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); >>>>> else >>>>> - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>>>> + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); >>>>> >>>> >>>> At least on AT91 platforms and likely on most ARM boards, >>>> memcpy_fromio == memcpy_toio == memcpy. >>> >>> Just to give more background, it's a bit more complicated than this: >>> >>> On big-endian kernels, memcpy_fromio/memcpy_toio are >>> always defined as _memcpy_fromio/_memcpy_toio so we >>> do byte load/store operations in the correct order, while >>> little-endian kernels have the optimized mmiocpy() that redirects >>> to memcpy(). This is true for all ARM platforms other than EBSA110 >>> IIRC. >>> >>> Also, mmiocpy is an exported symbol that aliases to the external >>> memcpy definition, but we can't call memcpy directly, because gcc >>> knows how to inline calls to memcpy() and replace them by direct >>> load/store instructions that might be unaligned and trap on uncached >>> mmio areas. >>> >>>> I got some sama5d2 hardware last week, so I'll try to test your patch >>>> within few days because as you said maybe memcpy() was broken when I >>>> developed this driver at first but now memcpy() is likely to have been >>>> fixed so it might be interesting to get rid of _memcpy_fromio() and >>>> _memcpy_toio() because this is not the first time those 2 functions have >>>> created issues when building the Atmel Quad SPI driver on other platforms. >>>> >>>> So thanks for you're patch, I'll give it a try :) >>> >> >> Bad news: >> >> I've just tested this patch, applied onto the spi-nor/next branch of >> l2-mtd (based on 4.14.0-rc4), with a sama5d2 xplained board + Macronix >> MX25L25673G Quad SPI memory but it fails on the very first SPI command, >> Read JEDEC ID (9Fh): >> >> atmel_qspi f0020000.spi: unrecognized JEDEC id bytes: c2, 20, 20 >> atmel_qspi: probe of f0020000.spi failed with error -2 > > Ah, too bad. Should we just add the !ARCH_EBSA110 dependency > then? If it fixes the issue then I'm fine with this proposal. Best regards, Cyrille > > Arnd >
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig index 293c8a4d1e49..22e5fc4080f8 100644 --- a/drivers/mtd/spi-nor/Kconfig +++ b/drivers/mtd/spi-nor/Kconfig @@ -41,7 +41,7 @@ config SPI_ASPEED_SMC config SPI_ATMEL_QUADSPI tristate "Atmel Quad SPI Controller" - depends on ARCH_AT91 || (ARM && COMPILE_TEST) + depends on ARCH_AT91 || (COMPILE_TEST && !ARCH_EBSA110) depends on OF && HAS_IOMEM help This enables support for the Quad SPI controller in master mode. diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c index ba76fa8f2031..ff3849106e77 100644 --- a/drivers/mtd/spi-nor/atmel-quadspi.c +++ b/drivers/mtd/spi-nor/atmel-quadspi.c @@ -208,9 +208,9 @@ static int atmel_qspi_run_transfer(struct atmel_qspi *aq, if (cmd->enable.bits.address) ahb_mem += cmd->address; if (cmd->tx_buf) - _memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); + memcpy_toio(ahb_mem, cmd->tx_buf, cmd->buf_len); else - _memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); + memcpy_fromio(cmd->rx_buf, ahb_mem, cmd->buf_len); return 0; }
I ran into a link-time error with the atmel-quadspi driver on the EBSA110 platform: drivers/mtd/built-in.o: In function `atmel_qspi_run_command': :(.text+0x1ee3c): undefined reference to `_memcpy_toio' :(.text+0x1ee48): undefined reference to `_memcpy_fromio' The problem is that _memcpy_toio/_memcpy_fromio are not available on that platform, and we have to prevent building the driver there. A related problem is that the functions are not portable APIs and should not be called directly from a device driver. On little-endian machines, the regular memcpy_toio/memcpy_fromio functions are defined as optimized versions using multi-byte transfers that are much faster. Cyrille mentioned that initially using memcpy_toio/memcpy_fromio did not work, but I suspect that this was the result of a bug that has since been fixed. With that change, we can also compile-test on other architectures. Link: http://lists.infradead.org/pipermail/linux-mtd/2016-July/068583.html Fixes: 161aaab8a067 ("mtd: atmel-quadspi: add driver for Atmel QSPI controller") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/mtd/spi-nor/Kconfig | 2 +- drivers/mtd/spi-nor/atmel-quadspi.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)