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@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