[U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

[Dirk Behme <dirk.behme@googlemail.com>]

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 <common.h>
>>+#include <asm/io.h>
>>+#include <asm/arch/mem.h>
>>+#include <linux/mtd/nand_ecc.h>
>>+
>>+#if defined(CONFIG_CMD_NAND)
>>+
>>+#include <nand.h>
> 
> 
> 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
>