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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
