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?