From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Tue, 6 Nov 2007 08:48:39 +0100
Subject: [U-Boot-Users] Mixing CFI and non-CFI flashs?
In-Reply-To: <20071105232442.GA9737@discworld.dascon.de>
References: <472B8E46.7000808@discworld.dascon.de>
	<200711051221.50299.sr@denx.de>
	<20071105232442.GA9737@discworld.dascon.de>
Message-ID: <200711060848.39359.sr@denx.de>
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

On Tuesday 06 November 2007, Michael Schwingen wrote:
> On Mon, Nov 05, 2007 at 12:21:50PM +0100, Stefan Roese wrote:
> > I don't like the indentation problem we get from this #ifdef here.
> > Perhaps we should add a __weak__ function flash_detect_legacy() in this
> > file, that can be overridden by board specific functions. Like this:
> >
> > ulong __flash_detect_legacy(ulong base, int banknum)
> > {
> > 	return 0;
> > }
> > ulong flash_detect_legacy(ulong base, int banknum) __attribute__((weak,
> > alias("__flash_detect_legacy")));
> >
> > This way we get rid of the #ifdef too.
>
> Hm - I need some common code at that point, which would otherwise be
> duplicated in every board code. I have now moved the code to a function
> flash_detect_legacy.

OK.

> > >  			for (j = 0; j < erase_region_count; j++) {
> > > +				if (sect_cnt >= CFG_MAX_FLASH_SECT)
> > > +					break;
> >
> > Please add an error output here too.
>
> Done.
>
>
> Okey, next version. The board-specific code may either fill out the
> complete flash_info struct as in the previous patch, or (preferred), only
> set info->portwidth, info->chipwidth and info->interface, in which case the
> code will probe the flash by jedec IDs and look up a table in
> jedec_flash.c. The table is a near copy of the table in the Linux
> jedec_flash.c, with the removal of some unused fields.

Wouldn't it be better not to remove those unused fields? This way porting of 
new devices from Linux to U-Boot would be easier.

> There is one problem with the AMD_ADDR_* definitions: I believe they are
> only correct for 16-bit flash ROMS (in 8/16 bit mode), but are wrong for
> 8-bit flashs. I think the CFI "interface" parameter should be the correct
> way to distinguish these cases. What is left is the number of bits in the
> unlock addresses - I have used 0x2AAA/0x5555, since all flash roms I know
> treat the upper bits as "don't care" (and specify that behaviour in the
> datasheet), so if the flash datasheet specifies 0xAAA/0x555, using the
> longer constants should do no harm. However, there are probably lots of
> flash roms I do *not* know, so I would appreciate feedback on this. If
> different addresses are really necessary, we would need to add them to the
> flash_info struct.

Correct. And this is the case in the current Linux implementation. So we 
should not remove it, even if we don't use it for now.

> This currently works on my IXP425 board (big endian) with one SST39VF020
> and one Intel TE28F640J3. I am not sure if the jedec flash code is correct
> in case of 16-bit JEDEC flashs or multiple 8-bit flash roms on a wider data
> bus.

Thanks. Very nice work.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================