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

[Vladimir Zapolskiy]

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:
>>
>>> 1) Fixed checkpatch script output in legacy code.
>>>    A single warning remaining.
>>
>>> The following warning from the legacy code is still present:
>>> lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
>>
>>> +static u_char lpc32xx_read_byte(struct mtd_info *mtd)
>>> +{
>>> +   struct nand_chip *this = mtd->priv;
>>> +   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
>>> +   volatile unsigned long tmp32;
>>> +   tmp32 = *preg;
>>> +   return (u_char)tmp32;
>>> +}
>>
>> The volatile above has no reason to exist; the warning is justified
>> here as we have accessors that guarantee that the access will not be
>> optimized away or reordered, juste like the 'volatile' above tries to
>> do (and yes, these accessors *use* 'volatile'. All the more a reason
>> not to use it again here).
>>
>> Besides, the code is quite verbose and not precise enough. Yes,
>> 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit,
>> that is explicit.
>>
>> All in all, the whole function could be expressed as:
>>
>>       static u_char lpc32xx_read_byte(struct mtd_info *mtd)
>>       {
>>               struct nand_chip *this = mtd->priv;
>>
>>               return (u_char)readl(this->IO_ADDR_R);
>>       }
>>
>> BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data
>> register 16-bits? And if so, then why the 32-bits read?
>>
> 
> The register is 16 bits; this implementation is the porting of the initial code.

hmm, you remember it was discussed yesterday that the data register is
32-bit...

----8<----
5.2 Data FIFO

There is only one Data FIFO. The Data FIFO is configured either in Read
or in Write mode.

1. When the Data FIFO is configured in Read mode, the sequencer reads
data from the NAND flash, and stores the data in the Data FIFO. The FIFO
is then emptied by 32-bit reads on the AHB bus from either the ARM or
the DMA.

2. When the Data FIFO is configured in Write mode, the ARM or the DMA
writes data to the FIFO with 32-bit AHB bus writes. The sequencer then
takes data out of the FIFO 8 bits at a time, and writes data to the NAND
flash.

----8<----

> I will wait for feedback and see how we want to approach this
> (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or
> update the driver as part of the porting effort).

now when I see the code I still haven't changed my opinion, I would
propose to add HW ECC processing on top of my trivial change.

Some general reasons:

* I agree with Albert that the code is a bit overcomplicated and can be
improved, basic functions like read_byte(), cmd_ctrl() etc are better in
my version IMHO --- for example just compare Kevin's monstrous
lpc32xx_nand_hwcontrol() and my lpc32xx_nand_cmd_ctrl() --- so if my
version is reviewed and accepted, then there is no need to fix all the
same issues in this legacy forward-ported code,

* this change strictly depends on DMA driver (the driver simply does not
work, if DMA is disabled), this means that DMA driver must be 1/3 and
SLC NAND should go as 2/3, this implies that DMA driver is reviewed and
accepted by maintainers firstly,

* I don't see any users of this new code, this addresses Albert's notice
about adding dead code ---
http://lists.denx.de/pipermail/u-boot/2015-July/219124.html

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

>> Also, I did not find where the IO_ADDR_R field is assigned. Did I miss
>> it?
> 
> "CONFIG_SYS_NAND_SELF_INIT" is not define; the legacy BSP driver is using the
> default initialization done within "nand_init_chip()" function inside "drivers/mtd/nand/nand.c".
> 
>>
>> This is just one case, but I suspect in many places, the code is
>> unnecessarily complex. I understand it was ported, not written from
>> scratch, but I think porting code should not prevent us from making it
>> smaller, more efficient and more maintainable.
>>
>> Amicalement,
>> --
>> Albert.
>>
> 
> Sylvain
>