Re: [PATCH] NAND: Fix NAND ECC errors on AMD Au1550

Sergei Shtylylov
2005-11-01T15:28:03Z

Hello.

Leif Lindholm wrote:

> On Mon, 2005-10-31 at 08:00 -0800, Pete Popov wrote:
>
>>>Just a comment (forgot to post this last time around):
>>>At least one of the chips we're using - identified as
>>>"NAND device: Manufacturer ID: 0xec, Chip ID: 0x76 (Samsung NAND 64MiB 3,3V 8-bit)"
>>>seems to also need the CS manually asserted during a READID operation.
>>>
>>>This made the if-statements ridicilously long, so I added a case
>>>statement and a variable keeping track of wether or not to force-enable
>>>CS to the au1550_command function.
>>>
>>>If there is any interest, I could post a patch, but am unsure of the
>>>correct way. Should I send a new patch against cvs HEAD containing my
>>>trivial tweak as well as Sergeis modifications, or...?
>>
>>Send an incremental patch on top of Sergey's patch if you can.
>
>
> Attached is an incremental patch on top of Sergeys latest patch
> (yesterday). Is that's ok, or should I do the same for the old one?

Alas, I have to NAK your patch.

> ------------------------------------------------------------------------
>
> --- mtd/drivers/mtd/nand/au1550nd.c 2005-10-31 18:37:29.000000000 +0100
> +++ mtd-new/drivers/mtd/nand/au1550nd.c 2005-11-01 15:39:43.000000000 +0100
> @@ -343,6 +343,16 @@ static void au1550_command(struct mtd_in
> int ce_override = 0, i;
> ulong flags;
>
> + /* Decide wether command needs to force-enable CE or not */
> + switch (command) {
> + case NAND_CMD_READ0:
> + case NAND_CMD_READ1:
> + case NAND_CMD_READOOB:
> + case NAND_CMD_READID:
> + ce_override = 1;
> + break;
> + }
> +
> /* Begin command latch cycle */
> this->hwcontrol(mtd, NAND_CTL_SETCLE);
> /*
> @@ -382,9 +392,7 @@ static void au1550_command(struct mtd_in
> if (page_addr != -1) {
> this->write_byte(mtd, (u8)(page_addr & 0xff));
>
> - if (command == NAND_CMD_READ0 ||
> - command == NAND_CMD_READ1 ||
> - command == NAND_CMD_READOOB) {
> + if (ce_override) {

Note, that with NAND_CMD_READID page_addr of -1 is always passed (so we
only write out the 1-byte column address) on the address phase, and we just
won't get there for READID command....

> /*
> * NAND controller will release -CE after
> * the last address byte is written, so we'll
> @@ -393,7 +401,6 @@ static void au1550_command(struct mtd_in
> * want the NOR flash or PCMCIA drivers to
> * steal our precious bytes of data...
> */
> - ce_override = 1;
> local_irq_save(flags);

Therefore, neither -CE wil be overridden nor local_irq_save() be executed
for READID case...

> this->hwcontrol(mtd, NAND_CTL_SETNCE);
> }
> @@ -412,25 +419,7 @@ static void au1550_command(struct mtd_in
> * Program and erase have their own busy handlers.
> * Status and sequential in need no delay.
> */
> - switch (command) {
> -
> - case NAND_CMD_PAGEPROG:
> - case NAND_CMD_ERASE1:
> - case NAND_CMD_ERASE2:
> - case NAND_CMD_SEQIN:
> - case NAND_CMD_STATUS:
> - return;
> -
> - case NAND_CMD_RESET:
> - break;
> -
> - case NAND_CMD_READ0:
> - case NAND_CMD_READ1:
> - case NAND_CMD_READOOB:
> - /* Check if we're really driving -CE low (just in case) */
> - if (unlikely(!ce_override))
> - break;
> -
> + if (ce_override) {
> /* Apply a short delay always to ensure that we do wait tWB. */
> ndelay(100);
> /* Wait for a chip to become ready... */
> @@ -442,6 +431,19 @@ static void au1550_command(struct mtd_in
> local_irq_restore(flags);

... and later you get the CPU flags corrupted.

> return;
> }
> +
> + switch (command) {
> +
> + case NAND_CMD_PAGEPROG:
> + case NAND_CMD_ERASE1:
> + case NAND_CMD_ERASE2:
> + case NAND_CMD_SEQIN:
> + case NAND_CMD_STATUS:
> + return;
> +
> + case NAND_CMD_RESET:
> + break;
> + }
> /* Apply this short delay always to ensure that we do wait tWB. */
> ndelay(100);

Has your READID problem arisen with or without my patch?

WBR, Sergei