[U-Boot] [PATCH 1/3] Monahan/PXA3xx: integrate Linux NAND controller driver

[Scott Wood]

Marcel Ziswiler wrote:
> On Thu, 2010-01-07 at 13:44 -0600, Scott Wood wrote:
>> I don't think we need this.
>>
>> In fact, it could make it harder to spot conflicts when merging new versions
>> of the file from linux, since patch could find the context it's looking for
>> in the middle of an #ifndef __U_BOOT__ section when the change really needs
>> to be applied to the other side of the #else.
>>
>> Plus, it makes it harder to read the code.  Let's share what we reasonably
>> can, and get rid of the rest.
> 
> OK, but it es exactly how it is done with the rest of the linux MTD
> stuff pulled into U-Boot.

There is some of this in the common NAND code, which was there when I 
took over the subsystem.  I'd rather move in the direction of having 
less of it than more.

I haven't seen much of this in the drivers.

> I don't quite see any other practical way of
> achieving such shared code where we have rather diverse requirements
> otherwise.

But you're not actually sharing the code.  You're reusing parts of it 
with lots of changes (which I'm guessing aren't going to make it back 
into Linux) and dead code.

I don't see how keeping the dead bits around helps keep things in sync. 
  It's usually easy enough to look at conflicts and figure out whether a 
change needs to be brought over, and how.  The information about what is 
different can be obtained at any point using diff.  One thing that would 
help is to keep track in the comments of which Linux version/SHA1 it 
corresponds to.

Note that stuff like mtd/compat.h is different, in that it's allowing 
some very commonly used Linux facilities to be used by the shared code 
as-is without needing any ifdefs in the main code.

>> This is marked deprecated in Linux; perhaps we should take the preferred
>> approach of platform-specified data here as well?
> 
> Hm, I thought platform data would be an over kill for U-Boot especially
> if it comes down to the NAND SPL case. Will look into how I could
> integrate that.

I suppose it depends what it ends up looking like -- but simply having 
platform code provide a struct with some timings and such seems pretty 
reasonable.  The method of getting that data to the driver would likely 
be different than in Linux.

>> I don't see this upstream.
> 
> That's because linux generally uses interrupts, but I wanted pure
> polling for U-Boot.

Right, the implication was that unless there's a plan to support polling 
in Linux, this amounts to another #ifdef __U_BOOT__.

> I actually thought about first getting some of my customization upstream
> but the last time I submitted a mere 2 line bug fix to this driver it
> got completely ignored. Don't quite know how to get any MTD stuff
> mainline in a timely manner!

It might have just been overlooked -- try pinging the maintainer and the 
MTD list.

>> Can we arrange to pull in the relevant functions from the rest of u-boot
>> rather than duplicate them?
> 
> Generally yes, but that would completely bloat the NAND SPL case.

I was thinking we could identify a subset that NAND SPL and similar 
things are generally going to need, and #ifndef the rest (similar to 
CONFIG_NS16550_MIN_FUNCTIONS).

Or, at least have a NAND SPL mini-library so that there's only one 
duplication instead of one per board/driver/etc.  At least memcpy can 
replace some open-coding done in existing NAND SPL code.

> It actually works just fine and boots various PXA3xx targets with my
> slightly modified nand_spl/nand_boot.c which I plan to submit as soon as
> we agree on this driver.

OK, that modified nand_boot.c is what I was wondering about (other than 
the size).

-Scott