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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
