[U-Boot] [PATCH v2 3/4] mtd: nand: add Freescale NFC driver

[Bill Pringlemeir]

> On 14 Aug 2014, stefan at agner.ch wrote:
> 
> This adds initial support for Freescale NFC (NAND Flash Controller)
> found in ARM Vybrid SoC's, Power Architecture MPC5125 and others.
> However, this driver is only tested on Vybrid.

This is only to expand on the nand controller register and SRAM use.

[snip]
 
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> new file mode 100644
> index 0000000..3150ac1
> --- /dev/null
> +++ b/drivers/mtd/nand/vf610_nfc.c

[snip]

> +static inline u32 vf610_nfc_read(struct mtd_info *mtd, uint reg)
> +{
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +
> +	return readl(nfc->regs + reg);
> +}
> +
> +static inline void vf610_nfc_write(struct mtd_info *mtd, uint reg, u32 val)
> +{
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +
> +	writel(val, nfc->regs + reg);
> +}

Ok, we always use readl/writel.  This is fine, but a little slower and
bigger.  I may try a register cache if I resubmit to the Linux MTD as
per Scott's suggestion.  Especially, this version is good for an
incremental patch.

I think these are in 'arch/arm/include/asm/io.h' of U-Boot.

#define dmb()           __asm__ __volatile__ ("" : : : "memory")
#define __iormb()       dmb()
#define __iowmb()       dmb()

#define readl(c)        ({ u32 __v = __arch_getl(c); __iormb(); __v; })
#define writel(v,c)     ({ u32 __v = v; __iowmb(); __arch_putl(__v,c); __v; })

Currently, these look like compiler barriers to me.  Fine so far.

> +
> +static inline void vf610_nfc_set(struct mtd_info *mtd, uint reg, u32 bits)
> +{
> +	vf610_nfc_write(mtd, reg, vf610_nfc_read(mtd, reg) | bits);
> +}
> +
> +static inline void vf610_nfc_clear(struct mtd_info *mtd, uint reg, u32 bits)
> +{
> +	vf610_nfc_write(mtd, reg, vf610_nfc_read(mtd, reg) & ~bits);
> +}
> +
> +static inline void vf610_nfc_set_field(struct mtd_info *mtd, u32 reg,
> +				       u32 mask, u32 shift, u32 val)
> +{
> +	vf610_nfc_write(mtd, reg,
> +			(vf610_nfc_read(mtd, reg) & (~mask)) | val << shift);
> +}
> +
> +/* Clear flags for upcoming command */
> +static inline void vf610_nfc_clear_status(struct mtd_info *mtd)
> +{
> +	u32 tmp = vf610_nfc_read(mtd, NFC_IRQ_STATUS);
> +	tmp |= CMD_DONE_CLEAR_BIT | IDLE_CLEAR_BIT;
> +	vf610_nfc_write(mtd, NFC_IRQ_STATUS, tmp);
> +}
> +
> +/* Wait for complete operation */
> +static inline void vf610_nfc_done(struct mtd_info *mtd)
> +{
> +	uint start;
> +
> +	vf610_nfc_set(mtd, NFC_FLASH_CMD2, START_BIT);
> +	barrier();

This barrier() is not needed then.  The  vf610_nfc_set() should have
done it twice already, plus everything is volatile.

[snip]

> +static inline void vf610_nfc_read_spare(struct mtd_info *mtd, void *buf,
> +					int len)
> +{
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +
> +	len = min(mtd->oobsize, (uint)len);
> +	if (len > 0)
> +		memcpy(buf, nfc->regs + mtd->writesize, len);

Notice the 'memcpy(.. nfc->regs);'...

> +}
> +
> +/* Read data from NFC buffers */
> +static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
> +{
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	uint c = nfc->column;
> +	uint l;
> +
> +	/* Handle main area */
> +	if (!nfc->spareonly) {
> +
> +		l = min((uint)len, mtd->writesize - c);
> +		nfc->column += l;
> +
> +		if (!nfc->alt_buf)
> +			memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, l);

Another 'memcpy(.. nfc->regs);'...

> +		else
> +			if (nfc->alt_buf & ALT_BUF_ID)
> +				*buf = vf610_nfc_get_id(mtd, c);
> +			else
> +				*buf = vf610_nfc_get_status(mtd);
> +
> +		buf += l;
> +		len -= l;
> +	}
> +
> +	/* Handle spare area access */
> +	if (len) {
> +		nfc->column += len;
> +		vf610_nfc_read_spare(mtd, buf, len);
> +	}
> +}
> +
> +/* Write data to NFC buffers */
> +static void vf610_nfc_write_buf(struct mtd_info *mtd, const u_char *buf,
> +				int len)
> +{
> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	uint c = nfc->column;
> +	uint l;
> +
> +	l = min((uint)len, mtd->writesize + mtd->oobsize - c);
> +	nfc->column += l;
> +	memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);

Another 'memcpy(.. nfc->regs);'...

[snip]

These memcpy's are the same 'bus' interface as the registers.  We should
be just as worried about this SRAM buffer memory as the memory mapped
registers, shouldn't we?  Is a barrier() before reading and a barrier()
after writing fine for U-Boot?  Personally, I think they are safe as
only the 'vf610_nfc_set(mtd, NFC_FLASH_CMD2, START_BIT)' needs some
care.  Maybe a comment is fine?  It seems the Vybrid is safe for
different access sizes, but it is possible that some other CPU might not
be able to access this memory via 32/16/8 bit accesses and 'memcpy()'
may not be appropriate.  It seems that 'natural' size of the NFC
controller itself is 32bits and the CPU interface does lane masking.
Ie, boot mode documentation talks about remapping
'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying
that bits 2,1 are not used (hopefully one based numbers).  This is just
my guess...

The VF6xx page has a documentation tab,
 http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=VF6xx

There is an app note, AN4947 'Understanding Vybrid Architecture', which
describes some timing details for the AHB bus (where this flash
controller is connected).  Pg21 Table 7 of that document gives some
measurements.  The QSPI is a similar peripheral on the AHB.  The first
and second lines give accesses of 4408 and subsequent accesses are 2770
Cortex-A5 clocks.  Normal SDRAM is 258 and 8 clocks.  Ie, it is quite
important in places to minimize accesses and try to make them
sequential.

However, it looks like most U-Boot NAND drivers use the memcpy()?   With
the exceptions of fsl_elbc_nand.c, fsl_ifc_nand.c, and mpc5121_nfc.c.
Maybe it doesn't matter...

Fwiw,
Bill Pringlemeir.