From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ie0-x22a.google.com ([2607:f8b0:4001:c03::22a]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VRWO0-0002NW-B9 for linux-mtd@lists.infradead.org; Thu, 03 Oct 2013 00:03:21 +0000 Received: by mail-ie0-f170.google.com with SMTP id x13so3778210ief.1 for ; Wed, 02 Oct 2013 17:02:58 -0700 (PDT) Date: Wed, 2 Oct 2013 17:02:53 -0700 From: Brian Norris To: Ezequiel Garcia Subject: Re: [PATCH 00/21] Armada 370/XP NAND support Message-ID: <20131003000253.GX23337@ld-irv-0074.broadcom.com> References: <1379606505-2529-1-git-send-email-ezequiel.garcia@free-electrons.com> <20130926195615.GK23337@ld-irv-0074.broadcom.com> <20130930122428.GA2417@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130930122428.GA2417@localhost> Cc: Thomas Petazzoni , Lior Amsalem , Jason Cooper , Tawfik Bayouk , Artem Bityutskiy , Daniel Mack , Huang Shijie , linux-mtd@lists.infradead.org, Gregory Clement , Willy Tarreau List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, Sep 30, 2013 at 09:24:29AM -0300, Ezequiel Garcia wrote: > On Thu, Sep 26, 2013 at 12:56:15PM -0700, Brian Norris wrote: > > On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote: > > > * Page layout > > > > > > The controller has a 2176 bytes FIFO buffer. Therefore, in order to support > > > larger pages, I/O operations on 4 KiB and 8 KiB pages is done with a set of > > > chunked transfers. > > > > > > For instance, if we choose a 2048 data chunk and BCH ECC we will have > > > to use this layout in the pages: > > > > > > ------------------------------------------------------------------------------ > > > | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... | > > > ------------------------------------------------------------------------------ > > > > > > The last remaining area is unusable (i.e. wasted). > > > > Why is it wasted? With a 2176B buffer, you can fit more than 2048+32+30. > > Are you simplifying things to support only NAND geometries like the > > tradiditional large page 2KB+64? (If so, what happens with 4KB pages > > that need higher level of ECC than can fit in 64 x 2 = 128B?) And in the > > 2KB+64 case, why is the 64-32-30=2B wasted? > > > > Hm... the wasted bytes are due to an 8-byte alignment controller requirement on the > data buffer. However, you're right: in a flash with a page of 4KB+224B (e.g.) you > should be able to access at least some of the remaining OOB area. Alignment requirement for the start of the buffer, length of the buffer, or both? > Notice OOB handling is still a bit confusing in this driver. Yes. > > > To match this, my current (already working!) implementation acts like > > > a layout "impedance matcher" to produce in an internal driver's buffer > > > > Maybe I'm in the wrong field, but I'm not sure how an impedance matcher > > applies here. > > > > > this layout: > > > > > > ------------------------------------------ > > > | 4096B data | 64B spare | > > > ------------------------------------------ > > > > What is this "internal buffer"? (I'll read through your patches in a bit > > and find out, perhaps?) Just the raw data + spare that you are ready to > > read/write? Are you dropping the ECC from this buffer? Doesn't it need > > to be returned to the callee (when the 'oob_required' flag is true)? > > > > Yes, the driver's internal buffer has the raw data + OOB that has > been read from the controller after a read operation, or that's > ready to be written to the flash before a program operation. > > That said, if the command issued was READOOB then the OOB is read fully: > spare and ECC code and the buffer is filled like this: > > ---------------------------------------------------------------------------------------------- > | 4096B data | 32B spare | 30B ECC* | 2x 0xff | 32B spare | 30B ECC | 2x 0xff | > ---------------------------------------------------------------------------------------------- > > (*) We need to add two bytes (0xff) to align the 30B ECC read. > > I was expecting to handle the above layout properly using this nand_ecclayout > structure: > > static struct nand_ecclayout ecc_layout_4KB_bch4bit = { > .eccbytes = 64, > .eccpos = { > 32, 33, 34, 35, 36, 37, 38, 39, > 40, 41, 42, 43, 44, 45, 46, 47, > 48, 49, 50, 51, 52, 53, 54, 55, > 56, 57, 58, 59, 60, 61, 62, 63, > 96, 97, 98, 99, 100, 101, 102, 103, > 104, 105, 106, 107, 108, 109, 110, 111, > 112, 113, 114, 115, 116, 117, 118, 119, > 120, 121, 122, 123, 124, 125, 126, 127}, > /* Bootrom looks in bytes 0 & 5 for bad blocks */ > .oobfree = { {1, 4}, {6, 26}, { 64, 32} } > }; > > However, I must be doing something wrong since currently the mtd_oobtest fails > at some point and I need to investigate the issue further. > > > > In order to achieve reading (for instance), we issue several READ0 commands > > > (with some additional controller-specific magic) and read two chunks of 2080B > > > (2048 data + 64 spare) each. > > > > The math is a little inconsistent here and elsewhere. Do you mean two > > chunks of 2080B (2048 data + 32 spare)? > > > > Yes. It should be: 2048B data + 32B spare. > > > > The driver accomodates this data to expose the NAND base a contiguous 4160B buffer > > > (4096 data + 64 spare). > > > > This math matches up right, if it's double the 2KB+32 instead of double > > 2KB+64. > > > > Yup. > > > > * ECC > > > > > > The controller has built-in hardware ECC capabilities. In addition it is completely > > > configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC > > > then the ECC code will be calculated for each transfered chunk and expected to be > > > located (when reading/programming) at specific locations. > > > > At "specific locations", but always within the 30B region that you've > > been mapping out? > > > > By 'specific location' I mean after the 32B spare area. > > > > So, repeating the above scheme, a 2048B data chunk will be followed by 32B spare, > > > and then the ECC controller will read/write the ECC code (30B in this case): > > > > > > ------------------------------------ > > > | 2048B data | 32B spare | 30B ECC | > > > ------------------------------------ > > > > Is ECC always 30B? Or is this just the maximum size, so you always > > reserve 30B? > > > > In the BCH case, yes: it's always 30B. So by "BCH" you actually are referring to BCH-4? Are there any other BCH modes supported? Or do you expect any future revisions to expand on the BCH options available? > AFAIK, the ECC works like this. When programming a page, you can > actually instruct the controller to transfer an arbitrary amount of bytes. > > (Details: you do this by adding NDCB0_LEN_OVRD to the command buffer 0, > and setting the i/o length at buffer 3). > > The ECC code will be written after the transfered bytes. So, when > programming a page we configure the length to be 2048B + 32B and get > the ECC after it. > > Equally, we can read an arbitrary length. and the ECC code is expected > to be in the area immediate the read area. > > Notice that this means you can control the ECC strength: since the ECC > is always 30B, and it's calculated on the transfered chunk you can > configure the controller to transfer the page in 1024B chunks and get a > 30B ECC for each of this chunk, thus doubling the ECC strength. This is a little odd. Your ECC HW is not parameterized by ECC strength, but instead by ECC sector size? I think I have some comments for patch 5, relevant to this topic. > This should be added to the driver, and I expect it to be fairly easy > with the infrastructure already in place. Brian