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

[Marek Vasut <marek.vasut@gmail.com>]

On Wednesday, September 28, 2011 11:26:45 PM Scott Wood wrote:
> 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++)

Good catch.

> 
> > +		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.

Does it really ?
> 
> > +/*
> > + * 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.

You see the page as is ... I see no problem with this part.
> 
> > + * 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.

My understanding of the original FSL driver is that you should never be allowed 
to access the physical NAND media at all. Only through the driver, which does 
the magic.
> 
> > +	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?

Remnant from FSL code.
> 
> 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?

DTTO
> 
> > + * 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.

DTTO
> 
> 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.

I can't test now, so this has to wait. I'd prefer to get this mainline and then 
start poking around fixing this.
> 
> -Scott