| Message ID | 20180618162124.21749-2-bugalski.piotr@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 18, 2018 at 06:21:23PM +0200, Piotr Bugalski wrote: > +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > +{ > + return 0; > +} If this can be empty should we adjust the callers to allow it to just be omitted? > +static int atmel_qspi_remove(struct platform_device *pdev) > +{ > + struct spi_controller *ctrl = platform_get_drvdata(pdev); > + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); > + > + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); > + clk_disable_unprepare(aq->clk); > + > + spi_unregister_controller(ctrl); > + > + return 0; > +} You should unregister the controller before disabling the hardware, otherwise something could come in and try to start an operation on the controller (or already be running one) while the hardware is disabled which might blow up.
Hi Mark, Thank you very much for quick answer. On Tue, 19 Jun 2018, Mark Brown wrote: > On Mon, Jun 18, 2018 at 06:21:23PM +0200, Piotr Bugalski wrote: > >> +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) >> +{ >> + return 0; >> +} > > If this can be empty should we adjust the callers to allow it to just be > omitted? > If I remember well some commits ago spi-mem required even empty adjust_op_size. Now it seems unnecessary, but I forgot to remove the code. I will fix it in next version. >> +static int atmel_qspi_remove(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctrl = platform_get_drvdata(pdev); >> + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); >> + >> + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); >> + clk_disable_unprepare(aq->clk); >> + >> + spi_unregister_controller(ctrl); >> + >> + return 0; >> +} > > You should unregister the controller before disabling the hardware, > otherwise something could come in and try to start an operation on the > controller (or already be running one) while the hardware is disabled > which might blow up. > Sure, deinit should be done in reverse order of init, you are perfectly right, just my mistake. I'll fix it in next version. Best Regards, Piotr -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Piotr, On Mon, 18 Jun 2018 18:21:23 +0200 Piotr Bugalski <bugalski.piotr@gmail.com> wrote: > Kernel contains QSPI driver strongly tied to MTD and nor-flash memory. > New spi-mem interface allows usage also other memory types, especially > much larger NAND with SPI interface. This driver works as SPI controller > and is not related to MTD, however can work with NAND-flash or other > peripherals using spi-mem interface. Thanks for working on that. > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > Signed-off-by: Piotr Bugalski <pbu@cryptera.com> > > --- > drivers/spi/Kconfig | 9 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 490 insertions(+) > create mode 100644 drivers/spi/spi-atmel-qspi.c > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > index 4a77cfa3213d..4f70a7005997 100644 > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -84,6 +84,15 @@ config SPI_ATMEL > This selects a driver for the Atmel SPI Controller, present on > many AT91 ARM chips. > > +config SPI_ATMEL_QSPI > + tristate "Atmel QuadSPI Controller" > + depends on ARCH_AT91 || (ARM && COMPILE_TEST) > + depends on OF && HAS_IOMEM > + help > + This selects a driver for the Atmel QSPI Controller on SAMA5D2. > + This controller does not support generic SPI, it supports only > + spi-mem interface. > + > config SPI_AU1550 > tristate "Au1550/Au1200/Au1300 SPI Controller" > depends on MIPS_ALCHEMY > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > index fe54d787cf4d..6245a5693b16 100644 > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o > obj-$(CONFIG_SPI_ALTERA) += spi-altera.o > obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o > obj-$(CONFIG_SPI_ATMEL) += spi-atmel.o > +obj-$(CONFIG_SPI_ATMEL_QSPI) += spi-atmel-qspi.o > obj-$(CONFIG_SPI_ATH79) += spi-ath79.o > obj-$(CONFIG_SPI_AU1550) += spi-au1550.o > obj-$(CONFIG_SPI_AXI_SPI_ENGINE) += spi-axi-spi-engine.o > diff --git a/drivers/spi/spi-atmel-qspi.c b/drivers/spi/spi-atmel-qspi.c > new file mode 100644 > index 000000000000..1ee626201b0d > --- /dev/null > +++ b/drivers/spi/spi-atmel-qspi.c > @@ -0,0 +1,480 @@ SPDX header here please: // SPDX-License-Identifier: GPL-2.0 > +/* > + * Atmel SAMA5D2 QuadSPI driver. > + * > + * Copyright (C) 2018 Cryptera A/S A non-negligible portion of this code has been copied from the existing driver. Please keep the existing copyright (you can still add Cryptera's one). > + * All Rights Reserved > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 (GPL v2) > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. Drop the license text. The SPDX header is here to replace it. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/of_device.h> > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/spi/spi-mem.h> > +#include <linux/delay.h> > + > +#define QSPI_CR 0x0000 > +#define QSPI_MR 0x0004 > +#define QSPI_RDR 0x0008 > +#define QSPI_TDR 0x000c > +#define QSPI_SR 0x0010 > +#define QSPI_IER 0x0014 > +#define QSPI_IDR 0x0018 > +#define QSPI_IMR 0x001c > +#define QSPI_SCR 0x0020 > + > +#define QSPI_IAR 0x0030 > +#define QSPI_ICR 0x0034 > +#define QSPI_IFR 0x0038 > + > +#define QSPI_WPMR 0x00e4 > +#define QSPI_WPSR 0x00e8 > + > +/* Bitfields in QSPI_CR (Control Register) */ I personally prefer when reg offsets are declared next to the reg fields. > +#define QSPI_CR_QSPIEN BIT(0) > +#define QSPI_CR_QSPIDIS BIT(1) > +#define QSPI_CR_SWRST BIT(7) > +#define QSPI_CR_LASTXFER BIT(24) > + > +/* Bitfields in QSPI_ICR (Instruction Code Register) */ > +#define QSPI_ICR_INST_MASK GENMASK(7, 0) > +#define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK) > +#define QSPI_ICR_OPT_MASK GENMASK(23, 16) > +#define QSPI_ICR_OPT(opt) (((opt) << 16) & QSPI_ICR_OPT_MASK) > + > +/* Bitfields in QSPI_MR (Mode Register) */ > +#define QSPI_MR_SMM BIT(0) > +#define QSPI_MR_LLB BIT(1) > +#define QSPI_MR_WDRBT BIT(2) > +#define QSPI_MR_SMRM BIT(3) > +#define QSPI_MR_CSMODE_MASK GENMASK(5, 4) > +#define QSPI_MR_CSMODE_NOT_RELOADED (0 << 4) > +#define QSPI_MR_CSMODE_LASTXFER (1 << 4) > +#define QSPI_MR_CSMODE_SYSTEMATICALLY (2 << 4) > +#define QSPI_MR_NBBITS_MASK GENMASK(11, 8) > +#define QSPI_MR_NBBITS(n) ((((n) - 8) << 8) & QSPI_MR_NBBITS_MASK) > +#define QSPI_MR_DLYBCT_MASK GENMASK(23, 16) > +#define QSPI_MR_DLYBCT(n) (((n) << 16) & QSPI_MR_DLYBCT_MASK) > +#define QSPI_MR_DLYCS_MASK GENMASK(31, 24) > +#define QSPI_MR_DLYCS(n) (((n) << 24) & QSPI_MR_DLYCS_MASK) > + > +/* Bitfields in QSPI_IFR (Instruction Frame Register) */ > +#define QSPI_IFR_WIDTH_MASK GENMASK(2, 0) > +#define QSPI_IFR_WIDTH_SINGLE_BIT_SPI (0 << 0) > +#define QSPI_IFR_WIDTH_DUAL_OUTPUT (1 << 0) > +#define QSPI_IFR_WIDTH_QUAD_OUTPUT (2 << 0) > +#define QSPI_IFR_WIDTH_DUAL_IO (3 << 0) > +#define QSPI_IFR_WIDTH_QUAD_IO (4 << 0) > +#define QSPI_IFR_WIDTH_DUAL_CMD (5 << 0) > +#define QSPI_IFR_WIDTH_QUAD_CMD (6 << 0) > +#define QSPI_IFR_INSTEN BIT(4) > +#define QSPI_IFR_ADDREN BIT(5) > +#define QSPI_IFR_OPTEN BIT(6) > +#define QSPI_IFR_DATAEN BIT(7) > +#define QSPI_IFR_OPTL_MASK GENMASK(9, 8) > +#define QSPI_IFR_OPTL_1BIT (0 << 8) > +#define QSPI_IFR_OPTL_2BIT (1 << 8) > +#define QSPI_IFR_OPTL_4BIT (2 << 8) > +#define QSPI_IFR_OPTL_8BIT (3 << 8) > +#define QSPI_IFR_ADDRL BIT(10) > +#define QSPI_IFR_TFRTYP_MASK GENMASK(13, 12) > +#define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) > +#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) > +#define QSPI_IFR_TFRTYP_TRSFR_WRITE (2 << 12) > +#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13) > +#define QSPI_IFR_CRM BIT(14) > +#define QSPI_IFR_NBDUM_MASK GENMASK(20, 16) > +#define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK) > + > +/* Bitfields in QSPI_SR/QSPI_IER/QSPI_IDR/QSPI_IMR */ > +#define QSPI_SR_RDRF BIT(0) > +#define QSPI_SR_TDRE BIT(1) > +#define QSPI_SR_TXEMPTY BIT(2) > +#define QSPI_SR_OVRES BIT(3) > +#define QSPI_SR_CSR BIT(8) > +#define QSPI_SR_CSS BIT(9) > +#define QSPI_SR_INSTRE BIT(10) > +#define QSPI_SR_QSPIENS BIT(24) > + > +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) Do you really to wait for both INSTRE and CSR to consider the command as complete? > + > + Drop a blank line here. > +/* Bitfields in QSPI_SCR (Serial Clock Register) */ > +#define QSPI_SCR_CPOL BIT(0) > +#define QSPI_SCR_CPHA BIT(1) > +#define QSPI_SCR_SCBR_MASK GENMASK(15, 8) > +#define QSPI_SCR_SCBR(n) (((n) << 8) & QSPI_SCR_SCBR_MASK) > +#define QSPI_SCR_DLYBS_MASK GENMASK(23, 16) > +#define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK) > + > +#define QSPI_WPMR_WPKEY_PASSWD (0x515350u << 8) > + > +struct atmel_qspi { > + struct platform_device *pdev; > + void __iomem *iobase; > + void __iomem *ahb_addr; > + int irq; This field is not used and should be killed. > + struct clk *clk; > + u32 clk_rate; This field can be removed. The same information is already stored in spi_device->max_speed_hz. > + struct completion cmd_done; > + u32 pending; > +}; > + > +struct qspi_mode { > + u8 cmd_buswidth; > + u8 addr_buswidth; > + u8 data_buswidth; > + u32 config; > +}; > + > +static const struct qspi_mode sama5d2_qspi_modes[] = { > + { 1, 1, 1, QSPI_IFR_WIDTH_SINGLE_BIT_SPI }, > + { 1, 1, 2, QSPI_IFR_WIDTH_DUAL_OUTPUT }, > + { 1, 1, 4, QSPI_IFR_WIDTH_QUAD_OUTPUT }, > + { 1, 2, 2, QSPI_IFR_WIDTH_DUAL_IO }, > + { 1, 4, 4, QSPI_IFR_WIDTH_QUAD_IO }, > + { 2, 2, 2, QSPI_IFR_WIDTH_DUAL_CMD }, > + { 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD }, > +}; > + > +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg) > +{ > + return readl_relaxed(aq->iobase + reg); > +} > + > +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value) > +{ > + writel_relaxed(value, aq->iobase + reg); > +} Please drop those wrappers and use readl/write_relaxed() directly. > + > +static int atmel_qspi_init(struct atmel_qspi *aq) > +{ > + unsigned long rate; > + u32 scbr; > + > + qspi_writel(aq, QSPI_WPMR, QSPI_WPMR_WPKEY_PASSWD); > + > + /* software reset */ > + qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST); > + > + /* set QSPI mode */ > + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); It's already done in ->exec_op(), I think you can remove it here. > + > + rate = clk_get_rate(aq->clk); > + if (!rate) > + return -EINVAL; > + > + /* set baudrate */ > + scbr = DIV_ROUND_UP(rate, aq->clk_rate); > + if (scbr > 0) > + scbr--; Add a blank line here. > + qspi_writel(aq, QSPI_SCR, QSPI_SCR_SCBR(scbr)); The baudrate setting should be done in an spi_controller->setup() hook, and the expected rate is available in spi_device->max_speed_hz. > + > + /* enable qspi controller */ > + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN); > + > + return 0; > +} > + > +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > +{ > + return 0; > +} > + > +static inline bool is_compatible(const struct spi_mem_op *op, > + const struct qspi_mode *mode) > +{ > + if (op->cmd.buswidth != mode->cmd_buswidth) > + return false; Add a blank line here > + if (op->addr.nbytes && op->addr.buswidth != mode->addr_buswidth) > + return false; here > + if (op->data.nbytes && op->data.buswidth != mode->data_buswidth) > + return false; and here. > + return true; > +} > + > +static int find_mode(const struct spi_mem_op *op) > +{ > + u32 i; > + > + for (i = 0; i < ARRAY_SIZE(sama5d2_qspi_modes); i++) > + if (is_compatible(op, &sama5d2_qspi_modes[i])) > + return i; > + return -1; return -ENOTSUPP; > +} > + > +static bool atmel_qspi_supports_op(struct spi_mem *mem, > + const struct spi_mem_op *op) > +{ > + if (find_mode(op) < 0) > + return false; > + > + // special case not supported by hardware We try to avoid using C++ style comments in the kernel. > + if ((op->addr.nbytes == 2) && (op->cmd.buswidth != op->addr.buswidth) && You don't need those extra parenthesis around each test. > + (op->dummy.nbytes == 0)) Always try to align on the open-parenthesis of the if () block: if (op->addr.nbytes == 2 && op->cmd.buswidth != op->addr.buswidth && op->dummy.nbytes == 0) > + return false; > + > + return true; > +} > + > +static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) > +{ > + struct spi_controller *ctrl = dev_id; > + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); > + u32 status, mask, pending; > + > + status = qspi_readl(aq, QSPI_SR); > + mask = qspi_readl(aq, QSPI_IMR); > + pending = status & mask; > + > + if (!pending) > + return IRQ_NONE; > + > + aq->pending |= pending; > + if ((aq->pending & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED) > + complete(&aq->cmd_done); > + > + return IRQ_HANDLED; > +} > + > +static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > +{ > + struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master); > + int mode; > + u32 addr = 0; > + u32 dummy_cycles = 0; > + u32 ifr = QSPI_IFR_INSTEN; > + u32 icr = QSPI_ICR_INST(op->cmd.opcode); > + > + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > + > + mode = find_mode(op); > + if (mode < 0) > + return -1; return mode; Add a blank line here. > + ifr |= sama5d2_qspi_modes[mode].config; > + > + if (op->dummy.buswidth && op->dummy.nbytes) > + dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth; > + > + if (op->addr.buswidth) { > + switch (op->addr.nbytes) { > + case 0: > + break; > + case 1: > + ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT; > + icr |= QSPI_ICR_OPT(op->addr.val & 0xff); > + break; > + case 2: > + if (dummy_cycles < 8 / op->addr.buswidth) { > + ifr &= ~QSPI_IFR_INSTEN; > + addr = (op->cmd.opcode << 16) | > + (op->addr.val & 0xffff); > + ifr |= QSPI_IFR_ADDREN; > + } else { > + addr = (op->addr.val << 8) & 0xffffff; > + dummy_cycles -= 8 / op->addr.buswidth; > + ifr |= QSPI_IFR_ADDREN; > + } > + break; > + case 3: > + addr = op->addr.val & 0xffffff; > + ifr |= QSPI_IFR_ADDREN; > + break; > + case 4: > + addr = op->addr.val; > + ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL; > + break; > + default: > + return -1; return -ENOTSUPP; This applies to other places where you return -1, you should always use well-known error codes. > + } > + } Add a blank line here. > + ifr |= QSPI_IFR_NBDUM(dummy_cycles); Add a blank line here. > + if (op->data.nbytes == 0) > + qspi_writel(aq, QSPI_IAR, addr); Is this really needed? You seem to write IAR a few lines below. > + else > + ifr |= QSPI_IFR_DATAEN; > + > + if ((op->data.dir == SPI_MEM_DATA_IN) && (op->data.nbytes > 0)) Drop the unneeded parenthesis. > + ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; > + else > + ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; This should probably be embedded in the op->data.nbytes != 0 block: if (op->data.nbytes) { ifr |= QSPI_IFR_DATAEN; if (op->data.dir == SPI_MEM_DATA_IN) ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; else ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; } > + > + qspi_writel(aq, QSPI_IAR, addr); > + qspi_writel(aq, QSPI_ICR, icr); > + qspi_writel(aq, QSPI_IFR, ifr); You should probably read the _SR register before starting a new operation to make sure all status flags have been cleared. > + qspi_readl(aq, QSPI_IFR); > + > + if (op->data.nbytes > 0) { > + if (op->data.dir == SPI_MEM_DATA_IN) > + _memcpy_fromio(op->data.buf.in, > + aq->ahb_addr + addr, op->data.nbytes); > + else > + _memcpy_toio(aq->ahb_addr + addr, > + op->data.buf.out, op->data.nbytes); > + > + qspi_writel(aq, QSPI_CR, QSPI_CR_LASTXFER); > + } > + > + aq->pending = qspi_readl(aq, QSPI_SR) & QSPI_SR_CMD_COMPLETED; > + if (aq->pending == QSPI_SR_CMD_COMPLETED) > + return 0; Add a blank line here. > + reinit_completion(&aq->cmd_done); > + qspi_writel(aq, QSPI_IER, QSPI_SR_CMD_COMPLETED); > + wait_for_completion(&aq->cmd_done); You should use wait_for_completion_timeout() to avoid stalling the system when the event you're waiting for never happens. > + qspi_writel(aq, QSPI_IDR, QSPI_SR_CMD_COMPLETED); > + > + return 0; > +} > + > +static const struct spi_controller_mem_ops atmel_qspi_mem_ops = { > + .adjust_op_size = atmel_qspi_adjust_op_size, > + .supports_op = atmel_qspi_supports_op, > + .exec_op = atmel_qspi_exec_op > +}; > + > +static int atmel_qspi_probe(struct platform_device *pdev) > +{ > + struct spi_controller *ctrl; > + struct atmel_qspi *aq; > + struct device_node *np = pdev->dev.of_node; > + struct device_node *child; > + struct resource *res; > + int irq, err = 0; > + > + ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq)); > + if (!ctrl) > + return -ENOMEM; > + > + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD; > + ctrl->bus_num = -1; > + ctrl->mem_ops = &atmel_qspi_mem_ops; > + ctrl->num_chipselect = 1; > + ctrl->dev.of_node = pdev->dev.of_node; > + platform_set_drvdata(pdev, ctrl); > + > + aq = spi_controller_get_devdata(ctrl); > + > + if (of_get_child_count(np) != 1) > + return -ENODEV; Hm, I think the core already complains if there are more children than ->num_chipselect, so I'd say this is useless. Also, you can have a controller without any devices under, so, nchildren == 0 is a valid case. > + child = of_get_next_child(np, NULL); > + > + aq->pdev = pdev; > + > + /* map registers */ > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base"); > + aq->iobase = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(aq->iobase)) { > + dev_err(&pdev->dev, "missing registers\n"); > + err = PTR_ERR(aq->iobase); > + goto err_put_ctrl; > + } > + > + /* map AHB memory */ > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap"); > + aq->ahb_addr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(aq->ahb_addr)) { > + dev_err(&pdev->dev, "missing AHB memory\n"); > + err = PTR_ERR(aq->ahb_addr); > + goto err_put_ctrl; > + } > + > + /* get 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 err_put_ctrl; > + } > + > + err = clk_prepare_enable(aq->clk); > + if (err) { > + dev_err(&pdev->dev, "failed to enable peripheral clock\n"); > + goto err_put_ctrl; > + } > + > + /* request 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), ctrl); Align dev_name() on the open-parenthesis (checkpatch --strict should complain about that). > + if (err) > + goto disable_clk; > + > + err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); You should let the SPI core parse that for you (it's then stored in spi_device->max_speed_hz and should be applied when ->setup() is called). > + if (err < 0) > + goto disable_clk; > + > + init_completion(&aq->cmd_done); > + > + err = atmel_qspi_init(aq); > + if (err) > + goto disable_clk; > + > + of_node_put(child); > + > + err = spi_register_controller(ctrl); > + if (err) > + goto disable_clk; > + > + return 0; > + > +disable_clk: > + clk_disable_unprepare(aq->clk); > +err_put_ctrl: > + spi_controller_put(ctrl); > + of_node_put(child); > + return err; > +} > + > +static int atmel_qspi_remove(struct platform_device *pdev) > +{ > + struct spi_controller *ctrl = platform_get_drvdata(pdev); > + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); > + > + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); > + clk_disable_unprepare(aq->clk); > + > + spi_unregister_controller(ctrl); > + > + return 0; > +} > + > +static const struct of_device_id atmel_qspi_dt_ids[] = { > + { > + .compatible = "atmel,sama5d2-spi-qspi" > + }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids); > + > +static struct platform_driver atmel_qspi_driver = { > + .driver = { > + .name = "atmel_spi_qspi", "atmel-qspi", > + .of_match_table = atmel_qspi_dt_ids > + }, > + .probe = atmel_qspi_probe, > + .remove = atmel_qspi_remove > +}; > + > +module_platform_driver(atmel_qspi_driver); > + > + > +MODULE_DESCRIPTION("Atmel SAMA5D2 QuadSPI Driver"); > +MODULE_AUTHOR("Piotr Bugalski <pbu@cryptera.com"); > +MODULE_LICENSE("GPL v2"); > + Drop this blank line. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Boris, I'm a bit allergic to personal preferences in coding style, anyway thank you for comments and some important findings. On Thu, 21 Jun 2018, Boris Brezillon wrote: > Hi Piotr, > > On Mon, 18 Jun 2018 18:21:23 +0200 > Piotr Bugalski <bugalski.piotr@gmail.com> wrote: > >> Kernel contains QSPI driver strongly tied to MTD and nor-flash memory. >> New spi-mem interface allows usage also other memory types, especially >> much larger NAND with SPI interface. This driver works as SPI controller >> and is not related to MTD, however can work with NAND-flash or other >> peripherals using spi-mem interface. > > Thanks for working on that. > >> >> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> >> Signed-off-by: Piotr Bugalski <pbu@cryptera.com> >> >> --- >> drivers/spi/Kconfig | 9 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 490 insertions(+) >> create mode 100644 drivers/spi/spi-atmel-qspi.c >> >> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig >> index 4a77cfa3213d..4f70a7005997 100644 >> --- a/drivers/spi/Kconfig >> +++ b/drivers/spi/Kconfig >> @@ -84,6 +84,15 @@ config SPI_ATMEL >> This selects a driver for the Atmel SPI Controller, present on >> many AT91 ARM chips. >> >> +config SPI_ATMEL_QSPI >> + tristate "Atmel QuadSPI Controller" >> + depends on ARCH_AT91 || (ARM && COMPILE_TEST) >> + depends on OF && HAS_IOMEM >> + help >> + This selects a driver for the Atmel QSPI Controller on SAMA5D2. >> + This controller does not support generic SPI, it supports only >> + spi-mem interface. >> + >> config SPI_AU1550 >> tristate "Au1550/Au1200/Au1300 SPI Controller" >> depends on MIPS_ALCHEMY >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index fe54d787cf4d..6245a5693b16 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o >> obj-$(CONFIG_SPI_ALTERA) += spi-altera.o >> obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o >> obj-$(CONFIG_SPI_ATMEL) += spi-atmel.o >> +obj-$(CONFIG_SPI_ATMEL_QSPI) += spi-atmel-qspi.o >> obj-$(CONFIG_SPI_ATH79) += spi-ath79.o >> obj-$(CONFIG_SPI_AU1550) += spi-au1550.o >> obj-$(CONFIG_SPI_AXI_SPI_ENGINE) += spi-axi-spi-engine.o >> diff --git a/drivers/spi/spi-atmel-qspi.c b/drivers/spi/spi-atmel-qspi.c >> new file mode 100644 >> index 000000000000..1ee626201b0d >> --- /dev/null >> +++ b/drivers/spi/spi-atmel-qspi.c >> @@ -0,0 +1,480 @@ > > SPDX header here please: > > // SPDX-License-Identifier: GPL-2.0 > ok >> +/* >> + * Atmel SAMA5D2 QuadSPI driver. >> + * >> + * Copyright (C) 2018 Cryptera A/S > > A non-negligible portion of this code has been copied from the existing > driver. Please keep the existing copyright (you can still add Cryptera's > one). > Technically this driver were written from scratch, with spi-fsl-qspi as example of new interface. Hence the name and code structure. But it's the same peripheral as Atmel's driver uses so code looks similar. I can unify the code to make comparsion even simpler and then update copyright. >> + * All Rights Reserved >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 (GPL v2) >> + * as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. > > Drop the license text. The SPDX header is here to replace it. > ok >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/of_device.h> >> +#include <linux/clk.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/spi/spi-mem.h> >> +#include <linux/delay.h> >> + >> +#define QSPI_CR 0x0000 >> +#define QSPI_MR 0x0004 >> +#define QSPI_RDR 0x0008 >> +#define QSPI_TDR 0x000c >> +#define QSPI_SR 0x0010 >> +#define QSPI_IER 0x0014 >> +#define QSPI_IDR 0x0018 >> +#define QSPI_IMR 0x001c >> +#define QSPI_SCR 0x0020 >> + >> +#define QSPI_IAR 0x0030 >> +#define QSPI_ICR 0x0034 >> +#define QSPI_IFR 0x0038 >> + >> +#define QSPI_WPMR 0x00e4 >> +#define QSPI_WPSR 0x00e8 >> + >> +/* Bitfields in QSPI_CR (Control Register) */ > > I personally prefer when reg offsets are declared next to the reg fields. I don't. But it may not be good place to discuss personal preferences. I can change it. > >> +#define QSPI_CR_QSPIEN BIT(0) >> +#define QSPI_CR_QSPIDIS BIT(1) >> +#define QSPI_CR_SWRST BIT(7) >> +#define QSPI_CR_LASTXFER BIT(24) >> + >> +/* Bitfields in QSPI_ICR (Instruction Code Register) */ >> +#define QSPI_ICR_INST_MASK GENMASK(7, 0) >> +#define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK) >> +#define QSPI_ICR_OPT_MASK GENMASK(23, 16) >> +#define QSPI_ICR_OPT(opt) (((opt) << 16) & QSPI_ICR_OPT_MASK) >> + >> +/* Bitfields in QSPI_MR (Mode Register) */ >> +#define QSPI_MR_SMM BIT(0) >> +#define QSPI_MR_LLB BIT(1) >> +#define QSPI_MR_WDRBT BIT(2) >> +#define QSPI_MR_SMRM BIT(3) >> +#define QSPI_MR_CSMODE_MASK GENMASK(5, 4) >> +#define QSPI_MR_CSMODE_NOT_RELOADED (0 << 4) >> +#define QSPI_MR_CSMODE_LASTXFER (1 << 4) >> +#define QSPI_MR_CSMODE_SYSTEMATICALLY (2 << 4) >> +#define QSPI_MR_NBBITS_MASK GENMASK(11, 8) >> +#define QSPI_MR_NBBITS(n) ((((n) - 8) << 8) & QSPI_MR_NBBITS_MASK) >> +#define QSPI_MR_DLYBCT_MASK GENMASK(23, 16) >> +#define QSPI_MR_DLYBCT(n) (((n) << 16) & QSPI_MR_DLYBCT_MASK) >> +#define QSPI_MR_DLYCS_MASK GENMASK(31, 24) >> +#define QSPI_MR_DLYCS(n) (((n) << 24) & QSPI_MR_DLYCS_MASK) >> + >> +/* Bitfields in QSPI_IFR (Instruction Frame Register) */ >> +#define QSPI_IFR_WIDTH_MASK GENMASK(2, 0) >> +#define QSPI_IFR_WIDTH_SINGLE_BIT_SPI (0 << 0) >> +#define QSPI_IFR_WIDTH_DUAL_OUTPUT (1 << 0) >> +#define QSPI_IFR_WIDTH_QUAD_OUTPUT (2 << 0) >> +#define QSPI_IFR_WIDTH_DUAL_IO (3 << 0) >> +#define QSPI_IFR_WIDTH_QUAD_IO (4 << 0) >> +#define QSPI_IFR_WIDTH_DUAL_CMD (5 << 0) >> +#define QSPI_IFR_WIDTH_QUAD_CMD (6 << 0) >> +#define QSPI_IFR_INSTEN BIT(4) >> +#define QSPI_IFR_ADDREN BIT(5) >> +#define QSPI_IFR_OPTEN BIT(6) >> +#define QSPI_IFR_DATAEN BIT(7) >> +#define QSPI_IFR_OPTL_MASK GENMASK(9, 8) >> +#define QSPI_IFR_OPTL_1BIT (0 << 8) >> +#define QSPI_IFR_OPTL_2BIT (1 << 8) >> +#define QSPI_IFR_OPTL_4BIT (2 << 8) >> +#define QSPI_IFR_OPTL_8BIT (3 << 8) >> +#define QSPI_IFR_ADDRL BIT(10) >> +#define QSPI_IFR_TFRTYP_MASK GENMASK(13, 12) >> +#define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) >> +#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) >> +#define QSPI_IFR_TFRTYP_TRSFR_WRITE (2 << 12) >> +#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13) >> +#define QSPI_IFR_CRM BIT(14) >> +#define QSPI_IFR_NBDUM_MASK GENMASK(20, 16) >> +#define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK) >> + >> +/* Bitfields in QSPI_SR/QSPI_IER/QSPI_IDR/QSPI_IMR */ >> +#define QSPI_SR_RDRF BIT(0) >> +#define QSPI_SR_TDRE BIT(1) >> +#define QSPI_SR_TXEMPTY BIT(2) >> +#define QSPI_SR_OVRES BIT(3) >> +#define QSPI_SR_CSR BIT(8) >> +#define QSPI_SR_CSS BIT(9) >> +#define QSPI_SR_INSTRE BIT(10) >> +#define QSPI_SR_QSPIENS BIT(24) >> + >> +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) > > Do you really to wait for both INSTRE and CSR to consider the command > as complete? > This part were really copied from Atmel driver. I wasn't sure so I used tested solution. >> + >> + > > Drop a blank line here. > ok >> +/* Bitfields in QSPI_SCR (Serial Clock Register) */ >> +#define QSPI_SCR_CPOL BIT(0) >> +#define QSPI_SCR_CPHA BIT(1) >> +#define QSPI_SCR_SCBR_MASK GENMASK(15, 8) >> +#define QSPI_SCR_SCBR(n) (((n) << 8) & QSPI_SCR_SCBR_MASK) >> +#define QSPI_SCR_DLYBS_MASK GENMASK(23, 16) >> +#define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK) >> + >> +#define QSPI_WPMR_WPKEY_PASSWD (0x515350u << 8) >> + >> +struct atmel_qspi { >> + struct platform_device *pdev; >> + void __iomem *iobase; >> + void __iomem *ahb_addr; >> + int irq; > > This field is not used and should be killed. > ok >> + struct clk *clk; >> + u32 clk_rate; > > This field can be removed. The same information is already stored in > spi_device->max_speed_hz. > Sounds good. There will be more clean up because of this improvement. >> + struct completion cmd_done; >> + u32 pending; >> +}; >> + >> +struct qspi_mode { >> + u8 cmd_buswidth; >> + u8 addr_buswidth; >> + u8 data_buswidth; >> + u32 config; >> +}; >> + >> +static const struct qspi_mode sama5d2_qspi_modes[] = { >> + { 1, 1, 1, QSPI_IFR_WIDTH_SINGLE_BIT_SPI }, >> + { 1, 1, 2, QSPI_IFR_WIDTH_DUAL_OUTPUT }, >> + { 1, 1, 4, QSPI_IFR_WIDTH_QUAD_OUTPUT }, >> + { 1, 2, 2, QSPI_IFR_WIDTH_DUAL_IO }, >> + { 1, 4, 4, QSPI_IFR_WIDTH_QUAD_IO }, >> + { 2, 2, 2, QSPI_IFR_WIDTH_DUAL_CMD }, >> + { 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD }, >> +}; >> + >> +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg) >> +{ >> + return readl_relaxed(aq->iobase + reg); >> +} >> + >> +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value) >> +{ >> + writel_relaxed(value, aq->iobase + reg); >> +} > > Please drop those wrappers and use readl/write_relaxed() directly. I like these wrappers, the same pattern were used in spi-fsl-qspi and atmel-quadspi drivers. Functions are inlined so why to remove nice looking code? > >> + >> +static int atmel_qspi_init(struct atmel_qspi *aq) >> +{ >> + unsigned long rate; >> + u32 scbr; >> + >> + qspi_writel(aq, QSPI_WPMR, QSPI_WPMR_WPKEY_PASSWD); >> + >> + /* software reset */ >> + qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST); >> + >> + /* set QSPI mode */ >> + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > > It's already done in ->exec_op(), I think you can remove it here. > I prefer initialize peripheral before first use. Single register write is not big effort. >> + >> + rate = clk_get_rate(aq->clk); >> + if (!rate) >> + return -EINVAL; >> + >> + /* set baudrate */ >> + scbr = DIV_ROUND_UP(rate, aq->clk_rate); >> + if (scbr > 0) >> + scbr--; > > Add a blank line here. > >> + qspi_writel(aq, QSPI_SCR, QSPI_SCR_SCBR(scbr)); > > The baudrate setting should be done in an spi_controller->setup() hook, > and the expected rate is available in spi_device->max_speed_hz. > Ok, I'll change it. >> + >> + /* enable qspi controller */ >> + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN); >> + >> + return 0; >> +} >> + >> +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) >> +{ >> + return 0; >> +} >> + >> +static inline bool is_compatible(const struct spi_mem_op *op, >> + const struct qspi_mode *mode) >> +{ >> + if (op->cmd.buswidth != mode->cmd_buswidth) >> + return false; > > Add a blank line here Looks like personal preferences again... I hate too personalized code. Maybe I'll just merge these three if-s into one horrible large. > >> + if (op->addr.nbytes && op->addr.buswidth != mode->addr_buswidth) >> + return false; > > here > >> + if (op->data.nbytes && op->data.buswidth != mode->data_buswidth) >> + return false; > > and here. > >> + return true; >> +} >> + >> +static int find_mode(const struct spi_mem_op *op) >> +{ >> + u32 i; >> + >> + for (i = 0; i < ARRAY_SIZE(sama5d2_qspi_modes); i++) >> + if (is_compatible(op, &sama5d2_qspi_modes[i])) >> + return i; >> + return -1; > > return -ENOTSUPP; Ok. > >> +} >> + >> +static bool atmel_qspi_supports_op(struct spi_mem *mem, >> + const struct spi_mem_op *op) >> +{ >> + if (find_mode(op) < 0) >> + return false; >> + >> + // special case not supported by hardware > > We try to avoid using C++ style comments in the kernel. > Sure, my mistake. >> + if ((op->addr.nbytes == 2) && (op->cmd.buswidth != op->addr.buswidth) && > > You don't need those extra parenthesis around each test. > It's just convention. Some people prefer to have extra bracket to make code easier to read, while other think opposite. I don't really know which option is better. >> + (op->dummy.nbytes == 0)) > > Always try to align on the open-parenthesis of the if () block: > > if (op->addr.nbytes == 2 && op->cmd.buswidth != op->addr.buswidth && > op->dummy.nbytes == 0) > ok >> + return false; >> + >> + return true; >> +} >> + >> +static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) >> +{ >> + struct spi_controller *ctrl = dev_id; >> + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); >> + u32 status, mask, pending; >> + >> + status = qspi_readl(aq, QSPI_SR); >> + mask = qspi_readl(aq, QSPI_IMR); >> + pending = status & mask; >> + >> + if (!pending) >> + return IRQ_NONE; >> + >> + aq->pending |= pending; >> + if ((aq->pending & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED) >> + complete(&aq->cmd_done); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) >> +{ >> + struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master); >> + int mode; >> + u32 addr = 0; >> + u32 dummy_cycles = 0; >> + u32 ifr = QSPI_IFR_INSTEN; >> + u32 icr = QSPI_ICR_INST(op->cmd.opcode); >> + >> + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); >> + >> + mode = find_mode(op); >> + if (mode < 0) >> + return -1; > > return mode; > > Add a blank line here. > >> + ifr |= sama5d2_qspi_modes[mode].config; >> + >> + if (op->dummy.buswidth && op->dummy.nbytes) >> + dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth; >> + >> + if (op->addr.buswidth) { >> + switch (op->addr.nbytes) { >> + case 0: >> + break; >> + case 1: >> + ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT; >> + icr |= QSPI_ICR_OPT(op->addr.val & 0xff); >> + break; >> + case 2: >> + if (dummy_cycles < 8 / op->addr.buswidth) { >> + ifr &= ~QSPI_IFR_INSTEN; >> + addr = (op->cmd.opcode << 16) | >> + (op->addr.val & 0xffff); >> + ifr |= QSPI_IFR_ADDREN; >> + } else { >> + addr = (op->addr.val << 8) & 0xffffff; >> + dummy_cycles -= 8 / op->addr.buswidth; >> + ifr |= QSPI_IFR_ADDREN; >> + } >> + break; >> + case 3: >> + addr = op->addr.val & 0xffffff; >> + ifr |= QSPI_IFR_ADDREN; >> + break; >> + case 4: >> + addr = op->addr.val; >> + ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL; >> + break; >> + default: >> + return -1; > > return -ENOTSUPP; > > This applies to other places where you return -1, you should always use > well-known error codes. > ok >> + } >> + } > > Add a blank line here. > >> + ifr |= QSPI_IFR_NBDUM(dummy_cycles); > > Add a blank line here. > >> + if (op->data.nbytes == 0) >> + qspi_writel(aq, QSPI_IAR, addr); > > Is this really needed? You seem to write IAR a few lines below. > Right, I'll fix it. >> + else >> + ifr |= QSPI_IFR_DATAEN; >> + >> + if ((op->data.dir == SPI_MEM_DATA_IN) && (op->data.nbytes > 0)) > > Drop the unneeded parenthesis. > >> + ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; >> + else >> + ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; > > This should probably be embedded in the op->data.nbytes != 0 block: > > if (op->data.nbytes) { > ifr |= QSPI_IFR_DATAEN; > > if (op->data.dir == SPI_MEM_DATA_IN) > ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; > else > ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; > } > It's correct. >> + >> + qspi_writel(aq, QSPI_IAR, addr); >> + qspi_writel(aq, QSPI_ICR, icr); >> + qspi_writel(aq, QSPI_IFR, ifr); > > You should probably read the _SR register before starting a new operation > to make sure all status flags have been cleared. > I'm not sure about it, but there was SR read in Atmel's driver. I can put it here. >> + qspi_readl(aq, QSPI_IFR); >> + >> + if (op->data.nbytes > 0) { >> + if (op->data.dir == SPI_MEM_DATA_IN) >> + _memcpy_fromio(op->data.buf.in, >> + aq->ahb_addr + addr, op->data.nbytes); >> + else >> + _memcpy_toio(aq->ahb_addr + addr, >> + op->data.buf.out, op->data.nbytes); >> + >> + qspi_writel(aq, QSPI_CR, QSPI_CR_LASTXFER); >> + } >> + >> + aq->pending = qspi_readl(aq, QSPI_SR) & QSPI_SR_CMD_COMPLETED; >> + if (aq->pending == QSPI_SR_CMD_COMPLETED) >> + return 0; > > Add a blank line here. > >> + reinit_completion(&aq->cmd_done); >> + qspi_writel(aq, QSPI_IER, QSPI_SR_CMD_COMPLETED); >> + wait_for_completion(&aq->cmd_done); > > You should use wait_for_completion_timeout() to avoid stalling the system > when the event you're waiting for never happens. > I forgot about it, good point. >> + qspi_writel(aq, QSPI_IDR, QSPI_SR_CMD_COMPLETED); >> + >> + return 0; >> +} >> + >> +static const struct spi_controller_mem_ops atmel_qspi_mem_ops = { >> + .adjust_op_size = atmel_qspi_adjust_op_size, >> + .supports_op = atmel_qspi_supports_op, >> + .exec_op = atmel_qspi_exec_op >> +}; >> + >> +static int atmel_qspi_probe(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctrl; >> + struct atmel_qspi *aq; >> + struct device_node *np = pdev->dev.of_node; >> + struct device_node *child; >> + struct resource *res; >> + int irq, err = 0; >> + >> + ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq)); >> + if (!ctrl) >> + return -ENOMEM; >> + >> + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD; >> + ctrl->bus_num = -1; >> + ctrl->mem_ops = &atmel_qspi_mem_ops; >> + ctrl->num_chipselect = 1; >> + ctrl->dev.of_node = pdev->dev.of_node; >> + platform_set_drvdata(pdev, ctrl); >> + >> + aq = spi_controller_get_devdata(ctrl); >> + >> + if (of_get_child_count(np) != 1) >> + return -ENODEV; > > Hm, I think the core already complains if there are more children than > ->num_chipselect, so I'd say this is useless. Also, you can have a > controller without any devices under, so, nchildren == 0 is a valid > case. > This trick were inspired by atmel-quadspi code. If clk frequency can be taken from spi_device, all this of-child related ugliness can be removed. >> + child = of_get_next_child(np, NULL); >> + >> + aq->pdev = pdev; >> + >> + /* map registers */ >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base"); >> + aq->iobase = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(aq->iobase)) { >> + dev_err(&pdev->dev, "missing registers\n"); >> + err = PTR_ERR(aq->iobase); >> + goto err_put_ctrl; >> + } >> + >> + /* map AHB memory */ >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap"); >> + aq->ahb_addr = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(aq->ahb_addr)) { >> + dev_err(&pdev->dev, "missing AHB memory\n"); >> + err = PTR_ERR(aq->ahb_addr); >> + goto err_put_ctrl; >> + } >> + >> + /* get 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 err_put_ctrl; >> + } >> + >> + err = clk_prepare_enable(aq->clk); >> + if (err) { >> + dev_err(&pdev->dev, "failed to enable peripheral clock\n"); >> + goto err_put_ctrl; >> + } >> + >> + /* request 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), ctrl); > > Align dev_name() on the open-parenthesis (checkpatch --strict should > complain about that). ok > >> + if (err) >> + goto disable_clk; >> + >> + err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); > > You should let the SPI core parse that for you (it's then stored in > spi_device->max_speed_hz and should be applied when ->setup() is called). > Sure. >> + if (err < 0) >> + goto disable_clk; >> + >> + init_completion(&aq->cmd_done); >> + >> + err = atmel_qspi_init(aq); >> + if (err) >> + goto disable_clk; >> + >> + of_node_put(child); >> + >> + err = spi_register_controller(ctrl); >> + if (err) >> + goto disable_clk; >> + >> + return 0; >> + >> +disable_clk: >> + clk_disable_unprepare(aq->clk); >> +err_put_ctrl: >> + spi_controller_put(ctrl); >> + of_node_put(child); >> + return err; >> +} >> + >> +static int atmel_qspi_remove(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctrl = platform_get_drvdata(pdev); >> + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); >> + >> + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); >> + clk_disable_unprepare(aq->clk); >> + >> + spi_unregister_controller(ctrl); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id atmel_qspi_dt_ids[] = { >> + { >> + .compatible = "atmel,sama5d2-spi-qspi" >> + }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids); >> + >> +static struct platform_driver atmel_qspi_driver = { >> + .driver = { >> + .name = "atmel_spi_qspi", > > "atmel-qspi", > I expected new driver to exists in parallel with atmel-qspi. If it's replacement, then re-use name makes sense. >> + .of_match_table = atmel_qspi_dt_ids >> + }, >> + .probe = atmel_qspi_probe, >> + .remove = atmel_qspi_remove >> +}; >> + >> +module_platform_driver(atmel_qspi_driver); >> + >> + >> +MODULE_DESCRIPTION("Atmel SAMA5D2 QuadSPI Driver"); >> +MODULE_AUTHOR("Piotr Bugalski <pbu@cryptera.com"); >> +MODULE_LICENSE("GPL v2"); >> + > > Drop this blank line. > > Regards, > > Boris > Regards, Piotr -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Piotr, On Fri, 22 Jun 2018 07:57:10 +0200 (CEST) Piotr Bugalski <bugalski.piotr@gmail.com> wrote: > Hi Boris, > > I'm a bit allergic to personal preferences in coding style, I'm just pointing things that are common kernel coding style practices. You might not like coding style rules of a particular project, but when you contribute to this project you should do your best to follow those rules and keep the coding style consistent. > anyway > thank you for comments and some important findings. > [...] > >> +/* > >> + * Atmel SAMA5D2 QuadSPI driver. > >> + * > >> + * Copyright (C) 2018 Cryptera A/S > > > > A non-negligible portion of this code has been copied from the existing > > driver. Please keep the existing copyright (you can still add Cryptera's > > one). > > > > Technically this driver were written from scratch, with spi-fsl-qspi > as example of new interface. Hence the name and code structure. > But it's the same peripheral as Atmel's driver uses so code looks > similar. I can unify the code to make comparsion even simpler and > then update copyright. Hm, ok. Some constructs really looked like they were copied from the old driver, hence my comment. I'll let Nicolas give his opinion on this aspect. > >> + */ > >> + > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/of_device.h> > >> +#include <linux/clk.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/io.h> > >> +#include <linux/spi/spi-mem.h> > >> +#include <linux/delay.h> > >> + > >> +#define QSPI_CR 0x0000 > >> +#define QSPI_MR 0x0004 > >> +#define QSPI_RDR 0x0008 > >> +#define QSPI_TDR 0x000c > >> +#define QSPI_SR 0x0010 > >> +#define QSPI_IER 0x0014 > >> +#define QSPI_IDR 0x0018 > >> +#define QSPI_IMR 0x001c > >> +#define QSPI_SCR 0x0020 > >> + > >> +#define QSPI_IAR 0x0030 > >> +#define QSPI_ICR 0x0034 > >> +#define QSPI_IFR 0x0038 > >> + > >> +#define QSPI_WPMR 0x00e4 > >> +#define QSPI_WPSR 0x00e8 > >> + > >> +/* Bitfields in QSPI_CR (Control Register) */ > > > > I personally prefer when reg offsets are declared next to the reg fields. > > I don't. But it may not be good place to discuss personal preferences. > I can change it. This one is actually not defined in the coding style rules, hence the "I personally prefer". The reasoning behind this approach is that it's easier for readers to identify the fields of a specific reg when they're defined next to each other. > > > > >> +#define QSPI_CR_QSPIEN BIT(0) > >> +#define QSPI_CR_QSPIDIS BIT(1) > >> +#define QSPI_CR_SWRST BIT(7) > >> +#define QSPI_CR_LASTXFER BIT(24) > >> + > >> +/* Bitfields in QSPI_ICR (Instruction Code Register) */ > >> +#define QSPI_ICR_INST_MASK GENMASK(7, 0) > >> +#define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK) > >> +#define QSPI_ICR_OPT_MASK GENMASK(23, 16) > >> +#define QSPI_ICR_OPT(opt) (((opt) << 16) & QSPI_ICR_OPT_MASK) > >> + > >> +/* Bitfields in QSPI_MR (Mode Register) */ > >> +#define QSPI_MR_SMM BIT(0) > >> +#define QSPI_MR_LLB BIT(1) > >> +#define QSPI_MR_WDRBT BIT(2) > >> +#define QSPI_MR_SMRM BIT(3) > >> +#define QSPI_MR_CSMODE_MASK GENMASK(5, 4) > >> +#define QSPI_MR_CSMODE_NOT_RELOADED (0 << 4) > >> +#define QSPI_MR_CSMODE_LASTXFER (1 << 4) > >> +#define QSPI_MR_CSMODE_SYSTEMATICALLY (2 << 4) > >> +#define QSPI_MR_NBBITS_MASK GENMASK(11, 8) > >> +#define QSPI_MR_NBBITS(n) ((((n) - 8) << 8) & QSPI_MR_NBBITS_MASK) > >> +#define QSPI_MR_DLYBCT_MASK GENMASK(23, 16) > >> +#define QSPI_MR_DLYBCT(n) (((n) << 16) & QSPI_MR_DLYBCT_MASK) > >> +#define QSPI_MR_DLYCS_MASK GENMASK(31, 24) > >> +#define QSPI_MR_DLYCS(n) (((n) << 24) & QSPI_MR_DLYCS_MASK) > >> + > >> +/* Bitfields in QSPI_IFR (Instruction Frame Register) */ > >> +#define QSPI_IFR_WIDTH_MASK GENMASK(2, 0) > >> +#define QSPI_IFR_WIDTH_SINGLE_BIT_SPI (0 << 0) > >> +#define QSPI_IFR_WIDTH_DUAL_OUTPUT (1 << 0) > >> +#define QSPI_IFR_WIDTH_QUAD_OUTPUT (2 << 0) > >> +#define QSPI_IFR_WIDTH_DUAL_IO (3 << 0) > >> +#define QSPI_IFR_WIDTH_QUAD_IO (4 << 0) > >> +#define QSPI_IFR_WIDTH_DUAL_CMD (5 << 0) > >> +#define QSPI_IFR_WIDTH_QUAD_CMD (6 << 0) > >> +#define QSPI_IFR_INSTEN BIT(4) > >> +#define QSPI_IFR_ADDREN BIT(5) > >> +#define QSPI_IFR_OPTEN BIT(6) > >> +#define QSPI_IFR_DATAEN BIT(7) > >> +#define QSPI_IFR_OPTL_MASK GENMASK(9, 8) > >> +#define QSPI_IFR_OPTL_1BIT (0 << 8) > >> +#define QSPI_IFR_OPTL_2BIT (1 << 8) > >> +#define QSPI_IFR_OPTL_4BIT (2 << 8) > >> +#define QSPI_IFR_OPTL_8BIT (3 << 8) > >> +#define QSPI_IFR_ADDRL BIT(10) > >> +#define QSPI_IFR_TFRTYP_MASK GENMASK(13, 12) > >> +#define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) > >> +#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) > >> +#define QSPI_IFR_TFRTYP_TRSFR_WRITE (2 << 12) > >> +#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13) > >> +#define QSPI_IFR_CRM BIT(14) > >> +#define QSPI_IFR_NBDUM_MASK GENMASK(20, 16) > >> +#define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK) > >> + > >> +/* Bitfields in QSPI_SR/QSPI_IER/QSPI_IDR/QSPI_IMR */ > >> +#define QSPI_SR_RDRF BIT(0) > >> +#define QSPI_SR_TDRE BIT(1) > >> +#define QSPI_SR_TXEMPTY BIT(2) > >> +#define QSPI_SR_OVRES BIT(3) > >> +#define QSPI_SR_CSR BIT(8) > >> +#define QSPI_SR_CSS BIT(9) > >> +#define QSPI_SR_INSTRE BIT(10) > >> +#define QSPI_SR_QSPIENS BIT(24) > >> + > >> +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) > > > > Do you really to wait for both INSTRE and CSR to consider the command > > as complete? > > > > This part were really copied from Atmel driver. I wasn't sure so I > used tested solution. Okay. I guess that's a question for Cyrille and/or Tudor then. > >> + > >> +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg) > >> +{ > >> + return readl_relaxed(aq->iobase + reg); > >> +} > >> + > >> +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value) > >> +{ > >> + writel_relaxed(value, aq->iobase + reg); > >> +} > > > > Please drop those wrappers and use readl/write_relaxed() directly. > > I like these wrappers, the same pattern were used in spi-fsl-qspi and > atmel-quadspi drivers. Functions are inlined so why to remove nice > looking code? It's not really about code optimization (gcc is good at determining when to inline simple functions). Kernel devs like to know when _relaxed() versions of IO accessors are used, and by providing those wrappers you kind of hide that fact. > > > > >> + > >> +static int atmel_qspi_init(struct atmel_qspi *aq) > >> +{ > >> + unsigned long rate; > >> + u32 scbr; > >> + > >> + qspi_writel(aq, QSPI_WPMR, QSPI_WPMR_WPKEY_PASSWD); > >> + > >> + /* software reset */ > >> + qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST); > >> + > >> + /* set QSPI mode */ > >> + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > > > > It's already done in ->exec_op(), I think you can remove it here. > > > > I prefer initialize peripheral before first use. Single register write > is not big effort. I'm fine with that, but then, why should we initialize the IP in SMM mode? Can't you write 0 to this _MR reg? > > >> + > >> + rate = clk_get_rate(aq->clk); > >> + if (!rate) > >> + return -EINVAL; > >> + > >> + /* set baudrate */ > >> + scbr = DIV_ROUND_UP(rate, aq->clk_rate); > >> + if (scbr > 0) > >> + scbr--; > > > > Add a blank line here. > > > >> + qspi_writel(aq, QSPI_SCR, QSPI_SCR_SCBR(scbr)); > > > > The baudrate setting should be done in an spi_controller->setup() hook, > > and the expected rate is available in spi_device->max_speed_hz. > > > > Ok, I'll change it. > > >> + > >> + /* enable qspi controller */ > >> + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN); > >> + > >> + return 0; > >> +} > >> + > >> +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > >> +{ > >> + return 0; > >> +} > >> + > >> +static inline bool is_compatible(const struct spi_mem_op *op, > >> + const struct qspi_mode *mode) > >> +{ > >> + if (op->cmd.buswidth != mode->cmd_buswidth) > >> + return false; > > > > Add a blank line here > > Looks like personal preferences again... I hate too personalized code. It's mainly about being able to quickly identify different if() blocks. > Maybe I'll just merge these three if-s into one horrible large. > > > > >> + if (op->addr.nbytes && op->addr.buswidth != mode->addr_buswidth) > >> + return false; > > > > here > > > >> + if (op->data.nbytes && op->data.buswidth != mode->data_buswidth) > >> + return false; > > > > and here. > > > >> + return true; > >> + > >> +static bool atmel_qspi_supports_op(struct spi_mem *mem, > >> + const struct spi_mem_op *op) > >> +{ > >> + if (find_mode(op) < 0) > >> + return false; > >> + > >> + // special case not supported by hardware > > > > We try to avoid using C++ style comments in the kernel. > > > > Sure, my mistake. > > >> + if ((op->addr.nbytes == 2) && (op->cmd.buswidth != op->addr.buswidth) && > > > > You don't need those extra parenthesis around each test. > > > > It's just convention. Some people prefer to have extra bracket to make > code easier to read, while other think opposite. Except it's not even consistent in this file, see the if (op->addr.nbytes && op->addr.buswidth != mode->addr_buswidth) test above. > I don't really know > which option is better. We try to avoid those parens when doing simple '2 operands' operations if (a == b && c == d) but add them in case one of the operand results from another operation if (a == (b * c) && c == d) > >> + > >> + qspi_writel(aq, QSPI_IAR, addr); > >> + qspi_writel(aq, QSPI_ICR, icr); > >> + qspi_writel(aq, QSPI_IFR, ifr); > > > > You should probably read the _SR register before starting a new operation > > to make sure all status flags have been cleared. > > > > I'm not sure about it, but there was SR read in Atmel's driver. > I can put it here. It's probably safer, otherwise you might get status from previous operations. > >> +static int atmel_qspi_probe(struct platform_device *pdev) > >> +{ > >> + struct spi_controller *ctrl; > >> + struct atmel_qspi *aq; > >> + struct device_node *np = pdev->dev.of_node; > >> + struct device_node *child; > >> + struct resource *res; > >> + int irq, err = 0; > >> + > >> + ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq)); > >> + if (!ctrl) > >> + return -ENOMEM; > >> + > >> + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD; > >> + ctrl->bus_num = -1; > >> + ctrl->mem_ops = &atmel_qspi_mem_ops; > >> + ctrl->num_chipselect = 1; > >> + ctrl->dev.of_node = pdev->dev.of_node; > >> + platform_set_drvdata(pdev, ctrl); > >> + > >> + aq = spi_controller_get_devdata(ctrl); > >> + > >> + if (of_get_child_count(np) != 1) > >> + return -ENODEV; > > > > Hm, I think the core already complains if there are more children than > > ->num_chipselect, so I'd say this is useless. Also, you can have a > > controller without any devices under, so, nchildren == 0 is a valid > > case. > > > > This trick were inspired by atmel-quadspi code. If clk frequency can > be taken from spi_device, all this of-child related ugliness can be > removed. Yep. > >> +static struct platform_driver atmel_qspi_driver = { > >> + .driver = { > >> + .name = "atmel_spi_qspi", > > > > "atmel-qspi", > > > > I expected new driver to exists in parallel with atmel-qspi. > If it's replacement, then re-use name makes sense. I think the old driver name was "atmel_qspi", but I prefer dashes to underscores :-). Anyway, _spi_qspi seems redundant, since qspi already implies spi. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Piotr, General things to consider for the limitation in performance: - is the serial flash memory operating in Quad SPI? - QSCLK should be as high as possible - transfer delays - I checked them, they have default values, we should be good. - use DMA, as you suggested On 06/22/2018 10:39 AM, Boris Brezillon wrote: > [...] > >>>> +/* >>>> + * Atmel SAMA5D2 QuadSPI driver. >>>> + * >>>> + * Copyright (C) 2018 Cryptera A/S >>> >>> A non-negligible portion of this code has been copied from the existing >>> driver. Please keep the existing copyright (you can still add Cryptera's >>> one). >>> >> >> Technically this driver were written from scratch, with spi-fsl-qspi >> as example of new interface. Hence the name and code structure. >> But it's the same peripheral as Atmel's driver uses so code looks >> similar. I can unify the code to make comparsion even simpler and >> then update copyright. > > Hm, ok. Some constructs really looked like they were copied > from the old driver, hence my comment. I'll let Nicolas give his > opinion on this aspect. This driver will be a conversion of the legacy one to the spi-mem interface. I would keep the legacy copyright and add Cryptera's below, as Boris suggested. [...] >>>> +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) >>> >>> Do you really to wait for both INSTRE and CSR to consider the command >>> as complete? >>> >> >> This part were really copied from Atmel driver. I wasn't sure so I >> used tested solution. > > Okay. I guess that's a question for Cyrille and/or Tudor then. We have to wait for both INSTRE and CSR. Best, ta -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tudor, Thank you very much for comments. On Tue, 26 Jun 2018, Tudor Ambarus wrote: > Hi, Piotr, > > General things to consider for the limitation in performance: > - is the serial flash memory operating in Quad SPI? Yes, I've checked signal using logic analyzer, data is transferred using all four lines. > - QSCLK should be as high as possible Sure, but when we are using lower frequency CPU impact should be negligible while efficiency is crap on every speed. > - transfer delays - I checked them, they have default values, we should be good. > - use DMA, as you suggested > I don't understand one thing. While CPU is not busy and during my tests 100% of CPU can be used for communication, efficiency is still very low. Why DMA has such impact? It is very interesting to observe signals using logic analyzer. When CPU is used for communication, there are long delays after every byte transferred. These delays are much longer than it should be only because of writing next value by CPU. I tried to change SPI frequency. If delay were CPU related, delay time should stay the same. Unfortunately results were different - lowering SPI freqency extends delay time. Using DMA makes these delays to disappear, but how to acheive CPU communication without delays? > On 06/22/2018 10:39 AM, Boris Brezillon wrote: >> [...] >> >>>>> +/* >>>>> + * Atmel SAMA5D2 QuadSPI driver. >>>>> + * >>>>> + * Copyright (C) 2018 Cryptera A/S >>>> >>>> A non-negligible portion of this code has been copied from the existing >>>> driver. Please keep the existing copyright (you can still add Cryptera's >>>> one). >>>> >>> >>> Technically this driver were written from scratch, with spi-fsl-qspi >>> as example of new interface. Hence the name and code structure. >>> But it's the same peripheral as Atmel's driver uses so code looks >>> similar. I can unify the code to make comparsion even simpler and >>> then update copyright. >> >> Hm, ok. Some constructs really looked like they were copied >> from the old driver, hence my comment. I'll let Nicolas give his >> opinion on this aspect. > > This driver will be a conversion of the legacy one to the spi-mem interface. I > would keep the legacy copyright and add Cryptera's below, as Boris suggested. > > [...] > >>>>> +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) >>>> >>>> Do you really to wait for both INSTRE and CSR to consider the command >>>> as complete? >>>> >>> >>> This part were really copied from Atmel driver. I wasn't sure so I >>> used tested solution. >> >> Okay. I guess that's a question for Cyrille and/or Tudor then. > > We have to wait for both INSTRE and CSR. > I've also found that information in datasheet, good we have this solution then. > Best, > ta > Best Regards, Piotr -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Piotr, On 06/27/2018 10:52 AM, Piotr Bugalski wrote: > >> General things to consider for the limitation in performance: >> - is the serial flash memory operating in Quad SPI? > > Yes, I've checked signal using logic analyzer, data is transferred using > all four lines. > >> - QSCLK should be as high as possible > > Sure, but when we are using lower frequency CPU impact should be > negligible while efficiency is crap on every speed. > >> - transfer delays - I checked them, they have default values, we should be good. >> - use DMA, as you suggested >> > > I don't understand one thing. While CPU is not busy and during my tests > 100% of CPU can be used for communication, efficiency is still very low. > Why DMA has such impact? > > It is very interesting to observe signals using logic analyzer. > When CPU is used for communication, there are long delays after > every byte transferred. These delays are much longer than it should be only because of writing next value by CPU. Are those consecutive transfers (same peripheral without removing chip select)? The delays between consecutive transfers can be set just in SPI mode. It would be strange to see this kind of delays in serial memory mode. > I tried to change SPI frequency. If delay were CPU related, > delay time should stay the same. Unfortunately results were different - > lowering SPI freqency extends delay time. If QSCK is less than f-perif-clock/2, then setting DLYBS to 1 will shorten the DLYBS delay, but this is peanuts. Thanks, ta > Using DMA makes these delays to disappear, but how to acheive CPU > communication without delays? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tudor, On Thu, 28 Jun 2018, Tudor Ambarus wrote: > Hi, Piotr, > > On 06/27/2018 10:52 AM, Piotr Bugalski wrote: >> >>> General things to consider for the limitation in performance: >>> - is the serial flash memory operating in Quad SPI? >> >> Yes, I've checked signal using logic analyzer, data is transferred using >> all four lines. >> >>> - QSCLK should be as high as possible >> >> Sure, but when we are using lower frequency CPU impact should be >> negligible while efficiency is crap on every speed. >> >>> - transfer delays - I checked them, they have default values, we should be good. >>> - use DMA, as you suggested >>> >> >> I don't understand one thing. While CPU is not busy and during my tests >> 100% of CPU can be used for communication, efficiency is still very low. >> Why DMA has such impact? >> >> It is very interesting to observe signals using logic analyzer. >> When CPU is used for communication, there are long delays after >> every byte transferred. These delays are much longer than it should be only because of writing next value by CPU. > > Are those consecutive transfers (same peripheral without removing chip select)? > The delays between consecutive transfers can be set just in SPI mode. It would > be strange to see this kind of delays in serial memory mode. > Yes, it's just single block transfer so no CS changes occurs. I find this delays strange also, but I have no idea how to avoid them. The same behaviour exists even when DMA is used in APB mode (write to registers). Only using SMM with DMA helps. >> I tried to change SPI frequency. If delay were CPU related, >> delay time should stay the same. Unfortunately results were different - >> lowering SPI freqency extends delay time. > > If QSCK is less than f-perif-clock/2, then setting DLYBS to 1 will shorten the > DLYBS delay, but this is peanuts. > I have DLYBS, DLYCS and DLYBCT set to zeros. I can try DLYBS=1 if you wish. > Thanks, > ta > >> Using DMA makes these delays to disappear, but how to acheive CPU >> communication without delays? > Thank you for comments, Piotr -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 4a77cfa3213d..4f70a7005997 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -84,6 +84,15 @@ config SPI_ATMEL This selects a driver for the Atmel SPI Controller, present on many AT91 ARM chips. +config SPI_ATMEL_QSPI + tristate "Atmel QuadSPI Controller" + depends on ARCH_AT91 || (ARM && COMPILE_TEST) + depends on OF && HAS_IOMEM + help + This selects a driver for the Atmel QSPI Controller on SAMA5D2. + This controller does not support generic SPI, it supports only + spi-mem interface. + config SPI_AU1550 tristate "Au1550/Au1200/Au1300 SPI Controller" depends on MIPS_ALCHEMY diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index fe54d787cf4d..6245a5693b16 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o obj-$(CONFIG_SPI_ALTERA) += spi-altera.o obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o obj-$(CONFIG_SPI_ATMEL) += spi-atmel.o +obj-$(CONFIG_SPI_ATMEL_QSPI) += spi-atmel-qspi.o obj-$(CONFIG_SPI_ATH79) += spi-ath79.o obj-$(CONFIG_SPI_AU1550) += spi-au1550.o obj-$(CONFIG_SPI_AXI_SPI_ENGINE) += spi-axi-spi-engine.o diff --git a/drivers/spi/spi-atmel-qspi.c b/drivers/spi/spi-atmel-qspi.c new file mode 100644 index 000000000000..1ee626201b0d --- /dev/null +++ b/drivers/spi/spi-atmel-qspi.c @@ -0,0 +1,480 @@ +/* + * Atmel SAMA5D2 QuadSPI driver. + * + * Copyright (C) 2018 Cryptera A/S + * All Rights Reserved + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 (GPL v2) + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/spi/spi-mem.h> +#include <linux/delay.h> + +#define QSPI_CR 0x0000 +#define QSPI_MR 0x0004 +#define QSPI_RDR 0x0008 +#define QSPI_TDR 0x000c +#define QSPI_SR 0x0010 +#define QSPI_IER 0x0014 +#define QSPI_IDR 0x0018 +#define QSPI_IMR 0x001c +#define QSPI_SCR 0x0020 + +#define QSPI_IAR 0x0030 +#define QSPI_ICR 0x0034 +#define QSPI_IFR 0x0038 + +#define QSPI_WPMR 0x00e4 +#define QSPI_WPSR 0x00e8 + +/* Bitfields in QSPI_CR (Control Register) */ +#define QSPI_CR_QSPIEN BIT(0) +#define QSPI_CR_QSPIDIS BIT(1) +#define QSPI_CR_SWRST BIT(7) +#define QSPI_CR_LASTXFER BIT(24) + +/* Bitfields in QSPI_ICR (Instruction Code Register) */ +#define QSPI_ICR_INST_MASK GENMASK(7, 0) +#define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK) +#define QSPI_ICR_OPT_MASK GENMASK(23, 16) +#define QSPI_ICR_OPT(opt) (((opt) << 16) & QSPI_ICR_OPT_MASK) + +/* Bitfields in QSPI_MR (Mode Register) */ +#define QSPI_MR_SMM BIT(0) +#define QSPI_MR_LLB BIT(1) +#define QSPI_MR_WDRBT BIT(2) +#define QSPI_MR_SMRM BIT(3) +#define QSPI_MR_CSMODE_MASK GENMASK(5, 4) +#define QSPI_MR_CSMODE_NOT_RELOADED (0 << 4) +#define QSPI_MR_CSMODE_LASTXFER (1 << 4) +#define QSPI_MR_CSMODE_SYSTEMATICALLY (2 << 4) +#define QSPI_MR_NBBITS_MASK GENMASK(11, 8) +#define QSPI_MR_NBBITS(n) ((((n) - 8) << 8) & QSPI_MR_NBBITS_MASK) +#define QSPI_MR_DLYBCT_MASK GENMASK(23, 16) +#define QSPI_MR_DLYBCT(n) (((n) << 16) & QSPI_MR_DLYBCT_MASK) +#define QSPI_MR_DLYCS_MASK GENMASK(31, 24) +#define QSPI_MR_DLYCS(n) (((n) << 24) & QSPI_MR_DLYCS_MASK) + +/* Bitfields in QSPI_IFR (Instruction Frame Register) */ +#define QSPI_IFR_WIDTH_MASK GENMASK(2, 0) +#define QSPI_IFR_WIDTH_SINGLE_BIT_SPI (0 << 0) +#define QSPI_IFR_WIDTH_DUAL_OUTPUT (1 << 0) +#define QSPI_IFR_WIDTH_QUAD_OUTPUT (2 << 0) +#define QSPI_IFR_WIDTH_DUAL_IO (3 << 0) +#define QSPI_IFR_WIDTH_QUAD_IO (4 << 0) +#define QSPI_IFR_WIDTH_DUAL_CMD (5 << 0) +#define QSPI_IFR_WIDTH_QUAD_CMD (6 << 0) +#define QSPI_IFR_INSTEN BIT(4) +#define QSPI_IFR_ADDREN BIT(5) +#define QSPI_IFR_OPTEN BIT(6) +#define QSPI_IFR_DATAEN BIT(7) +#define QSPI_IFR_OPTL_MASK GENMASK(9, 8) +#define QSPI_IFR_OPTL_1BIT (0 << 8) +#define QSPI_IFR_OPTL_2BIT (1 << 8) +#define QSPI_IFR_OPTL_4BIT (2 << 8) +#define QSPI_IFR_OPTL_8BIT (3 << 8) +#define QSPI_IFR_ADDRL BIT(10) +#define QSPI_IFR_TFRTYP_MASK GENMASK(13, 12) +#define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) +#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) +#define QSPI_IFR_TFRTYP_TRSFR_WRITE (2 << 12) +#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13) +#define QSPI_IFR_CRM BIT(14) +#define QSPI_IFR_NBDUM_MASK GENMASK(20, 16) +#define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK) + +/* Bitfields in QSPI_SR/QSPI_IER/QSPI_IDR/QSPI_IMR */ +#define QSPI_SR_RDRF BIT(0) +#define QSPI_SR_TDRE BIT(1) +#define QSPI_SR_TXEMPTY BIT(2) +#define QSPI_SR_OVRES BIT(3) +#define QSPI_SR_CSR BIT(8) +#define QSPI_SR_CSS BIT(9) +#define QSPI_SR_INSTRE BIT(10) +#define QSPI_SR_QSPIENS BIT(24) + +#define QSPI_SR_CMD_COMPLETED (QSPI_SR_INSTRE | QSPI_SR_CSR) + + +/* Bitfields in QSPI_SCR (Serial Clock Register) */ +#define QSPI_SCR_CPOL BIT(0) +#define QSPI_SCR_CPHA BIT(1) +#define QSPI_SCR_SCBR_MASK GENMASK(15, 8) +#define QSPI_SCR_SCBR(n) (((n) << 8) & QSPI_SCR_SCBR_MASK) +#define QSPI_SCR_DLYBS_MASK GENMASK(23, 16) +#define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK) + +#define QSPI_WPMR_WPKEY_PASSWD (0x515350u << 8) + +struct atmel_qspi { + struct platform_device *pdev; + void __iomem *iobase; + void __iomem *ahb_addr; + int irq; + struct clk *clk; + u32 clk_rate; + struct completion cmd_done; + u32 pending; +}; + +struct qspi_mode { + u8 cmd_buswidth; + u8 addr_buswidth; + u8 data_buswidth; + u32 config; +}; + +static const struct qspi_mode sama5d2_qspi_modes[] = { + { 1, 1, 1, QSPI_IFR_WIDTH_SINGLE_BIT_SPI }, + { 1, 1, 2, QSPI_IFR_WIDTH_DUAL_OUTPUT }, + { 1, 1, 4, QSPI_IFR_WIDTH_QUAD_OUTPUT }, + { 1, 2, 2, QSPI_IFR_WIDTH_DUAL_IO }, + { 1, 4, 4, QSPI_IFR_WIDTH_QUAD_IO }, + { 2, 2, 2, QSPI_IFR_WIDTH_DUAL_CMD }, + { 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD }, +}; + +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg) +{ + return readl_relaxed(aq->iobase + reg); +} + +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value) +{ + writel_relaxed(value, aq->iobase + reg); +} + +static int atmel_qspi_init(struct atmel_qspi *aq) +{ + unsigned long rate; + u32 scbr; + + qspi_writel(aq, QSPI_WPMR, QSPI_WPMR_WPKEY_PASSWD); + + /* software reset */ + qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST); + + /* set QSPI mode */ + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); + + rate = clk_get_rate(aq->clk); + if (!rate) + return -EINVAL; + + /* set baudrate */ + scbr = DIV_ROUND_UP(rate, aq->clk_rate); + if (scbr > 0) + scbr--; + qspi_writel(aq, QSPI_SCR, QSPI_SCR_SCBR(scbr)); + + /* enable qspi controller */ + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN); + + return 0; +} + +static int atmel_qspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) +{ + return 0; +} + +static inline bool is_compatible(const struct spi_mem_op *op, + const struct qspi_mode *mode) +{ + if (op->cmd.buswidth != mode->cmd_buswidth) + return false; + if (op->addr.nbytes && op->addr.buswidth != mode->addr_buswidth) + return false; + if (op->data.nbytes && op->data.buswidth != mode->data_buswidth) + return false; + return true; +} + +static int find_mode(const struct spi_mem_op *op) +{ + u32 i; + + for (i = 0; i < ARRAY_SIZE(sama5d2_qspi_modes); i++) + if (is_compatible(op, &sama5d2_qspi_modes[i])) + return i; + return -1; +} + +static bool atmel_qspi_supports_op(struct spi_mem *mem, + const struct spi_mem_op *op) +{ + if (find_mode(op) < 0) + return false; + + // special case not supported by hardware + if ((op->addr.nbytes == 2) && (op->cmd.buswidth != op->addr.buswidth) && + (op->dummy.nbytes == 0)) + return false; + + return true; +} + +static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) +{ + struct spi_controller *ctrl = dev_id; + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); + u32 status, mask, pending; + + status = qspi_readl(aq, QSPI_SR); + mask = qspi_readl(aq, QSPI_IMR); + pending = status & mask; + + if (!pending) + return IRQ_NONE; + + aq->pending |= pending; + if ((aq->pending & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED) + complete(&aq->cmd_done); + + return IRQ_HANDLED; +} + +static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) +{ + struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master); + int mode; + u32 addr = 0; + u32 dummy_cycles = 0; + u32 ifr = QSPI_IFR_INSTEN; + u32 icr = QSPI_ICR_INST(op->cmd.opcode); + + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); + + mode = find_mode(op); + if (mode < 0) + return -1; + ifr |= sama5d2_qspi_modes[mode].config; + + if (op->dummy.buswidth && op->dummy.nbytes) + dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth; + + if (op->addr.buswidth) { + switch (op->addr.nbytes) { + case 0: + break; + case 1: + ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT; + icr |= QSPI_ICR_OPT(op->addr.val & 0xff); + break; + case 2: + if (dummy_cycles < 8 / op->addr.buswidth) { + ifr &= ~QSPI_IFR_INSTEN; + addr = (op->cmd.opcode << 16) | + (op->addr.val & 0xffff); + ifr |= QSPI_IFR_ADDREN; + } else { + addr = (op->addr.val << 8) & 0xffffff; + dummy_cycles -= 8 / op->addr.buswidth; + ifr |= QSPI_IFR_ADDREN; + } + break; + case 3: + addr = op->addr.val & 0xffffff; + ifr |= QSPI_IFR_ADDREN; + break; + case 4: + addr = op->addr.val; + ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL; + break; + default: + return -1; + } + } + ifr |= QSPI_IFR_NBDUM(dummy_cycles); + if (op->data.nbytes == 0) + qspi_writel(aq, QSPI_IAR, addr); + else + ifr |= QSPI_IFR_DATAEN; + + if ((op->data.dir == SPI_MEM_DATA_IN) && (op->data.nbytes > 0)) + ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; + else + ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; + + qspi_writel(aq, QSPI_IAR, addr); + qspi_writel(aq, QSPI_ICR, icr); + qspi_writel(aq, QSPI_IFR, ifr); + qspi_readl(aq, QSPI_IFR); + + if (op->data.nbytes > 0) { + if (op->data.dir == SPI_MEM_DATA_IN) + _memcpy_fromio(op->data.buf.in, + aq->ahb_addr + addr, op->data.nbytes); + else + _memcpy_toio(aq->ahb_addr + addr, + op->data.buf.out, op->data.nbytes); + + qspi_writel(aq, QSPI_CR, QSPI_CR_LASTXFER); + } + + aq->pending = qspi_readl(aq, QSPI_SR) & QSPI_SR_CMD_COMPLETED; + if (aq->pending == QSPI_SR_CMD_COMPLETED) + return 0; + reinit_completion(&aq->cmd_done); + qspi_writel(aq, QSPI_IER, QSPI_SR_CMD_COMPLETED); + wait_for_completion(&aq->cmd_done); + qspi_writel(aq, QSPI_IDR, QSPI_SR_CMD_COMPLETED); + + return 0; +} + +static const struct spi_controller_mem_ops atmel_qspi_mem_ops = { + .adjust_op_size = atmel_qspi_adjust_op_size, + .supports_op = atmel_qspi_supports_op, + .exec_op = atmel_qspi_exec_op +}; + +static int atmel_qspi_probe(struct platform_device *pdev) +{ + struct spi_controller *ctrl; + struct atmel_qspi *aq; + struct device_node *np = pdev->dev.of_node; + struct device_node *child; + struct resource *res; + int irq, err = 0; + + ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq)); + if (!ctrl) + return -ENOMEM; + + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD; + ctrl->bus_num = -1; + ctrl->mem_ops = &atmel_qspi_mem_ops; + ctrl->num_chipselect = 1; + ctrl->dev.of_node = pdev->dev.of_node; + platform_set_drvdata(pdev, ctrl); + + aq = spi_controller_get_devdata(ctrl); + + if (of_get_child_count(np) != 1) + return -ENODEV; + child = of_get_next_child(np, NULL); + + aq->pdev = pdev; + + /* map registers */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base"); + aq->iobase = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(aq->iobase)) { + dev_err(&pdev->dev, "missing registers\n"); + err = PTR_ERR(aq->iobase); + goto err_put_ctrl; + } + + /* map AHB memory */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap"); + aq->ahb_addr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(aq->ahb_addr)) { + dev_err(&pdev->dev, "missing AHB memory\n"); + err = PTR_ERR(aq->ahb_addr); + goto err_put_ctrl; + } + + /* get 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 err_put_ctrl; + } + + err = clk_prepare_enable(aq->clk); + if (err) { + dev_err(&pdev->dev, "failed to enable peripheral clock\n"); + goto err_put_ctrl; + } + + /* request 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), ctrl); + if (err) + goto disable_clk; + + err = of_property_read_u32(child, "spi-max-frequency", &aq->clk_rate); + if (err < 0) + goto disable_clk; + + init_completion(&aq->cmd_done); + + err = atmel_qspi_init(aq); + if (err) + goto disable_clk; + + of_node_put(child); + + err = spi_register_controller(ctrl); + if (err) + goto disable_clk; + + return 0; + +disable_clk: + clk_disable_unprepare(aq->clk); +err_put_ctrl: + spi_controller_put(ctrl); + of_node_put(child); + return err; +} + +static int atmel_qspi_remove(struct platform_device *pdev) +{ + struct spi_controller *ctrl = platform_get_drvdata(pdev); + struct atmel_qspi *aq = spi_controller_get_devdata(ctrl); + + qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS); + clk_disable_unprepare(aq->clk); + + spi_unregister_controller(ctrl); + + return 0; +} + +static const struct of_device_id atmel_qspi_dt_ids[] = { + { + .compatible = "atmel,sama5d2-spi-qspi" + }, + {} +}; +MODULE_DEVICE_TABLE(of, atmel_qspi_dt_ids); + +static struct platform_driver atmel_qspi_driver = { + .driver = { + .name = "atmel_spi_qspi", + .of_match_table = atmel_qspi_dt_ids + }, + .probe = atmel_qspi_probe, + .remove = atmel_qspi_remove +}; + +module_platform_driver(atmel_qspi_driver); + + +MODULE_DESCRIPTION("Atmel SAMA5D2 QuadSPI Driver"); +MODULE_AUTHOR("Piotr Bugalski <pbu@cryptera.com"); +MODULE_LICENSE("GPL v2"); +
Kernel contains QSPI driver strongly tied to MTD and nor-flash memory. New spi-mem interface allows usage also other memory types, especially much larger NAND with SPI interface. This driver works as SPI controller and is not related to MTD, however can work with NAND-flash or other peripherals using spi-mem interface. Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> Signed-off-by: Piotr Bugalski <pbu@cryptera.com> --- drivers/spi/Kconfig | 9 + drivers/spi/Makefile | 1 + drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 490 insertions(+) create mode 100644 drivers/spi/spi-atmel-qspi.c