[U-Boot] Fix fsl_elbc_nand driver

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

This is my best try. I have test it with my old u-boot,
but not with master. Do not have a bench for it.
This is not very important patch. I do not find 
any other ONFI user in u-boot.
Andrei

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Mon, 2015-05-18 at 19:16 -0700, Andrei Yakimov wrote:
> This is my best try. I have test it with my old u-boot,
> but not with master. Do not have a bench for it.

A bench?

> This is not very important patch. I do not find 
> any other ONFI user in u-boot.
> Andrei
> 
> From d76b4ae8e866affa15dd9da860574d0600969d57 Mon Sep 17 00:00:00 2001
> From: Andrei Yakimov <ayakimov@iptec-inc.com>
> Date: Mon, 18 May 2015 18:50:12 -0700
> Subject: [PATCH] ONFI detect reading only first parameter page.
> 
> NAND_CMD_PARAM read only first parameter page. Need to read all 3
> (as per ONFI spec) due to no error correction for this area
> 
> Signed-off-by: Andrei Yakimov <ayakimov@iptec-inc.com>
> ---
> 
>  drivers/mtd/nand/fsl_elbc_nand.c |    9 +++++----
>  drivers/mtd/nand/fsl_ifc_nand.c  |   10 ++++++----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
> b/drivers/mtd/nand/fsl_elbc_nand.c
> index e85832d..8ac470f 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -336,11 +336,12 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
>  				    (FIR_OP_RBW << FIR_OP2_SHIFT));
>  		out_be32(&lbc->fcr, command << FCR_CMD0_SHIFT);
>  		/*
> -		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> +		 * although currently it's 8 bytes for READID
> +		 * param page containg at least 3 copy for
> +		 * robustnes, we need to read them all.
>  		 */
> -		out_be32(&lbc->fbcr, 256);
> -		ctrl->read_bytes = 256;
> +		out_be32(&lbc->fbcr, (command == NAND_CMD_PARAM) ? 786 : 8);
> +		ctrl->read_bytes = (command == NAND_CMD_PARAM) ? 786 : 8;

786?  Not 768?

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

On Tue, 2015-05-19 at 17:38 -0500, Scott Wood wrote:
> On Tue, 2015-05-19 at 15:29 -0700, Andrei Yakimov wrote:
> > I did not compiling latest, I still in 2011.9 and 2.6.38.
> > I have go over latest kernel and can see they using 
> > NAND_CMD_PARAM with sub command  0x40 - to get JEDEC
> > information, it is 3 mandatory copy by 512 bytes.
> 
> 3x512 or 3x256?
ONFI - 3x256 sub command 0x0
JEDEC - 3x512 sub command 0x40
> 
> > Going over kernel divers, figure out some read whole
> > page some 256 bytes.
> > Reading whole page (set fcbr = 0) have some sense - you do not need
> > to know anything about flash, but what to put in to read_bytes ?
> 
> You don't want fbcr = 0 here because that will enable ECC which isn't
> there.
 Is it correcting or just generating syndrome? It is working on
 my board, I would say it only generate or ignored for this command
(8313). It should corrupt data if it correcting but it does not.

> 
> > It looks like for universal patch 2K should be read.
> 
> Again, if we're going to do anything beyond s/256/768/ it should be a
> higher level function where the caller says how much it wants.
It is not normal nand  flow:  READ_ID and PARAM assuming it know the
size.

> > I have also check other vendor controllers like tegra,
> > there continuous data read trigger additional data transfer from
> > chip.
Can we do  (NOP CWO UA RWB RS RS RS RS) 
wait ltesr (cc) and after that 
next  read_buffer ( RB or RS)
all command have to start with NOP,
this will effectively terminate previous command.
And we do not care about locks in u-boot. kernel will be different
store, but again this code executed only during start up - so who care
holding CS to long.

there is only 4 places for PARAM:
drivers/mtd/nand/mxs_nand_spl.c	  chip->cmdfunc(mtd, NAND_CMD_PARAM, 0,
-1);
drivers/mtd/nand/nand_base.c: chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
drivers/mtd/nand/nand_base.c: chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
drivers/mtd/nand/nand_base.c: chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40,
-1);

latest kernel read it like this:
      chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
        for (i = 0; i < 3; i++) {
                for (j = 0; j < sizeof(*p); j++)
                        ((uint8_t *)p)[j] = chip->read_byte(mtd);
                if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
                                le16_to_cpu(p->crc)) {
                        break;
                }
        }

> 
> Yes, that's how the hardware works, but controllers like eLBC don't
> directly expose that interface.  We need to get all of the command's
> output at once.
> 
> >  This kind of implementation better, but I did not see how it could
> > be done for this controller.
> 
> I wouldn't say it's "better" so much as a closer fit to what the Linux
> NAND code is expecting.
> 
> > I am not sure how is small page (512 byte) flash should operate
> > also.
> 
> Is there any small-page ONFI flash?
I do not know.
ONFI parameter page will tell you page size:
80-83  M  Number of data bytes per page
84-85  M  Number of spare bytes per page
if we drop it, lets set to 2k and forget.
> 
> Why did you take this e-mail off-list?
Sorry just forgot.
> 
> -Scott
> 
> 

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Tue, 2015-05-19 at 16:42 -0700, Andrei Yakimov wrote:
> On Tue, 2015-05-19 at 17:38 -0500, Scott Wood wrote:
> > On Tue, 2015-05-19 at 15:29 -0700, Andrei Yakimov wrote:
> > > I did not compiling latest, I still in 2011.9 and 2.6.38.
> > > I have go over latest kernel and can see they using 
> > > NAND_CMD_PARAM with sub command  0x40 - to get JEDEC
> > > information, it is 3 mandatory copy by 512 bytes.
> > 
> > 3x512 or 3x256?
> ONFI - 3x256 sub command 0x0
> JEDEC - 3x512 sub command 0x40

So then we want 1536 bytes, not 768 (or 786) if we go with the simple
fix?

> > > Going over kernel divers, figure out some read whole
> > > page some 256 bytes.
> > > Reading whole page (set fcbr = 0) have some sense - you do not need
> > > to know anything about flash, but what to put in to read_bytes ?
> > 
> > You don't want fbcr = 0 here because that will enable ECC which isn't
> > there.
>  Is it correcting or just generating syndrome? It is working on
>  my board, I would say it only generate or ignored for this command
> (8313). It should corrupt data if it correcting but it does not.

Correcting.  Perhaps it's working because it's reporting an
uncorrectable error (thus not correcting anything) and you're ignoring
it?
 
> > > It looks like for universal patch 2K should be read.
> > 
> > Again, if we're going to do anything beyond s/256/768/ it should be a
> > higher level function where the caller says how much it wants.
> It is not normal nand  flow:  READ_ID and PARAM assuming it know the
> size.

I'm not sure what you're trying to say here.

> > > I have also check other vendor controllers like tegra,
> > > there continuous data read trigger additional data transfer from
> > > chip.
> Can we do  (NOP CWO UA RWB RS RS RS RS) 
> wait ltesr (cc) and after that 
> next  read_buffer ( RB or RS)
> all command have to start with NOP,
> this will effectively terminate previous command.
> And we do not care about locks in u-boot. kernel will be different
> store, but again this code executed only during start up - so who care
> holding CS to long.

You won't be holding CS that long.  It will drop as soon as the current
operation completes.  And I'm not interested in a solution that only
works in U-Boot's single-tasking environment, given that this code is
more or less shared with Linux.

I don't see what the objection is to adding a replaceable read_param()
method that is not so hostile to high-level controllers.

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

I will make a patch with 1536.

Where should I send linux patch?
They have bunch of mail lists for different subsystems.
Andrei


On Wed, 2015-05-20 at 17:25 -0500, Scott Wood wrote:
> On Tue, 2015-05-19 at 16:42 -0700, Andrei Yakimov wrote:
> > On Tue, 2015-05-19 at 17:38 -0500, Scott Wood wrote:
> > > On Tue, 2015-05-19 at 15:29 -0700, Andrei Yakimov wrote:
> > > > I did not compiling latest, I still in 2011.9 and 2.6.38.
> > > > I have go over latest kernel and can see they using 
> > > > NAND_CMD_PARAM with sub command  0x40 - to get JEDEC
> > > > information, it is 3 mandatory copy by 512 bytes.
> > > 
> > > 3x512 or 3x256?
> > ONFI - 3x256 sub command 0x0
> > JEDEC - 3x512 sub command 0x40
> 
> So then we want 1536 bytes, not 768 (or 786) if we go with the simple
> fix?
Yes
> 
> > > > Going over kernel divers, figure out some read whole
> > > > page some 256 bytes.
> > > > Reading whole page (set fcbr = 0) have some sense - you do not need
> > > > to know anything about flash, but what to put in to read_bytes ?
> > > 
> > > You don't want fbcr = 0 here because that will enable ECC which isn't
> > > there.
> >  Is it correcting or just generating syndrome? It is working on
> >  my board, I would say it only generate or ignored for this command
> > (8313). It should corrupt data if it correcting but it does not.
> 
> Correcting.  Perhaps it's working because it's reporting an
> uncorrectable error (thus not correcting anything) and you're ignoring
> it?
may be.
>  
> > > > It looks like for universal patch 2K should be read.
> > > 
> > > Again, if we're going to do anything beyond s/256/768/ it should be a
> > > higher level function where the caller says how much it wants.
> > It is not normal nand  flow:  READ_ID and PARAM assuming it know the
> > size.
> 
> I'm not sure what you're trying to say here.
I meant normal nand flow read/write - propagate size how much to read
or write, READ_ID and PARAM regular nand flow assume driver would know
how much to read.
> 
> > > > I have also check other vendor controllers like tegra,
> > > > there continuous data read trigger additional data transfer from
> > > > chip.
> > Can we do  (NOP CWO UA RWB RS RS RS RS) 
> > wait ltesr (cc) and after that 
> > next  read_buffer ( RB or RS)
> > all command have to start with NOP,
> > this will effectively terminate previous command.
> > And we do not care about locks in u-boot. kernel will be different
> > store, but again this code executed only during start up - so who care
> > holding CS to long.
> 
> You won't be holding CS that long.  It will drop as soon as the current
> operation completes.  And I'm not interested in a solution that only
> works in U-Boot's single-tasking environment, given that this code is
> more or less shared with Linux.
Are you saying elbc will drop CS even last fir instruction not 0?
You are at Freescale - you should know or can check :).
About lock, I was only saying linux will might need a lock for this
sequences, depend on nand flash detection can or can not run in parallel
if you have multiple chips - but I do not think it can - it is early
boot an it is not how nand initializes. MTD doing it at once.	
> 
> I don't see what the objection is to adding a replaceable read_param()
> method that is not so hostile to high-level controllers.
Sorry, I has not understand you completely.
Are you suggesting add read_param() method to whole nand infrastructure,
for NAND_CMD_PARAM method? It is huge changes and this will not change
fact some how we should get information about read size.
For elbc and imx due to we reading all at once, it can not be stateless,
we need to read more and more each time reissuing command or relay
on different way to ID chip - and this make it pointless if it can not
be done universally.
> 
> -Scott
> 
> 

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Wed, 2015-05-20 at 18:27 -0700, Andrei Yakimov wrote:
> I will make a patch with 1536.
> 
> Where should I send linux patch?
> They have bunch of mail lists for different subsystems.
> Andrei

The MAINTAINERS file shows where to send patches for different
subsystems.  In this case it would be the "MEMORY TECHNOLOGY DEVICES
(MTD)" section.

> On Wed, 2015-05-20 at 17:25 -0500, Scott Wood wrote:
> > On Tue, 2015-05-19 at 16:42 -0700, Andrei Yakimov wrote:
> > > On Tue, 2015-05-19 at 17:38 -0500, Scott Wood wrote:
> > > > On Tue, 2015-05-19 at 15:29 -0700, Andrei Yakimov wrote:
> > > > > I did not compiling latest, I still in 2011.9 and 2.6.38.
> > > > > I have go over latest kernel and can see they using 
> > > > > NAND_CMD_PARAM with sub command  0x40 - to get JEDEC
> > > > > information, it is 3 mandatory copy by 512 bytes.
> > > > 
> > > > 3x512 or 3x256?
> > > ONFI - 3x256 sub command 0x0
> > > JEDEC - 3x512 sub command 0x40
> > 
> > So then we want 1536 bytes, not 768 (or 786) if we go with the simple
> > fix?
> Yes
> > 
> > > > > Going over kernel divers, figure out some read whole
> > > > > page some 256 bytes.
> > > > > Reading whole page (set fcbr = 0) have some sense - you do not need
> > > > > to know anything about flash, but what to put in to read_bytes ?
> > > > 
> > > > You don't want fbcr = 0 here because that will enable ECC which isn't
> > > > there.
> > >  Is it correcting or just generating syndrome? It is working on
> > >  my board, I would say it only generate or ignored for this command
> > > (8313). It should corrupt data if it correcting but it does not.
> > 
> > Correcting.  Perhaps it's working because it's reporting an
> > uncorrectable error (thus not correcting anything) and you're ignoring
> > it?
> may be.
> >  
> > > > > It looks like for universal patch 2K should be read.
> > > > 
> > > > Again, if we're going to do anything beyond s/256/768/ it should be a
> > > > higher level function where the caller says how much it wants.
> > > It is not normal nand  flow:  READ_ID and PARAM assuming it know the
> > > size.
> > 
> > I'm not sure what you're trying to say here.
> I meant normal nand flow read/write - propagate size how much to read
> or write, READ_ID and PARAM regular nand flow assume driver would know
> how much to read.

No, they don't assume the driver knows how much to read.  They assume
the driver doesn't need to tell the hardware how much to read, which
isn't true with this controller.

> > > > > I have also check other vendor controllers like tegra,
> > > > > there continuous data read trigger additional data transfer from
> > > > > chip.
> > > Can we do  (NOP CWO UA RWB RS RS RS RS) 
> > > wait ltesr (cc) and after that 
> > > next  read_buffer ( RB or RS)
> > > all command have to start with NOP,
> > > this will effectively terminate previous command.
> > > And we do not care about locks in u-boot. kernel will be different
> > > store, but again this code executed only during start up - so who care
> > > holding CS to long.
> > 
> > You won't be holding CS that long.  It will drop as soon as the current
> > operation completes.  And I'm not interested in a solution that only
> > works in U-Boot's single-tasking environment, given that this code is
> > more or less shared with Linux.
> Are you saying elbc will drop CS even last fir instruction not 0?
> You are at Freescale - you should know or can check :).

That is my understanding of how the hardware works, yes (though I
haven't tested or asked someone who knows the implementation).  The bit
about NOP ending the operation sequence just means that the hardware
won't look at any subsequent fields in FIR once it finds a NOP.

> About lock, I was only saying linux will might need a lock for this
> sequences, depend on nand flash detection can or can not run in parallel
> if you have multiple chips - but I do not think it can - it is early
> boot an it is not how nand initializes. MTD doing it at once.	

What if the user inserts a NAND module after boot, while NOR activity is
going on in the background?

In any case, I really don't want to do such things in this driver.

> > I don't see what the objection is to adding a replaceable read_param()
> > method that is not so hostile to high-level controllers.
> Sorry, I has not understand you completely.
> Are you suggesting add read_param() method to whole nand infrastructure,
> for NAND_CMD_PARAM method?

Yes.

>  It is huge changes

It's not.  The default implementation would contain the more or less the
same code that runs today.

> and this will not change fact some how we should get information about read size.

The size to be read would be a parameter to read_param().

> For elbc and imx due to we reading all at once, it can not be stateless,
> we need to read more and more each time

Why do we need to?  Why can't we read all three copies at once?

> reissuing command or relay on different way to ID chip - and this make
> it pointless if it can not be done universally.

Or, we can reissue the command.  I don't see any big problem either way.
This is not performance critical.

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

On Wed, 2015-05-20 at 20:37 -0500, Scott Wood wrote:
> On Wed, 2015-05-20 at 18:27 -0700, Andrei Yakimov wrote:
> > I will make a patch with 1536.
> > 
> > Where should I send linux patch?
> > They have bunch of mail lists for different subsystems.
> > Andrei
> 
> The MAINTAINERS file shows where to send patches for different
> subsystems.  In this case it would be the "MEMORY TECHNOLOGY DEVICES
> (MTD)" section.
> 
> > On Wed, 2015-05-20 at 17:25 -0500, Scott Wood wrote:
> > > On Tue, 2015-05-19 at 16:42 -0700, Andrei Yakimov wrote:
> > > > On Tue, 2015-05-19 at 17:38 -0500, Scott Wood wrote:
> > > > > On Tue, 2015-05-19 at 15:29 -0700, Andrei Yakimov wrote:
> > > > > > I did not compiling latest, I still in 2011.9 and 2.6.38.
> > > > > > I have go over latest kernel and can see they using 
> > > > > > NAND_CMD_PARAM with sub command  0x40 - to get JEDEC
> > > > > > information, it is 3 mandatory copy by 512 bytes.
> > > > > 
> > > > > 3x512 or 3x256?
> > > > ONFI - 3x256 sub command 0x0
> > > > JEDEC - 3x512 sub command 0x40
> > > 
> > > So then we want 1536 bytes, not 768 (or 786) if we go with the simple
> > > fix?
> > Yes
> > > 
> > > > > > Going over kernel divers, figure out some read whole
> > > > > > page some 256 bytes.
> > > > > > Reading whole page (set fcbr = 0) have some sense - you do not need
> > > > > > to know anything about flash, but what to put in to read_bytes ?
> > > > > 
> > > > > You don't want fbcr = 0 here because that will enable ECC which isn't
> > > > > there.
> > > >  Is it correcting or just generating syndrome? It is working on
> > > >  my board, I would say it only generate or ignored for this command
> > > > (8313). It should corrupt data if it correcting but it does not.
> > > 
> > > Correcting.  Perhaps it's working because it's reporting an
> > > uncorrectable error (thus not correcting anything) and you're ignoring
> > > it?
> > may be.
> > >  
> > > > > > It looks like for universal patch 2K should be read.
> > > > > 
> > > > > Again, if we're going to do anything beyond s/256/768/ it should be a
> > > > > higher level function where the caller says how much it wants.
> > > > It is not normal nand  flow:  READ_ID and PARAM assuming it know the
> > > > size.
> > > 
> > > I'm not sure what you're trying to say here.
> > I meant normal nand flow read/write - propagate size how much to read
> > or write, READ_ID and PARAM regular nand flow assume driver would know
> > how much to read.
> 
> No, they don't assume the driver knows how much to read.  They assume
> the driver doesn't need to tell the hardware how much to read, which
> isn't true with this controller.
> 
> > > > > > I have also check other vendor controllers like tegra,
> > > > > > there continuous data read trigger additional data transfer from
> > > > > > chip.
> > > > Can we do  (NOP CWO UA RWB RS RS RS RS) 
> > > > wait ltesr (cc) and after that 
> > > > next  read_buffer ( RB or RS)
> > > > all command have to start with NOP,
> > > > this will effectively terminate previous command.
> > > > And we do not care about locks in u-boot. kernel will be different
> > > > store, but again this code executed only during start up - so who care
> > > > holding CS to long.
> > > 
> > > You won't be holding CS that long.  It will drop as soon as the current
> > > operation completes.  And I'm not interested in a solution that only
> > > works in U-Boot's single-tasking environment, given that this code is
> > > more or less shared with Linux.
> > Are you saying elbc will drop CS even last fir instruction not 0?
> > You are at Freescale - you should know or can check :).
> 
> That is my understanding of how the hardware works, yes (though I
> haven't tested or asked someone who knows the implementation).  The bit
> about NOP ending the operation sequence just means that the hardware
> won't look at any subsequent fields in FIR once it finds a NOP.
> 
> > About lock, I was only saying linux will might need a lock for this
> > sequences, depend on nand flash detection can or can not run in parallel
> > if you have multiple chips - but I do not think it can - it is early
> > boot an it is not how nand initializes. MTD doing it at once.	
> 
> What if the user inserts a NAND module after boot, while NOR activity is
> going on in the background?
> 
> In any case, I really don't want to do such things in this driver.
> 
> > > I don't see what the objection is to adding a replaceable read_param()
> > > method that is not so hostile to high-level controllers.
> > Sorry, I has not understand you completely.
> > Are you suggesting add read_param() method to whole nand infrastructure,
> > for NAND_CMD_PARAM method?
> 
> Yes.
> 
> >  It is huge changes
> 
> It's not.  The default implementation would contain the more or less the
> same code that runs today.
> 
> > and this will not change fact some how we should get information about read size.
> 
> The size to be read would be a parameter to read_param().
> 
> > For elbc and imx due to we reading all at once, it can not be stateless,
> > we need to read more and more each time
> 
> Why do we need to?  Why can't we read all three copies at once?
> 
> > reissuing command or relay on different way to ID chip - and this make
> > it pointless if it can not be done universally.
> 
> Or, we can reissue the command.  I don't see any big problem either way.
> This is not performance critical.
lets say  1 time you read 256 ( or 512) it go bad, next time you read
512 (or 1024) next time you read 768 ( or 1536).
Upper layer can maintain it.
Roughly like this:

Was:
	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
	for (i = 0; i < 3; i++) {
		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
						le16_to_cpu(p->crc)) {
			break;
		}
	}
	if (i == 3)
		return 0;

new:
	int read_size, offset;
	read_size = 256;
	offset =0;
	if(!chip->read_param)
		chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
	for (i = 0; i < 3; i++) {
		if(chip->read_param) chip->read_param( 0, read_size);
		chip->read_buf(mtd, (uint8_t *)p + offest, sizeof(*p));
		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
						le16_to_cpu(p->crc)) {
			break;
		}
		offset = read_size;
		read_size += 256; /* for ONFI only */ 
	}

	if (i == 3)
		return 0;

I have used old u-boot, but more or less like this.
chip structure would have extra member read_param().
> 
> -Scott
> 
> 

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Wed, 2015-05-20 at 18:55 -0700, Andrei Yakimov wrote:
> On Wed, 2015-05-20 at 20:37 -0500, Scott Wood wrote:
> > On Wed, 2015-05-20 at 18:27 -0700, Andrei Yakimov wrote:
> > > For elbc and imx due to we reading all at once, it can not be stateless,
> > > we need to read more and more each time
> > 
> > Why do we need to?  Why can't we read all three copies at once?
> > 
> > > reissuing command or relay on different way to ID chip - and this make
> > > it pointless if it can not be done universally.
> > 
> > Or, we can reissue the command.  I don't see any big problem either way.
> > This is not performance critical.
> lets say  1 time you read 256 ( or 512) it go bad, next time you read
> 512 (or 1024) next time you read 768 ( or 1536).

I was thinking read_param() would take the offset as a parameter and use
NAND_CMD_RNDOUT to skip ahead -- but that would change the default flow
which I'd rather avoid.  Another option is that read_param() just sets
up the read for the specified number of bytes, but the caller still uses
read_byte() to extract the data.  This way the code could specify
sizeof(struct)*3 as the size up front without needing three separate
buffers.

Note that whatever gets done should first be accepted in Linux, rather
than being a local U-Boot change.  If you want a short-term fix, just
stick 1536 in the eLBC driver.

> Upper layer can maintain it.
> Roughly like this:
> 
> Was:
> 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> 	for (i = 0; i < 3; i++) {
> 		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));

You're looking at old code.  It uses read_byte() now.

> 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> 						le16_to_cpu(p->crc)) {
> 			break;
> 		}
> 	}
> 	if (i == 3)
> 		return 0;
> 
> new:
> 	int read_size, offset;
> 	read_size = 256;
> 	offset =0;
> 	if(!chip->read_param)
> 		chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);

I don't want "if (chip->read_param)" all over the place; there should be
a default read_param() that does what the existing code does.

> 	for (i = 0; i < 3; i++) {
> 		if(chip->read_param) chip->read_param( 0, read_size);
> 		chip->read_buf(mtd, (uint8_t *)p + offest, sizeof(*p));

This isn't going to read the second or third copy; it's going to read
the first copy and write beyond the end of your buffer.

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

For now lets stick with 1536 in u-boot.
I will send a patch.
At least it will not loosing flash over time
as nand ages.

I understand what you wish, and will take a look
on it inside fresh new kernel. I found one more driver  -
marvel looks like have same problem.
I will check how NAND_CMD_RNDOUT is working.
Perhaps we do not need extra read_param(),
and use only NAND_CMD_RNDOUT to get next
block inside page loop.

Andrei

On Wed, 2015-05-20 at 21:27 -0500, Scott Wood wrote:
> On Wed, 2015-05-20 at 18:55 -0700, Andrei Yakimov wrote:
> > On Wed, 2015-05-20 at 20:37 -0500, Scott Wood wrote:
> > > On Wed, 2015-05-20 at 18:27 -0700, Andrei Yakimov wrote:
> > > > For elbc and imx due to we reading all at once, it can not be stateless,
> > > > we need to read more and more each time
> > > 
> > > Why do we need to?  Why can't we read all three copies at once?
> > > 
> > > > reissuing command or relay on different way to ID chip - and this make
> > > > it pointless if it can not be done universally.
> > > 
> > > Or, we can reissue the command.  I don't see any big problem either way.
> > > This is not performance critical.
> > lets say  1 time you read 256 ( or 512) it go bad, next time you read
> > 512 (or 1024) next time you read 768 ( or 1536).
> 
> I was thinking read_param() would take the offset as a parameter and use
> NAND_CMD_RNDOUT to skip ahead -- but that would change the default flow
> which I'd rather avoid.  Another option is that read_param() just sets
> up the read for the specified number of bytes, but the caller still uses
> read_byte() to extract the data.  This way the code could specify
> sizeof(struct)*3 as the size up front without needing three separate
> buffers.
> 
> Note that whatever gets done should first be accepted in Linux, rather
> than being a local U-Boot change.  If you want a short-term fix, just
> stick 1536 in the eLBC driver.
> 
> > Upper layer can maintain it.
> > Roughly like this:
> > 
> > Was:
> > 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > 	for (i = 0; i < 3; i++) {
> > 		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> 
> You're looking at old code.  It uses read_byte() now.
> 
> > 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > 						le16_to_cpu(p->crc)) {
> > 			break;
> > 		}
> > 	}
> > 	if (i == 3)
> > 		return 0;
> > 
> > new:
> > 	int read_size, offset;
> > 	read_size = 256;
> > 	offset =0;
> > 	if(!chip->read_param)
> > 		chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> 
> I don't want "if (chip->read_param)" all over the place; there should be
> a default read_param() that does what the existing code does.
> 
> > 	for (i = 0; i < 3; i++) {
> > 		if(chip->read_param) chip->read_param( 0, read_size);
> > 		chip->read_buf(mtd, (uint8_t *)p + offest, sizeof(*p));
> 
> This isn't going to read the second or third copy; it's going to read
> the first copy and write beyond the end of your buffer.
> 
> -Scott
> 
> 

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Wed, 2015-05-20 at 19:42 -0700, Andrei Yakimov wrote:
> For now lets stick with 1536 in u-boot.
> I will send a patch.
> At least it will not loosing flash over time
> as nand ages.
> 
> I understand what you wish, and will take a look
> on it inside fresh new kernel. I found one more driver  -
> marvel looks like have same problem.
> I will check how NAND_CMD_RNDOUT is working.
> Perhaps we do not need extra read_param(),
> and use only NAND_CMD_RNDOUT to get next
> block inside page loop.

Again, I'm a reluctant to use RNDOUT in the default read_param() because
that would change the flow for all controllers and chips, and while the
chip manual I'm looking at says it's OK, it introduces risk that it
doesn't work everywhere (e.g. some controller drivers that provide their
own cmdfunc don't implement RNDOUT).

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

On Wed, 2015-05-20 at 21:46 -0500, Scott Wood wrote:
> On Wed, 2015-05-20 at 19:42 -0700, Andrei Yakimov wrote:
> > For now lets stick with 1536 in u-boot.
> > I will send a patch.
> > At least it will not loosing flash over time
> > as nand ages.
> > 
> > I understand what you wish, and will take a look
> > on it inside fresh new kernel. I found one more driver  -
> > marvel looks like have same problem.
> > I will check how NAND_CMD_RNDOUT is working.
> > Perhaps we do not need extra read_param(),
> > and use only NAND_CMD_RNDOUT to get next
> > block inside page loop.
> 
> Again, I'm a reluctant to use RNDOUT in the default read_param() because
> that would change the flow for all controllers and chips, and while the
> chip manual I'm looking at says it's OK, it introduces risk that it
> doesn't work everywhere (e.g. some controller drivers that provide their
> own cmdfunc don't implement RNDOUT).
Forget about read_param(), 
just like this:
  for (i = 0; i < 3; i++) {
                for (j = 0; j < sizeof(*p); j++)
                        ((uint8_t *)p)[j] = chip->read_byte(mtd);
                if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
                                le16_to_cpu(p->crc)) {
                        break;
                }
		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1);
  }


and this is good - will be "no op"  or "bad command" error,
which could be ignored - so for this drivers operation flow is
unchanged.
> -Scott
> 
> 
I am still learning git/patman.
It will be day or two while I figure out patman.
By some reason after "git commit --amend" patman kill my patch.
I am missing something.
Andrei

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Wed, 2015-05-20 at 20:03 -0700, Andrei Yakimov wrote:
> On Wed, 2015-05-20 at 21:46 -0500, Scott Wood wrote:
> > On Wed, 2015-05-20 at 19:42 -0700, Andrei Yakimov wrote:
> > > For now lets stick with 1536 in u-boot.
> > > I will send a patch.
> > > At least it will not loosing flash over time
> > > as nand ages.
> > > 
> > > I understand what you wish, and will take a look
> > > on it inside fresh new kernel. I found one more driver  -
> > > marvel looks like have same problem.
> > > I will check how NAND_CMD_RNDOUT is working.
> > > Perhaps we do not need extra read_param(),
> > > and use only NAND_CMD_RNDOUT to get next
> > > block inside page loop.
> > 
> > Again, I'm a reluctant to use RNDOUT in the default read_param() because
> > that would change the flow for all controllers and chips, and while the
> > chip manual I'm looking at says it's OK, it introduces risk that it
> > doesn't work everywhere (e.g. some controller drivers that provide their
> > own cmdfunc don't implement RNDOUT).

RNDOUT is already used by nand_flash_detect_ext_param_page(), so this
isn't as much of a concern for ONFI, but it could be an issue with
nand_flash_detect_jedec().

> Forget about read_param(), 

Then how will it work on controllers like eLBC/IFC which is the whole
point?

> just like this:
>   for (i = 0; i < 3; i++) {
>                 for (j = 0; j < sizeof(*p); j++)
>                         ((uint8_t *)p)[j] = chip->read_byte(mtd);
>                 if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>                                 le16_to_cpu(p->crc)) {
>                         break;
>                 }
> 		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1);
>   }
> 
> 
> and this is good - will be "no op"  or "bad command" error,
> which could be ignored - so for this drivers operation flow is
> unchanged.

RNDOUT needs to come before read_buf() and it needs to specify the
offset you want.

-Scott

[U-Boot] Fix fsl_elbc_nand driver

[Andrei Yakimov]

On Wed, 2015-05-20 at 22:11 -0500, Scott Wood wrote:
> On Wed, 2015-05-20 at 20:03 -0700, Andrei Yakimov wrote:
> > On Wed, 2015-05-20 at 21:46 -0500, Scott Wood wrote:
> > > On Wed, 2015-05-20 at 19:42 -0700, Andrei Yakimov wrote:
> > > > For now lets stick with 1536 in u-boot.
> > > > I will send a patch.
> > > > At least it will not loosing flash over time
> > > > as nand ages.
> > > > 
> > > > I understand what you wish, and will take a look
> > > > on it inside fresh new kernel. I found one more driver  -
> > > > marvel looks like have same problem.
> > > > I will check how NAND_CMD_RNDOUT is working.
> > > > Perhaps we do not need extra read_param(),
> > > > and use only NAND_CMD_RNDOUT to get next
> > > > block inside page loop.
> > > 
> > > Again, I'm a reluctant to use RNDOUT in the default read_param() because
> > > that would change the flow for all controllers and chips, and while the
> > > chip manual I'm looking at says it's OK, it introduces risk that it
> > > doesn't work everywhere (e.g. some controller drivers that provide their
> > > own cmdfunc don't implement RNDOUT).
> 
> RNDOUT is already used by nand_flash_detect_ext_param_page(), so this
> isn't as much of a concern for ONFI, but it could be an issue with
> nand_flash_detect_jedec().
> 
> > Forget about read_param(), 
> 
> Then how will it work on controllers like eLBC/IFC which is the whole
> point?
> 
> > just like this:
 I miss this line:
      chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> >   for (i = 0; i < 3; i++) {
> >                 for (j = 0; j < sizeof(*p); j++)
> >                         ((uint8_t *)p)[j] = chip->read_byte(mtd);
> >                 if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >                                 le16_to_cpu(p->crc)) {
> >                         break;
> >                 }
> > 		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, <offest>, -1);
> >   }
> > 
> > 
> > and this is good - will be "no op"  or "bad command" error,
> > which could be ignored - so for this drivers operation flow is
> > unchanged.
> 
> RNDOUT needs to come before read_buf() and it needs to specify the
> offset you want.
> 
column address - it is exactly offset for RNDOUT.
First param read 256/512 bytes, if it fail, we do RNDOUT to get next.
we will not do extra RNDOUT - it is for() loop.

I can test it on my board. This is really good solution if it work.
it is just 1 line, and only when ONFI mark already read from flash.

And we can leave (NAND_CMD_PARAM, 0x40) (JDEC label) handling for kernel
folks.

My problem only jesd230B do not specify PARAM command,
ONFI4.0 - do not expect column address for PARAM.

Linux kernel cleary doing (NAND_CMD_PARAM, 0x40).

This is bit annoying.

Question is elbc/ifc old controllers  - is it worth the effort?

> -Scott
> 
> 

[U-Boot] Fix fsl_elbc_nand driver

[Scott Wood]

On Wed, 2015-05-20 at 20:54 -0700, Andrei Yakimov wrote:
> My problem only jesd230B do not specify PARAM command,
> ONFI4.0 - do not expect column address for PARAM.
> 
> Linux kernel cleary doing (NAND_CMD_PARAM, 0x40).
> 
> This is bit annoying.
> 
> Question is elbc/ifc old controllers  - is it worth the effort?

IFC isn't an old controller.

As for worth the effort, probably not.  1536 is easy. :-)

-Scott