Hello,
Atmel SAMA5D2 is equipped with two QSPI interfaces. These interfaces can
work as in SPI-compatible mode or use two / four lines to improve
communication speed. At the moment there is QSPI driver strongly tied to
NOR-flash memory and MTD subsystem.
Intention of this change is to provide new driver which will not be tied
to MTD and allows using QSPI with NAND-flash memory or other peripherals
New spi-mem API provides abstraction layer which can disconnect QSPI
from MTD. This driver doesn't support regular SPI interface, it should
be used with spi-mem interface only.
Unfortunately SAMA5D2 hardware by default supports only NOR-flash
memory. It allows 24- and 32-bit addressing while NAND-flash requires
16-bit long. To workaround hardware limitation driver is a bit more
complicated.
Request to spi-mem contains three fiels: opcode (command), address,
dummy bytes. SAMA5D2 QSPI hardware supports opcode, address, dummy and
option byte where address field can only be 24- or 32- bytes long.
Handling 8-bits long addresses is done using option field. For 16-bits
address behaviour depends of number of requested dummy bits. If there
are 8 or more dummy cycles, address is shifted and sent with first dummy
byte. Otherwise opcode is disabled and first byte of address contains
command opcode (works only if opcode and address use the same buswidth).
The limitation is when 16-bit address is used without enough dummy
cycles and opcode is using different buswidth than address. Other modes
are supported with described workaround.
It looks like hardware has some limitation in performance. The same issue
exists in current QSPI driver (MTD/nor-flash) and soft-pack (bare-metal
library from Atmel). Without using DMA read speed is much worse than
maximum bandwidth (efficiency 30-40%). Any help with performance
improvement is highly welcome, especially for NAND-flash memories which
offers higher capacity than NOR-flash used with previous driver.
Best Regards,
Piotr
Piotr Bugalski (2):
spi: Add QuadSPI driver for Atmel SAMA5D2
dt-bindings: spi: QuadSPI driver for Atmel SAMA5D2 documentation
.../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++
drivers/spi/Kconfig | 9 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++
4 files changed, 531 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
create mode 100644 drivers/spi/spi-atmel-qspi.c
--
2.11.0
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 <[email protected]>
Signed-off-by: Piotr Bugalski <[email protected]>
---
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 @@
+/*
+ * 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 <[email protected]");
+MODULE_LICENSE("GPL v2");
+
--
2.11.0
Documentation for DT-binding change.
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Piotr Bugalski <[email protected]>
---
.../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++++++++++++++++++++++
1 file changed, 41 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
diff --git a/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
new file mode 100644
index 000000000000..d52b534c9c2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
@@ -0,0 +1,41 @@
+* Atmel Quad Serial Peripheral Interface (QSPI)
+
+Required properties:
+- compatible: Should be "atmel,sama5d2-spi-qspi".
+- reg: Should contain the locations and lengths of the base registers
+ and the mapped memory.
+- reg-names: Should contain the resource reg names:
+ - qspi_base: configuration register address space
+ - qspi_mmap: memory mapped address space
+- interrupts: Should contain the interrupt for the device.
+- clocks: The phandle of the clock needed by the QSPI controller.
+- #address-cells: Should be <1>.
+- #size-cells: Should be <0>.
+
+Example:
+
+qspi1: spi@f0024000 {
+ compatible = "atmel,sama5d2-spi-qspi";
+ reg = <0xf0024000 0x100>, <0xd8000000 0x08000000>;
+ reg-names = "qspi_base", "qspi_mmap";
+ interrupts = <53 IRQ_TYPE_LEVEL_HIGH 7>;
+ clocks = <&qspi1_clk>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_qspi1_default>;
+ status = "okay";
+
+ flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "winbond,w25m02gv", "spi-nand";
+ reg = <0>;
+ spi-max-frequency = <83000000>;
+ spi-rx-bus-width = <4>;
+ spi-tx-bus-width = <4>;
+
+ ...
+ };
+};
+
--
2.11.0
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
Hi Piotr,
On Mon, 18 Jun 2018 18:21:24 +0200
Piotr Bugalski <[email protected]> wrote:
> Documentation for DT-binding change.
>
> Suggested-by: Boris Brezillon <[email protected]>
I'm pretty sure I didn't make a single suggestion about the DT
bindings you use here ;-).
> Signed-off-by: Piotr Bugalski <[email protected]>
>
> ---
> .../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
I'll comment on this aspect in more details when replying to the cover
letter, but I think you should re-use the bindings defined in
Documentation/devicetree/bindings/mtd/atmel-quadspi.txt (IOW, move the
existing file to the Documentation/devicetree/bindings/spi directory).
It's the same HW block, and just because you develop a new driver to
replace the old one doesn't mean you should have 2 different bindings in
parallel.
>
> diff --git a/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
> new file mode 100644
> index 000000000000..d52b534c9c2b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
> @@ -0,0 +1,41 @@
> +* Atmel Quad Serial Peripheral Interface (QSPI)
> +
> +Required properties:
> +- compatible: Should be "atmel,sama5d2-spi-qspi".
> +- reg: Should contain the locations and lengths of the base registers
> + and the mapped memory.
> +- reg-names: Should contain the resource reg names:
> + - qspi_base: configuration register address space
> + - qspi_mmap: memory mapped address space
> +- interrupts: Should contain the interrupt for the device.
> +- clocks: The phandle of the clock needed by the QSPI controller.
> +- #address-cells: Should be <1>.
> +- #size-cells: Should be <0>.
> +
> +Example:
> +
> +qspi1: spi@f0024000 {
> + compatible = "atmel,sama5d2-spi-qspi";
> + reg = <0xf0024000 0x100>, <0xd8000000 0x08000000>;
> + reg-names = "qspi_base", "qspi_mmap";
> + interrupts = <53 IRQ_TYPE_LEVEL_HIGH 7>;
> + clocks = <&qspi1_clk>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_qspi1_default>;
> + status = "okay";
> +
> + flash@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "winbond,w25m02gv", "spi-nand";
"winbond,w25m02gv" is undocumented and unnecessary since SPI NANDs are
automatically detected. Also, maybe you should declare a SPI NOR in the
example since SPI NAND support has not yet been merged.
> + reg = <0>;
> + spi-max-frequency = <83000000>;
> + spi-rx-bus-width = <4>;
> + spi-tx-bus-width = <4>;
> +
> + ...
> + };
> +};
> +
Regards,
Boris
Hi Piotr,
On Mon, 18 Jun 2018 18:21:22 +0200
Piotr Bugalski <[email protected]> wrote:
> Hello,
>
> Atmel SAMA5D2 is equipped with two QSPI interfaces. These interfaces can
> work as in SPI-compatible mode or use two / four lines to improve
> communication speed. At the moment there is QSPI driver strongly tied to
> NOR-flash memory and MTD subsystem.
> Intention of this change is to provide new driver which will not be tied
> to MTD and allows using QSPI with NAND-flash memory or other peripherals
> New spi-mem API provides abstraction layer which can disconnect QSPI
> from MTD. This driver doesn't support regular SPI interface, it should
> be used with spi-mem interface only.
Glad to see that people are starting to convert their SPI NOR
controller drivers to the SPI mem approach.
> Unfortunately SAMA5D2 hardware by default supports only NOR-flash
> memory. It allows 24- and 32-bit addressing while NAND-flash requires
> 16-bit long. To workaround hardware limitation driver is a bit more
> complicated.
>
> Request to spi-mem contains three fiels: opcode (command), address,
> dummy bytes. SAMA5D2 QSPI hardware supports opcode, address, dummy and
> option byte where address field can only be 24- or 32- bytes long.
> Handling 8-bits long addresses is done using option field. For 16-bits
> address behaviour depends of number of requested dummy bits. If there
> are 8 or more dummy cycles, address is shifted and sent with first dummy
> byte. Otherwise opcode is disabled and first byte of address contains
> command opcode (works only if opcode and address use the same buswidth).
> The limitation is when 16-bit address is used without enough dummy
> cycles and opcode is using different buswidth than address. Other modes
> are supported with described workaround.
>
> It looks like hardware has some limitation in performance. The same issue
> exists in current QSPI driver (MTD/nor-flash) and soft-pack (bare-metal
> library from Atmel). Without using DMA read speed is much worse than
> maximum bandwidth (efficiency 30-40%). Any help with performance
> improvement is highly welcome, especially for NAND-flash memories which
> offers higher capacity than NOR-flash used with previous driver.
>
> Best Regards,
> Piotr
>
> Piotr Bugalski (2):
> spi: Add QuadSPI driver for Atmel SAMA5D2
> dt-bindings: spi: QuadSPI driver for Atmel SAMA5D2 documentation
>
> .../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++
> drivers/spi/Kconfig | 9 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++
I'd like a solution where we remove the old driver. I definitely don't
want to have both in parallel. Did you test the new driver with a SPI
NOR to check if it still works correctly? If you did, then I'd suggest
that you add a patch updating defconfigs where the SPI_ATMEL_QUADSPI is
selected and another patch removing the old driver.
> 4 files changed, 531 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
This should be a simple mv from
Documentation/devicetree/bindings/mtd/atmel-quadspi.txt to
Documentation/devicetree/bindings/spi/spi-atmel-qspi.txt
> create mode 100644 drivers/spi/spi-atmel-qspi.c
>
Thanks,
Boris
Hi Boris,
Thank you very much for quick response.
On Wed, 20 Jun 2018, Boris Brezillon wrote:
> Hi Piotr,
>
> On Mon, 18 Jun 2018 18:21:22 +0200
> Piotr Bugalski <[email protected]> wrote:
>
>> Hello,
>>
>> Atmel SAMA5D2 is equipped with two QSPI interfaces. These interfaces can
>> work as in SPI-compatible mode or use two / four lines to improve
>> communication speed. At the moment there is QSPI driver strongly tied to
>> NOR-flash memory and MTD subsystem.
>> Intention of this change is to provide new driver which will not be tied
>> to MTD and allows using QSPI with NAND-flash memory or other peripherals
>> New spi-mem API provides abstraction layer which can disconnect QSPI
>> from MTD. This driver doesn't support regular SPI interface, it should
>> be used with spi-mem interface only.
>
> Glad to see that people are starting to convert their SPI NOR
> controller drivers to the SPI mem approach.
>
>> Unfortunately SAMA5D2 hardware by default supports only NOR-flash
>> memory. It allows 24- and 32-bit addressing while NAND-flash requires
>> 16-bit long. To workaround hardware limitation driver is a bit more
>> complicated.
>>
>> Request to spi-mem contains three fiels: opcode (command), address,
>> dummy bytes. SAMA5D2 QSPI hardware supports opcode, address, dummy and
>> option byte where address field can only be 24- or 32- bytes long.
>> Handling 8-bits long addresses is done using option field. For 16-bits
>> address behaviour depends of number of requested dummy bits. If there
>> are 8 or more dummy cycles, address is shifted and sent with first dummy
>> byte. Otherwise opcode is disabled and first byte of address contains
>> command opcode (works only if opcode and address use the same buswidth).
>> The limitation is when 16-bit address is used without enough dummy
>> cycles and opcode is using different buswidth than address. Other modes
>> are supported with described workaround.
>>
>> It looks like hardware has some limitation in performance. The same issue
>> exists in current QSPI driver (MTD/nor-flash) and soft-pack (bare-metal
>> library from Atmel). Without using DMA read speed is much worse than
>> maximum bandwidth (efficiency 30-40%). Any help with performance
>> improvement is highly welcome, especially for NAND-flash memories which
>> offers higher capacity than NOR-flash used with previous driver.
>>
>> Best Regards,
>> Piotr
>>
>> Piotr Bugalski (2):
>> spi: Add QuadSPI driver for Atmel SAMA5D2
>> dt-bindings: spi: QuadSPI driver for Atmel SAMA5D2 documentation
>>
>> .../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++
>> drivers/spi/Kconfig | 9 +
>> drivers/spi/Makefile | 1 +
>> drivers/spi/spi-atmel-qspi.c | 480 +++++++++++++++++++++
>
> I'd like a solution where we remove the old driver. I definitely don't
> want to have both in parallel. Did you test the new driver with a SPI
> NOR to check if it still works correctly? If you did, then I'd suggest
> that you add a patch updating defconfigs where the SPI_ATMEL_QUADSPI is
> selected and another patch removing the old driver.
>
I misunderstood a bit your idea. My main concern was NAND-flash with
QSPI interface and I expected original nor-flash driver to stay
untouched - at least now. However idea of replacement older driver
with spi-mem approach makes a lot of sense to me.
I'll test nor-flash with new driver first, it should work but I prefer
to be sure. In next version I'll follow your suggestion and replace
old nor-flash driver.
>> 4 files changed, 531 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
>
> This should be a simple mv from
> Documentation/devicetree/bindings/mtd/atmel-quadspi.txt to
> Documentation/devicetree/bindings/spi/spi-atmel-qspi.txt
>
>> create mode 100644 drivers/spi/spi-atmel-qspi.c
>>
>
> Thanks,
>
> Boris
>
Thank you for comments,
Piotr
Hi Boris,
On Wed, 20 Jun 2018, Boris Brezillon wrote:
> Hi Piotr,
>
> On Mon, 18 Jun 2018 18:21:24 +0200
> Piotr Bugalski <[email protected]> wrote:
>
>> Documentation for DT-binding change.
>>
>> Suggested-by: Boris Brezillon <[email protected]>
>
> I'm pretty sure I didn't make a single suggestion about the DT
> bindings you use here ;-).
>
Ok, I misunderstood a bit your idea, but I think from next release this
field will be in good place. So it was just prepared for the future ;-)
>> Signed-off-by: Piotr Bugalski <[email protected]>
>>
>> ---
>> .../devicetree/bindings/spi/spi_atmel-qspi.txt | 41 ++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
>
> I'll comment on this aspect in more details when replying to the cover
> letter, but I think you should re-use the bindings defined in
> Documentation/devicetree/bindings/mtd/atmel-quadspi.txt (IOW, move the
> existing file to the Documentation/devicetree/bindings/spi directory).
>
> It's the same HW block, and just because you develop a new driver to
> replace the old one doesn't mean you should have 2 different bindings in
> parallel.
I'll change it in next version.
>
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
>> new file mode 100644
>> index 000000000000..d52b534c9c2b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
>> @@ -0,0 +1,41 @@
>> +* Atmel Quad Serial Peripheral Interface (QSPI)
>> +
>> +Required properties:
>> +- compatible: Should be "atmel,sama5d2-spi-qspi".
>> +- reg: Should contain the locations and lengths of the base registers
>> + and the mapped memory.
>> +- reg-names: Should contain the resource reg names:
>> + - qspi_base: configuration register address space
>> + - qspi_mmap: memory mapped address space
>> +- interrupts: Should contain the interrupt for the device.
>> +- clocks: The phandle of the clock needed by the QSPI controller.
>> +- #address-cells: Should be <1>.
>> +- #size-cells: Should be <0>.
>> +
>> +Example:
>> +
>> +qspi1: spi@f0024000 {
>> + compatible = "atmel,sama5d2-spi-qspi";
>> + reg = <0xf0024000 0x100>, <0xd8000000 0x08000000>;
>> + reg-names = "qspi_base", "qspi_mmap";
>> + interrupts = <53 IRQ_TYPE_LEVEL_HIGH 7>;
>> + clocks = <&qspi1_clk>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_qspi1_default>;
>> + status = "okay";
>> +
>> + flash@0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "winbond,w25m02gv", "spi-nand";
>
> "winbond,w25m02gv" is undocumented and unnecessary since SPI NANDs are
> automatically detected. Also, maybe you should declare a SPI NOR in the
> example since SPI NAND support has not yet been merged.
>
I was mainly focusing on NAND-flash with QSPI inteface so I took
example from tested configuration. Next time I'll use NOR-flash.
>> + reg = <0>;
>> + spi-max-frequency = <83000000>;
>> + spi-rx-bus-width = <4>;
>> + spi-tx-bus-width = <4>;
>> +
>> + ...
>> + };
>> +};
>> +
>
> Regards,
>
> Boris
>
Thanks,
Piotr
Hi Piotr,
On Mon, 18 Jun 2018 18:21:23 +0200
Piotr Bugalski <[email protected]> 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 <[email protected]>
> Signed-off-by: Piotr Bugalski <[email protected]>
>
> ---
> 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 <[email protected]");
> +MODULE_LICENSE("GPL v2");
> +
Drop this blank line.
Regards,
Boris
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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Piotr Bugalski <[email protected]>
>>
>> ---
>> 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 <[email protected]");
>> +MODULE_LICENSE("GPL v2");
>> +
>
> Drop this blank line.
>
> Regards,
>
> Boris
>
Regards,
Piotr
Hi Piotr,
On Fri, 22 Jun 2018 07:57:10 +0200 (CEST)
Piotr Bugalski <[email protected]> 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
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
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
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?
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