[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Stefan Roese <sr@denx.de>]

Mark,

On Tuesday 18 May 2010 22:10:51 mark tomlinson wrote:
> Yes we do have 2 flash chips. Here's the mapping:
> 
> #define CONFIG_SYS_FLASH_BASE   0xf8000000  /* 2 chips*16M */

Hmmm. 2 * 16MByte, thats 32MByte == 0x2000000. So you should have one chip 
at base address 0xff000000 and one at 0xfe000000. Why 0xf8000000?

> #define CONFIG_SYS_MONITOR_BASE  TEXT_BASE  /* start of monitor */
> 
> and in our config.mk file:
> 
> TEXT_BASE = 0xfff40000
> 
> This is the same flash chip as that at 0xf8000000, but remapped at reset 
by
> a CPLD to the high memory area too.

This seems wrong. See my comments above.
 
> The conditional code in cfi_flash.c:
> #if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE) && \
> (!defined(CONFIG_MONITOR_IS_IN_RAM))
> is therefore included since 0xfff40000 is greater than 0xf8000000, but
> flash_get_info(0xfff40000) returns NULL (as expected).

I don't see why flash_get_info(0xfff40000) should return NULL. It should 
return the pointer to the 16MB FLASH chip starting at 0xff000000.
 
> I understand that not passing NULL to flash_protect() would be a better
> idea, and there's nothing wrong with doing both.

Agreed in general. But we have to keep the code compact. And unnecessary 
checks do increase the code size (at least a small bit).

> I was going to fix it in
> cfi_flash.c, but noticed that many other areas of code (in different
> flash.c files) do the same thing. In our own build, I have just removed
> the code that tries to protect the monitor area, and will use an
> auto-protect area instead to do the same job.

"auto-protect area"? Please explain what you mean with this? Perhaps this 
is an interesting "feature" for mainline as well.

Cheers,
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