NAND Flash Framework for review

[Grzegorz Bernacki]

Luiz Otavio O Souza wrote:
> On Mar 14, 2010, at 4:58 AM, Andrew Turner wrote:
> 
>> On Tue, 9 Mar 2010 00:28:53 +0100
>> Rafal Jaworowski <raj at semihalf.com> wrote:
>>> We are looking for review, comments and any other feedback.
>> Below is my first set of notes from reading the code. I haven't ported
>> the driver to my hardware yet so these are only from using with nandsim.
>>

First, thank you both for review the code. Below are responses for some of
your comments. I need to inspect the code to answer to rest of them. Soon I
will send another mail.

>> Chip drivers:
>> - lnand and snand have magic numbers to figure out which drive to use.
>>  We should move these to a flag in the chip parameters.
>
> We just need to add the chip size in nand_params and based on that we can
> calculate the number of address cycles (see below) and the type of chip 
> (if chip >= 128MB and pagesize > 512 then you have a large page device).
> 
Yes, I was thinking about adding size of page and column address to parameters
of nfc_send_address.

>> - nand_read_pages should be a device method so we can customise how we
>>  handle reading of pages.

By customising you mean, using interlea

>> - Split nand_generic into separate files with different device options
>>  so we can compile only the one we need.
>> - nand_generic.c should be split into multiple files to allow us to
>>  support only the chips we have.

There are some function that are common for all three drivers, that's why
I decided to put the all in one file.
> 
> - Fixed number of address cycles (and wrong count for large page devices).
> - Address cycles should not be sent by "driver" (this need to be moved up).
> - No support to read the ready/busy nand pin.
> - Bad block table for generic devices.
> 
> I'm not sure about slipt nand_generic.c, actually we can use the same kernel
> with different kind of chips (exactly what i'm doing here). Also there are only
> a few functions for each case.
> 
>> nandbus:
>> - nandbus should not know what commands to send to nand chips. These
>>  requests should be moved up to the chip driver.
Yes, you're right

>> - Why is nandbus not exposing it's interface via NEWBUS?
I didn't think about it, but that's good idea.

>> - Where is nandbus_destroy used?
It should be used in nand controller detach function.

>> - Why is malloc called with M_NOWAIT when allocating the nandbus ivar?
No reason, I will change it.

>> - What should happen when nandbus_send_command, nandbus_send_address,
>>  nandbus_start_command, nandbus_read_buffer or nandbus_write_buffer
>>  fail?

>> - Why does nandbus need to know if we are an ONFi part or not? We
>>  can push the check into the onand driver's probe function.
> 
> - nandbus have nandbus_select_cs() but not the equivalent nandbus_deselect_cs().
I didn't know we need that function. I will add it.

> 
>> nandsim:
>> - nandsim sample config has invalid time values (too small.
>> - There is a kernel crash in nandsim_log.

There are some bugs in nandsim. I will provide fixes for it soon.

>> General:
>> - malloc with M_WAITOK will always succeed, no need to check return
>>  value.
>> - Should define our own malloc type.
Ok, I will do it.

>>
>> Ideas:
>> - Can we move ecc and bbt handling into GEOM? This will allow us to
>>  bypass them when required.
> 
> This is a mandatory feature (disable ecc and may be the bbt checks) if you need to
> deal with some kind of unknown nand FS or unknown nand oob layout (like make a backup
> of your unknown nand data before erase it).

Ok, I didn't know that disabling ECC and BBT is so important. Let me think about moving it
to geom layer.

grzesiek