Hi Vladimir,
From: Vladimir Zapolskiy [mailto:vz@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@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@gmail.com slemieux.tyco@gmail.com wrote:
- 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.
- 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.
- 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.