Date: Mon, 15 Mar 2010 12:14:31 +0100 From: Grzegorz Bernacki <gjb@semihalf.com> To: Luiz Otavio O Souza <loos.br@gmail.com>, Andrew Turner <andrew@fubar.geek.nz> Cc: embedded@freebsd.org Subject: Re: NAND Flash Framework for review Message-ID: <4B9E1697.9090602@semihalf.com> In-Reply-To: <CC419602-A9E8-4FE2-A5A5-0BFBD8240EDD@gmail.com> References: <0AE04EFA-A3EB-4939-BD81-607C00355B67@semihalf.com> <20100314165825.121d346b@fubar.geek.nz> <CC419602-A9E8-4FE2-A5A5-0BFBD8240EDD@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B9E1697.9090602>