[U-Boot] [PATCH v2] ADS5121 NAND driver

[Scott Wood]

On Wed, Oct 29, 2008 at 05:37:52PM -0600, John Rigby wrote:
> Scott, thanks for your feedback.  I can easily fix most of the issues.
> 
> The one question I have is if this can go in only supporting 5121 rev2.  
> If I need to add rev1 support it will take more time than I have right now.

If rev1 is as dead as Wolfgang says, then I'm fine with leaving it out --
but that fact should be reflected in a comment, rather than the name of
the driver.  After all, this driver should support rev3 if there is one,
right? :-)

> >>+/*
> >>+ * Copyright 2004-2008 Freescale Semiconductor, Inc. All Rights Reserved.
> >>+ *
> >>+ * Based on drivers/mtd/nand/mpc5121_nand.c
> >>+ * which was forked from drivers/mtd/nand/mxc_nd.c
> >>    
> >
> >Forking's bad, mmkay?
> >  
> Yes, I'm guilty.  The original was pretty ugly. 

If it's anything like the i.MX driver that was posted earlier, I agree
entirely. :-)

I was just hoping that we could come up with one new, cleaned-up driver
that supports both.

> >>+static struct nand_ecclayout nand_hw_eccoob_4k = {
> >>+	.eccbytes = 64,	/* actually 72 but only room for 64 */
> >>    
> >
> >Let's fix that, then.
> >  
> This is defined in generic mtd code that has exposure to userland.
> 
> include/mtd/mtd-abi.h:
> /*
> * ECC layout control structure. Exported to userspace for
> * diagnosis and to allow creation of raw images
> */
> struct nand_ecclayout {
>    uint32_t eccbytes;
>    uint32_t eccpos[64];
>    uint32_t oobavail;
>    struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
> };

Grr...  I guess it's OK for now, as the main purpose of eccpos with
hardware ECC is to exclude segments from use by other layers.

Very short-sighted API design, though.

> >read_byte should never be called with 16-bit NAND.
> >  
> This is the replacement for nand_read_byte16 in nand_base.c.

Right, I should have verified my assumption before asserting it. :-P

Not sure why the generic nand_read_byte16 wasn't implemented in terms of
the read_word method.

> >>+/*!
> >>+ * This function writes data of length \b len from buffer \b buf to the 
> >>NAND
> >>+ * internal RAM buffer's MAIN area 0.
> >>+ *
> >>+ * @mtd		MTD structure for the NAND Flash
> >>+ * @buf		data to be written to NAND Flash
> >>+ * @len		number of bytes to be written
> >>+ */
> >>+static void mpc5121_nand_write_buf(struct mtd_info *mtd,
> >>+					const u_char *buf, int len)
> >>+{
> >>+	printf("re-work may be needed?\n");
> >>    
> >
> >Answer this before we apply the patch. :-)
> >  
> Perhaps I can just get rid of this whole routine?

I don't think so...

> >Why override these rather than let them go through the command function?
> >  
> Not sure, I think it is so we can call the copy to/from spare routine.

You can do that in write_buf/read_buf/etc.

> >I'd rather scan_bbt not be abused for this -- we need to fix the NAND
> >probe code so that drivers can do things between nand_scan() and
> >nand_scan_tail().
> >  
> And until then?

It's OK for now -- that was more of a note to myself to hurry up and fix
the NAND probing mechanism. :-)

-Scott