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

Tue Jul 28 18:28:49 CEST 2015

Hi Vladimir,

See comment below.

> From: Vladimir Zapolskiy [mailto:vz at mleia.com]
> Sent: 27-Jul-15 8:22 PM
>
> 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.

Already done, I will update the change based on the comment bellow
and submit a second version of the patch;

The test result, listed below, are with the NXP legacy BSP code on top
of your patch.

I will add a patch from the small page change; the change are taken
from the legacy NXP BSP and the mapping is matching the kernel driver.

Who will provide feedback on the DMA patch?

>
> > 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

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.