On Fri, Oct 03, 2008 at 12:40:25PM +0200, [EMAIL PROTECTED] 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot