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