From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Zapolskiy Date: Tue, 11 Aug 2015 02:43:36 +0300 Subject: [U-Boot] [PATCH v6 2/5] nand: lpc32xx: add hardware ECC support In-Reply-To: <4F172219764C784B84C2C1FF44E7DFB10300C5BF@003FCH1MPN2-041.003f.mgd2.msft.net> References: <1439208994-19072-2-git-send-email-slemieux.tyco@gmail.com> <55C8B2F8.1070900@mleia.com> <4F172219764C784B84C2C1FF44E7DFB10300C5BF@003FCH1MPN2-041.003f.mgd2.msft.net> Message-ID: <55C93728.6000301@mleia.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 10.08.2015 21:40, LEMIEUX, SYLVAIN wrote: > >> -----Original Message----- >> From: Vladimir Zapolskiy [mailto:vz at mleia.com] >> >> Hi Sylvain, >> >> On 10.08.2015 15:16, slemieux.tyco at gmail.com wrote: >>> From: Sylvain Lemieux >>> >>> Incorporate NAND SLC hardware ECC support from legacy >>> LPCLinux NXP BSP. >>> The code taken from the legacy patch is: >>> - lpc32xx SLC NAND driver (hardware ECC support) >>> - lpc3250 header file missing SLC NAND registers definition >>> >>> The legacy driver code was updated to integrate with the existing NAND SLC driver. >>> >>> Signed-off-by: Sylvain Lemieux >>> --- > > [...] > >>> >>> diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c >>> index 719a74d..2a32264 100644 >>> --- a/drivers/mtd/nand/lpc32xx_nand_slc.c >>> +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c >>> @@ -3,15 +3,48 @@ >>> * > > [...] > >>> + >>> +#define CONFIG_SYS_NAND_ECCBYTES 3 >>> +#define CONFIG_SYS_NAND_ECCSIZE 256 >> >> These defines are OK, but see my bottom comment below related to this >> subject. >> >> Also it's worth to mention that these defines are conflicting with the >> same defines from update to my board: >> http://lists.denx.de/pipermail/u-boot/2015-July/219251.html -- I don't >> understand why my changes are still not present in U-boot/master e.g. to >> catch this kind of problems. >> > >> So, CONFIG_SYS_NAND_ECCSIZE, CONFIG_SYS_NAND_ECCBYTES and >> CONFIG_SYS_NAND_OOBSIZE are used outside of the driver code (to be >> precise they are used in drivers/mtd/nand/nand_spl_simple.c), and >> therefore this is not the right place to define them IMHO. > > I missed the change; I applied only patch 2/4 of your series (NAND SLC driver). It is not your fault, please understand that the maintainers are very busy, some of the changes are not merged timely. > I can add an #if !defined() statement for both of them, as it was in previous > version of the patch. This way, you don't need to define them in your board > config if you are not using the SPL. This is ugly. It seems that an update to arch-lpc32xx/config.h is required here, I'll send a change tomorrow. >> >>> struct lpc32xx_nand_slc_regs { >>> u32 data; >>> @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs { >>> > [...] >>> >>> +#if defined(CONFIG_DMA_LPC32XX) >>> +/* Total ECC bytes and size; refer to board_nand_init() for details. */ >>> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE) >>> +#define TOTAL_ECCBYTES (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS) >>> +#define TOTAL_ECCSIZE (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS) >> >> See my comment below regarding these two defines. >> > > [...] > >>> + >>> + /* >>> + * ECC correction is done by page when using the DMA controller >>> + * scatter/gather mode through linked list; >>> + * refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3 >>> + * section 9.7 and tables 173 notes for details. >>> + * >>> + * The error correction is done on each page and process in multiple >>> + * steps of 256 bytes inside the driver. >>> + * >>> + * The total ECC size and bytes for a page are used; >>> + * NAND base code (HW ECC) will call the read / write buffer functions >>> + * using the total ECC size and bytes for a page as a single step. >>> + */ >>> + lpc32xx_chip->ecc.size = TOTAL_ECCSIZE; >>> + lpc32xx_chip->ecc.bytes = TOTAL_ECCBYTES; >> >> I still don't quite understand, why TOTAL_ECCSIZE and TOTAL_ECCBYTES are >> not equal to CONFIG_SYS_NAND_ECCSIZE and CONFIG_SYS_NAND_ECCBYTES >> correspondingly. IOW why total values are used here? >> >> This indicates a bug. >> > > The ECC correction is done by page when using the DMA controller > scatter/gather mode through linked list; the DMA is configured to > process 2 (small) or 8 (large) pages of 256 bytes and the ECC read > without any software intervention, resulting in a single DMA transfer. > > As per the "LPC32x0 and LPC32x0/01 User manual" table 173 notes and > section 9.7, the NAND SLC & DMA allowed single DMA transaction of a > page size (512 or 2048) that manage the ECC within that single transaction, > resulting in an ECCSIZE of 512 (small page) or 2048 (large page) size. > Here we are talking about ecc.size and ecc.bytes. I do believe that the change works correctly due to two compensating issues -- one is misusage of ecc.size and ecc.bytes and another one is exploitation of this misusage. Both must be corrected. If you open include/linux/mtd/nand.h you may note the following description: struct nand_ecc_ctrl - Control structure for ECC ... @bytes: ECC bytes per step @size: data bytes per ECC step ... Per ECC step, not total. Please fix it. -- With best wishes, Vladimir