On Mon, Aug 18, 2008 at 11:30:44AM +0200, Magnus Lilja wrote:
> +/* The bool type is used locally in this file, added for U-boot. */
> +typedef enum {false = 0, true = 1 } bool;
Please remove this.
> +struct nand_info {
> + bool bSpareOnly;
> + bool bStatusRequest;
No Hungarian notation. noJavaCaps.
> +static struct nand_ecclayout nand_hw_eccoob_2k = {
> + .eccbytes = 20,
> + .eccpos = {6, 7, 8, 9, 10, 22, 23, 24, 25, 26,
> + 38, 39, 40, 41, 42, 54, 55, 56, 57, 58},
> + .oobfree = {
> + {.offset = 0,
> + .length = 5},
Bytes 0 and 1 are not free (they're the bad block marker). Byte 5 *is*
free.
> + {.offset = 11,
> + .length = 10},
Length should be 11.
> +/* Define some generic bad / good block scan pattern which are used
> + * while scanning a device for factory marked good / bad blocks. */
> +static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
> +
> +static struct nand_bbt_descr smallpage_memorybased = {
> + .options = NAND_BBT_SCAN2NDPAGE,
> + .offs = 5,
> + .len = 1,
> + .pattern = scan_ff_pattern
> +};
> +
> +static struct nand_bbt_descr largepage_memorybased = {
> + .options = 0,
> + .offs = 0,
> + .len = 2,
> + .pattern = scan_ff_pattern
> +};
> +
> +/* Generic flash bbt decriptors */
> +static uint8_t bbt_pattern[] = { 'B', 'b', 't', '0' };
> +static uint8_t mirror_pattern[] = { '1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> + .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
> + | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> + .offs = 0,
> + .len = 4,
> + .veroffs = 4,
> + .maxblocks = 4,
> + .pattern = bbt_pattern
> +};
> +
> +static struct nand_bbt_descr bbt_mirror_descr = {
> + .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
> + | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> + .offs = 0,
> + .len = 4,
> + .veroffs = 4,
> + .maxblocks = 4,
> + .pattern = mirror_pattern
> +};
Is there any reason the default layout can't be used?
> +/**
> + * This function will maintains state of single bit Error
> + * in Main & spare area
> + *
> + * @param buf_id Specify Internal RAM Buffer number (0-3)
> + * @param spare set to true if only spare area needs correction
> + */
> +static void mxc_nd_correct_ecc(u8 buf_id, bool spare)
> +{
> +#ifdef CONFIG_MTD_NAND_MXC_ECC_CORRECTION_OPTION2
> + /* To maintain single bit error in previous page */
> + static int lastErrMain, lastErrSpare;
> +#endif
> + u16 value, ecc_status;
> +
> + /* Read the ECC result */
> + ecc_status = NFC_ECC_STATUS_RESULT;
> + MTDDEBUG(MTD_DEBUG_LEVEL3,
> + "mxc_nd_correct_ecc (Ecc status=%x)\n", ecc_status);
> +
> +#ifdef CONFIG_MTD_NAND_MXC_ECC_CORRECTION_OPTION2
What is this ifdef for? Please document.
> +/**
> + * This function requests the NANDFC to perform a read of the
> + * NAND device status and returns the current status.
> + *
> + * @return device status
> + */
> +static u16 get_dev_status(void)
> +{
> + volatile u16 *mainBuf = MAIN_AREA1;
> + u32 store;
> + u16 ret;
> + /* Issue status request to NAND device */
> +
> + /* store the main area1 first word, later do recovery */
> + store = *((u32 *) mainBuf);
> + /*
> + * NANDFC buffer 1 is used for device status to prevent
> + * corruption of read/write buffer on status requests.
> + */
> + NFC_BUF_ADDR = 1;
> +
> + /* Read status into main buffer */
> + NFC_CONFIG1 &= ~NFC_SP_EN;
> + NFC_CONFIG2 = NFC_STATUS;
> +
> + /* Wait for operation to complete */
> + wait_op_done(TROP_US_DELAY, 0, true);
> +
> + /* Status is placed in first word of main buffer */
> + /* get status, then recovery area 1 data */
> + ret = mainBuf[0];
> + *((u32 *) mainBuf) = store;
This cast violates C strict aliasing rules. Use a union if you
absolutely must use a 32-bit access here.
> +static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> + u_char *ecc_code)
> +{
> + /*
> + * Just return success. HW ECC does not read/write the NFC spare
> + * buffer. Only the FLASH spare area contains the calcuated ECC.
> + */
> + return 0;
> +}
Hmm, maybe you should implement write_page() instead.
You may want to do a read-back after write to fill in oob_poi.
> +/**
> + * This function reads byte from the NAND Flash
> + *
> + * @param mtd MTD structure for the NAND Flash
> + *
> + * @return data read from the NAND Flash
> + */
> +static u_char mxc_nand_read_byte(struct mtd_info *mtd)
> +{
> + u_char retVal = 0;
> + u16 col, rdWord;
> + volatile u16 *mainBuf = MAIN_AREA0;
> + volatile u16 *spareBuf = SPARE_AREA0;
> +
> + /* Check for status request */
> + if (g_nandfc_info.bStatusRequest)
> + return get_dev_status() & 0xFF;
> +
> + /* Get column for 16-bit access */
> + col = g_nandfc_info.colAddr >> 1;
> +
> + /* If we are accessing the spare region */
> + if (g_nandfc_info.bSpareOnly)
> + rdWord = spareBuf[col];
> + else
> + rdWord = mainBuf[col];
I thought you could only do 32-bit accesses?
> +/**
> + * This function writes data of length \b len to buffer \b buf. The data
> + * to be written on NAND Flash is first copied to RAMbuffer. After the
> + * Data Input Operation by the NFC, the data is written to NAND Flash.
> + *
> + * @param mtd MTD structure for the NAND Flash
> + * @param buf data to be written to NAND Flash
> + * @param len number of bytes to be written
> + */
> +static void mxc_nand_write_buf(struct mtd_info *mtd,
> + const u_char *buf, int len)
> +{
> + int n;
> + int col;
> + int i = 0;
> +
> + MTDDEBUG(MTD_DEBUG_LEVEL3,
> + "mxc_nand_write_buf(col = %d, len = %d)\n",
> + g_nandfc_info.colAddr, len);
> +
> + col = g_nandfc_info.colAddr;
> +
> + /* Adjust saved column address */
> + if (col < mtd->writesize && g_nandfc_info.bSpareOnly)
> + col += mtd->writesize;
> +
> + n = mtd->writesize + mtd->oobsize - col;
> + n = min(len, n);
If len exceeds mtd->writesize + mtd->oobsize - col, then print an error,
don't silently clip it.
> + MTDDEBUG(MTD_DEBUG_LEVEL3,
> + "%s:%d: col = %d, n = %d\n", __FUNCTION__, __LINE__, col, n);
> +
> + while (n) {
> + volatile u32 *p;
> + if (col < mtd->writesize)
> + p = (volatile u32 *)((ulong) (MAIN_AREA0) + (col & ~3));
> + else
> + p = (volatile u32 *)((ulong) (SPARE_AREA0) -
> + mtd->writesize + (col & ~3));
> +
> + MTDDEBUG(MTD_DEBUG_LEVEL3, "%s:%d: p = %p\n",
> + __FUNCTION__, __LINE__, p);
> +
> + if (((col | (int)&buf[i]) & 3) || n < 16) {
Don't cast pointers to "int"; use uintptr_t if you must cast to an
integer type.
> + u32 data = 0;
> +
> + if (col & 3 || n < 4)
> + data = *p;
> +
> + switch (col & 3) {
> + case 0:
> + if (n) {
> + data = (data & 0xffffff00) |
> + (buf[i++] << 0);
> + n--;
> + col++;
> + }
If this controller really insists on 32-bit accesses to the buffer
(yuck), and the requested access isn't aligned, then memcpy_32 the hw
buffer to a sw buffer, and do an ordinary memcpy from that to the
requested location.
For full page accesses, the buffer should be aligned, so the only likely
double-copy is for small OOB accesses, where the double copy doesn't
matter much.
Likewise in read_buf().
> +/**
> + * This function is used by the upper layer to verify the data in NAND Flash
> + * with the data in the \b buf.
> + *
> + * @param mtd MTD structure for the NAND Flash
> + * @param buf data to be verified
> + * @param len length of the data to be verified
> + *
> + * @return -EFAULT if error else 0
> + */
> +static int
> +mxc_nand_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
> + return -1; /* Was -EFAULT */
> +}
Is there any particular reason you don't implement this?
> +/**
> + * This function is used by upper layer for select and deselect of the NAND
> + * chip.
> + *
> + * @param mtd MTD structure for the NAND Flash
> + * @param chip val indicating select or deselect
> + */
> +static void mxc_nand_select_chip(struct mtd_info *mtd, int chip)
> +{
> +#ifdef CONFIG_MTD_NAND_MXC_FORCE_CE
What does this option do, and why is it an option?
> +/**
> + * This function is used by the upper layer to write command to NAND Flash
> + * for different operations to be carried out on NAND Flash
> + *
> + * @param mtd MTD structure for the NAND Flash
> + * @param command command for NAND Flash
> + * @param column column offset for the page read
> + * @param page_addr page to be read from NAND Flash
> + */
> +static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
> + int column, int page_addr)
> +{
> + bool useirq = false;
This is never set to anything but false.
> + case NAND_CMD_SEQIN:
> + if (column >= mtd->writesize) {
> + if (is2k_Pagesize) {
> + /*
> + * FIXME: before send SEQIN command for
> + * write OOB, we must read one page out.
> + * For K9F1GXX has no READ1 command to set
> + * current HW pointer to spare area, we must
> + * write the whole page including OOB
> + * together.
NACK. Large page devices have a 2-byte column address; use that with
READ0 to select the OOB.
> + /*
> + * Write out column address, if necessary
> + */
> + if (column != -1) {
> + /*
> + * MXC NANDFC can only perform full page+spare or
> + * spare-only read/write. When the upper layers
> + * layers perform a read/write buf operation,
> + * we will used the saved column adress to index into
> + * the full page.
> + */
> + send_addr(0, page_addr == -1);
> + if (is2k_Pagesize)
> + /* another col addr cycle for 2k page */
> + send_addr(0, false);
In the spare-only case, the second address byte should be
"mtd->writesize >> 8" (or ">> 9" for 16-bit devices).
> +#ifdef CONFIG_MXC_NAND_LOW_LEVEL_ERASE
> +static void mxc_low_erase(struct mtd_info *mtd)
> +{
> + struct nand_chip *this = mtd->priv;
> + unsigned int page_addr, addr;
> + u_char status;
> +
> + MTDDEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : mxc_low_erase:Erasing NAND\n");
> + for (addr = 0; addr < this->chipsize; addr += mtd->erasesize) {
> + page_addr = addr / mtd->writesize;
> + mxc_nand_command(mtd, NAND_CMD_ERASE1, -1, page_addr);
> + mxc_nand_command(mtd, NAND_CMD_ERASE2, -1, -1);
> + mxc_nand_command(mtd, NAND_CMD_STATUS, -1, -1);
> + status = mxc_nand_read_byte(mtd);
> + if (status & NAND_STATUS_FAIL) {
> + printk(KERN_ERR
> + "ERASE FAILED(block = %d,status = 0x%x)\n",
> + addr / mtd->erasesize, status);
> + }
> + }
> +
> +}
> +#endif
What is this for? Where is it called?
> + memset((char *)&g_nandfc_info, 0, sizeof(g_nandfc_info));
Unnecessary cast -- and unnecessary memset.
> + this = nand;
Why not just refer to it as "nand", or rename the parameter "this"?
> + mtd = &mxc_nand_data->mtd;
> + mtd->priv = this;
> + this->priv = mxc_nand_data;
> +
> + /* 50 us command delay time */
> + this->chip_delay = 5;
I don't think you need chip_delay if you implement cmdfunc and dev_ready.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
