All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB
@ 2013-01-31 19:47 Scott Wood
  2013-01-31 20:45 ` Benoît Thébaudeau
  0 siblings, 1 reply; 4+ messages in thread
From: Scott Wood @ 2013-01-31 19:47 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB
  2013-01-31 19:47 [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB Scott Wood
@ 2013-01-31 20:45 ` Benoît Thébaudeau
  2013-01-31 22:36   ` Scott Wood
  0 siblings, 1 reply; 4+ messages in thread
From: Benoît Thébaudeau @ 2013-01-31 20:45 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB
  2013-01-31 20:45 ` Benoît Thébaudeau
@ 2013-01-31 22:36   ` Scott Wood
  2013-01-31 23:01     ` Benoît Thébaudeau
  0 siblings, 1 reply; 4+ messages in thread
From: Scott Wood @ 2013-01-31 22:36 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB
  2013-01-31 22:36   ` Scott Wood
@ 2013-01-31 23:01     ` Benoît Thébaudeau
  0 siblings, 0 replies; 4+ messages in thread
From: Benoît Thébaudeau @ 2013-01-31 23:01 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-01-31 23:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-31 19:47 [U-Boot] [PATCH] nand/mxc: set host->page_addr for NAND_CMD_READOOB Scott Wood
2013-01-31 20:45 ` Benoît Thébaudeau
2013-01-31 22:36   ` Scott Wood
2013-01-31 23:01     ` Benoît Thébaudeau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.