[U-Boot] [PATCH] mtd: add altera quadspi driver

[Chin Liang See <clsee@altera.com>]

On Thu, 2015-11-05 at 03:53 +0100, marex at denx.de wrote:
> On Thursday, November 05, 2015 at 03:49:18 AM, Chin Liang See wrote:
> > Hi Marek,
> > 
> > On Wed, 2015-11-04 at 10:18 +0000, marex at denx.de wrote:
> > > On Wednesday, November 04, 2015 at 04:56:10 PM, Chin Liang See wrote:
> > > > On Tue, 2015-11-03 at 21:22 +0800, thomas at wytron.com.tw wrote:
> > > > > Add Altera Generic Quad SPI Controller support. The controller
> > > > > converts SPI NOR flash to parallel flash interface. So it is
> > > > > not like other SPI flash, but rather like CFI flash.
> > > > > 
> > > > > Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> > > > > ---
> > > > > 
> > > > >  doc/device-tree-bindings/mtd/altera_qspi.txt |  35 +++
> > > > >  drivers/mtd/Kconfig                          |   9 +
> > > > >  drivers/mtd/Makefile                         |   1 +
> > > > >  drivers/mtd/altera_qspi.c                    | 312
> > > > >  +++++++++++++++++++++++++++ 4 files changed, 357 insertions(+)
> > > > >  create mode 100644 doc/device-tree-bindings/mtd/altera_qspi.txt
> > > > >  create mode 100644 drivers/mtd/altera_qspi.c
> > > > >  ...
> > > > > 
> > > > > diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
> > > > > new file mode 100644
> > > > > index 0000000..06bc53e
> > > > > --- /dev/null
> > > > > +++ b/drivers/mtd/altera_qspi.c
> > > > > @@ -0,0 +1,312 @@
> > > > > +/*
> > > > > + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw>
> > > > > + *
> > > > > + * SPDX-License-Identifier:	GPL-2.0+
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <dm.h>
> > > > > +#include <errno.h>
> > > > > +#include <fdt_support.h>
> > > > > +#include <flash.h>
> > > > > +#include <mtd.h>
> > > > > +#include <asm/io.h>
> > > > > +
> > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > +
> > > > > +/*
> > > > > + * The QUADSPI_MEM_OP register is used to do memory protect and
> > > > > erase operations + */
> > > > > +#define QUADSPI_MEM_OP_BULK_ERASE		0x00000001
> > > > > +#define QUADSPI_MEM_OP_SECTOR_ERASE		0x00000002
> > > > > +#define QUADSPI_MEM_OP_SECTOR_PROTECT		0x00000003
> > > > > +
> > > > > +/*
> > > > > + * The QUADSPI_ISR register is used to determine whether an invalid
> > > > > write or + * erase operation trigerred an interrupt
> > > > > + */
> > > > > +#define QUADSPI_ISR_ILLEGAL_ERASE		BIT(0)
> > > > > +#define QUADSPI_ISR_ILLEGAL_WRITE		BIT(1)
> > > > > +
> > > > > +struct altera_qspi_regs {
> > > > > +	u32	rd_status;
> > > > > +	u32	rd_sid;
> > > > > +	u32	rd_rdid;
> > > > > +	u32	mem_op;
> > > > > +	u32	isr;
> > > > > +	u32	imr;
> > > > > +	u32	chip_select;
> > > > > +};
> > > > > +
> > > > > +struct altera_qspi_platdata {
> > > > > +	struct altera_qspi_regs *regs;
> > > > > +	void *base;
> > > > > +	unsigned long size;
> > > > > +};
> > > > > +
> > > > > +flash_info_t flash_info[CONFIG_SYS_MAX_FLASH_BANKS];	/* FLASH chips
> > > 
> > > info
> > > 
> > > > > */ +
> > > > > +void flash_print_info(flash_info_t *info)
> > > > > +{
> > > > > +	printf("Altera QSPI flash  Size: %ld MB in %d Sectors\n",
> > > > > +	       info->size >> 20, info->sector_count);
> > > > > +}
> > > > > +
> > > > > +int flash_erase(flash_info_t *info, int s_first, int s_last)
> > > > > +{
> > > > > +	struct mtd_info *mtd = info->mtd;
> > > > > +	struct erase_info instr;
> > > > > +	int ret;
> > > > > +
> > > > > +	memset(&instr, 0, sizeof(instr));
> > > > > +	instr.addr = mtd->erasesize * s_first;
> > > > > +	instr.len = mtd->erasesize * (s_last + 1 - s_first);
> > > > > +	ret = mtd_erase(mtd, &instr);
> > > > > +	if (ret)
> > > > > +		return ERR_NOT_ERASED;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int write_buff(flash_info_t *info, uchar *src, ulong addr, ulong
> > > > > cnt) +{
> > > > > +	struct mtd_info *mtd = info->mtd;
> > > > > +	struct udevice *dev = mtd->dev;
> > > > > +	struct altera_qspi_platdata *pdata = dev_get_platdata(dev);
> > > > > +	ulong base = (ulong)pdata->base;
> > > > > +	loff_t to = addr - base;
> > > > > +	size_t retlen;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = mtd_write(mtd, to, cnt, &retlen, src);
> > > > > +	if (ret)
> > > > > +		return ERR_NOT_ERASED;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +unsigned long flash_init(void)
> > > > > +{
> > > > > +	struct udevice *dev;
> > > > > +
> > > > > +	/* probe every MTD device */
> > > > > +	for (uclass_first_device(UCLASS_MTD, &dev);
> > > > > +	     dev;
> > > > > +	     uclass_next_device(&dev)) {
> > > > > +	}
> > > > > +
> > > > > +	return flash_info[0].size;
> > > > > +}
> > > > > +
> > > > > +static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info
> > > > > *instr) +{
> > > > > +	struct udevice *dev = mtd->dev;
> > > > > +	struct altera_qspi_platdata *pdata = dev_get_platdata(dev);
> > > > > +	struct altera_qspi_regs *regs = pdata->regs;
> > > > > +	size_t addr = instr->addr;
> > > > > +	size_t len = instr->len;
> > > > > +	size_t end = addr + len;
> > > > > +	u32 sect;
> > > > > +	u32 stat;
> > > > > +
> > > > > +	instr->state = MTD_ERASING;
> > > > > +	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
> > > > > +	while (addr < end) {
> > > > > +		sect = addr / mtd->erasesize;
> > > > > +		sect <<= 8;
> > > > > +		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
> > > > > +		debug("erase %08x\n", sect);
> > > > > +		writel(sect, &regs->mem_op);
> > > > > +		stat = readl(&regs->isr);
> > > > > +		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
> > > > > +			/* erase failed, sector might be protected */
> > > > > +			debug("erase %08x fail %x\n", sect, stat);
> > > > > +			writel(stat, &regs->isr); /* clear isr */
> > > > > +			instr->state = MTD_ERASE_FAILED;
> > > > > +			return -EIO;
> > > > > +		}
> > > > > +		addr += mtd->erasesize;
> > > > > +	}
> > > > > +	instr->state = MTD_ERASE_DONE;
> > > > > +	mtd_erase_callback(instr);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int altera_qspi_read(struct mtd_info *mtd, loff_t from,
> > > > > size_t len, +			    size_t *retlen, u_char *buf)
> > > > > +{
> > > > > +	struct udevice *dev = mtd->dev;
> > > > > +	struct altera_qspi_platdata *pdata = dev_get_platdata(dev);
> > > > > +
> > > > > +	memcpy(buf, pdata->base + from, len);
> > > > > +	*retlen = len;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static inline u32 add_byte(u32 data, u8 byte, int shift)
> > > > > +{
> > > > > +	data &= ~(0xff << shift);
> > > > > +	data |= byte << shift;
> > > > > +	return data;
> > > > > +}
> > > > > +
> > > > > +static int altera_qspi_write_word(struct mtd_info *mtd, loff_t to,
> > > > > +				  u32 data)
> > > > > +{
> > > > > +	struct udevice *dev = mtd->dev;
> > > > > +	struct altera_qspi_platdata *pdata = dev_get_platdata(dev);
> > > > > +	struct altera_qspi_regs *regs = pdata->regs;
> > > > > +	u32 pos = (u32)to;
> > > > > +	u32 stat;
> > > > > +
> > > > > +	/* write to flash 32 bits at a time */
> > > > > +	writel(data, pdata->base + pos);
> > > > > +	/* check whether write triggered a illegal write interrupt */
> > > > > +	stat = readl(&regs->isr);
> > > > > +	if (stat & QUADSPI_ISR_ILLEGAL_WRITE) {
> > > > > +		/* write failed, sector might be protected */
> > > > > +		debug("write %08x fail %x\n", pos, stat);
> > > > > +		writel(stat, &regs->isr); /* clear isr */
> > > > > +		return -EIO;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int altera_qspi_write(struct mtd_info *mtd, loff_t to, size_t
> > > > > len, +			     size_t *retlen, const u_char *buf)
> > > > > +{
> > > > > +	const u_char *end = buf + len;
> > > > > +	unsigned shift;
> > > > > +	u32 data;
> > > > > +	int ret;
> > > > > +
> > > > > +	shift = (to & (sizeof(u32) - 1)) * 8; /* first shift to add byte 
> */
> > > > > +	to &= ~(sizeof(u32) - 1); /* get lower aligned address */
> > > > > +	while (buf < end) {
> > > > > +		data = 0xffffffff; /* pad data */
> > > > > +		while (buf < end && shift < 32) {
> > > > > +			/* add byte from buf */
> > > > > +			data = add_byte(data, *buf++, shift);
> > > > > +			shift += 8;
> > > > > +		}
> > > > > +		ret = altera_qspi_write_word(mtd, to, data);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +		to += sizeof(u32);
> > > > > +		shift = 0;
> > > > > +	}
> > > > > +	*retlen = len;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > 
> > > > Hi Thomas,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > I notice you are writing in word style which might have concern in
> > > > performance. As the burst count can go up to 64, we can write larger
> > > > data through memcpy. This will avoid redundancy of data header (opcode
> > > > + address + dummy).
> > > 
> > > You cannot do that, memcpy works on memory while write*() operators work
> > > on I/O. You should use readsl() and friends then.
> > 
> > Actually I am thinking to take advantage the cache fill and dump. But
> > after rethinking, this might limit some of the use case as we want the
> > driver to support NIOS II without cache. With that, just ignore this
> > comment for now.
> 
> I'm not sure I want to ask for details here. I think we're reading data from
> some sort of IO device, so we should just use readl() or readsl() to read
> them out (and write*() for the other direction). I don't think cache operations
> can be involved in any way. Correct me if I'm wrong please.
> 

Sure, I can share more. Since the read can support up to burst of 64
byte, we can use the a cache fill method which eventually trigger a read
of 32 byte (which is size of a cache line) to the Quad SPI controller.
To ensure we don't read from old data, we need to invalidate that cache
line (through address). By doing this, we can gain better performance as
we are reading 32 bytes of data instead 4 per transaction.

> > But your comment lead to the fact that the read part is now using
> > memcpy. Thomas needs to fix that to use the readl :)
> 
> Uhm, I don't think I understand this remark, sorry. I never suggested to use
> memcpy() in this entire thread, did I ?


Nope, but this trigger me that we need to do the same for read. The
memcpy might lead to the driver reading old data that stay on cache
instead from controller. Another way to get rid of this is invalidate
the cache.

Thanks
Chin Liang