[U-Boot] [PATCH v6 2/5] nand: lpc32xx: add hardware ECC support

[Vladimir Zapolskiy]

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 <slemieux at tycoint.com>
>>>
>>> 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 <slemieux at tycoint.com>
>>> ---
> 
> [...]
> 
>>>
>>> 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