From mboxrd@z Thu Jan  1 00:00:00 1970
From: Luka Perkov <uboot@lukaperkov.net>
Date: Tue, 14 Feb 2012 01:51:28 +0100
Subject: [U-Boot] [PATCH v2] fix CFI flash driver for 8-bit bus support
In-Reply-To: <20120213103230.F187225E9F5@gemini.denx.de>
References: <20120213013045.GA25085@w500.lan>
	<20120213103230.F187225E9F5@gemini.denx.de>
Message-ID: <20120214005128.GA26487@w500.lan>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Wolfgang,

thank you for your pointers. Like I have said I'm not the one that
submited v1 and in v2 I only changed what was pointed after v1.

On Mon, Feb 13, 2012 at 11:32:30AM +0100, Wolfgang Denk wrote:
> > -			udelay(1);
> 
> This is an unrelated change.  It has no place in this patch.

Yes, you are right. I'll leave this alone for now.

> Please submit separately, with respective comments for why uyou are
> doing this, if you really want to change this.
> 
> >  static flash_sect_t find_sector (flash_info_t * info, ulong addr)
> >  {
> >  	static flash_sect_t saved_sector = 0; /* previously found sector */
> > -	static flash_info_t *saved_info = 0; /* previously used flash bank */
> >  	flash_sect_t sector = saved_sector;
> >  
> > -	if ((info != saved_info) || (sector >= info->sector_count))
> > -		sector = 0;
> 
> I think this is bogus.   Please clean up your patch!

Yes, this probably is not needed.
 
> > -		p[i] = flash_read_uchar(info, start + i);
> > +		p[i] = flash_read_uchar(info, start + (i * 2));
> 
> On which systems / in which configurations has this code been tested?

Answer at the end of this mail.

> > -	/* Issue FLASH reset command */
> > -	flash_cmd_reset(info);
> 
> really?
> 
> >  	for (cfi_offset=0;
> >  	     cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint);
> >  	     cfi_offset++) {
> > +		/* Issue FLASH reset command */
> > +		flash_cmd_reset(info);
> 
> really??

Nobody commented this in v1. I'll test without this change.

> >  		if (flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP, 'Q')
> > -		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 1, 'R')
> > -		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
> > +		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'R')
> > +		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 4, 'Y')) {
> >  			flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
> >  					sizeof(struct cfi_qry));

...

> Which configurations have been actually tested?

DLink DNS323, board that I'm working on (orion5x, arm926ejs). Also it
was tested in v1:

http://lists.denx.de/pipermail/u-boot/2011-April/089611.html

I will test this also on a MIPS board that I have in the next few days.
I did not have time to do it today.

I'll resend v3 after more testing and code reading.

Regards,
Luka