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

[mark tomlinson]

Sorry, no. The flash chips are 0xf8000000-0xf9ffffff (32MB). (Actually we have twice this area reserved for 64MB flash in the future). The flash chip from 0xf8000000 is also mirrored at 0xfff00000 at boot time. I don't know if you consider this a hardware bug to not have all the flash at the very top of memory, but that is the way our product has been designed. Here's a memory map of the high memory area: 

f8000000-fbffffff   64M Flash 
fe000000-fe0fffff    1M Battery-backed RAM 
ff000000-ff00ffff   64K On-board logic 
ff700000-ff7fffff    1M CCSR 
fff00000-ffffffff    1M Flash (mirror of f8000000). 

So we have CONFIG_SYS_FLASH_BASE pointing to the entire Flash, and CONFIG_SYS_MONITOR_BASE pointing to the mirrored section which contains u-boot. 

The auto protect was already there; nothing new. See CONFIG_SYS_FLASH_AUTOPROTECT_LIST in cfi_flash.c. 

>>> Stefan Roese <sr at denx.de> 5/19/2010 9:44 PM >>>
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

NOTICE: This message contains privileged and confidential
information intended only for the use of the addressee
named above. If you are not the intended recipient of
this message you are hereby notified that you must not
disseminate, copy or take any action in reliance on it.
If you have received this message in error please
notify Allied Telesis Labs Ltd immediately.
Any views expressed in this message are those of the
individual sender, except where the sender has the
authority to issue and specifically states them to
be the views of Allied Telesis Labs.