[U-Boot] [PATCH] nand: lpc32xx: add SLC NAND controller support

[LEMIEUX, SYLVAIN]

Hi Vladimir,

Thanks for taking time to read my feedback.
You can see my comments and my answer below.

Sylvain

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vz at mleia.com]
> Sent: 15-Jul-15 8:20 PM
> To: LEMIEUX, SYLVAIN; Albert ARIBAUD
> Cc: Scott Wood; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] nand: lpc32xx: add SLC NAND controller support
>
> Hi Sylvain,
>
> On 15.07.2015 22:23, LEMIEUX, SYLVAIN wrote:
> > Hi Vladimir and Albert,
> >
> > During this merge window (once our issues with our exchange server are
> resolve), we were planning on submitting a few patches for the LPC32xx.
>
> great, feel free to add me to Cc.

Will do;

>
> > Some of the patches are the porting of the legacy NXP BSP (u-boot) drivers
> into the latest version; the drivers are the DMA, the SLC NAND and the USB.
>
> If DMA and USB are added, I'll gratefully reuse this on my board :)

I will submit the LPC32xx patches using an alternate e-mail for now, until the problem with our e-mail infrastructure is resolve.

First, I need to do some rework (matching the naming convention of your NAND SLC patch and update our porting effort based on the feedback from Albert).

>
> > This original NXP implementation of the SLC NAND was using the DMA. I am
> also planning on testing this patch to compare the flashing time, with and
> without the DMA.
>
> Sounds good. Also since DMA is going to be supported it would be nice to
> add HW ECC calculation to the SLC NAND driver.

Hardware ECC was already supported in the legacy BSP; it will be part of the patches I will submit.

>
> FYI here are performance test results of my PIO version:
>
>   => gettime; nand read.raw 0x80000000 0x0 0x6000; gettime
>   Timer val: 63952
>   Seconds : 63
>   Remainder : 952
>   sys_hz = 1000
>
>   NAND read:  51904512 bytes read: OK
>   Timer val: 113352
>   Seconds : 113
>   Remainder : 352
>   sys_hz = 1000
>
>
> 1.002 MiB per second, quite slow, but not drastically slow.

FYI, I did the same testing on my side using the legacy NXP BSP implementation;
the test was done with the CPU clock at 208MHz and 266MHz.

For those test, we have no timing optimization for the SLC NAND.

Clock configuration:
CPU clock: 266MHz / AHB bus clock: 133MHz / Peripheral clock: 13MHz
==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime
Timer val: 22949
Seconds : 22
Remainder : 949
sys_hz = 1000

NAND read:  51904512 bytes read: OK
Timer val: 44803
Seconds : 44
Remainder : 803
sys_hz = 1000
--> 2.265 MiB per second

==> gettime; nand read.e 0x80000000 0xd00000 0x3180000; gettime
Timer val: 66054
Seconds : 66
Remainder : 54
sys_hz = 1000

NAND read: device 0 offset 0xd00000, size 0x3180000
 51904512 bytes read: OK
Timer val: 89214
Seconds : 89
Remainder : 214
sys_hz = 1000
--> 2.137 MiB per second

Clock configuration:
CPU clock: 208MHz / AHB bus clock: 104MHz / Peripheral clock: 13MHz

==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime
Timer val: 24605
Seconds : 24
Remainder : 605
sys_hz = 1000

NAND read:  51904512 bytes read: OK
Timer val: 52458
Seconds : 52
Remainder : 458
sys_hz = 1000
--> 1.777 MiB per second

==> gettime; nand read.e 0x80000000 0x00d00000 0x3180000; gettime
Timer val: 134819
Seconds : 134
Remainder : 819
sys_hz = 1000

NAND read: device 0 offset 0xd00000, size 0x3180000
 51904512 bytes read: OK
Timer val: 164465
Seconds : 164
Remainder : 465
sys_hz = 1000
--> 1.669 MiB per second

>
> > I have two questions:
> > 1) How do you suggest to approach this, as some patches may be similar or
> conflicting with what Vladimir is planning on submitting?
>
> I presume the only conflicting place is SLC NAND driver.

Yes, this will be the only conflicting patch.

>
> Here I see some benefits of my version:
>
> * the driver is very tiny, practically it is read_buf()/write_buf() and timing
> configuration, all the rest I managed to offload to existing mtd/nand and
> spl/nand frameworks at the price of more added CONFIG_* defines in a
> board header file,
> * not sure what OOB layout is coming from NXP BSP (I don't have this BSP to
> check, unfortunately), but I would prefer to see the same OOB layout in U-
> boot and in vanilla Linux --- this is done in my version,
> * the driver can be included to SPL binary,
> * the driver is well tested on my environment,
> * the code has been published for review.

This is the benefits (I am thinking we get) from the legacy NXP BSP porting:
* The driver went through multiple iteration (the latest version of the legacy patch was 1.07).
* The BSP, from LPC Linux, was most likely review and tested by multiple users; it was the initial u-boot reference for the LPC32xx development boards.
* The SLC NAND  implementation is integrated with the DMA, and already support hardware ECC.
* The OOB layout from the legacy BSP is matching the LPC32xx NAND SLC Linux driver.

>
> The only two missing things from the driver I see at the moment are based
> on working DMA driver:
> * data transfer by means of DMA,
> * HW ECC calculation (data correction is always done by software).
>
> Also my driver has not been tested with small page NAND chips, not sure, if it
> is relevant for you.

The legacy BSP driver was only tested with large page NAND on our side.

>
> If DMA works, I hope it should be easy to add some lpc32xx_chip.ecc.*
> callbacks to my version of the driver.
>
> > 2) For submitting legacy NXP BSP driver porting patch, would you like to see
> a 3 patches series (original driver, checkpatch script fix and the update for
> latest u-boot) to have history of the change or a single patch with the final
> result?
> >
>
> If it were related to Linux kernel project, I know the clear answer, but please
> let me leave U-boot maintenance specifics to be explained by Albert.
>
> --
> 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.