[U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB

[U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB

[Scott Wood]

Without this, all OOB reads are from the last page normally read
(or zero at boot).  This results in bad block scans failing to look
in the right place, and so no bad blocks are found.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---

[U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB

[Benoît Thébaudeau]

Hi Scott,

On Thursday, January 31, 2013 8:47:55 PM, Scott Wood wrote:
> Without this, all OOB reads are from the last page normally read
> (or zero at boot).  This results in bad block scans failing to look
> in the right place, and so no bad blocks are found.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> From IRC discussion with a2cypher.  Compile-tested only; testing
> would be appreciated.
> 
>  drivers/mtd/nand/mxc_nand.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index d0ded48..32ba340 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1021,6 +1021,7 @@ void mxc_nand_command(struct mtd_info *mtd, unsigned
> command,
>  		break;
>  
>  	case NAND_CMD_READOOB:
> +		host->page_addr = page_addr;
>  		host->col_addr = column;
>  		host->spare_only = true;
>  		if (host->pagesize_2k)

For which NFC version and NAND Flash page size is this?

Do you have a means of duplicating the issue?

I wonder if the appropriate fix would not rather be to replace all occurrences
of "host->page_addr" with "page", except in mxc_nand_correct_data() and
mxc_nand_command(). Otherwise, it looks like there will still be weird things
going on with this variable.

Best regards,
Beno?t

[U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB

[Scott Wood]

On 01/31/2013 02:45:02 PM, Beno?t Th?baudeau wrote:
> Hi Scott,
> 
> On Thursday, January 31, 2013 8:47:55 PM, Scott Wood wrote:
> > Without this, all OOB reads are from the last page normally read
> > (or zero at boot).  This results in bad block scans failing to look
> > in the right place, and so no bad blocks are found.

Looking more closely, the calls to send_addr() use page_addr rather  
than host->page_addr, and the oob page read uses "page" in the call to  
cmdfunc, so maybe it's just the debug output that was wrong?

> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > ---
> > From IRC discussion with a2cypher.  Compile-tested only; testing
> > would be appreciated.
> >
> >  drivers/mtd/nand/mxc_nand.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/mtd/nand/mxc_nand.c  
> b/drivers/mtd/nand/mxc_nand.c
> > index d0ded48..32ba340 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -1021,6 +1021,7 @@ void mxc_nand_command(struct mtd_info *mtd,  
> unsigned
> > command,
> >  		break;
> >
> >  	case NAND_CMD_READOOB:
> > +		host->page_addr = page_addr;
> >  		host->col_addr = column;
> >  		host->spare_only = true;
> >  		if (host->pagesize_2k)
> 
> For which NFC version and NAND Flash page size is this?

All, presumably.

> Do you have a means of duplicating the issue?

Not personally; it was found during an IRC discussion with a2cypher.

> I wonder if the appropriate fix would not rather be to replace all  
> occurrences
> of "host->page_addr" with "page", except in mxc_nand_correct_data()  
> and
> mxc_nand_command(). Otherwise, it looks like there will still be  
> weird things
> going on with this variable.

Hmm, what's going on in mxc_nand_read_page_raw_syndrome()?  The caller  
of that calls cmdfunc, then the mxc_nand_read_page_raw_syndrome() calls  
it again, using host->page_addr that was set the first time cmdfunc was  
called? :-P

-Scott

[U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB

[Benoît Thébaudeau]

On Thursday, January 31, 2013 11:36:11 PM, Scott Wood wrote:
> On 01/31/2013 02:45:02 PM, Beno?t Th?baudeau wrote:
> > Hi Scott,
> > 
> > On Thursday, January 31, 2013 8:47:55 PM, Scott Wood wrote:
> > > Without this, all OOB reads are from the last page normally read
> > > (or zero at boot).  This results in bad block scans failing to look
> > > in the right place, and so no bad blocks are found.
> 
> Looking more closely, the calls to send_addr() use page_addr rather
> than host->page_addr, and the oob page read uses "page" in the call to
> cmdfunc, so maybe it's just the debug output that was wrong?

Looks like.

> > > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > > ---
> > > From IRC discussion with a2cypher.  Compile-tested only; testing
> > > would be appreciated.
> > >
> > >  drivers/mtd/nand/mxc_nand.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/mtd/nand/mxc_nand.c
> > b/drivers/mtd/nand/mxc_nand.c
> > > index d0ded48..32ba340 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -1021,6 +1021,7 @@ void mxc_nand_command(struct mtd_info *mtd,
> > unsigned
> > > command,
> > >  		break;
> > >
> > >  	case NAND_CMD_READOOB:
> > > +		host->page_addr = page_addr;
> > >  		host->col_addr = column;
> > >  		host->spare_only = true;
> > >  		if (host->pagesize_2k)
> > 
> > For which NFC version and NAND Flash page size is this?
> 
> All, presumably.

My i.MX platforms with NFC v2.1 and v3 can detect bad blocks (both true ones,
and the fake ones created at the end of the NAND for the BBT), so there is
something else going on if a2cypher had an issue.

> > Do you have a means of duplicating the issue?
> 
> Not personally; it was found during an IRC discussion with a2cypher.

OK.

> > I wonder if the appropriate fix would not rather be to replace all
> > occurrences
> > of "host->page_addr" with "page", except in mxc_nand_correct_data()
> > and
> > mxc_nand_command(). Otherwise, it looks like there will still be
> > weird things
> > going on with this variable.
> 
> Hmm, what's going on in mxc_nand_read_page_raw_syndrome()?  The caller
> of that calls cmdfunc, then the mxc_nand_read_page_raw_syndrome() calls
> it again, using host->page_addr that was set the first time cmdfunc was
> called? :-P

Indeed. Whatever the caller does, it's supposed to pass the right page to
mxc_nand_read_page_raw_syndrome(), so let's use the passed "page" everywhere,
and use "host->page_addr" only to keep track of accessed pages for
mxc_nand_correct_data(), so that there is no dependency between calls?

Best regards,
Beno?t