[U-Boot] [PATCH 4/4 V3] PXA: Adapt Voipac PXA270 to OneNAND SPL

[Marek Vasut]

> On 11/03/2011 07:55 PM, Marek Vasut wrote:
> >> On 11/03/2011 04:52 PM, Marek Vasut wrote:
> >> Why do we want to separate them?  What is the fundamental difference
> >> between OneNAND, and a high-level NAND controller such as fsl_elbc?
> > 
> > Honestly, I'm not the author of the subsystem, but please check the
> > documentation. The way we retrieve data from onenand is different to
> > NAND.
> 
> What documentation?  How is it different?  There are substantial
> differences in how we "retrieve data" between drivers that use the NAND
> subsystem.  Surely you've seen that in the mxs_nand driver. :-)
> 
> >> Maybe there would be some differences on init if we can't produce
> >> "normal" ID data, but that doesn't justify duplicating the whole
> >> subsystem.
> > 
> > Where do you see such duplication? cmd_onenand ?
> 
> cmd_onenand and env_onenand are the most irritating, since they're at a
> layer that really shouldn't care about the differences -- we should
> probably have a plain "mtd" command instead, for most of the functionality.
> 
> There's also onenand_bbt.c -- what are the hardware-based differences in
> how the bad block table is managed?
> 
> nand_base.c/onenand_base.c are less clear.  Obviously much of what is in
> onenand_base.c would be in the controller driver if it used the NAND
> subsystem.  But it looks like some of it is duplication.
> 
> >> Why should the code that just wants to use an API to move data around
> >> need to care which it is?  Why should there be behavioral differences
> >> that aren't rooted in the actual hardware?  Another approach might be to
> >> use MTD as the common interface, but factor out common code into
> >> libraries that drivers can use, and avoid the main nand_base.c code even
> >> for things like fsl_elbc.
> > 
> > I think you're mistaken here. OneNAND != NAND.
> 
> Well, last I tried I couldn't find any public documentation, so all I
> have to go on is the code, some marketing-type info, and asking
> questions of people that appear to know more about OneNAND than I do. :-)
> 
> From what I can see, it looks like NAND with an integrated controller
> that exposes an unusual command set, but still for the most part
> provides the same operations.  Several of our existing NAND-subsystem
> drivers have to fake the command set for the generic layer as well.
> Initialization/identification might be a problem area that current
> drivers don't have to deal with, though.  Actually integrating OneNAND
> with NAND would likely involve an already-needed restructuring of the
> subsystem.
> 
> If the answer really is that it makes sense to consider OneNAND to be a
> totally different thing from NAND, then it's outside my jurisdiction as
> NAND custodian -- which is fine with me.  Frankly, even if it does make
> sense to merge them, I'd rather not be custodian of the OneNAND stuff
> unless someone is actually willing to do the merge.  I don't have access
> to hardware or documentation, and it's an entirely separate codebase.
> People just started sending me the patches a few years back.
> 
> >> This is not a new complaint -- I've asked for this before but nobody's
> >> put the time into sorting out the mess (and I have neither time nor
> >> hardware nor documentation).  The SPL load_image function is a simple
> >> enough interface to start with, though. :-)
> > 
> > Well, it seems what you are proposing is way beyond the scope of this
> > patchset.
> > 
> >> In fact, it should probably just be spl_load_image() with whatever boot
> >> source has been configured into this SPL build.
> > 
> > What if you have two boot sources?
> 
> Traditionally SPL has been small and purpose-built to do exactly one
> thing -- so we decide at compile-time things that we might otherwise
> decide at runtime.
> 
> If there's a requirement for multiple boot sources decided at runtime,
> then we'll obviously need a runtime mechanism. -- but it seems a bit
> hackish to why does it matter whether it's two different types of
> device, or two of the same type of device (possibly with different
> controller types)?  If the answer is that, for example, NAND versus USB
> versus MMC is a likely use case, but two different NANDs is not likely,
> is NAND versus OneNAND any more likely?
> 
> Maybe spl_load_image should be a function pointer that board code sets,
> with each implementation being distinctly named (in which case
> nand_spl_load_image would become nand_spl_simple_load_image, unless we
> move it to nand_spl_load.c and make nand_read_page a function pointer).
>  If needed to save a few bytes, we could use #defines to eliminate the
> function pointers in a single-target SPL build.
> 
> For now, until we decide to do something SPL-wide, call it what you want.
> 

Well basically from what I see, you'd like me to do the NAND/OneNAND merge. As I 
already said, that's way out of the scope of this patchset so I'm not doing 
that. Either way, are you OK with current state of the patch?

M