Re: [U-Boot] [PATCH v2 10/17] SPEAr : FSMC driver support added

...

• • Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com.
• • See file CREDITS for list of people who contributed to this
• • project.
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#include <common.h> +#include <nand.h> +#include <asm/io.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/mtd/nand_ecc.h> +#include <linux/mtd/fsmc_nand.h> +#include <asm/arch/hardware.h>

+static u32 fsmc_version; +static struct fsmc_regs *const fsmc_regs_p =

• (struct fsmc_regs *)CONFIG_SYS_FSMC_BASE;

CONFIG_SYS_FSMC_BASE is not defined until 11/17 This make this change non-bisectable. Fix.

Since this driver is implemented independent of the spear platform, and it becomes the responsibility of the platform using this driver to #define this macro before using it. Should I do it some other way ?

...

• i = 0;
• while (num_err--) {

•    change_bit(0, &err_idx[i]);
  

•    change_bit(1, &err_idx[i]);
  

•    if (err_idx[i] <= 512 * 8) {
  

•        change_bit(err_idx[i], dat);
  

•        i++;
  

•    }
  

• }
• return i;

+}

+static int fsmc_read_hwecc(struct mtd_info *mtd,

•            const u_char *data, u_char *ecc)
  

+{

• u_int ecc_tmp;
• switch (fsmc_version) {
• case FSMC_VER8:

•    while (!(readl(&fsmc_regs_p->sts) & FSMC_CODE_RDY))
  

•        ;
  

Add a comment that this is a busy wait.

OK.

...

• uint16_t ecc_oob[7];
• for (i = 0, s = 0; s < eccsteps; s++, i += eccbytes, p += eccsize) {

•    chip->cmdfunc(mtd, NAND_CMD_READ0, s * eccsize, page);
  

•    chip->ecc.hwctl(mtd, NAND_ECC_READ);
  

•    chip->read_buf(mtd, p, eccsize);
  

•    for (j = 0; j < eccbytes;) {
  

•        off = fsmc_eccpl.eccplace[s].offset;
  

•        len = fsmc_eccpl.eccplace[s].length;
  

•        /*
  

•         * length is intentionally kept a higher multiple of 2
  

•         * to read at least 13 bytes even in case of 16 bit NAND
  

•         * devices
  

•         */
  

•        len = roundup(len, 2);
  

•        chip->cmdfunc(mtd, NAND_CMD_READOOB, off, page);
  

•        chip->read_buf(mtd, (uint8_t *)&ecc_oob[j], len);
  

•        j += len;
  

•    }
  

•    memcpy(&ecc_code[i], ecc_oob, 13);
  

•    chip->ecc.calculate(mtd, p, &ecc_calc[i]);
  

•    stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
  

•    if (stat < 0)
  

•        mtd->ecc_stats.failed++;
  

•    else
  

•        mtd->ecc_stats.corrected += stat;
  

• }
• return 0;

+}

+int fsmc_nand_init(struct nand_chip *nand) +{

• u32 peripid2 = readl(&fsmc_regs_p->peripid2);
• fsmc_version = (peripid2 >> FSMC_REVISION_SHFT) &

•           FSMC_REVISION_MSK;
  

+#if defined(CONFIG_SYS_FSMC_NAND_16BIT)

16BIT is never defined This is dead code Remove

Since this is a generic driver, any platform using fsmc peripheral can enable this feature by #defining CONFIG_SYS_FSMC_NAND_16BIT in respective include/configs/xxx.h

All other comments are accepted and would be fixed in v3

Thanks and Regards Vipin