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));elsep = (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