From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Date: Fri, 03 Oct 2008 19:08:41 +0200 Subject: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support In-Reply-To: <20081003165203.GA11156@ld0162-tx32.am.freescale.net> References: <48e5f6a5.0305560a.0a0e.ffffe071@mx.google.com> <20081003165203.GA11156@ld0162-tx32.am.freescale.net> Message-ID: <48E65199.20806@googlemail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Scott, many thanks for the review! As this code is directly taken from some TI code, it seems I have to find somebody who can answer your questions and rework the code now. Will do so now. Unfortunately, I don't know a lot about NAND. Thanks Dirk Scott Wood wrote: > On Fri, Oct 03, 2008 at 12:40:25PM +0200, dirk.behme at googlemail.com wrote: > >>+#include >>+#include >>+#include >>+#include >>+ >>+#if defined(CONFIG_CMD_NAND) >>+ >>+#include > > > Move the #ifdef to the Makefile. > > >>+/* >>+ * nand_read_buf16 - [DEFAULT] read chip data into buffer >>+ * @mtd: MTD device structure >>+ * @buf: buffer to store date >>+ * @len: number of bytes to read >>+ * >>+ * Default read function for 16bit buswith >>+ */ >>+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len) >>+{ >>+ int i; >>+ struct nand_chip *this = mtd->priv; >>+ u16 *p = (u16 *) buf; >>+ len >>= 1; >>+ >>+ for (i = 0; i < len; i++) >>+ p[i] = readw(this->IO_ADDR_R); >>+} > > > How does this differ from the default implementation? > > >>+static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, >>+ int len) >>+{ >>+ int i; >>+ int j = 0; >>+ struct nand_chip *chip = mtd->priv; >>+ >>+ for (i = 0; i < len; i++) { >>+ writeb(buf[i], chip->IO_ADDR_W); >>+ for (j = 0; j < 10; j++) ; >>+ } >>+ >>+} >>+ >>+/* >>+ * omap_nand_read_buf - read data from NAND controller into buffer >>+ * @mtd: MTD device structure >>+ * @buf: buffer to store date >>+ * @len: number of bytes to read >>+ * >>+ */ >>+static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) >>+{ >>+ int i; >>+ int j = 0; >>+ struct nand_chip *chip = mtd->priv; >>+ >>+ for (i = 0; i < len; i++) { >>+ buf[i] = readb(chip->IO_ADDR_R); >>+ while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS) & GPMC_BUF_FULL)); >>+ } >>+} > > > I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL] > when writing, but with 8-bit NAND, you check it when reading? And 8-bit > writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads > have no delay at all? > > >>+static void omap_hwecc_init(struct nand_chip *chip) >>+{ >>+ unsigned long val = 0x0; >>+ >>+ /* Init ECC Control Register */ >>+ /* Clear all ECC | Enable Reg1 */ >>+ val = ((0x00000001 << 8) | 0x00000001); >>+ __raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL); >>+ __raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG); >>+} > > > Why raw? > > >>+/* >>+ * omap_correct_data - Compares the ecc read from nand spare area with >>+ * ECC registers values >>+ * and corrects one bit error if it has occured >>+ * @mtd: MTD device structure >>+ * @dat: page data >>+ * @read_ecc: ecc read from nand flash >>+ * @calc_ecc: ecc read from ECC registers >>+ */ >>+static int omap_correct_data(struct mtd_info *mtd, u_char *dat, >>+ u_char *read_ecc, u_char *calc_ecc) >>+{ >>+ return 0; >>+} > > > This obviously is not correcting anything. If the errors are corrected > automatically by the controller, say so in the comments. > > >>+/* >>+ * omap_calculate_ecc - Generate non-inverted ECC bytes. >>+ * >>+ * Using noninverted ECC can be considered ugly since writing a blank >>+ * page ie. padding will clear the ECC bytes. This is no problem as >>+ * long nobody is trying to write data on the seemingly unused page. >>+ * Reading an erased page will produce an ECC mismatch between >>+ * generated and read ECC bytes that has to be dealt with separately. > > > Is this a hardware limitation? If so, say so in the comment. If not, > why do it this way? > > >>+ * @mtd: MTD structure >>+ * @dat: unused >>+ * @ecc_code: ecc_code buffer >>+ */ >>+static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat, >>+ u_char *ecc_code) >>+{ >>+ unsigned long val = 0x0; >>+ unsigned long reg; >>+ >>+ /* Start Reading from HW ECC1_Result = 0x200 */ >>+ reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT); >>+ val = __raw_readl(reg); >>+ >>+ *ecc_code++ = ECC_P1_128_E(val); >>+ *ecc_code++ = ECC_P1_128_O(val); >>+ *ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val) << 4; > > > These macros are used nowhere else; why obscure what it's doing by moving > it to the top of the file? > > >>+static void omap_enable_hwecc(struct mtd_info *mtd, int mode) >>+{ >>+ struct nand_chip *chip = mtd->priv; >>+ unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG); >>+ unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1; >>+ >>+ switch (mode) { >>+ case NAND_ECC_READ: >>+ __raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL); >>+ /* ECC col width | CS | ECC Enable */ >>+ val = (dev_width << 7) | (cs << 1) | (0x1); >>+ break; >>+ case NAND_ECC_READSYN: >>+ __raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL); >>+ /* ECC col width | CS | ECC Enable */ >>+ val = (dev_width << 7) | (cs << 1) | (0x1); >>+ break; >>+ case NAND_ECC_WRITE: >>+ __raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL); >>+ /* ECC col width | CS | ECC Enable */ >>+ val = (dev_width << 7) | (cs << 1) | (0x1); >>+ break; >>+ default: >>+ printf("Error: Unrecognized Mode[%d]!\n", mode); >>+ break; >>+ } >>+ >>+ __raw_writel(val, GPMC_BASE + GPMC_ECC_CONFIG); >>+} > > > Is it OK if config gets written before control, or if this whole thing > gets done out of order with respect to other raw writes? > > >>+static struct nand_ecclayout hw_nand_oob_64 = { >>+ .eccbytes = 12, >>+ .eccpos = { >>+ 2, 3, 4, 5, >>+ 6, 7, 8, 9, >>+ 10, 11, 12, 13}, >>+ .oobfree = { {20, 50} } /* don't care */ > > > Bytes 64-69 of a 64-byte OOB are free? > What don't we care about? > > >>+ if (nand->options & NAND_BUSWIDTH_16) { >>+ mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 2); >>+ if (nand->ecc.layout->eccbytes & 0x01) >>+ mtd->oobavail--; >>+ } else >>+ mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 1); >>+} > > > oobavail is calculated by the generic NAND code. You don't need to do it > here. > > >>+/* >>+ * Board-specific NAND initialization. The following members of the >>+ * argument are board-specific (per include/linux/mtd/nand_new.h): >>+ * - IO_ADDR_R?: address to read the 8 I/O lines of the flash device >>+ * - IO_ADDR_W?: address to write the 8 I/O lines of the flash device >>+ * - hwcontrol: hardwarespecific function for accesing control-lines >>+ * - dev_ready: hardwarespecific function for accesing device ready/busy line >>+ * - enable_hwecc?: function to enable (reset) hardware ecc generator. Must >>+ * only be provided if a hardware ECC is available >>+ * - eccmode: mode of ecc, see defines >>+ * - chip_delay: chip dependent delay for transfering data from array to >>+ * read regs (tR) >>+ * - options: various chip options. They can partly be set to inform >>+ * nand_scan about special functionality. See the defines for further >>+ * explanation >>+ * Members with a "?" were not set in the merged testing-NAND branch, >>+ * so they are not set here either. > > > IO_ADDR_R and IO_ADDR_W have a "?" but are set here. If you have a > question about the API, ask it on the list, rather than encoding it in > the source. > > >>=================================================================== >>--- u-boot-arm.orig/drivers/mtd/nand/nand_base.c >>+++ u-boot-arm/drivers/mtd/nand/nand_base.c >>@@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd) >> return 0; >> } >> >>+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \ >>+ || defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO)) >>+void nand_switch_ecc(struct mtd_info *mtd) > > > NACK. First, explain why you need to be able to dynamically switch ECC > modes. > > Then, if it is justified, implement it in a separate patch, without all > the duplication of code, and without platform-specific #ifdefs. > > -Scott >