[U-Boot] [PATCH 6/6] EA20: do not use subpage write for NAND

From: Scott Wood <scottwood@freescale.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 6/6] EA20: do not use subpage write for NAND
Date: Wed, 13 Apr 2011 11:24:53 -0500	[thread overview]
Message-ID: <20110413112453.12670f34@schlenkerla.am.freescale.net> (raw)
In-Reply-To: <4DA41EFA.4060402@denx.de>

On Tue, 12 Apr 2011 11:44:26 +0200
Stefano Babic <sbabic@denx.de> wrote:

> On 04/11/2011 09:16 PM, Scott Wood wrote:
> > This only controls the davinci driver, so it should be
> > CONFIG_SYS_DAVINCI_NAND_NO_SUBPAGE.
> > 
> > Is this really board-specific?
> 
> No, really not.
> 
> >  Does the davinci driver ever support
> > subpage?
> 
> No, it does not. This problem affects all boards using the davinci NAND
> controller. At least with 850 or OMAP-l1 processors, that I have tested.
> 
> >> @@ -193,7 +193,8 @@ typedef enum {
> >>  					&& (chip->page_shift > 9))
> >>  
> >>  /* Mask to zero out the chip options, which come from the id table */
> >> -#define NAND_CHIPOPTIONS_MSK	(0x0000ffff & ~NAND_NO_AUTOINCR)
> >> +#define NAND_CHIPOPTIONS_MSK	(0x0000ffff & ~NAND_NO_AUTOINCR & \
> >> +				~NAND_NO_SUBPAGE_WRITE)
> > 
> > I wonder what we really need CHIPOPTIONS_MSK for?  Silently ignoring what
> > the driver asked for doesn't seem right.  Can't we just OR the two
> > sources?  And it would be a driver error to specify a flag that doesn't
> > make sense to be set this way (i.e. only do it with the "doesn't support
> > X" flags).
> 
> Ben reports quite the same issue, as this patch mixes chip options with
> NAND controller capabilities.
> 
> Probably it is better as I answered to Ben to use
> CONFIG_SYS_DAVINCI_NAND_NO_SUBPAGE in nand_base.c to switch off subpage
> access instead of setting the chip as not able to handle subpages,
> changing the  NAND_CHIPOPTIONS_MSK.

Davinci-specific #defines do not belong in nand_base.c[1].  The controller
driver should be able to set "this isn't supported" options just as well as
the chip data -- I just don't think it should be limited to this specific
one.

For example, fsl_elbc_nand.c sets NAND_NO_READRDY and NAND_NO_AUTOINCR.
Before this thread, I didn't realize it they were getting ignored.  Things
work anyway because the former is an optimization, and the latter is getting
forced on after the masking, for some reason -- does autoincr simply not
work?  Can we remove the code? :-)

-Scott

[1] Nor should it be turned back into a non-davinci define -- what if there
are multiple NAND controllers supported, and only one requires this?  It's
not so bad in U-Boot (I'd still rather avoid it though), but this approach
is not going to go over well in Linux.

How is Linux handling this?