[U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

[Scott Wood]

On Fri, Oct 03, 2008 at 12:40:25PM +0200, dirk.behme at googlemail.com wrote:
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/mem.h>
> +#include <linux/mtd/nand_ecc.h>
> +
> +#if defined(CONFIG_CMD_NAND)
> +
> +#include <nand.h>

Move the #ifdef to the Makefile.

> +/*
> + * nand_read_buf16 - [DEFAULT] read chip data into buffer
> + * @mtd:	MTD device structure
> + * @buf:	buffer to store date
> + * @len:	number of bytes to read
> + *
> + * Default read function for 16bit buswith
> + */
> +static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
> +{
> +	int i;
> +	struct nand_chip *this = mtd->priv;
> +	u16 *p = (u16 *) buf;
> +	len >>= 1;
> +
> +	for (i = 0; i < len; i++)
> +		p[i] = readw(this->IO_ADDR_R);
> +}

How does this differ from the default implementation?

> +static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
> +				int len)
> +{
> +	int i;
> +	int j = 0;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	for (i = 0; i < len; i++) {
> +		writeb(buf[i], chip->IO_ADDR_W);
> +		for (j = 0; j < 10; j++) ;
> +	}
> +
> +}
> +
> +/*
> + * omap_nand_read_buf - read data from NAND controller into buffer
> + * @mtd:        MTD device structure
> + * @buf:        buffer to store date
> + * @len:        number of bytes to read
> + *
> + */
> +static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	int i;
> +	int j = 0;
> +	struct nand_chip *chip = mtd->priv;
> +
> +	for (i = 0; i < len; i++) {
> +		buf[i] = readb(chip->IO_ADDR_R);
> +		while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS) & GPMC_BUF_FULL));
> +	}
> +}

I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL]
when writing, but with 8-bit NAND, you check it when reading?  And 8-bit
writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads
have no delay at all?

> +static void omap_hwecc_init(struct nand_chip *chip)
> +{
> +	unsigned long val = 0x0;
> +
> +	/* Init ECC Control Register */
> +	/* Clear all ECC  | Enable Reg1 */
> +	val = ((0x00000001 << 8) | 0x00000001);
> +	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
> +	__raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
> +}

Why raw?

> +/*
> + * omap_correct_data - Compares the ecc read from nand spare area with
> + *                     ECC registers values
> + *			and corrects one bit error if it has occured
> + * @mtd:		 MTD device structure
> + * @dat:		 page data
> + * @read_ecc:		 ecc read from nand flash
> + * @calc_ecc: 		 ecc read from ECC registers
> + */
> +static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
> +			     u_char *read_ecc, u_char *calc_ecc)
> +{
> +	return 0;
> +}

This obviously is not correcting anything.  If the errors are corrected
automatically by the controller, say so in the comments.

> +/*
> + *  omap_calculate_ecc - Generate non-inverted ECC bytes.
> + *
> + *  Using noninverted ECC can be considered ugly since writing a blank
> + *  page ie. padding will clear the ECC bytes. This is no problem as
> + *  long nobody is trying to write data on the seemingly unused page.
> + *  Reading an erased page will produce an ECC mismatch between
> + *  generated and read ECC bytes that has to be dealt with separately.

Is this a hardware limitation?  If so, say so in the comment.  If not,
why do it this way?

> + *  @mtd:	MTD structure
> + *  @dat:	unused
> + *  @ecc_code:	ecc_code buffer
> + */
> +static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> +			      u_char *ecc_code)
> +{
> +	unsigned long val = 0x0;
> +	unsigned long reg;
> +
> +	/* Start Reading from HW ECC1_Result = 0x200 */
> +	reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
> +	val = __raw_readl(reg);
> +
> +	*ecc_code++ = ECC_P1_128_E(val);
> +	*ecc_code++ = ECC_P1_128_O(val);
> +	*ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val) << 4;

These macros are used nowhere else; why obscure what it's doing by moving
it to the top of the file?

> +static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG);
> +	unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
> +
> +	switch (mode) {
> +	case NAND_ECC_READ:
> +		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
> +		/* ECC col width | CS | ECC Enable */
> +		val = (dev_width << 7) | (cs << 1) | (0x1);
> +		break;
> +	case NAND_ECC_READSYN:
> +		__raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL);
> +		/* ECC col width | CS | ECC Enable */
> +		val = (dev_width << 7) | (cs << 1) | (0x1);
> +		break;
> +	case NAND_ECC_WRITE:
> +		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
> +		/* ECC col width | CS | ECC Enable */
> +		val = (dev_width << 7) | (cs << 1) | (0x1);
> +		break;
> +	default:
> +		printf("Error: Unrecognized Mode[%d]!\n", mode);
> +		break;
> +	}
> +
> +	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONFIG);
> +}

Is it OK if config gets written before control, or if this whole thing
gets done out of order with respect to other raw writes?

> +static struct nand_ecclayout hw_nand_oob_64 = {
> +	.eccbytes = 12,
> +	.eccpos = {
> +		   2, 3, 4, 5,
> +		   6, 7, 8, 9,
> +		   10, 11, 12, 13},
> +	.oobfree = { {20, 50} }	/* don't care */

Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?

> +	if (nand->options & NAND_BUSWIDTH_16) {
> +		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 2);
> +		if (nand->ecc.layout->eccbytes & 0x01)
> +			mtd->oobavail--;
> +	} else
> +		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 1);
> +}

oobavail is calculated by the generic NAND code.  You don't need to do it
here.

> +/*
> + * Board-specific NAND initialization. The following members of the
> + * argument are board-specific (per include/linux/mtd/nand_new.h):
> + * - IO_ADDR_R?: address to read the 8 I/O lines of the flash device
> + * - IO_ADDR_W?: address to write the 8 I/O lines of the flash device
> + * - hwcontrol: hardwarespecific function for accesing control-lines
> + * - dev_ready: hardwarespecific function for  accesing device ready/busy line
> + * - enable_hwecc?: function to enable (reset)  hardware ecc generator. Must
> + *   only be provided if a hardware ECC is available
> + * - eccmode: mode of ecc, see defines
> + * - chip_delay: chip dependent delay for transfering data from array to
> + *   read regs (tR)
> + * - options: various chip options. They can partly be set to inform
> + *   nand_scan about special functionality. See the defines for further
> + *   explanation
> + * Members with a "?" were not set in the merged testing-NAND branch,
> + * so they are not set here either.

IO_ADDR_R and IO_ADDR_W have a "?" but are set here.  If you have a
question about the API, ask it on the list, rather than encoding it in
the source.

> ===================================================================
> --- u-boot-arm.orig/drivers/mtd/nand/nand_base.c
> +++ u-boot-arm/drivers/mtd/nand/nand_base.c
> @@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
> +	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
> +void nand_switch_ecc(struct mtd_info *mtd)

NACK.  First, explain why you need to be able to dynamically switch ECC
modes.

Then, if it is justified, implement it in a separate patch, without all
the duplication of code, and without platform-specific #ifdefs.

-Scott