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

Thu Jul 16 02:19:48 CEST 2015

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.

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

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

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.

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

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.

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.

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

> 
> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
> Sent: 15-Jul-15 5:21 AM
> To: Vladimir Zapolskiy
> Cc: Scott Wood; Albert ARIBAUD; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] nand: lpc32xx: add SLC NAND controller support
> 
> Hello Vladimir,
> 
> On Wed, 15 Jul 2015 11:49:01 +0300, Vladimir Zapolskiy <vz at mleia.com>
> wrote:
>> Hello Albert,
>>
>> On 15.07.2015 10:05, Albert ARIBAUD wrote:
>>> Hello Vladimir,
>>>
>>> On Tue, 14 Jul 2015 23:23:57 +0300, Vladimir Zapolskiy
>>> <vz at mleia.com>
>>> wrote:
>>>> The change adds support of LPC32xx SLC NAND controller.
>>>>
>>>> LPC32xx SoC has two different mutually exclusive NAND controllers
>>>> to communicate with single and multiple layer chips.
>>>>
>>>> This simple driver allows to specify NAND chip timings and defines
>>>> custom read_buf()/write_buf() operations, because access to 8-bit
>>>> data register must be 32-bit aligned.
>>>>
>>>> Support of hardware ECC calculation is not implemented (data
>>>> correction is always done by software), since it requires a working
>>>> DMA engine.
>>>>
>>>> The driver can be included to an SPL image.
>>>
>>> This is needed for an upcoming new board support patch, right?
>>
>> you are correct, I plan to extend current support of devkit3250 board.
>>
>>> If so, then I suggest you put together all patches for this new
>>> board in a single series. This will make it clear(er) you're not
>>> adding dead code here.
>>>
>>
>> I got the point, I will be able to complete board specific changes
>> today tonight and send them for review.
> 
> Thanks.
> 
>> Please let me ask you for one more advice, if I want to add
>> peripherals support on the board (by the way thank you for your
>> drivers) and SPL image building support, both changes touch defconfig
>> and board config header files. Should I split these changes into
>> separate ones or is one "board support extension" patch preferred?
> 
> A commit should ideally be a single, self-contained, logical change.
> 
> So I would say each driver addition should be one commit, and the SPL support addition should be its own commit, even though each of these commits touches the defconfig and header config files.
> 
> This has at least two benefits:
> 
> - each commit is simpler to review (and to design and test, too). If a
>   commit contains several logical changes, it is harder to sort out
>   which change(s) a given patch chunk is about.
> 
> - in case a change was applied to U-Boot and later proves to cause
>   an issue, then we can easily revert this change, and only
>   this change, by reverting its commit. If the commit contains
>   several change, then we cannot simply revert the commit, we need to
>   manually "patch out" the problematic change while keeping the others.
> 
>> --
>> With best wishes,
>> Vladimir
> 
> Amicalement,
> --
> Albert.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 
> ________________________________
> 
> 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.
> 