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

[Marcel Ziswiler]

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. I don't quite see any other practical way of
achieving such shared code where we have rather diverse requirements
otherwise.

> Don't undef it, just don't define it.  Maybe someone wants to turn debugging
> on system-wide.
> 
> > +#ifdef DEBUG
> > +#define DEBUGF(fmt, args...) printf(fmt, ##args)
> > +#else /* DEBUG */
> > +#define DEBUGF(x...)
> > +#endif /* DEBUG */
> 
> We've already got MTDDEBUG.

OK, agreed.

> 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 don't see this upstream.

That's because linux generally uses interrupts, but I wanted pure
polling for 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!

> 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. OK as
later targets do no more have a 4K limit but rather 16K it might
actually work out. I am just fearing further dependencies.

> Or try to avoid the need in the NAND loader -- for example, msleep is only
> used in handling NAND_CMD_RESET, which you shouldn't need.
> 
> In fact, I'm not sure how this driver can be used at all with NAND_SPL.  The
> normal nand_spl/nand_boot.c uses cmd_ctrl.  More complicated controllers
> that can't use cmd_ctrl generally need their own nand_spl driver (such as
> nand_spl/nand_boot_fsl_elbc.c).  If you have a particularly large NAND boot
> region, you might be able to create a nand_spl driver that uses this driver,
> but typically space is too tight.

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.

> You can (and should) use __raw_writel/__raw_readl in U-Boot as well.
> 
> There is also a prototype for__raw_writesl/__raw_readsl, but the
> implementation seems to be missing -- perhaps you could add it generically,
> rather than just in one driver.

I actually had a version doing it that way but it had some nasty further
dependencies so I finally decided against doing it that way. Will look
into it again.

Thanks Scott!

-Marcel