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

[LEMIEUX, SYLVAIN]

Hi Vladimir,

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

DMA can be the first patch of the series.
No problem, I can try to add the support for hardware ECC and DMA inside
your driver. However, I will not be able to this until the following week.

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

We did the porting of the legacy NXP BSP driver internally, as we are using it
on a custom LPC32xx board running u-boot.

As LPC32xx driver became available in u-boot (thanks to Albert and Vladimir),
we start using them (I2C, Ethernet, GPIO). After migrating to those drivers,
we did the porting of the remaining legacy drivers to the latest u-boot version.

The intent of those patches was to bring the remaining legacy drivers,
not wet available in u-boot.

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

We are not using the SPL;
may be the SLC NAND driver can use the DMA only for non-SPL build.

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

________________________________

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.