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

login
register
mail settings
Submitter Stefano Babic
Date April 9, 2011, 6:05 p.m.
Message ID <1302372335-30232-6-git-send-email-sbabic@denx.de>
Download mbox | patch
Permalink /patch/90480/
State Changes Requested
Delegated to: Sandeep Paulraj
Headers show

Comments

Stefano Babic - April 9, 2011, 6:05 p.m.
The NAND controller does not support subpage accessing. This is
not used at all for MLC NAND, but it is set for SLC NAND. UBI tries
to access to subpages, and because it fails, it starts "torture tests"
on the whole page that are always successful, making an endless loop
with the UBI background task taking most CPU time.
On the console, the issue is recognized by the messages:

UBI error: ubi_io_read: error -74 (ECC error) while reading 512 bytes from
PEB 37:512, read 512 bytes
UBI: run torture test for PEB 37
UBI: PEB 37 passed torture test, do not mark it a bad

Signed-off-by: Stefano Babic <sbabic@denx.de>
CC: Ben Gardiner<bengardiner@nanometrics.ca>
CC: Sandeep Paulraj <s-paulraj@ti.com>
CC: Scott Wood <scottwood@freescale.com>
---
 README                          |    4 ++++
 drivers/mtd/nand/davinci_nand.c |    3 +++
 include/configs/ea20.h          |    1 +
 include/linux/mtd/nand.h        |    3 ++-
 4 files changed, 10 insertions(+), 1 deletions(-)
Ben Gardiner - April 11, 2011, 2:04 p.m.
Hi Stefano,

Thanks for sharing this patch -- I have been using the "-O 2048" (VID
header offset) option to prevent subpages here.

On Sat, Apr 9, 2011 at 2:05 PM, Stefano Babic <sbabic@denx.de> wrote:
> The NAND controller does not support subpage accessing. This is
> not used at all for MLC NAND, but it is set for SLC NAND. UBI tries
> to access to subpages, and because it fails, it starts "torture tests"

"tries to access subpages" is maybe a little too vague; I think Jon
Povey described the problem quite succinctly:

"the problem is that the ECC generated for an all-FFs page is not
all-FFs, and subpage writes are handled by
nand_do_write_ops() by writing a full page with FFs in the unset data
areas." [1]

> on the whole page that are always successful, making an endless loop
> with the UBI background task taking most CPU time.
> On the console, the issue is recognized by the messages:
>
> UBI error: ubi_io_read: error -74 (ECC error) while reading 512 bytes from
> PEB 37:512, read 512 bytes
> UBI: run torture test for PEB 37
> UBI: PEB 37 passed torture test, do not mark it a bad
>
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> CC: Ben Gardiner<bengardiner@nanometrics.ca>
> CC: Sandeep Paulraj <s-paulraj@ti.com>
> CC: Scott Wood <scottwood@freescale.com>

Is it worth mentioning that this will create UBI partitions that are
not usable in Linux with a similar patch or a VID header offset
workaround?

> ---
>  README                          |    4 ++++
>  drivers/mtd/nand/davinci_nand.c |    3 +++
>  include/configs/ea20.h          |    1 +
>  include/linux/mtd/nand.h        |    3 ++-
>  4 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/README b/README
> index 21cd71b..8d664eb 100644
> --- a/README
> +++ b/README
> @@ -2907,6 +2907,10 @@ Low Level (hardware related) configuration options:
>                that is executed before the actual U-Boot. E.g. when
>                compiling a NAND SPL.
>
> +- CONFIG_SYS_NAND_NO_SUBPAGE
> +               Some drivers (davinci) do not support access to NAND subpage.
> +
> +
>  Building the Software:
>  ======================
>
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index d41579c..f6c7d09 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand)
>  #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
>        nand->options     |= NAND_USE_FLASH_BBT;
>  #endif
> +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE
> +       nand->options     |= NAND_NO_SUBPAGE_WRITE;
> +#endif
>  #ifdef CONFIG_SYS_NAND_HW_ECC
>        nand->ecc.mode = NAND_ECC_HW;
>        nand->ecc.size = 512;
> diff --git a/include/configs/ea20.h b/include/configs/ea20.h
> index 1843dae..9e5dda4 100644
> --- a/include/configs/ea20.h
> +++ b/include/configs/ea20.h
> @@ -171,6 +171,7 @@
>
>  #define CONFIG_NAND_DAVINCI
>  #define        CONFIG_SYS_NAND_PAGE_2K
> +#define CONFIG_SYS_NAND_NO_SUBPAGE
>  #define CONFIG_SYS_NAND_CS             2
>  #define CONFIG_SYS_NAND_BASE           DAVINCI_ASYNC_EMIF_DATA_CE2_BASE
>  #undef CONFIG_SYS_NAND_HW_ECC
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 987a2ec..215e781 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -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)

First let me say that I would be happy to have a fix for this merged
-- so I think the change is "good enough" since it makes UBI work
without specifying a VID header offset equal to the NAND page size.
What follows is more topical musings that any particular criticisms
that should be considered blockers of your patch.

I'm wondering about the long-term path for davinci NAND and subpage writes...

But the sentiment I've heard about adding an exception to
NAND_CHIPOPTIONS_MSK as above is that we are mixing features of the
NAND chip and features of the NAND controller (If you have a modern
SLC NAND then your chip probably supports subpage writes whereas it is
the controller that needs to be limited).

What's more is that the davinci nand controller could do subpage
writing if the writing operation were informed of the extents of the
subpage being written instead of being handed a buffer with 0xFF in
the non-target page areas. Which, I believe is Artem's primary
motivation for the introduction of nand_write_subpage() to the davinci
NAND controller driver [2].

So if the nand_write_subpage family of functions was introduced to the
Linux kernel instead of adding another exception to
NAND_CHIPOPTIONS_MSK then we would have 3 ways to use UBI on davinci
NAND:
1) no patch: VID offset <page size> required
2) chip NAND_NO_SUBPAGE_WRITE patch
3) subpage writes support with nand_write_subpage()

systems with 2) or 3) could always use UBI as in 1) and a UBI volume
made with 2) would need to be attached as in 1) on systems with 2) or
3) ; but a UBI volume made with 3) could not be used on systems with
1) or 2) which means that we could not make use of the efficiency
benefits of nand_write_subpage() if/when it is available on systems
which need access to UBI from U-boot.

I've cc'd Artem and Jon in the hopes that they can correct any of my
leaps above -- what I'd really like is to understand a migration
strategy to actually using subpage writes with davinci NAND in the
future.

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.linux.davinci/19853
[2] http://article.gmane.org/gmane.linux.drivers.mtd/31581

---
Nanometrics Inc.
http://www.nanometrics.ca
Scott Wood - April 11, 2011, 7:16 p.m.
On Sat, Apr 09, 2011 at 08:05:35PM +0200, Stefano Babic wrote:
> diff --git a/README b/README
> index 21cd71b..8d664eb 100644
> --- a/README
> +++ b/README
> @@ -2907,6 +2907,10 @@ Low Level (hardware related) configuration options:
>  		that is executed before the actual U-Boot. E.g. when
>  		compiling a NAND SPL.
>  
> +- CONFIG_SYS_NAND_NO_SUBPAGE
> +		Some drivers (davinci) do not support access to NAND subpage.

This only controls the davinci driver, so it should be
CONFIG_SYS_DAVINCI_NAND_NO_SUBPAGE.

Is this really board-specific?  Does the davinci driver ever support
subpage?

> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 987a2ec..215e781 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -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).

-Scott
Jon Povey - April 12, 2011, 12:45 a.m.
Ben Gardiner wrote:
> "tries to access subpages" is maybe a little too vague; I think Jon
> Povey described the problem quite succinctly:

That's me, I better wake up!

> What's more is that the davinci nand controller could do subpage
> writing if the writing operation were informed of the extents of the
> subpage being written instead of being handed a buffer with 0xFF in
> the non-target page areas. Which, I believe is Artem's primary
> motivation for the introduction of nand_write_subpage() to the
> davinci NAND controller driver [2].

I can't remember if I posted about this at the time, but what I ended up
doing was propagating the subpage start offset and length through to all
the write_page() functions. At the appropriate place the ECC for any
"unwritten" subpages is FF'd out. It's a fairly ugly platform-specific
hack but it works, if anyone wants to see it I can dig out the patch.

--
Jon Povey
jon.povey@racelogic.co.uk

Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .

The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network
Stefano Babic - April 12, 2011, 9:08 a.m.
On 04/11/2011 04:04 PM, Ben Gardiner wrote:
> Hi Stefano,

Hi Ben,

> 
> Thanks for sharing this patch -- I have been using the "-O 2048" (VID
> header offset) option to prevent subpages here.

Yes, this works too, at least with Linux.

> 
> On Sat, Apr 9, 2011 at 2:05 PM, Stefano Babic <sbabic@denx.de> wrote:
>> The NAND controller does not support subpage accessing. This is
>> not used at all for MLC NAND, but it is set for SLC NAND. UBI tries
>> to access to subpages, and because it fails, it starts "torture tests"
> 
> "tries to access subpages" is maybe a little too vague; I think Jon
> Povey described the problem quite succinctly:
> 
> "the problem is that the ECC generated for an all-FFs page is not
> all-FFs, and subpage writes are handled by
> nand_do_write_ops() by writing a full page with FFs in the unset data
> areas." [1]

Thanks - I have not found this link. I will merge Jon's comment.

>> on the whole page that are always successful, making an endless loop
>> with the UBI background task taking most CPU time.
>> On the console, the issue is recognized by the messages:
>>
>> UBI error: ubi_io_read: error -74 (ECC error) while reading 512 bytes from
>> PEB 37:512, read 512 bytes
>> UBI: run torture test for PEB 37
>> UBI: PEB 37 passed torture test, do not mark it a bad
>>
>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>> CC: Ben Gardiner<bengardiner@nanometrics.ca>
>> CC: Sandeep Paulraj <s-paulraj@ti.com>
>> CC: Scott Wood <scottwood@freescale.com>
> 
> Is it worth mentioning that this will create UBI partitions that are
> not usable in Linux with a similar patch or a VID header offset
> workaround?

Yes, better to explain ;-)

> First let me say that I would be happy to have a fix for this merged
> -- so I think the change is "good enough" since it makes UBI work
> without specifying a VID header offset equal to the NAND page size.
> What follows is more topical musings that any particular criticisms
> that should be considered blockers of your patch.
> 
> I'm wondering about the long-term path for davinci NAND and subpage writes...
> 
> But the sentiment I've heard about adding an exception to
> NAND_CHIPOPTIONS_MSK as above is that we are mixing features of the
> NAND chip and features of the NAND controller (If you have a modern
> SLC NAND then your chip probably supports subpage writes whereas it is
> the controller that needs to be limited).

That is true. With this patch I constrained the SLC NAND on my board to
be considered as MLC, as a MLC NAND cannot be accessed in subpage mode.

However, I set also a CONFIG_SYS_NAND_NO_SUBPAGE, and instead of
dropping the option in the mask we could protect the code where the
option is checked. In other words, we could change nand_base.c in this way:

#ifndef CONFIG_SYS_NAND_NO_SUBPAGE
        if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
            !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
                switch(chip->ecc.steps) {
                case 2:
                        mtd->subpage_sft = 1;
                        break;
                case 4:
                case 8:
                case 16:
                        mtd->subpage_sft = 2;
                        break;
                }
        }
#endif

Then it could be clearer that the restriction is due to the NAND
controller, and not to the chip itself.

> 
> What's more is that the davinci nand controller could do subpage
> writing if the writing operation were informed of the extents of the
> subpage being written instead of being handed a buffer with 0xFF in
> the non-target page areas. Which, I believe is Artem's primary
> motivation for the introduction of nand_write_subpage() to the davinci
> NAND controller driver [2].

Well, of course, if the davinci NAND can handle subpages, we can remove
the limitation. What do you think to add this patch in the way I suggest
now (without touching NAND_CHIPOPTIONS_MSK, that makes the MTD
inconsistent with Linux) until a subpage_write is added to davinci ?

> 
> So if the nand_write_subpage family of functions was introduced to the
> Linux kernel instead of adding another exception to
> NAND_CHIPOPTIONS_MSK then we would have 3 ways to use UBI on davinci
> NAND:
> 1) no patch: VID offset <page size> required
> 2) chip NAND_NO_SUBPAGE_WRITE patch
> 3) subpage writes support with nand_write_subpage()
> 
> systems with 2) or 3) could always use UBI as in 1) and a UBI volume
> made with 2) would need to be attached as in 1) on systems with 2) or
> 3) ; but a UBI volume made with 3) could not be used on systems with
> 1) or 2) which means that we could not make use of the efficiency
> benefits of nand_write_subpage() if/when it is available on systems
> which need access to UBI from U-boot.

I agree with your analyses. As personal feeling, solution one has the
drawback that it cannot be used in u-boot, and it seems to me very easy
to have a wrong UBI on the target. We can consider 2) as temporary
solution, until 3) is available.

Best regards,
Stefano Babic
Stefano Babic - April 12, 2011, 9:44 a.m.
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.

Best regards,
Stefano Babic
Ben Gardiner - April 12, 2011, 12:47 p.m.
On Tue, Apr 12, 2011 at 5:08 AM, Stefano Babic <sbabic@denx.de> wrote:
> Ben Gardiner wrote:
>> Thanks for sharing this patch -- I have been using the "-O 2048" (VID
>> header offset) option to prevent subpages here.
>
> Yes, this works too, at least with Linux.

(being picky / for archival purposes) In u-boot, specifying the VID
header offset as an argument to the ubi attach command -- i.e. 'ubi
part <mtd_partition> 2048' -- also works (on da850evm, at least).


> [...]
>> First let me say that I would be happy to have a fix for this merged
>> -- so I think the change is "good enough" since it makes UBI work
>> without specifying a VID header offset equal to the NAND page size.
>> What follows is more topical musings that any particular criticisms
>> that should be considered blockers of your patch.
>>
>> I'm wondering about the long-term path for davinci NAND and subpage writes...
>>
>> But the sentiment I've heard about adding an exception to
>> NAND_CHIPOPTIONS_MSK as above is that we are mixing features of the
>> NAND chip and features of the NAND controller (If you have a modern
>> SLC NAND then your chip probably supports subpage writes whereas it is
>> the controller that needs to be limited).
>
> That is true. With this patch I constrained the SLC NAND on my board to
> be considered as MLC, as a MLC NAND cannot be accessed in subpage mode.
>
> However, I set also a CONFIG_SYS_NAND_NO_SUBPAGE, and instead of
> dropping the option in the mask we could protect the code where the
> option is checked. In other words, we could change nand_base.c in this way:
>
> #ifndef CONFIG_SYS_NAND_NO_SUBPAGE
>        if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
>            !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
>                switch(chip->ecc.steps) {
>                case 2:
>                        mtd->subpage_sft = 1;
>                        break;
>                case 4:
>                case 8:
>                case 16:
>                        mtd->subpage_sft = 2;
>                        break;
>                }
>        }
> #endif
>
> Then it could be clearer that the restriction is due to the NAND
> controller, and not to the chip itself.

I agree -- it keeps the chip and controller options separated.

>> What's more is that the davinci nand controller could do subpage
>> writing if the writing operation were informed of the extents of the
>> subpage being written instead of being handed a buffer with 0xFF in
>> the non-target page areas. Which, I believe is Artem's primary
>> motivation for the introduction of nand_write_subpage() to the davinci
>> NAND controller driver [2].
>
> Well, of course, if the davinci NAND can handle subpages, we can remove
> the limitation. What do you think to add this patch in the way I suggest
> now (without touching NAND_CHIPOPTIONS_MSK, that makes the MTD
> inconsistent with Linux) until a subpage_write is added to davinci ?

Yes, I like the way your proposed changes disabled subpage
initialization instead of adding an exception to the chip options
mask. I'm still OK with merging a patch to disable subpage writes on
da850 -- as an interim solution. i.e. until a subpage_write is added
to the mtd subsystem, and davinci-nand has an override and u-boot gets
a similar change.

>> So if the nand_write_subpage family of functions was introduced to the
>> Linux kernel instead of adding another exception to
>> NAND_CHIPOPTIONS_MSK then we would have 3 ways to use UBI on davinci
>> NAND:
>> 1) no patch: VID offset <page size> required
>> 2) chip NAND_NO_SUBPAGE_WRITE patch
>> 3) subpage writes support with nand_write_subpage()
>>
>> systems with 2) or 3) could always use UBI as in 1) and a UBI volume
>> made with 2) would need to be attached as in 1) on systems with 2) or
>> 3) ; but a UBI volume made with 3) could not be used on systems with
>> 1) or 2) which means that we could not make use of the efficiency
>> benefits of nand_write_subpage() if/when it is available on systems
>> which need access to UBI from U-boot.
>
> I agree with your analyses. As personal feeling, solution one has the
> drawback that it cannot be used in u-boot, and it seems to me very easy
> to have a wrong UBI on the target. We can consider 2) as temporary
> solution, until 3) is available.

Thanks for confirming, Stefano.

I'm not intimately familiar with the ea20 so there could be additional
quirks in the way -- but I can say that option 1) works in u-boot on
the da850evm (+UI board). I use 'ubi part ubi_part 2048' -- ubi_part
is the mtdpart name of my ubi part :)

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca
Scott Wood - April 13, 2011, 4:24 p.m.
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?
Stefano Babic - April 15, 2011, 5:34 p.m.
On 04/13/2011 06:24 PM, Scott Wood wrote:

Hi Scott,

> 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.

surely, but it is not clear to me how. There is no entry for a
write_subpage function, as this issue does not happen with other
controllers, and I do not see a callback for the driver after the
nand_scan() function, where I thought the driver could change the
options according to its capabilities.

> 
> 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? :-)

Well, the options are simply ignored, I agree about removing them.

> 
> -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?

What I have seen (kernel 2.6.38), the options are ignored in Linux as
well, and this issue is open for the davinci processors.

Best regards,
Stefano Babic
Scott Wood - April 15, 2011, 8:29 p.m.
On Fri, 15 Apr 2011 19:34:48 +0200
Stefano Babic <sbabic@denx.de> wrote:

> On 04/13/2011 06:24 PM, Scott Wood wrote:
> 
> Hi Scott,
> 
> > 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.
> 
> surely, but it is not clear to me how. There is no entry for a
> write_subpage function, as this issue does not happen with other
> controllers,

I don't see where subpage writes are done at all, actually.

> and I do not see a callback for the driver after the
> nand_scan() function, where I thought the driver could change the
> options according to its capabilities.

nand_scan() is broken into head and tail functions.  In Linux, the driver
calls these, and can look at the chip info before tail is called.  In
U-boot, common code drives this, and the controller driver is not involved
-- but it would be good to change this.

> > 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? :-)
> 
> Well, the options are simply ignored, I agree about removing them.

I think it can be enabled by the controller driver between head and tail
(at least on Linux), though I don't see any drivers that do this as far as
a quick grep shows.

-Scott
Stefano Babic - April 22, 2011, 7:13 a.m.
On 04/15/2011 10:29 PM, Scott Wood wrote:

> 
> nand_scan() is broken into head and tail functions.  In Linux, the driver
> calls these, and can look at the chip info before tail is called.  In
> U-boot, common code drives this, and the controller driver is not involved
> -- but it would be good to change this.

Agree, we can do in this way.

> 
>>> 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? :-)
>>
>> Well, the options are simply ignored, I agree about removing them.
> 
> I think it can be enabled by the controller driver between head and tail
> (at least on Linux), though I don't see any drivers that do this as far as
> a quick grep shows.

The only point is we need that the controller checks the parameters
after the tail part, as the tail part scans the chip and sets its
options, as the SUBPAGE flag. We could add a pre_adjust()  and
post_adjust() functions to be called after the head and tail part of the
nand_scan().

Best regards,
Stefano
Scott Wood - April 25, 2011, 5:37 p.m.
On Fri, 22 Apr 2011 09:13:34 +0200
Stefano Babic <sbabic@denx.de> wrote:

> The only point is we need that the controller checks the parameters
> after the tail part, as the tail part scans the chip and sets its
> options, as the SUBPAGE flag. We could add a pre_adjust()  and
> post_adjust() functions to be called after the head and tail part of the
> nand_scan().

This is what the tail function is supposed to be for.  I don't see
where NAND_NO_AUTOINCR or NAND_NO_SUBPAGE_WRITE are called in the head
(nand_scan_ident/nand_get_flash_type) functions.  I only see
NAND_BUSWIDTH_16 and NAND_4PAGE_ARRAY checked there.

-Scott

Patch

diff --git a/README b/README
index 21cd71b..8d664eb 100644
--- a/README
+++ b/README
@@ -2907,6 +2907,10 @@  Low Level (hardware related) configuration options:
 		that is executed before the actual U-Boot. E.g. when
 		compiling a NAND SPL.
 
+- CONFIG_SYS_NAND_NO_SUBPAGE
+		Some drivers (davinci) do not support access to NAND subpage.
+
+
 Building the Software:
 ======================
 
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index d41579c..f6c7d09 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -609,6 +609,9 @@  void davinci_nand_init(struct nand_chip *nand)
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
 	nand->options	  |= NAND_USE_FLASH_BBT;
 #endif
+#ifdef CONFIG_SYS_NAND_NO_SUBPAGE
+	nand->options	  |= NAND_NO_SUBPAGE_WRITE;
+#endif
 #ifdef CONFIG_SYS_NAND_HW_ECC
 	nand->ecc.mode = NAND_ECC_HW;
 	nand->ecc.size = 512;
diff --git a/include/configs/ea20.h b/include/configs/ea20.h
index 1843dae..9e5dda4 100644
--- a/include/configs/ea20.h
+++ b/include/configs/ea20.h
@@ -171,6 +171,7 @@ 
 
 #define CONFIG_NAND_DAVINCI
 #define	CONFIG_SYS_NAND_PAGE_2K
+#define CONFIG_SYS_NAND_NO_SUBPAGE
 #define CONFIG_SYS_NAND_CS		2
 #define CONFIG_SYS_NAND_BASE		DAVINCI_ASYNC_EMIF_DATA_CE2_BASE
 #undef CONFIG_SYS_NAND_HW_ECC
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 987a2ec..215e781 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -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)
 
 /* Non chip related options */
 /* Use a flash based bad block table. This option is passed to the