[U-Boot] [PATCH] Add support for boards where the NOR FLASH is not memory-mapped

[U-Boot] [PATCH] Add support for boards where the NOR FLASH is not memory-mapped

[Stefan Roese]

This patch adds the CONFIG_FLASH_NOT_MEM_MAPPED define which can be
used on boards where the NOR FLASH is not memory-mapped and
special accessor functions are needed to access the NOR FLASH.
This affects the memory commands (cmd_mem.c) and the environment
(env_flash.c).

Boards using CONFIG_FLASH_NOT_MEM_MAPPED need to additionally specify
CONFIG_FLASH_BASE and CONFIG_FLASH_END so that the NOR FLASH region
can be detected.

This will be used by the upcoming VCTH board support.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 common/cmd_mem.c    |   66 +++++++++++++++++++++++++++++++++++++++-----------
 common/env_flash.c  |   61 +++++++++++++++++++++++++++++++++++++----------
 lib_generic/crc32.c |   11 ++++++++
 3 files changed, 110 insertions(+), 28 deletions(-)

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index d7666c2..f545f6a 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -43,6 +43,43 @@
 #define PRINTF(fmt,args...)
 #endif
 
+#if defined(CONFIG_HAS_DATAFLASH)
+#define CONFIG_FLASH_NOT_MEM_MAPPED
+#define OK		DATAFLASH_OK
+#endif
+
+#if !defined(OK)
+#define OK		1
+#endif
+
+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)
+{
+#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
+}
+#endif /* defined(CONFIG_FLASH_NOT_MEM_MAPPED) */
+
+static inline int is_flash_addr(u32 addr)
+{
+#if defined(CONFIG_HAS_DATAFLASH)
+	return addr_dataflash(addr);
+#elif defined(CONFIG_FLASH_NOT_MEM_MAPPED)
+	return (addr2info(addr) != NULL);
+#else
+	return 1;
+#endif
+}
+
 static int mod_mem(cmd_tbl_t *, int, int, int, char *[]);
 
 /* Display values from last command.
@@ -63,7 +100,7 @@ static	ulong	base_address = 0;
 int do_mem_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {
 	ulong	addr, length;
-#if defined(CONFIG_HAS_DATAFLASH)
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
 	ulong	nbytes, linebytes;
 #endif
 	int	size;
@@ -100,7 +137,7 @@ int do_mem_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 			length = simple_strtoul(argv[2], NULL, 16);
 	}
 
-#if defined(CONFIG_HAS_DATAFLASH)
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
 	/* Print the lines.
 	 *
 	 * We buffer all read data, so we can make sure data is read only
@@ -112,10 +149,13 @@ int do_mem_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		void* p;
 		linebytes = (nbytes>DISP_LINE_LEN)?DISP_LINE_LEN:nbytes;
 
-		rc = read_dataflash(addr, (linebytes/size)*size, linebuf);
-		p = (rc == DATAFLASH_OK) ? linebuf : (void*)addr;
+		rc = flash_read(linebuf, (void *)addr, (linebytes/size)*size);
+		p = (rc == OK) ? linebuf : (void *)addr;
 		print_buffer(addr, p, size, linebytes/size, DISP_LINE_LEN/size);
 
+		/* Clear rc, otherwise command repeat doesn't work */
+		rc = 0;
+
 		nbytes -= linebytes;
 		addr += linebytes;
 		if (ctrlc()) {
@@ -294,9 +334,9 @@ int do_mem_cmp (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 	count = simple_strtoul(argv[3], NULL, 16);
 
-#ifdef CONFIG_HAS_DATAFLASH
-	if (addr_dataflash(addr1) | addr_dataflash(addr2)){
-		puts ("Comparison with DataFlash space not supported.\n\r");
+#ifdef CONFIG_FLASH_NOT_MEM_MAPPED
+	if (is_flash_addr(addr1) || is_flash_addr(addr2)) {
+		puts ("Comparison with FLASH space not supported.\n\r");
 		return 0;
 	}
 #endif
@@ -385,11 +425,7 @@ int do_mem_cp ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 #ifndef CONFIG_SYS_NO_FLASH
 	/* check if we are copying to Flash */
-	if ( (addr2info(dest) != NULL)
-#ifdef CONFIG_HAS_DATAFLASH
-	   && (!addr_dataflash(dest))
-#endif
-	   ) {
+	if (is_flash_addr(dest)) {
 		int rc;
 
 		puts ("Copy to Flash... ");
@@ -1010,9 +1046,9 @@ mod_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 		addr += base_address;
 	}
 
-#ifdef CONFIG_HAS_DATAFLASH
-	if (addr_dataflash(addr)){
-		puts ("Can't modify DataFlash in place. Use cp instead.\n\r");
+#ifdef CONFIG_FLASH_NOT_MEM_MAPPED
+	if (is_flash_addr(addr)) {
+		puts ("Can't modify FLASH in place. Use cp instead.\n\r");
 		return 0;
 	}
 #endif
diff --git a/common/env_flash.c b/common/env_flash.c
index 75ee8dd..9c53ec0 100644
--- a/common/env_flash.c
+++ b/common/env_flash.c
@@ -85,9 +85,41 @@ 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);
+
+static inline u8 read_env8(u8 *addr)
+{
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
+	if (((u32)addr >= CONFIG_FLASH_BASE) && ((u32)addr < CONFIG_FLASH_END))
+		return flash_read8((void *)addr);
+#endif
+	return *addr;
+}
+
+static inline u32 read_env32(u32 *addr)
+{
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
+	if (((u32)addr >= CONFIG_FLASH_BASE) && ((u32)addr < CONFIG_FLASH_END))
+		return flash_read32((void *)addr);
+#endif
+	return *addr;
+}
+
+#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 */
 
 uchar env_get_char_spec (int index)
 {
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
+	if (((u32)gd->env_addr >= CONFIG_FLASH_BASE) &&
+	    ((u32)gd->env_addr < CONFIG_FLASH_END))
+		return (flash_read8((void *)gd->env_addr + index));
+#endif
 	return ( *((uchar *)(gd->env_addr + index)) );
 }
 
@@ -97,15 +129,17 @@ int  env_init(void)
 {
 	int crc1_ok = 0, crc2_ok = 0;
 
-	uchar flag1 = flash_addr->flags;
-	uchar flag2 = flash_addr_new->flags;
+	uchar flag1 = read_env8(&flash_addr->flags);
+	uchar flag2 = read_env8(&flash_addr_new->flags);
 
 	ulong addr_default = (ulong)&default_environment[0];
 	ulong addr1 = (ulong)&(flash_addr->data);
 	ulong addr2 = (ulong)&(flash_addr_new->data);
 
-	crc1_ok = (crc32(0, flash_addr->data, ENV_SIZE) == flash_addr->crc);
-	crc2_ok = (crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc);
+	crc1_ok = (crc32(0, flash_addr->data, ENV_SIZE) ==
+		   read_env32(&flash_addr->crc));
+	crc2_ok = (crc32(0, flash_addr_new->data, ENV_SIZE) ==
+		   read_env32(&flash_addr_new->crc));
 
 	if (crc1_ok && ! crc2_ok) {
 		gd->env_addr  = addr1;
@@ -169,8 +203,9 @@ int saveenv(void)
 				up_data);
 			goto Done;
 		}
-		memcpy(saved_data,
-			(void *)((long)flash_addr_new + CONFIG_ENV_SIZE), up_data);
+		FLASH_READ_MEMCPY(saved_data,
+				  (void *)((long)flash_addr_new + CONFIG_ENV_SIZE),
+				  up_data);
 		debug ("Data (start 0x%x, len 0x%x) saved at 0x%x\n",
 			   (long)flash_addr_new + CONFIG_ENV_SIZE,
 				up_data, saved_data);
@@ -246,7 +281,7 @@ Done:
 
 int  env_init(void)
 {
-	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
+	if (crc32(0, env_ptr->data, ENV_SIZE) == read_env32(&env_ptr->crc)) {
 		gd->env_addr  = (ulong)&(env_ptr->data);
 		gd->env_valid = 1;
 		return(0);
@@ -282,7 +317,7 @@ int saveenv(void)
 		flash_sect_addr, (ulong)flash_addr, flash_offset);
 
 	/* copy old contents to temporary buffer */
-	memcpy (env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
+	FLASH_READ_MEMCPY(env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
 
 	/* copy current environment to temporary buffer */
 	memcpy ((uchar *)((unsigned long)env_buffer + flash_offset),
@@ -346,9 +381,9 @@ void env_relocate_spec (void)
 		end_addr_new = ltmp;
 	}
 
-	if (flash_addr_new->flags != OBSOLETE_FLAG &&
+	if (read_env8(&flash_addr_new->flags) != OBSOLETE_FLAG &&
 	    crc32(0, flash_addr_new->data, ENV_SIZE) ==
-	    flash_addr_new->crc) {
+	    read_env32(&flash_addr_new->crc)) {
 		char flag = OBSOLETE_FLAG;
 
 		gd->env_valid = 2;
@@ -359,8 +394,8 @@ void env_relocate_spec (void)
 		flash_sect_protect (1, (ulong)flash_addr_new, end_addr_new);
 	}
 
-	if (flash_addr->flags != ACTIVE_FLAG &&
-	    (flash_addr->flags & ACTIVE_FLAG) == ACTIVE_FLAG) {
+	if (read_env8(&flash_addr->flags) != ACTIVE_FLAG &&
+	    (read_env8(&flash_addr->flags) & ACTIVE_FLAG) == ACTIVE_FLAG) {
 		char flag = ACTIVE_FLAG;
 
 		gd->env_valid = 2;
@@ -376,7 +411,7 @@ void env_relocate_spec (void)
 		      "reading environment; recovered successfully\n\n");
 #endif /* CONFIG_ENV_ADDR_REDUND */
 #ifdef CMD_SAVEENV
-	memcpy (env_ptr, (void*)flash_addr, CONFIG_ENV_SIZE);
+	FLASH_READ_MEMCPY(env_ptr, (void*)flash_addr, CONFIG_ENV_SIZE);
 #endif
 #endif /* ! ENV_IS_EMBEDDED || CONFIG_ENV_ADDR_REDUND */
 }
diff --git a/lib_generic/crc32.c b/lib_generic/crc32.c
index b6a7a91..d698cc6 100644
--- a/lib_generic/crc32.c
+++ b/lib_generic/crc32.c
@@ -148,7 +148,18 @@ const uint32_t * ZEXPORT get_crc_table()
 #endif
 
 /* ========================================================================= */
+#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
+u8 flash_read8(void *addr);
+#define DO1(buf) \
+	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); \
+	}
+#else
 #define DO1(buf) crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> 8);
+#endif
 #define DO2(buf)  DO1(buf); DO1(buf);
 #define DO4(buf)  DO2(buf); DO2(buf);
 #define DO8(buf)  DO4(buf); DO4(buf);
-- 
1.6.0.4

[U-Boot] [PATCH] Add support for boards where the NOR FLASH is not memory-mapped

[Wolfgang Denk]

Dear Stefan Roese,

Sorry for the late review...

In message <1226493138-28470-1-git-send-email-sr@denx.de> you wrote:
> This patch adds the CONFIG_FLASH_NOT_MEM_MAPPED define which can be
> used on boards where the NOR FLASH is not memory-mapped and
> special accessor functions are needed to access the NOR FLASH.
> This affects the memory commands (cmd_mem.c) and the environment
> (env_flash.c).
> 
> Boards using CONFIG_FLASH_NOT_MEM_MAPPED need to additionally specify
> CONFIG_FLASH_BASE and CONFIG_FLASH_END so that the NOR FLASH region
> can be detected.

You have to document this (at least in the README).

But there is a general problem with this approach: U-Boot is based on
the design that the actual flash size is auto-detected, i. e.  it  is
always  assumed  to  be  unknown  at  compile  time.  Therefore it is
impossible to set something like CONFIG_FLASH_END at compile time.


> +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> +	if (((u32)addr >= CONFIG_FLASH_BASE) && ((u32)addr < CONFIG_FLASH_END))

And this looks as if CONFIG_FLASH_END was not the "FLASH_END" address,
i. e. the last address in flash, but "FLASH_END" + 1 ?

> +#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 */

This is really, really ugly - and error prone, as you must be
extremely careful which of the functions you may call might use
memcpy() or similar internally.

You know that I know of the specific problems of this hardware, but
anyway - I really dislike having to add such code.

> +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> +u8 flash_read8(void *addr);
> +#define DO1(buf) \
> +	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); \
> +	}
> +#else
>  #define DO1(buf) crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> 8);
> +#endif
>  #define DO2(buf)  DO1(buf); DO1(buf);
>  #define DO4(buf)  DO2(buf); DO2(buf);
>  #define DO8(buf)  DO4(buf); DO4(buf);

Please wrap all such macros in the usual "do { ... } while (0)"
wrappers.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is nothing in this world constant but inconstancy.      - Swift

[U-Boot] [PATCH] Add support for boards where the NOR FLASH is not memory-mapped

[Stefan Roese]

Hi Wolfgang,

On Wednesday 10 December 2008, Wolfgang Denk wrote:
> Sorry for the late review...
>
> In message <1226493138-28470-1-git-send-email-sr@denx.de> you wrote:
> > This patch adds the CONFIG_FLASH_NOT_MEM_MAPPED define which can be
> > used on boards where the NOR FLASH is not memory-mapped and
> > special accessor functions are needed to access the NOR FLASH.
> > This affects the memory commands (cmd_mem.c) and the environment
> > (env_flash.c).
> >
> > Boards using CONFIG_FLASH_NOT_MEM_MAPPED need to additionally specify
> > CONFIG_FLASH_BASE and CONFIG_FLASH_END so that the NOR FLASH region
> > can be detected.
>
> You have to document this (at least in the README).

No, I missed it. I'll send an updated patch later this week.

> But there is a general problem with this approach: U-Boot is based on
> the design that the actual flash size is auto-detected, i. e.  it  is
> always  assumed  to  be  unknown  at  compile  time.  Therefore it is
> impossible to set something like CONFIG_FLASH_END at compile time.

Yes, this approach is not optimal but I found no other way to work with the 
CFI driver and especially the environment in NOR without such an "extension". 
But CONFIG_FLASH_END doesn't have point to the real end of the NOR FLASH but 
the highest possible address. So auto-detection should still be possible in 
such an area. Here an example from the VCTH board port:

/*
 * For the non-memory-mapped NOR FLASH, we need to define the
 * NOR FLASH area. This can't be detected via the addr2info()
 * function, since we check for flash access in the very early
 * U-Boot code, before the NOR FLASH is detected.
 */
#define CONFIG_FLASH_BASE               0xb0000000
#define CONFIG_FLASH_END                0xbfffffff

So the FLASH size could be a maximum of 256MB in this case. Not optimal but at 
least not totally fixed.

> > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> > +	if (((u32)addr >= CONFIG_FLASH_BASE) && ((u32)addr < CONFIG_FLASH_END))
>
> And this looks as if CONFIG_FLASH_END was not the "FLASH_END" address,
> i. e. the last address in flash, but "FLASH_END" + 1 ?

Good catch. I'll fix this to "((u32)addr <= CONFIG_FLASH_END).

> > +#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 */
>
> This is really, really ugly - and error prone, as you must be
> extremely careful which of the functions you may call might use
> memcpy() or similar internally.
>
> You know that I know of the specific problems of this hardware, but
> anyway - I really dislike having to add such code.

Yes, we agree that this is not a "nice" solution. But this is the best I could 
come up after several approaches. I'm open to better suggestions.

> > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED)
> > +u8 flash_read8(void *addr);
> > +#define DO1(buf) \
> > +	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); \
> > +	}
> > +#else
> >  #define DO1(buf) crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >>
> > 8); +#endif
> >  #define DO2(buf)  DO1(buf); DO1(buf);
> >  #define DO4(buf)  DO2(buf); DO2(buf);
> >  #define DO8(buf)  DO4(buf); DO4(buf);
>
> Please wrap all such macros in the usual "do { ... } while (0)"
> wrappers.

OK, will do in the next version.

Thanks for the review.

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