[U-Boot] [PATCH RFC] nand: remove spurious read cycle in OOB first page reads
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; }
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
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.
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
participants (2)
-
Nick Thompson -
Scott Wood