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

[Scott Wood]

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