Hello Vladimir, On Sat, 18 Jul 2015 02:10:01 +0300, Vladimir Zapolskiy <v...@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-boun...@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.t...@gmail.com > >> <slemieux.t...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot