Hi Scott,
please review the updated patch (will be posted as a follow-up).
Scott Wood wrote:
Please look at drivers/mtd/nand/mpc5121_nfc.c, which AFAICT is very similar hardware, and see if anything can be factored out into common code, and try to keep the rest looking the same except where the hardware actually differs.
Hmm... For now we just can't spend enough effort for this...
+static struct nand_ecclayout nand_hw_eccoob_16 = {
- .eccbytes = 5,
- .eccpos = {6, 7, 8, 9, 10},
- .oobfree = {{0, 6}, {12, 4}, }
+};
This implies the bad block marker is one byte at offset 11 (it's all that's left), but I don't see any override of the bad block pattern.
This is surely a bug. Fixed.
+static void *mxc_nand_memcpy32(void *dest, void *source, size_t size) +{
- uint32_t *s = source, *d = dest;
- size >>= 2;
- while (size--)
*d++ = *s++;- return dest;
+}
This should probably be using I/O accessors (possibly raw) -- and should take uint32_t *, not void *.
Fixed.
+/*
- This function requests the NANDFC to perform a read of the
- NAND device status and returns the current status.
- */
+static uint16_t get_dev_status(struct mxc_nand_host *host) +{
- void __iomem *main_buf = host->regs->main_area1;
- uint32_t store;
- uint16_t ret, tmp;
- /* Issue status request to NAND device */
- /* store the main area1 first word, later do recovery */
- store = readl(main_buf);
- /*
* NANDFC buffer 1 is used for device status to prevent* corruption of read/write buffer on status requests.*/- writew(1, &host->regs->nfc_buf_addr);
But it looks like buffer 1 is used for data with large page flash.
Well, we save first word of the buffer and then recover it.
Other drivers don't seem to have any problem with status reads clobbering the buffer...
+/* This functions is used by upper layer to checks if device is ready */ +static int mxc_nand_dev_ready(struct mtd_info *mtd)
"This functions is"? I'd say this comment is pretty redundant with respect to the function name anyway...
Fixed.
+static u_char mxc_nand_read_byte(struct mtd_info *mtd) +{
- struct nand_chip *nand_chip = mtd->priv;
- struct mxc_nand_host *host = nand_chip->priv;
- uint8_t ret = 0;
- uint16_t col, rd_word;
- uint16_t __iomem *main_buf =
(uint16_t __iomem *)host->regs->main_area0;- uint16_t __iomem *spare_buf =
(uint16_t __iomem *)host->regs->spare_area0;According to Magnus Lilja, "the nand flash controller can only handle 32 bit read/write operations, any other size will cause an abort (or something like that)". But now we're accessing it as 16-bit?
16-bit accesses work quite well. Problem was with 8-bit accesses.
+static uint16_t mxc_nand_read_word(struct mtd_info *mtd) +{
- struct nand_chip *nand_chip = mtd->priv;
- struct mxc_nand_host *host = nand_chip->priv;
- uint16_t col, rd_word, ret;
- uint16_t __iomem *p;
- MTDDEBUG(MTD_DEBUG_LEVEL3,
"mxc_nand_read_word(col = %d)\n", host->col_addr);- col = host->col_addr;
- /* Adjust saved column address */
- if (col < mtd->writesize && host->spare_only)
col += mtd->writesize;- if (col < mtd->writesize) {
p = (uint16_t __iomem *)(host->regs->main_area0 + (col >> 1));- } else {
p = (uint16_t __iomem *)(host->regs->spare_area0 +((col - mtd->writesize) >> 1));- }
- if (col & 1) {
rd_word = readw(p);ret = (rd_word >> 8) & 0xff;rd_word = readw(&p[1]);ret |= (rd_word << 8) & 0xff00;col should never be odd if you're reading words.
It can be odd if previously we've read a byte.
+/*
- Write data of length len to buffer 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
- */
+static void mxc_nand_write_buf(struct mtd_info *mtd,
const u_char *buf, int len)+{
- struct nand_chip *nand_chip = mtd->priv;
- struct mxc_nand_host *host = nand_chip->priv;
- int n, col, i = 0;
- MTDDEBUG(MTD_DEBUG_LEVEL3,
"mxc_nand_write_buf(col = %d, len = %d)\n", host->col_addr,len);- col = host->col_addr;
- /* Adjust saved column address */
- if (col < mtd->writesize && host->spare_only)
col += mtd->writesize;- n = mtd->writesize + mtd->oobsize - col;
- n = min(len, n);
- MTDDEBUG(MTD_DEBUG_LEVEL3,
"%s:%d: col = %d, n = %d\n", __func__, __LINE__, col, n);- while (n) {
Safer to do while (n > 0), especially when the code is this complex.
Fixed.
void __iomem *p;if (col < mtd->writesize) {p = host->regs->main_area0 + (col & ~3);} else {p = host->regs->spare_area0 -mtd->writesize + (col & ~3);}MTDDEBUG(MTD_DEBUG_LEVEL3, "%s:%d: p = %p\n", __func__,__LINE__, p);if (((col | (int)&buf[i]) & 3) || n < 16) {Do not cast pointers to "int". Use "uintptr_t" or "unsigned long" if you must.
Why < 16 and not < 4?
Fixed.
uint32_t data = 0;if (col & 3 || n < 4)data = readl(p);If that condition doesn't hold, the data we use is zero?
If that condition doesn't hold we are going to rewrite a whole 32-bit word so there is no need to read it's old content. But I've changed that piece of code as you suggested anyway.
switch (col & 3) {case 0:if (n) {data = (data & 0xffffff00) |(buf[i++] << 0);n--;col++;}case 1:if (n) {data = (data & 0xffff00ff) |(buf[i++] << 8);n--;col++;}case 2:if (n) {data = (data & 0xff00ffff) |(buf[i++] << 16);n--;col++;}case 3:if (n) {data = (data & 0x00ffffff) |(buf[i++] << 24);n--;col++;}}writel(data, p);Might I suggest this?
union { uint32_t word; uint8_t bytes[4]; } nfc_word;
nfc_word.word = readl(p); nfc_word.bytes[col & 3] = buf[i++]; n--; col++;
writel(nfc_word.word, p);
As a side benefit, you lose the endian dependency.
Thanks! I've used your code.
mxc_nand_memcpy32(p, (void *)&buf[i], m);Unnecessary cast.
Fixed.
- /* Update saved column address */
- host->col_addr = col;
+}
No blank lines before the brace at the end of a block.
Fixed.
+/*
- Used by the upper layer to verify the data in NAND Flash
- with the data in the buf.
- */
+static int mxc_nand_verify_buf(struct mtd_info *mtd,
const u_char *buf, int len)+{
- return -EFAULT;
+}
Umm...
I've added verify_buf function.
- case NAND_CMD_SEQIN:
if (column >= mtd->writesize) {/** 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.*/if (host->pagesize_2k) {/* call ourself to read a page */mxc_nand_command(mtd, NAND_CMD_READ0, 0,page_addr);}Should this be #ifdef HWECC? And update the comment to indicate the actual problem, which is the unusual hardware ECC implementation. I don't see what the lack of a "READ1" command has to do with it.
I've updated the comment.
And is this actually a FIXME if it's already being done?
+#ifdef CONFIG_MXC_NAND_HWECC
- this->ecc.calculate = mxc_nand_calculate_ecc;
- this->ecc.hwctl = mxc_nand_enable_hwecc;
- this->ecc.correct = mxc_nand_correct_data;
- this->ecc.mode = NAND_ECC_HW;
- this->ecc.size = 512;
- this->ecc.bytes = 3;
- this->ecc.layout = &nand_hw_eccoob_8;
- tmp = readw(&host->regs->nfc_config1);
- tmp |= NFC_ECC_EN;
- writew(tmp, &host->regs->nfc_config1);
+#else
- this->ecc.size = 512;
- this->ecc.bytes = 3;
- this->ecc.layout = &nand_hw_eccoob_8;
- this->ecc.mode = NAND_ECC_SOFT;
- tmp = readw(&host->regs->nfc_config1);
- tmp &= ~NFC_ECC_EN;
- writew(tmp, &host->regs->nfc_config1);
+#endif
Soft ECC only supports 256-byte ECC blocks (anything you set here to the contrary will just be overwritten), and you'll need a layout that accommodates that with enough ECC bytes.
Alternatively, you can implement 512-byte soft ECC if you don't want to waste those 3 extra OOB bytes. :-)
- host->pagesize_2k = 0;
So large page is currently unsupported?
Linux driver was fixed recently and now it claims to support 2K page size... I've added all needed fixes but I can understand how this driver should detect the pagesize... Linux driver calls nand_scan_ident() itself for this... Do you think I can calls nand_scan_ident() from my board_nand_init() function?
Regards, Ilya.