Hi Vladimir, See comment below.
> From: Vladimir Zapolskiy [mailto:v...@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.b...@aribaud.net] > >> Sent: 18-Jul-15 1:50 AM > >> > >> Hello Vladimir, > >> > >> On Sat, 18 Jul 2015 02:10:01 +0300, Vladimir Zapolskiy <v...@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-boun...@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.t...@gmail.com > >>>>> <slemieux.t...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
