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, [EMAIL PROTECTED] 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
>
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
