[U-Boot] [PATCH v3] Freescale NFC NAND driver

[Scott Wood]

John Rigby wrote:
> My only concern is that the u-boot and linux nand drivers need to have the
> same approach regarding ecc.  The linux driver recently submitted only
> supports sw ecc because using hw ecc means the spare area is not writeable.
> The u-boot driver that I submitted supported hw_ecc only and was compatible
> with the linux driver in ltib.  Unusable spare is an issue for JFFS2 but not
> UBIFS.  If I were to make the decision I would just say not to JFFS2 on
> NAND.  UBIFS is a far better solution for NAND anyway.

How much of a performance hit is the sw ecc?  We should at least support it as 
an option, and probably by default if that's what Linux does.

> I have seen at least one other controller where the controller included the
> spare in the ECC so I don't know if this is a trend or not.

I hope not. :-(

> On Thu, Jun 4, 2009 at 7:18 AM, Stefan Roese <sr at denx.de> wrote:
> 
>> Hi Scott,
>>
>> I'll try to continue with this patch so that we can integrate it hopefully
>> soon. I already addressed some of your comments (the easy ones ;)). Please
>> find some further questions below (I'm still new to the FSL NFC):

Thanks!

>> On Thursday 06 November 2008 00:06:48 Scott Wood wrote:
>>>> +static struct fsl_nfc_private {
>>>> +   struct mtd_info mtd;
>>>> +   char spare_only;
>>>> +   char status_req;
>>>> +   u16 col_addr;
>>>> +   int writesize;
>>>> +   int sparesize;
>>>> +   int width;
>>>> +   int chipsel;
>>>> +} *priv;
>>> Is it plausable that there will ever be a chip with more than one of
>>> these controllers?
>> I have no idea. What do you suggest I should change here?

Remove the "*priv" at the end and use mtd->priv instead.

>>>> +#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
>>>> +static void fsl_nfc_select_chip(u8 cs)
>>>> +{
>>>> +   u32 val = NFC_READW(NFC_BUF_ADDR);
>>>> +
>>>> +   val &= ~0x60;
>>>> +   val |= cs << 5;
>>>> +   NFC_WRITEW(NFC_BUF_ADDR, val);
>>>> +}
>>>> +#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
>>>> +#endif
>>>> +
>>>> +
>>>> +/*!
>>>> + * This function is used by upper layer for select and deselect of the
>>>> NAND + * chip
>>>> + *
>>>> + * @mtd            MTD structure for the NAND Flash
>>>> + * @chip   val indicating select or deselect
>>>> + */
>>>> +static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)
>>> This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is
>>> not defined.
>> Works fine here on a board without CONFIG_FSL_NFC_BOARD_CS_FUNC defined. So
>> I'll leave it as is.

But there are two definitions of fsl_nfc_select_chip in that case... or am I 
missing something?

>>>> +   case NAND_CMD_READOOB:
>>>> +           priv->col_addr = column;
>>>> +           priv->spare_only = 1;
>>>> +           command = NAND_CMD_READ0;       /* only READ0 is valid */
>>>> +           break;
>>> What about small-page flash that takes an actual READOOB command?
>> I don't have access to a board with small-page NAND. So I can't test
>> anything
>> here.

Still, better to have code that might work than code that will definitely break. :-)

Alternatively, make it obvious that the driver does not support small-page.

>>>> +/*
>>>> + * These are identical to the generic versions except
>>>> + * for the offsets.
>>>> + */
>>>> +static struct nand_bbt_descr bbt_main_descr = {
>>>> +   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
>>>> +       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
>>>> +   .offs = 0,
>>>> +   .len = 4,
>>>> +   .veroffs = 4,
>>>> +   .maxblocks = 4,
>>>> +   .pattern = bbt_pattern
>>>> +};
>>>> +
>>>> +static struct nand_bbt_descr bbt_mirror_descr = {
>>>> +   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
>>>> +       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
>>>> +   .offs = 0,
>>>> +   .len = 4,
>>>> +   .veroffs = 4,
>>>> +   .maxblocks = 4,
>>>> +   .pattern = mirror_pattern
>>>> +};
>>> This will overlap the bad block marker on large-page flash.
>> Good catch. Do you have any idea how can this be solved?

Change the offset. :-)

Perhaps with different offsets for small and large page.

-Scott