From patchwork Sun Nov 2 05:51:38 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Werner Almesberger X-Patchwork-Id: 6832 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 525F8DDDF3 for ; Sun, 2 Nov 2008 16:54:24 +1100 (EST) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.68 #1 (Red Hat Linux)) id 1KwVsZ-0002lv-4B; Sun, 02 Nov 2008 05:52:03 +0000 Received: from mail.openmoko.org ([88.198.124.205]) by bombadil.infradead.org with esmtps (Exim 4.68 #1 (Red Hat Linux)) id 1KwVsT-0002lm-NC for linux-mtd@lists.infradead.org; Sun, 02 Nov 2008 05:51:57 +0000 Received: from ol194-228.fibertel.com.ar ([24.232.228.194] helo=ws) by mail.openmoko.org with esmtpsa (TLS-1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.63) (envelope-from ) id 1KwVwR-0006mG-Ke; Sun, 02 Nov 2008 05:56:06 +0000 Received: by ws (sSMTP sendmail emulation); Sun, 02 Nov 2008 03:51:38 -0200 Date: Sun, 2 Nov 2008 03:51:38 -0200 From: Werner Almesberger To: Alexey Korolev Subject: clarify calculation in nand_read_subpage and fix possible bug Message-ID: <20081102055138.GB5177@almesberger.net> MIME-Version: 1.0 Content-Disposition: inline X-Spam-Score: 0.0 (/) Cc: linux-mtd@lists.infradead.org X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Alexey, while debugging a NAND ECC problem, I tried to figure out how the optimized ECC position calculation in nand_read_subpage worked. A few pulled hairs later :-) ... I think it could be simplified a bit, which also helps GCC to generate more compact code for it. If I understand things correctly, it also seems that the original algorithm had a small bug that would show with busw = 2 and non-contiguous ECC blocks. My analysis is below. Could you please have a look at it and ack it if you think it's okay ? Thanks ! - Werner ---------------------------------- cut here ----------------------------------- clarify-nand-ecc-access.patch The ECC access calculation in nand_read_subpage is quite hard to understand. This patch makes it more readable. There is a small change in what the algorithm does: while if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1)) looks at the position of the ECC byte following the bytes we're currently reading, aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw); only looks at their length plus the additional data we have to read due to aligning the start position. I believe the latter to be more correct than the former, since the next ECC byte could be located anywhere and its location therefore may not give the alignment information we seek. I'm not even sure its position is always defined (what if we're reading the last ECC of the page ?). The last byte considered in the "gaps" test that checks that all ECC bytes are contiguous is eccfrag_len-2+start_step*ecc.bytes+1 = num_steps*ecc.bytes-2+start_step*ecc.bytes+1 = (start_step+num+steps)*ecc.bytes-1 so it doesn't include (start_step + num_steps) * chip->ecc.bytes. The change also saves 44 bytes on ARM. Acked-by: Alexey Korolev Index: ktrack/drivers/mtd/nand/nand_base.c =================================================================== --- ktrack.orig/drivers/mtd/nand/nand_base.c 2008-11-02 02:28:19.000000000 -0200 +++ ktrack/drivers/mtd/nand/nand_base.c 2008-11-02 02:38:22.000000000 -0200 @@ -851,12 +851,12 @@ } else { /* send the command to read the particular ecc bytes */ /* take care about buswidth alignment in read_buf */ - aligned_pos = eccpos[start_step * chip->ecc.bytes] & ~(busw - 1); - aligned_len = eccfrag_len; - if (eccpos[start_step * chip->ecc.bytes] & (busw - 1)) - aligned_len++; - if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1)) - aligned_len++; + + int pos; + + pos = eccpos[start_step * chip->ecc.bytes]; + aligned_pos = pos & ~(busw - 1); + aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw); chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + aligned_pos, -1); chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);