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