| Message ID | 20160714013242.GG54628@google.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Cyrille, On Wed, Jul 13, 2016 at 06:32:42PM -0700, Brian Norris wrote: > On Mon, Jun 13, 2016 at 05:10:26PM +0200, Cyrille Pitchen wrote: > > This driver add support to the new Atmel QSPI controller embedded into > > sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI > > controller. > > > > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > > --- > > drivers/mtd/spi-nor/Kconfig | 9 + > > drivers/mtd/spi-nor/Makefile | 1 + > > drivers/mtd/spi-nor/atmel-quadspi.c | 741 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 751 insertions(+) > > create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c > > > > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig > > index d42c98e1f581..c546efd1357b 100644 > > --- a/drivers/mtd/spi-nor/Kconfig > > +++ b/drivers/mtd/spi-nor/Kconfig > > @@ -38,6 +38,15 @@ config SPI_FSL_QUADSPI > > This controller does not support generic SPI. It only supports > > SPI NOR. > > > > +config SPI_ATMEL_QUADSPI > > + tristate "Atmel Quad SPI Controller" > > + depends on ARCH_AT91 || (ARM && COMPILE_TEST) > > + depends on OF && HAS_IOMEM > > + help > > + This enables support for the Quad SPI controller in master mode. > > + This driver does not support generic SPI. The implementation only > > + supports SPI NOR. > > + > > config SPI_NXP_SPIFI > > tristate "NXP SPI Flash Interface (SPIFI)" > > depends on OF && (ARCH_LPC18XX || COMPILE_TEST) > > diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile > > index 0bf3a7f81675..1525913698ad 100644 > > --- a/drivers/mtd/spi-nor/Makefile > > +++ b/drivers/mtd/spi-nor/Makefile > > @@ -2,3 +2,4 @@ obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o > > obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o > > obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o > > obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o > > +obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o > > diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c > > new file mode 100644 > > index 000000000000..06d1bf276dd0 > > --- /dev/null > > +++ b/drivers/mtd/spi-nor/atmel-quadspi.c > > @@ -0,0 +1,741 @@ > > [...] > > > +struct atmel_qspi_command { > > + union { > > + struct { > > + u32 instruction:1; > > + u32 address:3; > > + u32 mode:1; > > + u32 dummy:1; > > + u32 data:1; > > + u32 reserved:25; > > + } bits; > > Are you sure this bitfield is going to do what you want, all the time? > What about on big-endian architectures? And are you guaranteed it'll > pack properly, with no padding? IIRC, the answer to the first 2 is "no", > and I have no idea about the 3rd. Honestly, I've been scared away from > using bitfields on anything where the ordering mattered, and I thought > that's because I was hurt by the lack of guarantee once. But I easily > could be misguided. > > > + u32 word; > > + } enable; > > + u8 instruction; > > + u8 mode; > > + u8 num_mode_cycles; > > + u8 num_dummy_cycles; > > + u32 address; > > + > > + size_t buf_len; > > + const void *tx_buf; > > + void *rx_buf; > > +}; > > + > > [...] > > > +static int atmel_qspi_probe(struct platform_device *pdev) > > +{ > > + struct device_node *child, *np = pdev->dev.of_node; > > + char modalias[SPI_NAME_SIZE]; > > + const char *name = NULL; > > + struct atmel_qspi *aq; > > + struct resource *res; > > + struct spi_nor *nor; > > + struct mtd_info *mtd; > > + int irq, err = 0; > > + > > + if (of_get_child_count(np) != 1) > > + return -ENODEV; > > + child = of_get_next_child(np, NULL); > > + > > + aq = devm_kzalloc(&pdev->dev, sizeof(*aq), GFP_KERNEL); > > + if (!aq) { > > + err = -ENOMEM; > > + goto exit; > > + } > > + > > + platform_set_drvdata(pdev, aq); > > + init_completion(&aq->cmd_completion); > > + aq->pdev = pdev; > > + > > + /* Map the registers */ > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base"); > > + aq->regs = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(aq->regs)) { > > + dev_err(&pdev->dev, "missing registers\n"); > > + err = PTR_ERR(aq->regs); > > + goto exit; > > + } > > + > > + /* Map the AHB memory */ > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap"); > > + aq->mem = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(aq->mem)) { > > + dev_err(&pdev->dev, "missing AHB memory\n"); > > + err = PTR_ERR(aq->mem); > > + goto exit; > > + } > > + > > + /* Get the peripheral clock */ > > + aq->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(aq->clk)) { > > + dev_err(&pdev->dev, "missing peripheral clock\n"); > > + err = PTR_ERR(aq->clk); > > + goto exit; > > + } > > + > > + /* Enable the peripheral clock */ > > + err = clk_prepare_enable(aq->clk); > > + if (err) { > > + dev_err(&pdev->dev, "failed to enable the peripheral clock\n"); > > + goto exit; > > + } > > + > > + /* Request the IRQ */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(&pdev->dev, "missing IRQ\n"); > > + err = irq; > > + goto disable_clk; > > + } > > + err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt, > > + 0, dev_name(&pdev->dev), aq); > > + if (err) > > + goto disable_clk; > > + > > + /* Setup the spi-nor */ > > + nor = &aq->nor; > > + mtd = &nor->mtd; > > + > > + nor->dev = &pdev->dev; > > + spi_nor_set_flash_node(nor, child); > > + nor->priv = aq; > > + mtd->priv = nor; > > + > > + nor->read_reg = atmel_qspi_read_reg; > > + nor->write_reg = atmel_qspi_write_reg; > > + nor->read = atmel_qspi_read; > > + nor->write = atmel_qspi_write; > > + nor->erase = atmel_qspi_erase; > > + > > + err = of_modalias_node(child, modalias, sizeof(modalias)); > > + if (err < 0) > > + goto disable_clk; > > + > > + if (strcmp(modalias, "spi-nor")) > > + name = modalias; > > What is this modalias handling for? Are you trying to support passing in > a flash-specific string, like m25p80 does? And you're enforcing that to > be only the first entry in the 'compatible' list? Seems fragile; m25p80 > is a special case (and it's already fragile) that I'd rather not > imitate. > > I understand that we discussed extending the use of compatible for > spi-nor drivers, but I still don't think that issue is resolved. And if > we are going to extend the use of compatible, then I think we should > fixup all the spi-nor drivers in the same way. > > > + > > + err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); > > + if (err < 0) > > + goto disable_clk; > > + > > + err = atmel_qspi_init(aq); > > + if (err) > > + goto disable_clk; > > + > > + err = spi_nor_scan(nor, name, SPI_NOR_QUAD); > > + if (err) > > + goto disable_clk; > > + > > + err = mtd_device_register(mtd, NULL, 0); > > + if (err) > > + goto disable_clk; > > + > > + of_node_put(child); > > + > > + return 0; > > + > > +disable_clk: > > + clk_disable_unprepare(aq->clk); > > +exit: > > + of_node_put(child); > > + > > + return err; > > +} > > + > > +static int atmel_qspi_remove(struct platform_device *pdev) > > +{ > > + struct atmel_qspi *aq = platform_get_drvdata(pdev); > > + > > + mtd_device_unregister(&aq->nor.mtd); > > + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); > > + clk_disable_unprepare(aq->clk); > > + return 0; > > +} > > + > > + > > +static const struct of_device_id atmel_qspi_dt_ids[] = { > > + { .compatible = "atmel,sama5d2-qspi" }, > > + { /* sentinel */ } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids); > > + > > +static struct platform_driver atmel_qspi_driver = { > > + .driver = { > > + .name = "atmel_qspi", > > + .of_match_table = atmel_qspi_dt_ids, > > + }, > > + .probe = atmel_qspi_probe, > > + .remove = atmel_qspi_remove, > > +}; > > +module_platform_driver(atmel_qspi_driver); > > + > > +MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@atmel.com>"); > > +MODULE_DESCRIPTION("Atmel QSPI Controller driver"); > > +MODULE_LICENSE("GPL v2"); > > I'm likely to apply this driver, with the following diff (the bitfield > issue (if there is one) is less of a maintenance issue), barring any > major objections. > > diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c > index 06d1bf276dd0..47937d9beec6 100644 > --- a/drivers/mtd/spi-nor/atmel-quadspi.c > +++ b/drivers/mtd/spi-nor/atmel-quadspi.c > @@ -591,8 +591,6 @@ static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) > static int atmel_qspi_probe(struct platform_device *pdev) > { > struct device_node *child, *np = pdev->dev.of_node; > - char modalias[SPI_NAME_SIZE]; > - const char *name = NULL; > struct atmel_qspi *aq; > struct resource *res; > struct spi_nor *nor; > @@ -673,13 +671,6 @@ static int atmel_qspi_probe(struct platform_device *pdev) > nor->write = atmel_qspi_write; > nor->erase = atmel_qspi_erase; > > - err = of_modalias_node(child, modalias, sizeof(modalias)); > - if (err < 0) > - goto disable_clk; > - > - if (strcmp(modalias, "spi-nor")) > - name = modalias; > - > err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); > if (err < 0) > goto disable_clk; > @@ -688,7 +679,7 @@ static int atmel_qspi_probe(struct platform_device *pdev) > if (err) > goto disable_clk; > > - err = spi_nor_scan(nor, name, SPI_NOR_QUAD); > + err = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); > if (err) > goto disable_clk; > Applied to l2-mtd.git with that fixup.
On Friday, July 15, 2016 5:45:07 PM CEST Brian Norris wrote:
> Applied to l2-mtd.git with that fixup.
I'm getting this build error now on a randconfig build:
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'
On ARCH_EBSA, which doesn't build the file that contains the two
functions. I don't see any other driver on ARM using those two
functions directly. What is the specific reason for using them
here? Do you require byte-wise accesses, or could you use
the normal memcpy_toio/memcpy_fromio that turn into aligned
32-bit word accesses instead? If you have to use the non-portable
functions, maybe we can just make the driver depend on !ARCH_EBSA?
Arnd
On Mon, Jul 18, 2016 at 09:35:39PM +0200, Arnd Bergmann wrote: > On Friday, July 15, 2016 5:45:07 PM CEST Brian Norris wrote: > > Applied to l2-mtd.git with that fixup. > > I'm getting this build error now on a randconfig build: > > 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' Whoops, I noticed those during review, but I don't know why I forgot to mention them nor fix them up before applying. > On ARCH_EBSA, which doesn't build the file that contains the two > functions. I don't see any other driver on ARM using those two > functions directly. What is the specific reason for using them > here? Do you require byte-wise accesses, or could you use > the normal memcpy_toio/memcpy_fromio that turn into aligned > 32-bit word accesses instead? Good questions. I would suspect that aligned 32-bit accesses are what they're looking for, but I'm not absolutely sure. > If you have to use the non-portable > functions, maybe we can just make the driver depend on !ARCH_EBSA? I don't see an ARCH_EBSA. Did you mean ARCH_EBSA110? Or we could just drop the '|| (ARM && COMPILE_TEST)' clause for now: depends on ARCH_AT91 || (ARM && COMPILE_TEST) Brian
On Monday, July 18, 2016 12:55:11 PM CEST Brian Norris wrote: > On Mon, Jul 18, 2016 at 09:35:39PM +0200, Arnd Bergmann wrote: > > On Friday, July 15, 2016 5:45:07 PM CEST Brian Norris wrote: > > > Applied to l2-mtd.git with that fixup. > > > > I'm getting this build error now on a randconfig build: > > > > 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' > > Whoops, I noticed those during review, but I don't know why I forgot to > mention them nor fix them up before applying. > > > On ARCH_EBSA, which doesn't build the file that contains the two > > functions. I don't see any other driver on ARM using those two > > functions directly. What is the specific reason for using them > > here? Do you require byte-wise accesses, or could you use > > the normal memcpy_toio/memcpy_fromio that turn into aligned > > 32-bit word accesses instead? > > Good questions. I would suspect that aligned 32-bit accesses are what > they're looking for, but I'm not absolutely sure. Ok, so we should look at that first. If the driver supports 32-bit access, using the regular accessors will also make the transfers much faster. > > If you have to use the non-portable > > functions, maybe we can just make the driver depend on !ARCH_EBSA? > > I don't see an ARCH_EBSA. Did you mean ARCH_EBSA110? Yes, sorry for the typo. > Or we could just drop the '|| (ARM && COMPILE_TEST)' clause for now: > > depends on ARCH_AT91 || (ARM && COMPILE_TEST) I'd prefer to keep the COMPILE_TEST option, after all it's how I found the problem. On a related note, what is the ARM dependency for? Is that just for the _memcpy_toio/_memcpy_fromio? Maybe we can drop too if we find the right architecture-independent replacement for those two calls. Arnd
Hi all, Le 18/07/2016 à 21:59, Arnd Bergmann a écrit : > On Monday, July 18, 2016 12:55:11 PM CEST Brian Norris wrote: >> On Mon, Jul 18, 2016 at 09:35:39PM +0200, Arnd Bergmann wrote: >>> On Friday, July 15, 2016 5:45:07 PM CEST Brian Norris wrote: >>>> Applied to l2-mtd.git with that fixup. >>> >>> I'm getting this build error now on a randconfig build: >>> >>> 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' >> >> Whoops, I noticed those during review, but I don't know why I forgot to >> mention them nor fix them up before applying. >> >>> On ARCH_EBSA, which doesn't build the file that contains the two >>> functions. I don't see any other driver on ARM using those two >>> functions directly. What is the specific reason for using them >>> here? Do you require byte-wise accesses, or could you use >>> the normal memcpy_toio/memcpy_fromio that turn into aligned >>> 32-bit word accesses instead? >> >> Good questions. I would suspect that aligned 32-bit accesses are what >> they're looking for, but I'm not absolutely sure. > > Ok, so we should look at that first. If the driver supports 32-bit > access, using the regular accessors will also make the transfers > much faster. > >>> If you have to use the non-portable >>> functions, maybe we can just make the driver depend on !ARCH_EBSA? >> >> I don't see an ARCH_EBSA. Did you mean ARCH_EBSA110? > > Yes, sorry for the typo. > >> Or we could just drop the '|| (ARM && COMPILE_TEST)' clause for now: >> >> depends on ARCH_AT91 || (ARM && COMPILE_TEST) > > I'd prefer to keep the COMPILE_TEST option, after all it's how > I found the problem. On a related note, what is the ARM dependency > for? Is that just for the _memcpy_toio/_memcpy_fromio? Maybe we > can drop too if we find the right architecture-independent > replacement for those two calls. > > Arnd > Indeed I added the ARM dependency for the COMPILE_TEST case only for _memcpy_toio() and _memcpy_fromio(). I thought it would be enough. Also, I use _memcpy_toio() and _memcpy_fromio() on purpose as opposed to memcpy_toio() and mempcy_fromio(). I've tested the two latest functions quite a long time ago and it didn't work. If I remember, on our architecture, memcpy_toio() and memcpy_fromio() are simply implemented with the regular memcpy(). The additional memory barriers inserted by readb() and writeb() seems to be needed to guarantee the IO accesses are not reordered on the system bus. In the atmel-quadspi.c driver, there is a comment on the use of memcpy(): /* Then fallback to a PIO transfer (memcpy() DOES NOT work!) */ Best regards, Cyrille
Hi Brian, Le 16/07/2016 à 02:45, Brian Norris a écrit : > Hi Cyrille, > > On Wed, Jul 13, 2016 at 06:32:42PM -0700, Brian Norris wrote: >> On Mon, Jun 13, 2016 at 05:10:26PM +0200, Cyrille Pitchen wrote: >>> This driver add support to the new Atmel QSPI controller embedded into >>> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI >>> controller. >>> >>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>> --- >>> drivers/mtd/spi-nor/Kconfig | 9 + >>> drivers/mtd/spi-nor/Makefile | 1 + >>> drivers/mtd/spi-nor/atmel-quadspi.c | 741 ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 751 insertions(+) >>> create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c >>> >>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>> index d42c98e1f581..c546efd1357b 100644 >>> --- a/drivers/mtd/spi-nor/Kconfig >>> +++ b/drivers/mtd/spi-nor/Kconfig >>> @@ -38,6 +38,15 @@ config SPI_FSL_QUADSPI >>> This controller does not support generic SPI. It only supports >>> SPI NOR. >>> >>> +config SPI_ATMEL_QUADSPI >>> + tristate "Atmel Quad SPI Controller" >>> + depends on ARCH_AT91 || (ARM && COMPILE_TEST) >>> + depends on OF && HAS_IOMEM >>> + help >>> + This enables support for the Quad SPI controller in master mode. >>> + This driver does not support generic SPI. The implementation only >>> + supports SPI NOR. >>> + >>> config SPI_NXP_SPIFI >>> tristate "NXP SPI Flash Interface (SPIFI)" >>> depends on OF && (ARCH_LPC18XX || COMPILE_TEST) >>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile >>> index 0bf3a7f81675..1525913698ad 100644 >>> --- a/drivers/mtd/spi-nor/Makefile >>> +++ b/drivers/mtd/spi-nor/Makefile >>> @@ -2,3 +2,4 @@ obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o >>> obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o >>> obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o >>> obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o >>> +obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o >>> diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c >>> new file mode 100644 >>> index 000000000000..06d1bf276dd0 >>> --- /dev/null >>> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >>> @@ -0,0 +1,741 @@ >> >> [...] >> >>> +struct atmel_qspi_command { >>> + union { >>> + struct { >>> + u32 instruction:1; >>> + u32 address:3; >>> + u32 mode:1; >>> + u32 dummy:1; >>> + u32 data:1; >>> + u32 reserved:25; >>> + } bits; >> >> Are you sure this bitfield is going to do what you want, all the time? >> What about on big-endian architectures? And are you guaranteed it'll >> pack properly, with no padding? IIRC, the answer to the first 2 is "no", >> and I have no idea about the 3rd. Honestly, I've been scared away from >> using bitfields on anything where the ordering mattered, and I thought >> that's because I was hurt by the lack of guarantee once. But I easily >> could be misguided. >> The struct atmel_qspi_command is used by the driver only for internal purpose, especially the data are not sent to the hardware, they are only used by the CPU. Hence the actual order of the fields does not matter. On a little-endian architecture, instruction will be bit0, then address bits[3:1] and so on up to 31 whereas on a big-endian architecture, instruction would be bit31 then address bits[30:28] down to 0. No field crosses the byte boundary so I guess there should be no issue even on a big-endian architecture. I could have also use u8 type like this: u8 instruction u8 address u8 mode u8 dummy u8 data However I've just wanted to reduce the size of the structure (not a big deal here). Otherwise I could have use bitmask but I've just thought bitfields would have been a little bit more easy to read. For instance: #define ATMEL_QSPI_ENABLE_CMD BIT(0) #define ATMEL_QSPI_ENABLE_ADDR_MSK GENMASK(3, 1) #define ATMEL_QSPI_ENABLE_ADDR(addr) \ (((addr) << 1) & ATMEL_QSPI_ENABLE_ADDR_MSK) #define ATMEL_QSPI_ENABLE_MODE BIT(4) #define ATMEL_QSPI_ENABLE_DUMMY BIT(5) #define ATMEL_QSPI_ENABLE_DATA BIT(6) u32 enable; .enable = (ATMEL_QSPI_ENABLE_CMD | ATMEL_QSPI_ENABLE_ADDR(3) | ATMEL_QSPI_ENABLE_DUMMY | ATMEL_QSPI_ENABLE_DATA); Maybe a little bit more optimal considering the generated code and endianness independent but a little less easy to read? >>> + u32 word; >>> + } enable; >>> + u8 instruction; >>> + u8 mode; >>> + u8 num_mode_cycles; >>> + u8 num_dummy_cycles; >>> + u32 address; >>> + >>> + size_t buf_len; >>> + const void *tx_buf; >>> + void *rx_buf; >>> +}; >>> + >> >> [...] >> >>> +static int atmel_qspi_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *child, *np = pdev->dev.of_node; >>> + char modalias[SPI_NAME_SIZE]; >>> + const char *name = NULL; >>> + struct atmel_qspi *aq; >>> + struct resource *res; >>> + struct spi_nor *nor; >>> + struct mtd_info *mtd; >>> + int irq, err = 0; >>> + >>> + if (of_get_child_count(np) != 1) >>> + return -ENODEV; >>> + child = of_get_next_child(np, NULL); >>> + >>> + aq = devm_kzalloc(&pdev->dev, sizeof(*aq), GFP_KERNEL); >>> + if (!aq) { >>> + err = -ENOMEM; >>> + goto exit; >>> + } >>> + >>> + platform_set_drvdata(pdev, aq); >>> + init_completion(&aq->cmd_completion); >>> + aq->pdev = pdev; >>> + >>> + /* Map the registers */ >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base"); >>> + aq->regs = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(aq->regs)) { >>> + dev_err(&pdev->dev, "missing registers\n"); >>> + err = PTR_ERR(aq->regs); >>> + goto exit; >>> + } >>> + >>> + /* Map the AHB memory */ >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap"); >>> + aq->mem = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(aq->mem)) { >>> + dev_err(&pdev->dev, "missing AHB memory\n"); >>> + err = PTR_ERR(aq->mem); >>> + goto exit; >>> + } >>> + >>> + /* Get the peripheral clock */ >>> + aq->clk = devm_clk_get(&pdev->dev, NULL); >>> + if (IS_ERR(aq->clk)) { >>> + dev_err(&pdev->dev, "missing peripheral clock\n"); >>> + err = PTR_ERR(aq->clk); >>> + goto exit; >>> + } >>> + >>> + /* Enable the peripheral clock */ >>> + err = clk_prepare_enable(aq->clk); >>> + if (err) { >>> + dev_err(&pdev->dev, "failed to enable the peripheral clock\n"); >>> + goto exit; >>> + } >>> + >>> + /* Request the IRQ */ >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) { >>> + dev_err(&pdev->dev, "missing IRQ\n"); >>> + err = irq; >>> + goto disable_clk; >>> + } >>> + err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt, >>> + 0, dev_name(&pdev->dev), aq); >>> + if (err) >>> + goto disable_clk; >>> + >>> + /* Setup the spi-nor */ >>> + nor = &aq->nor; >>> + mtd = &nor->mtd; >>> + >>> + nor->dev = &pdev->dev; >>> + spi_nor_set_flash_node(nor, child); >>> + nor->priv = aq; >>> + mtd->priv = nor; >>> + >>> + nor->read_reg = atmel_qspi_read_reg; >>> + nor->write_reg = atmel_qspi_write_reg; >>> + nor->read = atmel_qspi_read; >>> + nor->write = atmel_qspi_write; >>> + nor->erase = atmel_qspi_erase; >>> + >>> + err = of_modalias_node(child, modalias, sizeof(modalias)); >>> + if (err < 0) >>> + goto disable_clk; >>> + >>> + if (strcmp(modalias, "spi-nor")) >>> + name = modalias; >> >> What is this modalias handling for? Are you trying to support passing in >> a flash-specific string, like m25p80 does? And you're enforcing that to >> be only the first entry in the 'compatible' list? Seems fragile; m25p80 >> is a special case (and it's already fragile) that I'd rather not >> imitate. >> >> I understand that we discussed extending the use of compatible for >> spi-nor drivers, but I still don't think that issue is resolved. And if >> we are going to extend the use of compatible, then I think we should >> fixup all the spi-nor drivers in the same way. >> Yes you're right this part of the code was introduce at the driver side when I had to use the Macronix mx25l25673g memory, which shares the very same JEDEC ID as other Macronix memories but with other hardware features. I no longer use it since I use my series about SFDP parsing. So I'm perfectly fine with removing this modalias handling from the driver. Now I guess I will focus on the SFDP solution. Also this qspi driver is ported to mailine from our linux-at91 kernels since we need to provide users a way to support QSPI memories in Linux with our QSPI controller. >>> + >>> + err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); >>> + if (err < 0) >>> + goto disable_clk; >>> + >>> + err = atmel_qspi_init(aq); >>> + if (err) >>> + goto disable_clk; >>> + >>> + err = spi_nor_scan(nor, name, SPI_NOR_QUAD); >>> + if (err) >>> + goto disable_clk; >>> + >>> + err = mtd_device_register(mtd, NULL, 0); >>> + if (err) >>> + goto disable_clk; >>> + >>> + of_node_put(child); >>> + >>> + return 0; >>> + >>> +disable_clk: >>> + clk_disable_unprepare(aq->clk); >>> +exit: >>> + of_node_put(child); >>> + >>> + return err; >>> +} >>> + >>> +static int atmel_qspi_remove(struct platform_device *pdev) >>> +{ >>> + struct atmel_qspi *aq = platform_get_drvdata(pdev); >>> + >>> + mtd_device_unregister(&aq->nor.mtd); >>> + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); >>> + clk_disable_unprepare(aq->clk); >>> + return 0; >>> +} >>> + >>> + >>> +static const struct of_device_id atmel_qspi_dt_ids[] = { >>> + { .compatible = "atmel,sama5d2-qspi" }, >>> + { /* sentinel */ } >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids); >>> + >>> +static struct platform_driver atmel_qspi_driver = { >>> + .driver = { >>> + .name = "atmel_qspi", >>> + .of_match_table = atmel_qspi_dt_ids, >>> + }, >>> + .probe = atmel_qspi_probe, >>> + .remove = atmel_qspi_remove, >>> +}; >>> +module_platform_driver(atmel_qspi_driver); >>> + >>> +MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@atmel.com>"); >>> +MODULE_DESCRIPTION("Atmel QSPI Controller driver"); >>> +MODULE_LICENSE("GPL v2"); >> >> I'm likely to apply this driver, with the following diff (the bitfield >> issue (if there is one) is less of a maintenance issue), barring any >> major objections. >> >> diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c >> index 06d1bf276dd0..47937d9beec6 100644 >> --- a/drivers/mtd/spi-nor/atmel-quadspi.c >> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c >> @@ -591,8 +591,6 @@ static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) >> static int atmel_qspi_probe(struct platform_device *pdev) >> { >> struct device_node *child, *np = pdev->dev.of_node; >> - char modalias[SPI_NAME_SIZE]; >> - const char *name = NULL; >> struct atmel_qspi *aq; >> struct resource *res; >> struct spi_nor *nor; >> @@ -673,13 +671,6 @@ static int atmel_qspi_probe(struct platform_device *pdev) >> nor->write = atmel_qspi_write; >> nor->erase = atmel_qspi_erase; >> >> - err = of_modalias_node(child, modalias, sizeof(modalias)); >> - if (err < 0) >> - goto disable_clk; >> - >> - if (strcmp(modalias, "spi-nor")) >> - name = modalias; >> - >> err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); >> if (err < 0) >> goto disable_clk; >> @@ -688,7 +679,7 @@ static int atmel_qspi_probe(struct platform_device *pdev) >> if (err) >> goto disable_clk; >> >> - err = spi_nor_scan(nor, name, SPI_NOR_QUAD); >> + err = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); >> if (err) >> goto disable_clk; >> > > Applied to l2-mtd.git with that fixup. > It's OK for me, thanks! :) Best regards, Cyrille
On Tuesday, July 19, 2016 12:03:11 PM CEST Cyrille Pitchen wrote: > > Indeed I added the ARM dependency for the COMPILE_TEST case only for > _memcpy_toio() and _memcpy_fromio(). I thought it would be enough. > > Also, I use _memcpy_toio() and _memcpy_fromio() on purpose as opposed to > memcpy_toio() and mempcy_fromio(). I've tested the two latest functions quite > a long time ago and it didn't work. If I remember, on our architecture, > memcpy_toio() and memcpy_fromio() are simply implemented with the regular > memcpy(). They are implemented using "mmiocpy" when CONFIG_BIG_ENDIAN is not set. mmiocpy is an alias for the out-of-line memcpy() implementation, but it's different from calling memcpy() as the compiler can decide to replace small memcpy with a pointer dereference, and that can be unaligned on ARMv7 for non-MMIO pointers. > The additional memory barriers inserted by readb() and writeb() seems to be > needed to guarantee the IO accesses are not reordered on the system bus. Those barriers should not be needed at all, I've never seen an I/O bus that reorders consecutive reads to the same address range. > In the atmel-quadspi.c driver, there is a comment on the use of memcpy(): > /* Then fallback to a PIO transfer (memcpy() DOES NOT work!) */ Please try again, I think this was fixed with commit 1bd46782d08b01b73df0085b51ea1021b19b44fd Author: Russell King <rmk+kernel@arm.linux.org.uk> Date: Fri Jul 3 15:22:54 2015 +0100 ARM: avoid unwanted GCC memset()/memcpy() optimisations for IO variants We don't want GCC optimising our memset_io(), memcpy_fromio() or memcpy_toio() variants, so we must not call one of the standard functions. Provide a separate name for our assembly memcpy() and memset() functions, and use that instead, thereby bypassing GCC's ability to optimise these operations. GCCs optimisation may introduce unaligned accesses which are invalid for device mappings. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Arnd
diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c index 06d1bf276dd0..47937d9beec6 100644 --- a/drivers/mtd/spi-nor/atmel-quadspi.c +++ b/drivers/mtd/spi-nor/atmel-quadspi.c @@ -591,8 +591,6 @@ static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) static int atmel_qspi_probe(struct platform_device *pdev) { struct device_node *child, *np = pdev->dev.of_node; - char modalias[SPI_NAME_SIZE]; - const char *name = NULL; struct atmel_qspi *aq; struct resource *res; struct spi_nor *nor; @@ -673,13 +671,6 @@ static int atmel_qspi_probe(struct platform_device *pdev) nor->write = atmel_qspi_write; nor->erase = atmel_qspi_erase; - err = of_modalias_node(child, modalias, sizeof(modalias)); - if (err < 0) - goto disable_clk; - - if (strcmp(modalias, "spi-nor")) - name = modalias; - err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); if (err < 0) goto disable_clk; @@ -688,7 +679,7 @@ static int atmel_qspi_probe(struct platform_device *pdev) if (err) goto disable_clk; - err = spi_nor_scan(nor, name, SPI_NOR_QUAD); + err = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); if (err) goto disable_clk;