From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Thu, 11 Dec 2008 12:01:11 +0100
Subject: [U-Boot] [PATCH v2] Add support for boards where the NOR FLASH
	is not memory-mapped
In-Reply-To: <20081210175658.GA19779@ld0162-tx32.am.freescale.net>
References: <1228905671-30411-1-git-send-email-sr@denx.de>
	<20081210175658.GA19779@ld0162-tx32.am.freescale.net>
Message-ID: <200812111201.12242.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 Wednesday 10 December 2008, Scott Wood wrote:
> On Wed, Dec 10, 2008 at 11:41:11AM +0100, Stefan Roese wrote:
> > +void *board_flash_read_memcpy(void *__to, __const__ void *__from, int
> > n); +
> > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> > +static inline int flash_read(void *__to, __const__ void *__from, int n)
>
> __const__?  The other underscores seem gratuitous as well.

Not sure where I copied this from. I'll remove the underscores in the next 
version.

> > +{
> > +#if defined(CONFIG_HAS_DATAFLASH)
> > +	return read_dataflash(__from, n, __to);
> > +#elif defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> > +	if ((addr2info((u32)__from) != NULL))
> > +		board_flash_read_memcpy(__to, __from, n);
> > +	else
> > +		memcpy(__to, __from, n);
> > +	return OK;
> > +#endif
> > +}
>
> What if a board has dataflash *and* NOR flash?

You probably mean a board with dataflash *and* NOR FLASH that can't be 
accessed memory-mapped. We currently don't have such a board. The upcoming 
VCT board port is the first with such an (cumbersome) local bus interface. 
And I hope it will be the last.

> > diff --git a/common/env_flash.c b/common/env_flash.c
> > index 75ee8dd..ee7c4d7 100644
> > --- a/common/env_flash.c
> > +++ b/common/env_flash.c
> > @@ -85,9 +85,50 @@ static ulong end_addr_new = CONFIG_ENV_ADDR_REDUND +
> > CONFIG_ENV_SECT_SIZE - 1; extern uchar default_environment[];
> >  extern int default_environment_size;
> >
> > +void *board_flash_read_memcpy(void *__to, __const__ void *__from, int
> > n); +u8 flash_read8(void *addr);
> > +u32 flash_read32(void *addr);
>
> Should go in a header file.

OK.

> > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> > +#define FLASH_READ_MEMCPY(d, s, n)	board_flash_read_memcpy(d, s, n)
> > +#else
> > +#define FLASH_READ_MEMCPY(d, s, n)	memcpy(d, s, n);
> > +#endif /* CONFIG_FLASH_NOT_MEM_MAPPED */
>
> No semicolon after memcpy, or FLASH_READ_MEMCPY won't behave like a
> function call in certain contexts (if/else, for loops, etc).

Right. Thanks for spotting.

> > +#define DO1(buf)							\
> > +	do {								\
> > +		if (((u32)buf >= CONFIG_FLASH_BASE) &&			\
> > +		    ((u32)buf < CONFIG_FLASH_END)) {			\
> > +			crc = crc_table[ ((int)crc ^			\
> > +					 (flash_read8((void *)buf++))) & 0xff] ^ \
> > +				(crc >> 8);				\
> > +		} else {						\
> > +			crc = crc_table[((int)crc ^			\
> > +					 (*buf++)) & 0xff] ^ (crc >> 8); \
> > +		}							\
> > +	} while (0)
>
> Maybe make this an inline function?

That would be a bigger change since this macro changes "buf" and "crc" without 
having those parameters passed to it. I didn't want to make this bigger 
change if not really necessary.

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