[U-Boot] [PATCH 08/13 v3] ARM: OMAP3: Add NAND support

[Scott Wood <scottwood@freescale.com>]

dirk.behme at googlemail.com wrote:
> +unsigned char cs;
> +volatile unsigned long gpmc_cs_base_add;

Make these static.  gpmc_cs_base_add should be a pointer, not "unsigned 
long".  Volatile isn't needed since you use I/O accessors, and 
definitely isn't needed on the address itself.

> +/*
> + * omap_nand_hwcontrol - Set the address pointers corretly for the
> + *			following address/data/command operation
> + */
> +static void omap_nand_hwcontrol(struct mtd_info *mtd, int cmd,
> +				unsigned int ctrl)
> +{
> +	register struct nand_chip *this = mtd->priv;
> +
> +	/* Point the IO_ADDR to DATA and ADDRESS registers instead
> +	   of chip address */
> +	switch (ctrl) {
> +	case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> +		this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_CMD;
> +		this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> +		this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_ADR;
> +		this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
> +		break;
> +	case NAND_CTRL_CHANGE | NAND_NCE:
> +		this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
> +		this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
> +		break;
> +	}

IO_ADDR_R never seems to change; you can leave it out of here and 
omap_nand_wait.

> +/*
> + * omap_nand_wait - called primarily after a program/erase operation
> + *			so that we access NAND again only after the device
> + *			is ready again.
> + * @mtd:        MTD device structure
> + * @chip:	nand_chip structure
> + */
> +static int omap_nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> +{
> +	register struct nand_chip *this = mtd->priv;
> +	int status = 0;
> +
> +	this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_CMD;
> +	this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
> +	/* Send the status command and loop until the device is free */
> +	while (!(status & 0x40)) {
> +		writeb(NAND_CMD_STATUS & 0xFF, this->IO_ADDR_W);
> +		status = readb(this->IO_ADDR_R);
> +	}

Maybe should just do this, to avoid changing client-visible state:
writeb(NAND_CMD_STATUS, &gpmc_cs_base_add[GPMC_NAND_CMD]);

No need for the "& 0xFF".

> +	/* Init ECC Control Register */
> +	/* Clear all ECC  | Enable Reg1 */
> +	val = ((0x00000001 << 8) | 0x00000001);
> +	writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
> +	writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);

Symbolic constants for the bit values would be nice.

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

Where is it dealt with separately?

> +	unsigned long val = 0x0;

Unnecessary initialization.

> +	unsigned long reg;
> +
> +	/* Start Reading from HW ECC1_Result = 0x200 */
> +	reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
> +	val = readl(reg);

readl() takes a pointer.  ARM gets away without a warning here because 
it uses macros rather than inline functions, but it's bad practice.

> +	/* Stop reading anymore ECC vals and clear old results
> +	 * enable will be called if more reads are required */
> +	reg = (unsigned long) (GPMC_BASE + GPMC_ECC_CONFIG);
> +	writel(0x000, reg);

Likewise.

> +void omap_nand_switch_ecc(int hardware)
> +{
> +	struct nand_chip *nand;
> +
> +	if (nand_curr_device < 0 ||
> +	    nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
> +	    !nand_info[nand_curr_device].name) {
> +		printf("Error: Can't switch ecc, no devices available\n");
> +		return;
> +	}
> +
> +	nand = (&nand_info[nand_curr_device])->priv;
> +
> +	if (!hardware) {
> +		nand->ecc.mode = NAND_ECC_SOFT;
> +		nand->ecc.layout = &sw_nand_oob_64;
> +		nand->ecc.size = 256;	/* set default eccsize */
> +		nand->ecc.bytes = 3;
> +		nand->ecc.steps = 8;
> +		nand->ecc.hwctl = 0;
> +		nand->ecc.calculate = nand_calculate_ecc;
> +		nand->ecc.correct = nand_correct_data;
> +	} else {
> +		nand->ecc.mode = NAND_ECC_HW;
> +		nand->ecc.layout = &hw_nand_oob_64;
> +		nand->ecc.size = 512;
> +		nand->ecc.bytes = 3;
> +		nand->ecc.steps = 4;
> +		nand->ecc.hwctl = omap_enable_hwecc;
> +		nand->ecc.correct = omap_correct_data;
> +		nand->ecc.calculate = omap_calculate_ecc;
> +		omap_hwecc_init(nand);
> +	}

Do you need to do anything similar to omap_hwecc_init() when switching 
to SW ECC to tell the hardware to stop doing ECC?

-Scott