[U-Boot-Users] [PATCH] NAND read/write.jffs2 fix

Scott Wood
2008-05-16T16:45:08Z

Morten Ebbell Hestnes wrote:
>>> + "nand read[.jffs2, .i] addr off|partition size\n"
>>> + "nand write[.jffs2, .i] addr off|partition size\n"
>>
>> What about .e? Is it just for backwards compatibility that we have
>> three commands that mean the same thing? Do we want to document all
>> three?
>
> The doc/README.nand only mention .jffs2 and .i
> The option .jffs2 only has skipping bad blocks (and ECC) in common with
> the jffs2 file system.
> If we want to be backwards compatible we should use [.jffs2|.i|.e]
> otherwise [.i|.e] or [.skip] would be better.
> I leave this choice to you Scott.

I think we should stay compatible; I was just curious why some were
documented and others weren't.

If we *weren't* maintaining backward compatibility, I'd make the
block-skipping mode the default...

>>> + " - read/write `size' bytes starting at offset `off' to/from
>>> memory address `addr'\n"
>>
>> Maybe mention the effect of .jffs2/.i/.e here in addition to the
>> offline documentation.
>
> Agree.
> Something like this ?
> "nand - NAND sub-system\n",
> "info - show available NAND devices\n"
> "nand device [dev] - show or set current device\n"
> "nand read[.jffs2|.i|.e] addr off|partition size - read `size' bytes\n"
> " starting at offset `off' to memory address `addr'. Using one of\n"
> " the `.' options bad blocks is skipped otherwise read as 0xff\n"
> "nand write[.jffs2|.i|.e] addr off|partition size - write `size'\n"
> " bytes starting at offset `off' from memory address `addr'. Using\n"
> " one of `.' options bad blocks is skipped otherwise write fails\n"
> "nand erase [clean] [off size] - erase `size' bytes from offset\n"
> " `off' (entire device if not specified)\n"
> "nand bad - show bad blocks\n"
> "nand dump[.oob] off - dump page\n"
> "nand scrub - really clean NAND erasing bad blocks (UNSAFE)\n"
> "nand markbad off - mark bad block at offset (UNSAFE)\n"
> "nand biterr off - make a bit error at offset (UNSAFE)\n"
> "nand lock [tight] [status] - bring nand to lock state or display\n"
> " locked pages\n"
> "nand unlock [offset] [size] - unlock section\n");

s/blocks is skipped otherwise read/blocks are skipped, otherwise they
are read/

Looks OK otherwise.

>>> + block_len = nand->erasesize - (offset & (nand->erasesize - 1));
>>> +
>>> + if (!nand_block_isbad (nand, offset))
>>> + len_excl_bad += block_len;
>>
>> Hmm, is it safe to call nand_block_isbad with an offset that isn't the
>> start of the block? The BBT implementation seems OK with it, but what
>> about block_bad callbacks?
>
> Both nand_block_bad and nand_isbad_bbt seems ok.
> But just in case it could be changed to "if (!nand_block_isbad (nand,
> offset & (~nand->erasesize + 1))"
>
> I do not understand what you mean about block_bad callbacks.

An individual NAND driver can provide its own block_bad() method in the
nand_chip struct, and the semantics of the "ofs" parameter aren't described.

There's no code currently in-tree that would have a problem with it, but
it might be better to mask it anyway.

-Scott