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

[Marek Vasut]

On Wednesday, September 28, 2011 11:57:49 PM Scott Wood wrote:
> On 09/28/2011 04:42 PM, Marek Vasut wrote:
> > On Wednesday, September 28, 2011 11:26:45 PM Scott Wood wrote:
> >> On 09/11/2011 11:06 PM, Marek Vasut wrote:
> >>> +		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 ?
> 
> Very small functions (and anything used only once) get inlined
> automatically.
> 
> It might need the hint if you have a larger function that collapses down
> to something small due to constant propagation (and of course if it's a
> header or otherwise *must* be inlined), but usually it does a decent job
> on its own.
> 
> >> 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.
> 
> What if the raw access is being done to e.g. force bit flips for testing?
> 
> There seems to be a difference between U-Boot and Linux here.
> 
> Linux has this in mtd.h:
> >  * MTD_OOB_RAW:         mode to read oob and data without doing ECC
> >  checking
> 
> Doesn't say anything about layout.
> 
> Whereas U-Boot says:
> >  * MTD_OOB_RAW:         mode to read raw data+oob in one chunk. The oob
> >  data *                      is inserted into the data. Thats a raw
> >  image of the *                      flash contents.
> 
> Linux used to say what U-Boot says, but changed in commit
> b64d39d8b03fea88417d53715ccbebf71d4dcc9f
> 
> This commit message includes the comment "Now MTD_OOB_RAW behaves just
> like MTD_OOB_PLACE, but doesn't do ECC validation".
> 
> So I think if you need something that changes the layout from normal
> operations, it needs to be a new mode.  And it's about time to sync up
> U-Boot's NAND code with Linux again...

Well aren't you the maintainer that should take care of it ? ;-)
> 
> >>> +	/*
> >>> +	 * 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.
> 
> I'm not talking about circumventing the driver, just accessing some user
> OOB bytes through it.

There are no user OOB bytes, the driver does the ECC so user has no need to 
write any OOB.

> 
> >> 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.
> 
> OK.  It's been on my TODO list for a while now...
> 
> -Scott