Hi Vladimir, > From: Vladimir Zapolskiy [mailto:v...@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-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<---- > > > 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot