Re: NAND review

[Ross Younger <wry@ecoscentric.com>]

Hi Simon,

Thanks for the feedback.

> * [io/nand & devs/nand]

I took the view that the path to the driver package ought to imply what
subsystem it's for. Also, putting NAND drivers in devs/flash/nand/* just
doesn't feel right. The flash subsystem takes flash drivers, the nand
subsystem takes nand drivers, and never the twain shall meet.


> * Ross added very primitive support for partitions. Each partition is
> defined by a first and a last block. Reading/writing pages and erasing
> sectors is always done in the context of a partition. BUT the page and
> block addresses are still absolute addresses to the flash chip base, not
> relative to the given partition. This is slightly confusing and I wonder
> if the better approach would be to change this to relative addresses.

I perceive a need for some sort of mechanism to draw the line between
different regions of an array. It seems quite common to have a "/boot
partition" containing one's kernel image and so forth, and to use the rest
of the array as the root filesystem.

My choice of absolute addressing was really a decision for simplicity in the
early days; I suspect that relative addressing could be factored in without
much trouble. YAFFS works just fine with absolute addressing (startBlock and
endBlock); there's no reason why we couldn't pull the wool over its eyes and
tell it to use blocks 0 through whatever, translating them as we pass
addresses onto the NAND layer, but at the same time this is not much of an
argument either way.


> I also wonder if the current approach for initializing the partition
> table is wise. Currently the flash device driver uses a macro
> NAND_DEV_PARTITION_INIT to call a potential function defined the HAL
> platform to initialize the partition table. This is flexible but we will
> have a lot of code duplication in HAL platforms if the partition table
> is going to be defined via CDL options. Couldn't this be implemented in
> a more generic fashion?

The partition definition is necessarily platform-specific, so doesn't fit
anywhere else. When I get round to working through code changes from the
feedback I've received, I'll have a think about whether this could be done
better.


> The interface between the common NAND code
> and the flash devices is very high-level (initialization,
> reading/writing pages, erasing blocks, checking factory bad blocks).

This was a conscious design decision, in order to try and keep the
complexity down.


> In the current design the
> NAND flash controller is implemented in the HAL platform. The interface
> between the chip driver and the controller is defined in the chip driver
> itself (it just calls to a few functions which the platforms needs to
> implement). Is it intended that future drivers use the same interface or
> are drivers free to define their own? 

Unclear. I think that to prescribe too rigidly risks being unduly
constrictive. NAND chips with a 16-bit data bus seem to need 16 bit read and
write functions, and 8-bit for some operations (Read ID, write command,
write address?), and I have heard of some cases where multiple chips are
ganged together to provide a virtual 32-bit device. We could try to work all
this into one place, but I'm worried that we could end up with a wobbly
complex mess which isn't quite right for anything.


> I also wonder if the NAND flash controller functionality
> belongs to the platform. In my opinion this should go to the HAL variant
> instead.

This is a tricky call. In some cases the variant HAL could be the right
place, in others it would most certainly be better suited to the platform.
For example, the NAND chip is hooked up on the EA 2468 board via the memory
controller; all my driver does is write to appropriate MMIO locations which
in turn wiggle the chip's legs. However, would it be right to put something
into the LPC2xxx variant (and if so, what)? Another LPC2xxx-based platform
might have NAND on-board which you could only access by talking to a CPLD
which was effectively a bespoke NAND controller.


> * The current code does not read the flash chip timings from the chip
> signature. The timings are hardcoded into the driver. This might or
> might not be the best approach. 

Timings are necessarily chip-specific, and I've only written the one
real-hardware driver to date. It's entirely possible that my k9 driver could
be adaptable into something more generic, at which point a lookup table
keyed off at least the device ID makes sense.


> * The current synth driver is rather limited in its configurability. It
> does not yet support small page flash chips for example. This should be
> improved but I think that's pretty easy to do.

I was aiming for compatibility with the Linux MTD layer in using a Bad Block
Table, ECC and the layout of the on-chip out-of-band (spare) area. Devices
with 256-byte pages aren't implemented simply because I haven't (yet?)
figured out how the BBT works on them. I might have missed something, but
the location for the BBT identity pattern is at offset 8 of the (8-byte!)
OOB area of the first page of the block that holds the BBT. I'll take
another look into this in due course.


By the way, speaking of Linux compatibility, I did a bit of digging to
confirm the licensing status of the ECC algorithm in the Linux MTD layer.
The latest source in the Linux kernel tree is full-GPL, but an older version
is GPL+LibException. packages/io/nand/current/src/nand_ecc_mtd.c contains
the links I used to verify this. At the time, I discussed this with the
in-house maintainers, and it seemed likely that would be OK to incorporate
into eCos, so I went with it.


Regards,


Ross

-- 
Embedded Software Engineer, eCosCentric Limited.
Barnwell House, Barnwell Drive, Cambridge CB5 8UU, UK.
Registered in England no. 4422071.                  www.ecoscentric.com