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

[LEMIEUX, SYLVAIN]

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

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