Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Dear Chris Packham, In message loom.20100520t005209-...@post.gmane.org you wrote: While it would be possible to shuffle the memory map around there is one problem with the hardware design that I don't think can be overcome (I'd love to be proven wrong). The boot chip select is mapped to the _bottom_ of the first flash chip. It was done this way so that we could expand the flash in the future as a rolling production change (we're now shipping units with 64MB fitted). i.e. we knew we could rely on a fixed base address so thats where we pointed the boot chip select. I don't see why this should be relevant. Usually you can set up nearly any memory map in software, independent of the CPU state at boot time. Which exact processor family are we talking about? I think in hindsight we could have modified our flash detection code to start at the top and jump backwards looking for extra chips. Unfortunately What do you mean by our flash detection code? Are you using any private code for that? Why? U-Boot already has all the needed stuff, just use it. we're not able to change the hardware design for this product but we can take this into account on future designs. I'm not convinced that any hardware changes would be needed. 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: w...@denx.de Don't panic. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Hi Wolfgang, On Thu, May 20, 2010 at 1:35 AM, Wolfgang Denk w...@denx.de wrote: Dear Chris Packham, In message loom.20100520t005209-...@post.gmane.org you wrote: While it would be possible to shuffle the memory map around there is one problem with the hardware design that I don't think can be overcome (I'd love to be proven wrong). The boot chip select is mapped to the _bottom_ of the first flash chip. It was done this way so that we could expand the flash in the future as a rolling production change (we're now shipping units with 64MB fitted). i.e. we knew we could rely on a fixed base address so thats where we pointed the boot chip select. I don't see why this should be relevant. Usually you can set up nearly any memory map in software, independent of the CPU state at boot time. Which exact processor family are we talking about? Freescale MPC8541. CS0 is mapped by an external CPLD to either the bottom block of flash _or_ to a plug in card which has physical EPROMs which we use when we're bringing the board up. On newer designs we actually use pre-programmed flash chips in the factory and JTAG in circuit debuggers for development. I think in hindsight we could have modified our flash detection code to start at the top and jump backwards looking for extra chips. Unfortunately What do you mean by our flash detection code? Are you using any private code for that? Why? U-Boot already has all the needed stuff, just use it. This design was originally done for a proprietary boot loader before we saw the light and switched to u-boot. That was what I meant by our flash detection code. Recently we've just switched to using the CFI driver in the latest u-boot version. Prior to this (based on version 1.1.4) we still had our own board/.../flash.c. we're not able to change the hardware design for this product but we can take this into account on future designs. I'm not convinced that any hardware changes would be needed. I think we'd need 2 different boot loaders configurations, one for running from EPROM, one for running from flash. If we ignore the EPROM it wouldn't be too hard to re-define the memory map such that we have flash arranged with the boot loader and environment in the bottom 1MB of the first flash chip with the file system in the top 31MB and optionally the second flash chip. The plug in EPROM card is still very useful when you accidentally brick your board so we'd still want to have that, which could be done easily with the existing configuration. I'll look into shuffling the memory map around. It'd also be a good opportunity to start using the mtd maps in Linux. - C ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
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 0xf800 /* 2 chips*16M */ Hmmm. 2 * 16MByte, thats 32MByte == 0x200. So you should have one chip at base address 0xff00 and one at 0xfe00. Why 0xf800? #define CONFIG_SYS_MONITOR_BASE TEXT_BASE /* start of monitor */ and in our config.mk file: TEXT_BASE = 0xfff4 This is the same flash chip as that at 0xf800, 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 0xfff4 is greater than 0xf800, but flash_get_info(0xfff4) returns NULL (as expected). I don't see why flash_get_info(0xfff4) should return NULL. It should return the pointer to the 16MB FLASH chip starting at 0xff00. 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: off...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Sorry, no. The flash chips are 0xf800-0xf9ff (32MB). (Actually we have twice this area reserved for 64MB flash in the future). The flash chip from 0xf800 is also mirrored at 0xfff0 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: f800-fbff 64M Flash fe00-fe0f1M Battery-backed RAM ff00-ff00 64K On-board logic ff70-ff7f1M CCSR fff0-1M Flash (mirror of f800). 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 s...@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 0xf800 /* 2 chips*16M */ Hmmm. 2 * 16MByte, thats 32MByte == 0x200. So you should have one chip at base address 0xff00 and one at 0xfe00. Why 0xf800? #define CONFIG_SYS_MONITOR_BASE TEXT_BASE /* start of monitor */ and in our config.mk file: TEXT_BASE = 0xfff4 This is the same flash chip as that at 0xf800, 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 0xfff4 is greater than 0xf800, but flash_get_info(0xfff4) returns NULL (as expected). I don't see why flash_get_info(0xfff4) should return NULL. It should return the pointer to the 16MB FLASH chip starting at 0xff00. 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: off...@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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Dear mark tomlinson, Please restrict your line length to some 70 characters or so, and stop posting HTML! In message 4bf4fc5e02b800012...@gwia.alliedtelesyn.co.nz you wrote: Sorry, no. The flash chips are 0xf800-0xf9ff (32MB). (Actually we have twice this area reserved for 64MB flash in the future). The flash chip from 0xf800 is also mirrored at 0xfff0 at boot time. I do n'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 desig ned. Here's a memory map of the high memory area: It's not a hardware bug, but a configuration error. f800-fbff 64M Flash fe00-fe0f1M Battery-backed RAM ff00-ff00 64K On-board logic ff70-ff7f1M CCSR fff0-1M Flash (mirror of f800). This makes no sense. Fix your memory map, and map the flash (all of it) to the end of the address space. 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. As I mentioned above: misconigured. NOTICE: This message contains privileged and confidential information intended only for the use of the addressee ... And please stop posting these silly disclaimers. 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: w...@denx.de Insufficient facts always invite danger. -- Spock, Space Seed, stardate 3141.9 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Disclaimer: I'm workmate of Mark's Wolfgang Denk wd at denx.de writes: It's not a hardware bug, but a configuration error. f800-fbff 64M Flash fe00-fe0f1M Battery-backed RAM ff00-ff00 64K On-board logic ff70-ff7f1M CCSR fff0-1M Flash (mirror of f800). This makes no sense. Fix your memory map, and map the flash (all of it) to the end of the address space. While it would be possible to shuffle the memory map around there is one problem with the hardware design that I don't think can be overcome (I'd love to be proven wrong). The boot chip select is mapped to the _bottom_ of the first flash chip. It was done this way so that we could expand the flash in the future as a rolling production change (we're now shipping units with 64MB fitted). i.e. we knew we could rely on a fixed base address so thats where we pointed the boot chip select. I think in hindsight we could have modified our flash detection code to start at the top and jump backwards looking for extra chips. Unfortunately we're not able to change the hardware design for this product but we can take this into account on future designs. NOTICE: This message contains privileged and confidential information intended only for the use of the addressee ... And please stop posting these silly disclaimers. Corporate overlords have been flogged :). General response has been to go sign up to gmail. Trust me it annoys us as much as it annoys you. - C ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Hi Mark, On Tuesday 18 May 2010 07:26:34 Mark Tomlinson wrote: Our hardware has part of the flash mapped in two address ranges. The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at a lower address. Just to be sure: You have 2 FLASH chips? Mapped at which addresses? And where does CONFIG_SYS_MONITOR_BASE point to? This all works fine until the code in cfi_flash.c:flash_init(), which uses flash_get_info() to find the flash_info_t associated with the monitor and environment. These are not in the probed flash range, so flash_get_info() returns NULL. You mean that flash_get_info(CONFIG_SYS_MONITOR_BASE) returns NULL? Please explain again, why is this the case? This is not checked and is passed directly to flash_protect(). Since flash_protect() was not checking for this NULL pointer either, random memory would be clobbered causing the device to lock up. This patch changes flash_protect() to check for the NULL, and the error goes unreported. I would prefer to fix the real problem, that flash_get_info() returns NULL, instead. 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: off...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Yes we do have 2 flash chips. Here's the mapping: #define CONFIG_SYS_FLASH_BASE 0xf800 /* 2 chips*16M */ #define CONFIG_SYS_MONITOR_BASE TEXT_BASE /* start of monitor */ and in our config.mk file: TEXT_BASE = 0xfff4 This is the same flash chip as that at 0xf800, but remapped at reset by a CPLD to the high memory area too. 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 0xfff4 is greater than 0xf800, but flash_get_info(0xfff4) returns NULL (as expected). I understand that not passing NULL to flash_protect() would be a better idea, and there's nothing wrong with doing both. 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. - Mark Stefan Roese s...@denx.de 5/18/2010 8:20 PM Hi Mark, On Tuesday 18 May 2010 07:26:34 Mark Tomlinson wrote: Our hardware has part of the flash mapped in two address ranges. The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at a lower address. Just to be sure: You have 2 FLASH chips? Mapped at which addresses? And where does CONFIG_SYS_MONITOR_BASE point to? This all works fine until the code in cfi_flash.c:flash_init(), which uses flash_get_info() to find the flash_info_t associated with the monitor and environment. These are not in the probed flash range, so flash_get_info() returns NULL. You mean that flash_get_info(CONFIG_SYS_MONITOR_BASE) returns NULL? Please explain again, why is this the case? This is not checked and is passed directly to flash_protect(). Since flash_protect() was not checking for this NULL pointer either, random memory would be clobbered causing the device to lock up. This patch changes flash_protect() to check for the NULL, and the error goes unreported. I would prefer to fix the real problem, that flash_get_info() returns NULL, instead. 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: off...@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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors
Our hardware has part of the flash mapped in two address ranges. The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at a lower address. This all works fine until the code in cfi_flash.c:flash_init(), which uses flash_get_info() to find the flash_info_t associated with the monitor and environment. These are not in the probed flash range, so flash_get_info() returns NULL. This is not checked and is passed directly to flash_protect(). Since flash_protect() was not checking for this NULL pointer either, random memory would be clobbered causing the device to lock up. This patch changes flash_protect() to check for the NULL, and the error goes unreported. Mark Tomlinson (1): flash: Check info pointer in flash_protect(). common/flash.c | 10 +++--- 1 files changed, 7 insertions(+), 3 deletions(-) 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
