From owner-freebsd-embedded@FreeBSD.ORG Mon Mar 15 11:32:37 2010 Return-Path: Delivered-To: embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CAA53106566C for ; Mon, 15 Mar 2010 11:32:37 +0000 (UTC) (envelope-from gjb@semihalf.com) Received: from smtp.semihalf.com (smtp.semihalf.com [213.17.239.109]) by mx1.freebsd.org (Postfix) with ESMTP id 38BC48FC24 for ; Mon, 15 Mar 2010 11:32:36 +0000 (UTC) Received: from localhost (unknown [213.17.239.109]) by smtp.semihalf.com (Postfix) with ESMTP id 92CA4C42D4; Mon, 15 Mar 2010 12:17:58 +0100 (CET) X-Virus-Scanned: by amavisd-new at semihalf.com Received: from smtp.semihalf.com ([213.17.239.109]) by localhost (smtp.semihalf.com [213.17.239.109]) (amavisd-new, port 10024) with ESMTP id Wl5qOpla4mvJ; Mon, 15 Mar 2010 12:17:57 +0100 (CET) Received: from [192.168.1.12] (actl96.neoplus.adsl.tpnet.pl [83.11.65.96]) by smtp.semihalf.com (Postfix) with ESMTPA id 50714C42D1; Mon, 15 Mar 2010 12:17:57 +0100 (CET) Message-ID: <4B9E1697.9090602@semihalf.com> Date: Mon, 15 Mar 2010 12:14:31 +0100 From: Grzegorz Bernacki User-Agent: Thunderbird 2.0.0.16 (X11/20090618) MIME-Version: 1.0 To: Luiz Otavio O Souza , Andrew Turner References: <0AE04EFA-A3EB-4939-BD81-607C00355B67@semihalf.com> <20100314165825.121d346b@fubar.geek.nz> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: embedded@freebsd.org Subject: Re: NAND Flash Framework for review X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Mar 2010 11:32:38 -0000 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 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