Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611819 > unrolled thread

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

Started byMarek Vasut <marek.vasut@gmail.com>
First post2017-03-29 13:00 +0200
Last post2017-03-30 12:20 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Marek Vasut <marek.vasut@gmail.com> - 2017-03-29 13:00 +0200
    Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Ludovic BARRE <ludovic.barre@st.com> - 2017-03-29 14:30 +0200
      Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Marek Vasut <marek.vasut@gmail.com> - 2017-03-29 15:20 +0200
        Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Ludovic BARRE <ludovic.barre@st.com> - 2017-03-29 15:40 +0200
          Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Marek Vasut <marek.vasut@gmail.com> - 2017-03-29 16:00 +0200
            Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Ludovic BARRE <ludovic.barre@st.com> - 2017-03-29 18:50 +0200
              Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash  controller Marek Vasut <marek.vasut@gmail.com> - 2017-03-30 12:20 +0200

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

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-03-29 13:00 +0200
SubjectRe: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller
Message-ID<tqjux-5Ii-11@gated-at.bofh.it>
On 03/27/2017 02:54 PM, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
> 
> 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 <ludovic.barre@st.com>

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

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 ?

> +	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 ?

> +	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.

> +		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.

> +	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.

> +	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;
> +}

[...]

-- 
Best regards,
Marek Vasut

[toc] | [next] | [standalone]

#1611885

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-03-29 14:30 +0200
Message-ID<tqkTD-6On-1@gated-at.bofh.it>
In reply to#1611819
hi Marek

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 <ludovic.barre@st.com>
>>
>> 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 <ludovic.barre@st.com>
> 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
>
> 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
>
>> +	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.
>> +	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
>
>> +		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.
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
>
>> +	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;
>> +}
> [...]
>

[toc] | [prev] | [next] | [standalone]

#1611923

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-03-29 15:20 +0200
Message-ID<tqlG1-7mo-5@gated-at.bofh.it>
In reply to#1611885
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 <ludovic.barre@st.com>
>>>
>>> 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 <ludovic.barre@st.com>
>> 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

> 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;
>>> +}
>> [...]
>>
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1611960

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-03-29 15:40 +0200
Message-ID<tqlZo-7yI-23@gated-at.bofh.it>
In reply to#1611923
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 <ludovic.barre@st.com>
>>>>
>>>> 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 <ludovic.barre@st.com>
>>> 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;
>>>> +}
>>> [...]
>>>
>

[toc] | [prev] | [next] | [standalone]

#1611970

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-03-29 16:00 +0200
Message-ID<tqmiL-7Hz-31@gated-at.bofh.it>
In reply to#1611960
On 03/29/2017 03:35 PM, Ludovic BARRE wrote:

[...]

>>>>> +    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.

Errr, now I am totally lost :) Is that some internal SPI NOR ? Shouldn't
the size be coming from DT or something ?

> 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).

Is it needed before the scan ?

> 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;
>>>>> +}
>>>> [...]
>>>>
>>
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1612151

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-03-29 18:50 +0200
Message-ID<tqoXf-188-13@gated-at.bofh.it>
In reply to#1611970

On 03/29/2017 03:57 PM, Marek Vasut wrote:
> On 03/29/2017 03:35 PM, Ludovic BARRE wrote:
>
> [...]
>
>>>>>> +    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.
> Errr, now I am totally lost :) Is that some internal SPI NOR ? Shouldn't
> the size be coming from DT or something ?
>
>> 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).
> Is it needed before the scan ?
yes it's needed before scan (fix a "stm32 qspi controller" issue)

sorry, I try to reformulate:

The nor flash (external component like micron n25q128a13
or spansion s25fl512s ...) is connected to stm32 by classic
spi-nor interface cs, clock and 1/2/4 IO lines.

the stm32 microprocessor has a dedicated controller to
manage spi-nor interface, it's stm32 qspi.

In stm32 qspi controller there is a register with fsize field
which define the size of nor flash (n25q128a13 or s25fl512s...).

fsize can't be null, else the stm32 qspi controller doesn't send
spi-nor command. it's "stm32 qspi controller" issue.

Before the "spi_nor_scan" the size of nor flash (n25q128a13
or s25fl512s...) is not know. So we set a temporary value just
to discover the nor flash with "spi_nor_scan". After we can
set the right value (mtd->size) in fsize.
>> 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;
>>>>>> +}
>>>>> [...]
>>>>>
>

[toc] | [prev] | [next] | [standalone]

#1612875

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-03-30 12:20 +0200
Message-ID<tqFlp-4ME-47@gated-at.bofh.it>
In reply to#1612151
On 03/29/2017 06:38 PM, Ludovic BARRE wrote:
> 
> 
> On 03/29/2017 03:57 PM, Marek Vasut wrote:
>> On 03/29/2017 03:35 PM, Ludovic BARRE wrote:
>>
>> [...]
>>
>>>>>>> +    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.
>> Errr, now I am totally lost :) Is that some internal SPI NOR ? Shouldn't
>> the size be coming from DT or something ?
>>
>>> 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).
>> Is it needed before the scan ?
> yes it's needed before scan (fix a "stm32 qspi controller" issue)
> 
> sorry, I try to reformulate:
> 
> The nor flash (external component like micron n25q128a13
> or spansion s25fl512s ...) is connected to stm32 by classic
> spi-nor interface cs, clock and 1/2/4 IO lines.
> 
> the stm32 microprocessor has a dedicated controller to
> manage spi-nor interface, it's stm32 qspi.
> 
> In stm32 qspi controller there is a register with fsize field
> which define the size of nor flash (n25q128a13 or s25fl512s...).
> 
> fsize can't be null, else the stm32 qspi controller doesn't send
> spi-nor command. it's "stm32 qspi controller" issue.
> 
> Before the "spi_nor_scan" the size of nor flash (n25q128a13
> or s25fl512s...) is not know. So we set a temporary value just
> to discover the nor flash with "spi_nor_scan". After we can
> set the right value (mtd->size) in fsize.

I see, now it makes sense. Such a beefy comment would be nice :)

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web