[U-Boot] [PATCH 08/13 v3] ARM: OMAP3: Add NAND support

[Dirk Behme]

Scott Wood wrote:
> dirk.behme at googlemail.com wrote:
> 
>> +unsigned char cs;
>> +volatile unsigned long gpmc_cs_base_add;
> 
> 
> Make these static.  gpmc_cs_base_add should be a pointer, not "unsigned 
> long".  Volatile isn't needed since you use I/O accessors, and 
> definitely isn't needed on the address itself.

Changed in attachment.

>> +/*
>> + * omap_nand_hwcontrol - Set the address pointers corretly for the
>> + *            following address/data/command operation
>> + */
>> +static void omap_nand_hwcontrol(struct mtd_info *mtd, int cmd,
>> +                unsigned int ctrl)
>> +{
>> +    register struct nand_chip *this = mtd->priv;
>> +
>> +    /* Point the IO_ADDR to DATA and ADDRESS registers instead
>> +       of chip address */
>> +    switch (ctrl) {
>> +    case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
>> +        this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_CMD;
>> +        this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
>> +        break;
>> +    case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
>> +        this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_ADR;
>> +        this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
>> +        break;
>> +    case NAND_CTRL_CHANGE | NAND_NCE:
>> +        this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
>> +        this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
>> +        break;
>> +    }
> 
> 
> IO_ADDR_R never seems to change; you can leave it out of here and 
> omap_nand_wait.

Yes, thanks. Removed in attachment.

>> +/*
>> + * omap_nand_wait - called primarily after a program/erase operation
>> + *            so that we access NAND again only after the device
>> + *            is ready again.
>> + * @mtd:        MTD device structure
>> + * @chip:    nand_chip structure
>> + */
>> +static int omap_nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>> +{
>> +    register struct nand_chip *this = mtd->priv;
>> +    int status = 0;
>> +
>> +    this->IO_ADDR_W = (void *) gpmc_cs_base_add + GPMC_NAND_CMD;
>> +    this->IO_ADDR_R = (void *) gpmc_cs_base_add + GPMC_NAND_DAT;
>> +    /* Send the status command and loop until the device is free */
>> +    while (!(status & 0x40)) {
>> +        writeb(NAND_CMD_STATUS & 0xFF, this->IO_ADDR_W);
>> +        status = readb(this->IO_ADDR_R);
>> +    }
> 
> 
> Maybe should just do this, to avoid changing client-visible state:
> writeb(NAND_CMD_STATUS, &gpmc_cs_base_add[GPMC_NAND_CMD]);
> 
> No need for the "& 0xFF".

Modified.

>> +    /* Init ECC Control Register */
>> +    /* Clear all ECC  | Enable Reg1 */
>> +    val = ((0x00000001 << 8) | 0x00000001);
>> +    writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
>> +    writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
> 
> 
> Symbolic constants for the bit values would be nice.

Added. Hope you like them ;)

>> +/*
>> + *  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.
> 
> 
> Where is it dealt with separately?

We already talked about this and extended the comment. To my 
understanding this special handling can't be done in 
omap_calculate_ecc() as it is called from generic NAND code and 
doesn't know if ECC it calculates is correct or not?

Do you have any proposals where and how to handle this?

Any propsals from OMAP experts?

Easiest short term solution would be to add a "FIXME" to comment?

>> +    unsigned long val = 0x0;
> 
> 
> Unnecessary initialization.

Removed.

>> +    unsigned long reg;
>> +
>> +    /* Start Reading from HW ECC1_Result = 0x200 */
>> +    reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
>> +    val = readl(reg);
> 
> 
> readl() takes a pointer.  ARM gets away without a warning here because 
> it uses macros rather than inline functions, but it's bad practice.

Helper variable removed.

>> +    /* Stop reading anymore ECC vals and clear old results
>> +     * enable will be called if more reads are required */
>> +    reg = (unsigned long) (GPMC_BASE + GPMC_ECC_CONFIG);
>> +    writel(0x000, reg);
> 
> 
> Likewise.

Ditto.

>> +void omap_nand_switch_ecc(int hardware)
>> +{
>> +    struct nand_chip *nand;
>> +
>> +    if (nand_curr_device < 0 ||
>> +        nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
>> +        !nand_info[nand_curr_device].name) {
>> +        printf("Error: Can't switch ecc, no devices available\n");
>> +        return;
>> +    }
>> +
>> +    nand = (&nand_info[nand_curr_device])->priv;
>> +
>> +    if (!hardware) {
>> +        nand->ecc.mode = NAND_ECC_SOFT;
>> +        nand->ecc.layout = &sw_nand_oob_64;
>> +        nand->ecc.size = 256;    /* set default eccsize */
>> +        nand->ecc.bytes = 3;
>> +        nand->ecc.steps = 8;
>> +        nand->ecc.hwctl = 0;
>> +        nand->ecc.calculate = nand_calculate_ecc;
>> +        nand->ecc.correct = nand_correct_data;
>> +    } else {
>> +        nand->ecc.mode = NAND_ECC_HW;
>> +        nand->ecc.layout = &hw_nand_oob_64;
>> +        nand->ecc.size = 512;
>> +        nand->ecc.bytes = 3;
>> +        nand->ecc.steps = 4;
>> +        nand->ecc.hwctl = omap_enable_hwecc;
>> +        nand->ecc.correct = omap_correct_data;
>> +        nand->ecc.calculate = omap_calculate_ecc;
>> +        omap_hwecc_init(nand);
>> +    }
> 
> Do you need to do anything similar to omap_hwecc_init() when switching 
> to SW ECC to tell the hardware to stop doing ECC?

I don't think so, but maybe the experts can help here.

Comment in omap_calculate_ecc() tells us:

/* Stop reading anymore ECC vals and clear old result, enable will be 
called if more reads are required */

Sounds like an "auto stop and re-init if needed" (in omap_enable_hwecc()).

To summarize: If you agree with changes in attachment, last open point 
is the "ECC mismatch" issue. Do you agree?

Thanks for re-review!

Dirk
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 08_omap3_nand.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081010/2da0dbef/attachment-0001.txt