[U-Boot] [PATCH RFC] nand: remove spurious read cycle in OOB first page reads

[Nick Thompson]

On 20/11/09 20:17, Scott Wood wrote:
> On Fri, Nov 20, 2009 at 03:30:46PM +0000, Nick Thompson wrote:
>> In the case of a nand controller that needs the OOB data before
>> it can read the page data, an unnecessary read sequence is sent
>> to the nand. This reduces read performance.
> 
> By how much?  Is a similar patch going into Linux?

"How much" is difficult to answer. My platform is an 300MHz ARM9, with a
100MHz bus clock speed (to the 8 bit NAND). I have optimised the
nand_read_buf function in a architecture specific way that makes it almost
120% faster than the default (4.3MB/s -> 9.3MB/s).

In that context the overhead of the extra read operation is about 10%. Not
huge, but I have 13MBytes to pull out of NAND at boot time and every
little helps. There is another potential 10% in the OOB first read
function, which I intend to go after next. There may be another ~10% that
can be squeezed out, but that needs more analysis.

I do intend to address these same issues in the Linux driver later.

> 
>> This sequence is sent by default before all page reads, but the
>> OOB first page read function immediately issues a new command, a
>> simulated READOOB command, which overrides the previous sequence.
> 
> Is it just a performance issue, or could some NAND chips get confused by the
> aborted read command?

I'm not aware that the sequence is invalid. It seems not to cause an issue
on the device I'm using. I can only present it as a performance issue - and
that it looks confusing on a bus analyser.

> 
>> This patch (fragment) prevents the initial read sequence from
>> being sent if chip->ecc.mode indicates OOB first operation.
> 
> I can apply this if it's needed, but I'm hesitant to keep adding workarounds
> for layering problems.  Is there any way we can push all cmdfunc invocations

That was my main concern as well. Also I wanted to guage reaction to
submitting patches for code that essentially is copied from Linux, before I
start with any major monkeying around in there.

> into replaceable functions?  Have nand_do_read_ops just be a loop around
> high-level "read page" functions that are replaceable, and which can keep
> their own state to determine whether autoinc is applicable.

Good questions. Needs more thought, but sounds reasonable to me. I will
look into the implications of this. I can only test OOB first page read and
only 2k page devices.

The code does spend a lot of time waiting around for the NAND to be ready
when it could actually be doing something useful at the same time. Autoinc
should be maintained and also sequential-read commands could help
performance, rather than falling back to read0 all the time. [read0 has
25us busy time, read sequential has 3us]
 
> Ideally a high-level driver like fsl_elbc_nand wouldn't have to implement
> cmdfunc at all.

I'm not sure I know what you mean here. I'll have a look at that code. cmdfunc
should probably be simplified to only send command sequences and not invoke
any nand wait functions. This gets in the way of some useful optimisation. If
some drivers are replacing it, it will be more difficult to coordinate
changes.

Nick.