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

[Albert ARIBAUD]

Hello Vladimir,

On Sat, 18 Jul 2015 02:10:01 +0300, Vladimir Zapolskiy <vz at 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-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<----

This is about the width of the data FIFO bus transfers, not about the
width of the DATA register.

The width of the data register is defined in UM10326 rev. 1 chapter 9,
section 6.1, page 192/193.

While, for other registers only bits 0..7 are specified, for SLC_DATA,
described as "a 16-bit wide register", bits 0..15 are specified, with
bits 8..15 being described as "Reserved, user software should not write
ones to reserved bits. The value read from a reserved bit is not
defined".

This makes me interpret the 'word' in the statement "SLC_DATA must be
accessed as a word register, although only 8 bits of data are used
during a write or provided during a read" as being a 16-, not 32-bit
quantity.

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

Ok, so there is a slight misunderstanding: when I said which version
should be submitted was to be discussed between you and Sylvain, I
meant that you come out with a single proposal.

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

That's what patch series are for. Just submit the series and it should
be accepted as a whole and in order, or rejected as a whole (unless some
patch in it is really independent, in which case it can be applied
independently (and any subsequent version of the series will have one
less patch).

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

Actually, my notice about dead code is not that there should already
be a use for new code, it is that there must appear a use for new code
in the same patch series where the new code is introduced. I don't want
to see independent patches for the code and use, because the first one
might get applied and the second one dropped and then we have dead code.

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

Amicalement,
-- 
Albert.