Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755385AbdC2Ngx (ORCPT ); Wed, 29 Mar 2017 09:36:53 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:14234 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754438AbdC2Ngv (ORCPT ); Wed, 29 Mar 2017 09:36:51 -0400 Subject: Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller To: Marek Vasut , Cyrille Pitchen References: <1490619296-8168-1-git-send-email-ludovic.Barre@st.com> <1490619296-8168-3-git-send-email-ludovic.Barre@st.com> <1be39452-83b0-5c32-39fe-d6dd5134d1ef@gmail.com> <2c364b99-512c-8eb6-7044-7989ba21d53b@st.com> <28454969-0f56-7752-b087-5e02a1a20c23@gmail.com> CC: David Woodhouse , Brian Norris , Boris Brezillon , Richard Weinberger , Alexandre Torgue , Rob Herring , , , From: Ludovic BARRE Message-ID: <2410fcde-1d1d-57b5-01ee-bd6bc3bc863b@st.com> Date: Wed, 29 Mar 2017 15:35:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <28454969-0f56-7752-b087-5e02a1a20c23@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.50] X-ClientProxiedBy: SFHDAG1NODE2.st.com (10.75.127.2) To SFHDAG6NODE1.st.com (10.75.127.16) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-29_10:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10948 Lines: 329 On 03/29/2017 03:09 PM, Marek Vasut wrote: > On 03/29/2017 02:24 PM, Ludovic BARRE wrote: >> hi Marek > Hi! > >> thanks for review >> comment below >> >> On 03/29/2017 12:54 PM, Marek Vasut wrote: >>> On 03/27/2017 02:54 PM, Ludovic Barre wrote: >>>> From: Ludovic Barre >>>> >>>> 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 >>> 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 ... > >>> Mostly minor nits below ... >>> >>>> --- >>>> 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 >>> [...] >>> >>>> +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 . > >>>> + 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. > >>>> + 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 > >>>> + if (ret) { >>>> + dev_err(qspi->dev, "fifo timeout (stat:%#x)\n", sr); >>>> + break; >>>> + } >>>> + tx_fifo(buf++, qspi->io_base + QUADSPI_DR); >>>> + } >>>> + >>>> + return ret; >>>> +} >>> [...] >>> >>>> +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 >>>> + dev_err(qspi->dev, "%s abort err:%d\n", __func__, err); >>>> + return err; >>>> +} >>> [...] >>> >>>> +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; >> 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. > >>>> + 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; >>>> +} >>> [...] >>> >