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

[Scott Wood]

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

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

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