[U-Boot] [PATCH 09/15] iMX28: Add GPMI NAND driver

[Scott Wood]

On 09/11/2011 11:06 PM, Marek Vasut wrote:
> +static void mxs_nand_return_dma_descs(struct mxs_nand_info *info)
> +{
> +	int i = info->desc_index;
> +	struct mxs_dma_desc *desc;
> +
> +	for (--i; i >= 0; i--) {

This is an awkward construct.

Why not just the usual:

for (i = 0; i < info->desc_index; i++)

> +		desc = info->desc[i];
> +		memset(desc, 0, sizeof(struct mxs_dma_desc));
> +		desc->address = (dma_addr_t)desc;
> +	}
> +
> +	info->desc_index = 0;
> +}
> +
> +static inline uint32_t mxs_nand_ecc_chunk_cnt(uint32_t page_data_size)
> +{
> +	return page_data_size / MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
> +}

No need for inline in .c files, GCC should take care of this automatically.

> +/*
> + * There are several places in this driver where we have to handle the OOB and
> + * block marks. This is the function where things are the most complicated, so
> + * this is where we try to explain it all. All the other places refer back to
> + * here.
> + *
> + * These are the rules, in order of decreasing importance:
> + *
> + * 1) Nothing the caller does can be allowed to imperil the block mark, so all
> + *    write operations take measures to protect it.
> + *
> + * 2) In read operations, the first byte of the OOB we return must reflect the
> + *    true state of the block mark, no matter where that block mark appears in
> + *    the physical page.
> + *
> + * 3) ECC-based read operations return an OOB full of set bits (since we never
> + *    allow ECC-based writes to the OOB, it doesn't matter what ECC-based reads
> + *    return).
> + *
> + * 4) "Raw" read operations return a direct view of the physical bytes in the
> + *    page, using the conventional definition of which bytes are data and which
> + *    are OOB. This gives the caller a way to see the actual, physical bytes
> + *    in the page, without the distortions applied by our ECC engine.

Hmm, I thought raw was just supposed to disable ECC, not change the
layout from what is used in normal operation.

> + * It turns out that knowing whether we want an "ECC-based" or "raw" read is not
> + * easy. When reading a page, for example, the NAND Flash MTD code calls our
> + * ecc.read_page or ecc.read_page_raw function. Thus, the fact that MTD wants an
> + * ECC-based or raw view of the page is implicit in which function it calls
> + * (there is a similar pair of ECC-based/raw functions for writing).

This is an issue for the eLBC driver as well -- we need to know whether
to enable or disable ECC before we begin the operation (in cmdfunc), not
just at the time the buffer is accessed.  Currently we just always have
ECC on, and raw accesses aren't raw.  It would be nice to fix this more
generally (starting in Linux, so we don't diverge).

It's too bad we can't rely on chip->ops being used... maybe we could
copy the data into there?  Or just have a separate mtd_oob_mode_t in
nand_chip.

> +/*
> + * Write OOB data to NAND.
> + */
> +static int mxs_nand_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *nand,
> +					int page)
> +{
> +	struct mxs_nand_info *nand_info = nand->priv;
> +	uint8_t block_mark = 0;
> +
> +	/*
> +	 * There are fundamental incompatibilities between the i.MX GPMI NFC and
> +	 * the NAND Flash MTD model that make it essentially impossible to write
> +	 * the out-of-band bytes.
> +	 *
> +	 * We permit *ONE* exception. If the *intent* of writing the OOB is to
> +	 * mark a block bad, we can do that.
> +	 */

Is this just an issue with writing OOB separately from the main data
(which would also be an issue on MLC chips that don't allow multiple
partial programming), or can you not even write user OOB bytes as part
of a full page write?

Based on fake_ecc_layout I'm guessing the latter.

> +	if (nand_info->marking_block_bad) {
> +		printf("NXS NAND: Writing OOB isn't supported\n");
> +		return -EIO;
> +	}

Shouldn't this be if (!nand_info->marking_block_bad)?

> +/*
> + * Claims all blocks are good.
> + *
> + * In principle, this function is *only* called when the NAND Flash MTD system
> + * isn't allowed to keep an in-memory bad block table, so it is forced to ask
> + * the driver for bad block information.
> + *
> + * In fact, we permit the NAND Flash MTD system to have an in-memory BBT, so
> + * this function is *only* called when we take it away.
> + *
> + * We take away the in-memory BBT when the user sets the "ignorebad" parameter,
> + * which indicates that all blocks should be reported good.
> + *
> + * Thus, this function is only called when we want *all* blocks to look good,
> + * so it *always* return success.
> + */
> +static int mxs_nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
> +{
> +	return 0;
> +}

What/where is the "ignorebad" parameter?

Other than when scrubbing (which has its own override), when would you
want to do this?

> +/*
> + * Nominally, the purpose of this function is to look for or create the bad
> + * block table. In fact, since the HIL calls this function at the very end of

HIL?

> + * the initialization process started by nand_scan(), and the HIL doesn't have a
> + * more formal mechanism, everyone "hooks" this function to continue the
> + * initialization process.

Everyone?  I only see diskonchip doing this.

The nand_base.c code actually does have a split here
(nand_scan_ident/nand_scan_tail), but U-Boot's glue code is too
inflexible, and insists on calling nand_scan.  The right fix is to let
drivers call nand_scan_ident/nand_scan_tail themselves.

-Scott