Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller

On 03/29/2017 03:09 PM, Marek Vasut wrote:

On 03/29/2017 02:24 PM, Ludovic BARRE wrote:

quoted

hi Marek

Hi!

quoted

thanks for review
comment below

On 03/29/2017 12:54 PM, Marek Vasut wrote:

quoted

On 03/27/2017 02:54 PM, Ludovic Barre wrote:

quoted

From: Ludovic Barre <redacted>

The quadspi is a specialized communication interface targeting single,
dual or quad SPI Flash memories.

It can operate in any of the following modes:
-indirect mode: all the operations are performed using the quadspi
   registers
-read memory-mapped mode: the external Flash memory is mapped to the
   microcontroller address space and is seen by the system as if it was
   an internal memory

Signed-off-by: Ludovic Barre <redacted>

Hi! I presume this is not compatible with the Cadence QSPI or any other
QSPI controller, huh ?

it's not a cadence QSPI, it's specific for stm32 platform

OK, got it. Didn't ST use Cadence IP somewhere too though ?
That's probably what confused me, oh well ...

quoted

quoted

Mostly minor nits below ...

quoted

---
   drivers/mtd/spi-nor/Kconfig         |   7 +
   drivers/mtd/spi-nor/Makefile        |   1 +
   drivers/mtd/spi-nor/stm32-quadspi.c | 679
++++++++++++++++++++++++++++++++++++
   3 files changed, 687 insertions(+)
   create mode 100644 drivers/mtd/spi-nor/stm32-quadspi.c

[...]

quoted

+struct stm32_qspi_cmd {
+    struct {
+        u32 addr_width:8;
+        u32 dummy:8;
+        u32 data:1;
+    } conf;

This could all be u8 ? Why this awful construct ?

yes I could replace each u32 by u8

Yeah, please do .

quoted

quoted

quoted

+    u8 opcode;
+    u32 framemode;
+    u32 qspimode;
+    u32 addr;
+    size_t len;
+    void *buf;
+};
+
+static int stm32_qspi_wait_cmd(struct stm32_qspi *qspi)
+{
+    u32 cr;
+    int err = 0;

Can readl_poll_timeout() or somesuch replace this function ?

in fact stm32_qspi_wait_cmd test if the transfer has been complete (TCF
flag)
else initialize an interrupt completion.
like the time may be long I prefer keep the interrupt wait, if you agree.

I don't really mind if it fits the hardware better and I agree with you
it does here.

quoted

quoted

quoted

+    if (readl_relaxed(qspi->io_base + QUADSPI_SR) & SR_TCF)
+        return 0;
+
+    reinit_completion(&qspi->cmd_completion);
+    cr = readl_relaxed(qspi->io_base + QUADSPI_CR);
+    writel_relaxed(cr | CR_TCIE, qspi->io_base + QUADSPI_CR);
+
+    if
(!wait_for_completion_interruptible_timeout(&qspi->cmd_completion,
+                               msecs_to_jiffies(1000)))
+        err = -ETIMEDOUT;
+
+    writel_relaxed(cr, qspi->io_base + QUADSPI_CR);
+    return err;
+}
+
+static int stm32_qspi_wait_nobusy(struct stm32_qspi *qspi)
+{
+    u32 sr;
+
+    return readl_relaxed_poll_timeout(qspi->io_base + QUADSPI_SR, sr,
+                      !(sr & SR_BUSY), 10, 100000);
+}
+
+static void stm32_qspi_set_framemode(struct spi_nor *nor,
+                     struct stm32_qspi_cmd *cmd, bool read)
+{
+    u32 dmode = CCR_DMODE_1;
+
+    cmd->framemode = CCR_IMODE_1;
+
+    if (read) {
+        switch (nor->flash_read) {
+        case SPI_NOR_NORMAL:
+        case SPI_NOR_FAST:
+            dmode = CCR_DMODE_1;
+            break;
+        case SPI_NOR_DUAL:
+            dmode = CCR_DMODE_2;
+            break;
+        case SPI_NOR_QUAD:
+            dmode = CCR_DMODE_4;
+            break;
+        }
+    }
+
+    cmd->framemode |= cmd->conf.data ? dmode : 0;
+    cmd->framemode |= cmd->conf.addr_width ? CCR_ADMODE_1 : 0;
+}
+
+static void stm32_qspi_read_fifo(u8 *val, void __iomem *addr)
+{
+    *val = readb_relaxed(addr);
+}
+
+static void stm32_qspi_write_fifo(u8 *val, void __iomem *addr)
+{
+    writeb_relaxed(*val, addr);
+}
+
+static int stm32_qspi_tx_poll(struct stm32_qspi *qspi,
+                  const struct stm32_qspi_cmd *cmd)
+{
+    void (*tx_fifo)(u8 *, void __iomem *);
+    u32 len = cmd->len, sr;
+    u8 *buf = cmd->buf;
+    int ret;
+
+    if (cmd->qspimode == CCR_FMODE_INDW)
+        tx_fifo = stm32_qspi_write_fifo;
+    else
+        tx_fifo = stm32_qspi_read_fifo;
+
+    while (len--) {
+        ret = readl_relaxed_poll_timeout(qspi->io_base + QUADSPI_SR,
+                         sr, (sr & SR_FTF),
+                         10, 30000);

Add macros for those ad-hoc timeouts.

I will add 2 defines (for stm32_qspi_wait_nobusy, stm32_qspi_tx_poll)
#define STM32_QSPI_FIFO_TIMEOUT_US 30000
#define STM32_QSPI_BUSY_TIMEOUT_US 100000

Super

quoted

quoted

quoted

+        if (ret) {
+            dev_err(qspi->dev, "fifo timeout (stat:%#x)\n", sr);
+            break;
+        }
+        tx_fifo(buf++, qspi->io_base + QUADSPI_DR);
+    }
+
+    return ret;
+}

[...]

quoted

+static int stm32_qspi_send(struct stm32_qspi_flash *flash,
+               const struct stm32_qspi_cmd *cmd)
+{
+    struct stm32_qspi *qspi = flash->qspi;
+    u32 ccr, dcr, cr;
+    int err;
+
+    err = stm32_qspi_wait_nobusy(qspi);
+    if (err)
+        goto abort;
+
+    dcr = readl_relaxed(qspi->io_base + QUADSPI_DCR) & ~DCR_FSIZE_MASK;
+    dcr |= DCR_FSIZE(flash->fsize);
+    writel_relaxed(dcr, qspi->io_base + QUADSPI_DCR);
+
+    cr = readl_relaxed(qspi->io_base + QUADSPI_CR);
+    cr &= ~CR_PRESC_MASK & ~CR_FSEL;
+    cr |= CR_PRESC(flash->presc);
+    cr |= flash->cs ? CR_FSEL : 0;
+    writel_relaxed(cr, qspi->io_base + QUADSPI_CR);
+
+    if (cmd->conf.data)
+        writel_relaxed(cmd->len - 1, qspi->io_base + QUADSPI_DLR);
+
+    ccr = cmd->framemode | cmd->qspimode;
+
+    if (cmd->conf.dummy)
+        ccr |= CCR_DCYC(cmd->conf.dummy);
+
+    if (cmd->conf.addr_width)
+        ccr |= CCR_ADSIZE(cmd->conf.addr_width - 1);
+
+    ccr |= CCR_INST(cmd->opcode);
+    writel_relaxed(ccr, qspi->io_base + QUADSPI_CCR);
+
+    if (cmd->conf.addr_width && cmd->qspimode != CCR_FMODE_MM)
+        writel_relaxed(cmd->addr, qspi->io_base + QUADSPI_AR);
+
+    err = stm32_qspi_tx(qspi, cmd);
+    if (err)
+        goto abort;
+
+    if (cmd->qspimode != CCR_FMODE_MM) {
+        err = stm32_qspi_wait_cmd(qspi);
+        if (err)
+            goto abort;
+        writel_relaxed(FCR_CTCF, qspi->io_base + QUADSPI_FCR);
+    }
+
+    return err;
+
+abort:
+    writel_relaxed(readl_relaxed(qspi->io_base + QUADSPI_CR)
+               | CR_ABORT, qspi->io_base + QUADSPI_CR);

Use a helper variable here too.

ok

quoted

quoted

+    dev_err(qspi->dev, "%s abort err:%d\n", __func__, err);
+    return err;
+}

[...]

quoted

+static int stm32_qspi_flash_setup(struct stm32_qspi *qspi,
+                  struct device_node *np)
+{
+    u32 width, flash_read, presc, cs_num, max_rate = 0;
+    struct stm32_qspi_flash *flash;
+    struct mtd_info *mtd;
+    int ret;
+
+    of_property_read_u32(np, "reg", &cs_num);
+    if (cs_num >= STM32_MAX_NORCHIP)
+        return -EINVAL;
+
+    of_property_read_u32(np, "spi-max-frequency", &max_rate);
+    if (!max_rate)
+        return -EINVAL;
+
+    presc = DIV_ROUND_UP(qspi->clk_rate, max_rate) - 1;
+
+    if (of_property_read_u32(np, "spi-rx-bus-width", &width))
+        width = 1;
+
+    if (width == 4)
+        flash_read = SPI_NOR_QUAD;
+    else if (width == 2)
+        flash_read = SPI_NOR_DUAL;
+    else if (width == 1)
+        flash_read = SPI_NOR_NORMAL;
+    else
+        return -EINVAL;
+
+    flash = &qspi->flash[cs_num];
+    flash->qspi = qspi;
+    flash->cs = cs_num;
+    flash->presc = presc;
+
+    flash->nor.dev = qspi->dev;
+    spi_nor_set_flash_node(&flash->nor, np);
+    flash->nor.priv = flash;
+    mtd = &flash->nor.mtd;
+    mtd->priv = &flash->nor;
+
+    flash->nor.read = stm32_qspi_read;
+    flash->nor.write = stm32_qspi_write;
+    flash->nor.erase = stm32_qspi_erase;
+    flash->nor.read_reg = stm32_qspi_read_reg;
+    flash->nor.write_reg = stm32_qspi_write_reg;
+    flash->nor.prepare = stm32_qspi_prep;
+    flash->nor.unprepare = stm32_qspi_unprep;
+
+    writel_relaxed(LPTR_DFT_TIMEOUT, qspi->io_base + QUADSPI_LPTR);
+
+    writel_relaxed(CR_PRESC(presc) | CR_FTHRES(3) | CR_TCEN | CR_SSHIFT
+               | CR_EN, qspi->io_base + QUADSPI_CR);
+
+    /* a minimum fsize must be set to sent the command id */
+    flash->fsize = 25;

I don't understand why this is needed and the comment doesn't make
sense. Please fix.

fsize field defines the size of external memory.

What external memory ? Unclear

oops, fsize field defined the size of "flash memory" in stm32 qspi 
controller.
Number of bytes in Flash memory = 2 ^[FSIZE+1].
To sent a nor cmd this field must be set (hardware issue),
but before "spi_nor_scan" the size of flash nor is not know.
So I set a temporary value (workaround).

After "spi_nor_scan" the fsize is overwritten by the right value
flash->fsize = __fls(mtd->size) - 1;

quoted

Normaly, this field is used only for memory map mode,
but in fact is check in indirect mode.
So while flash scan "spi_nor_scan":
-I can't let 0.
-I not know yet the size of flash.
So I fix a temporary value

I will update my comment

Please do, also please consider that I'm reading the comment and I
barely have any clue about this hardware , so make sure I can understand it.

quoted

quoted

quoted

+    ret = spi_nor_scan(&flash->nor, NULL, flash_read);
+    if (ret) {
+        dev_err(qspi->dev, "device scan failed\n");
+        return ret;
+    }
+
+    flash->fsize = __fls(mtd->size) - 1;
+
+    writel_relaxed(DCR_CSHT(1), qspi->io_base + QUADSPI_DCR);
+
+    ret = mtd_device_register(mtd, NULL, 0);
+    if (ret) {
+        dev_err(qspi->dev, "mtd device parse failed\n");
+        return ret;
+    }
+
+    dev_dbg(qspi->dev, "read mm:%s cs:%d bus:%d\n",
+        qspi->read_mode == CCR_FMODE_MM ? "yes" : "no", cs_num, width);
+
+    return 0;
+}

[...]

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html