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

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

[Nick Thompson]

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.

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.

This patch (fragment) prevents the initial read sequence from
being sent if chip->ecc.mode indicates OOB first operation.

Signed-off-by: Nick Thompson <nick.thompson@gefanuc.com>
---

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 426bb95..cf85bde 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1261,7 +1272,9 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
                        bufpoi = aligned ? buf : chip->buffers->databuf;

                        if (likely(sndcmd)) {
-                               chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
+                               if (chip->ecc.mode != NAND_ECC_HW_OOB_FIRST)
+                                       chip->cmdfunc(mtd, NAND_CMD_READ0,
+                                                     0x00, page);
                                sndcmd = 0;
                        }

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

[Scott Wood]

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?

> 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?

> 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
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.

Ideally a high-level driver like fsl_elbc_nand wouldn't have to implement
cmdfunc at all.

-Scott

[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.

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

[Scott Wood]

On Mon, Nov 23, 2009 at 10:38:02AM +0000, Nick Thompson wrote:
> > 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.

The eLBC NAND controller presents an interface that runs entire NAND
operations at once.  In order to implement cmdfunc, we have to interpret
what the higher levels are trying to do and then turn it into something the
hardware will accept.

It would be much more straightforward to implement high-level "erase block",
"progam page", "read page", etc. operations.

> 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.

For the eLBC, we have to issue commands and wait as one operation, or else
the bus controller can change chipselects in the meantime, causing problems
with shared pins.  More reason to not make cmdfunc the only way to hook in a
driver...

-Scott