[U-Boot] [PATCH 1/3] nand: lpc32xx: add SLC NAND driver

Tue Jul 28 02:21:35 CEST 2015

Hi Sylvain,

On 28.07.2015 00:45, LEMIEUX, SYLVAIN wrote:
> Hi Vladimir and Albert,
> 
> See comment, questions and test results below;
> 
>> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
>> Sent: 18-Jul-15 1:50 AM
>>
>> Hello Vladimir,
>>
>> On Sat, 18 Jul 2015 02:10:01 +0300, Vladimir Zapolskiy <vz at mleia.com>
>> wrote:
>>> Hi Sylvain, Albert,
>>>
>>> On 18.07.2015 01:24, LEMIEUX, SYLVAIN wrote:
>>>> Hi Albert,
>>>>
>>>> Thanks for the feedback.
>>>>
>>>>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
>>>>> Sent: 17-Jul-15 5:20 PM
>>>>>
>>>>> Hello Sylvain,
>>>>>
>>>>> On Fri, 17 Jul 2015 16:48:52 -0400, slemieux.tyco at gmail.com
>>>>> <slemieux.tyco at gmail.com> wrote:
>>>>>
>>>>>>
> 
>  ....
> 
>>
>>> * What about functional support in SPL? Is it correct, that if I want to
>>> have this code in SPL, then I have to pull in DMA driver as a mandatory
>>> dependency to tiny SPL?
>>
>> You have to ensure DMA, but whether you pull the whole BSP driver or
>> hard-code the necessary parts into a single self-contained SLC NAND +
>> DMA file is a design decision. That said, I would personally go for
>> pulling the driver *and* adding preprocessing out any part of it not
>> necessary for SPL -- not so much for size (the compiler and linker
>> should optimize useless parts away on their own anyway) than for clarity
>> and maintenance (readers of the driver code would know which part is
>> needed for SPL and which is not).
>>
> 
> I am in the process of putting a patch together to add the hardware ECC
> support on top of the SLC NAND driver patch
> (https://patchwork.ozlabs.org/patch/497308/).
> 
> A have a few questions:
> 1) The LPC32xx SLC NAND driver is using a custom NAND ECC Layout for small page;
>     You can refer to the Kernel driver:
>      https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/mtd/nand/lpc32xx_slc.c?id=refs/tags/v4.1.3
> 
>     Are you planning to update your driver with the change or should I submit a separate
>     patch for it before adding the support for DMA and hardware ECC?

if you don't object to rebase your changes on top of mine (see my
closing comment), I'm fine, if custom NAND ECC Layout for small page
chip is added in a separate patch. For your information on my side I
won't be able to test it though.

> 2) As suggested by Albert, I will add a conditional compile option to ensure the
>      original version of the driver (no DMA) can be used for SPL.

Great, thank you.

>     I was planning to do it using the configuration option use to select the LPC32xx
>     DMA driver (CONFIG_DMA_LPC32XX). Are you OK with this approach?

I'm not sure, if DMA is really needed in SPL binary, but if you want to
add this option, I don't object. To separate code for SPL and normal
image please use CONFIG_SPL_BUILD macro.

> 3) Should I separate the NAND SLC /DMA & the USB into 2 separate patch?

I would say DMA driver as a prerequisite should go first, then in
arbitrary order NAND SLC updates specific to available/compiled DMA and
USB changes.

> 
> Test result (full command log below):
> 
> Clock configuration:
> CPU clock: 266MHz / AHB bus clock: 133MHz / Peripheral clock: 13MHz
> 
> 1) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
>      NAND read:  51904512 bytes
>      read.raw: 1.444 MiB per second / read: 0.625 MiB per second
> 
> 2) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
>      & Legacy BSP porting (hardware ECC & DMA)
>      NAND read:  51904512 bytes
>      read.raw: 2.272 MiB per second / read: 2.139 MiB per second
> 
> ------------------
> Full log:
> 
> NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
> 
> ==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime
> Timer val: 259607
> Seconds : 259
> Remainder : 607
> sys_hz = 1000
> 
> NAND read:  51904512 bytes read: OK
> Timer val: 293885
> Seconds : 293
> Remainder : 885
> sys_hz = 1000
> --> 1.444 MiB per second
> 
> ==> gettime; nand read.e 0x80000000 0xd00000 0x3180000; gettime
> Timer val: 11304
> Seconds : 11
> Remainder : 304
> sys_hz = 1000
> 
> NAND read: device 0 offset 0xd00000, size 0x3180000
>  51904512 bytes read: OK
> Timer val: 90495
> Seconds : 90
> Remainder : 495
> sys_hz = 1000
> --> 0.625 MiB per second
> 
> NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
> & Legacy BSP porting (hardware ECC & DMA)
> 
> ==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime
> Timer val: 49705
> Seconds : 49
> Remainder : 705
> sys_hz = 1000
> 
> NAND read:  51904512 bytes read: OK
> Timer val: 71483
> Seconds : 71
> Remainder : 483
> sys_hz = 1000
> --> 2.272 MiB per second
> 
> Timer val: 280282
> Seconds : 280
> Remainder : 282
> sys_hz = 1000
> 
> NAND read: device 0 offset 0xd00000, size 0x3180000
>  51904512 bytes read: OK
> Timer val: 303423
> Seconds : 303
> Remainder : 423
> sys_hz = 1000
> --> 2.139 MiB per second
> 

Thank you for tests. So, as expected if DMA and hardware ECC is enabled,
then the performance is 1.5 (raw read) or 3.5 times (read & ecc) better.

I do recognize and appreciate this work, advocating my change I can
repeat my last arguments:
* it has a ready valid user in U-boot, my change is not a dead code,
* it is more functional -- read from NAND in SPL is supported,
* it is extremely simple (180 LoC vs. 500 LoC + ~200 LoC of DMA driver),
* it has been reviewed by Scott, all review comments are fixed and
published in v4, and it is ready for inclusion to the next release IMHO.

If there is no more review comments to my version of the driver, I would
kindly ask you to rebase legacy SLC NAND driver on top of my version of
the driver, it is a doable and simple task in my opinion, performance
optimization with the associated code complexity may be added later on.

--
With best wishes,
Vladimir