[U-Boot-Users] [PATCH] cfi_flash.c bug fix
this fix a possible bug which occur when the config file defines a flash which is actually smaller than the physical one. on top of that, if you want to force your flash to appear smaller you can define CONFIG_FORCE_FLASH_BANK_SIZE the a corresponding value.
Signed-off-by: Eran Liberty eran.liberty@gmail.com
Index: drivers/cfi_flash.c =================================================================== --- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69) @@ -1281,7 +1281,9 @@ erase_region_count = (tmp & 0xffff) + 1; debug ("erase_region_count = %d erase_region_size = %d\n", erase_region_count, erase_region_size); - for (j = 0; j < erase_region_count; j++) { + for (j = 0; + j < erase_region_count + && sect_cnt < CFG_MAX_FLASH_SECT; j++) { info->start[sect_cnt] = sector; sector += (erase_region_size * size_ratio);
@@ -1323,6 +1325,9 @@ }
flash_write_cmd (info, 0, 0, info->cmd_reset); +#if defined(CONFIG_FORCE_FLASH_BANK_SIZE) + info->size = CONFIG_FORCE_FLASH_BANK_SIZE; +#endif return (info->size); }
On 7/3/07, eran.liberty@gmail.com eran.liberty@gmail.com wrote:
this fix a possible bug which occur when the config file defines a flash which is actually smaller than the physical one. on top of that, if you want to force your flash to appear smaller you can define CONFIG_FORCE_FLASH_BANK_SIZE the a corresponding value.
Signed-off-by: Eran Liberty eran.liberty@gmail.com
Index: drivers/cfi_flash.c
--- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69) @@ -1281,7 +1281,9 @@ erase_region_count = (tmp & 0xffff) + 1; debug ("erase_region_count = %d erase_region_size = %d\n", erase_region_count, erase_region_size);
for (j = 0; j < erase_region_count; j++) {
for (j = 0;j < erase_region_count&& sect_cnt < CFG_MAX_FLASH_SECT; j++) { info->start[sect_cnt] = sector; sector += (erase_region_size * size_ratio);
Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.
ie: if (erase_region_count > CFG_MAX_FLASH_SECT - sect_cnt) erase_region_count = CFG_MAX_FLASH_SECT - sect_cnt; (but double check that I haven't created an off-by-one error)
Cheers, g.
On 7/3/07, Grant Likely grant.likely@secretlab.ca wrote:
On 7/3/07, eran.liberty@gmail.com eran.liberty@gmail.com wrote:
this fix a possible bug which occur when the config file defines a flash which is actually smaller than the physical one. on top of that, if you want to force your flash to appear smaller you can define CONFIG_FORCE_FLASH_BANK_SIZE the a corresponding value.
Signed-off-by: Eran Liberty eran.liberty@gmail.com
Index: drivers/cfi_flash.c
--- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69) @@ -1281,7 +1281,9 @@ erase_region_count = (tmp & 0xffff) + 1; debug ("erase_region_count = %d erase_region_size = %d\n", erase_region_count, erase_region_size);
for (j = 0; j < erase_region_count; j++) {
for (j = 0;j < erase_region_count&& sect_cnt < CFG_MAX_FLASH_SECT; j++) { info->start[sect_cnt] = sector; sector += (erase_region_size * size_ratio);Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.
ie: if (erase_region_count > CFG_MAX_FLASH_SECT - sect_cnt) erase_region_count = CFG_MAX_FLASH_SECT - sect_cnt; (but double check that I haven't created an off-by-one error)
Cheers, g.
-- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. grant.likely@secretlab.ca (403) 399-0195
the code with my patch flows like this... sect_cnt = 0; for (i = 0; i < num_erase_regions; i++) { ... ... erase_region_count = (tmp & 0xffff) + 1; for (j = 0; j < erase_region_count && sect_cnt < CFG_MAX_FLASH_SECT; j++){ ... ... sect_cnt++; } }
If I had not missed anything, your suggestion tie together two variables which are not necessarily connected. True, your suggestion will prevent the j variable from letting the sect_cnt overflow in the , should be last, iteration. But i feel it is circumventive and less straight forward. The variable causing the spill over is sect_cnt, there for it should be the one that being tested against its own boundaries.
"Happy Happy Joy Joy", Liberty
On 7/4/07, eran liberty eran.liberty@gmail.com wrote:
On 7/3/07, Grant Likely grant.likely@secretlab.ca wrote:
On 7/3/07, eran.liberty@gmail.com eran.liberty@gmail.com wrote:
--- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69) @@ -1281,7 +1281,9 @@ erase_region_count = (tmp & 0xffff) + 1; debug ("erase_region_count = %d erase_region_size = %d\n", erase_region_count, erase_region_size);
for (j = 0; j < erase_region_count; j++) {
for (j = 0;j < erase_region_count&& sect_cnt < CFG_MAX_FLASH_SECT; j++) { info->start[sect_cnt] = sector; sector += (erase_region_size * size_ratio);Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.
If I had not missed anything, your suggestion tie together two variables which are not necessarily connected. True, your suggestion will prevent the j variable from letting the sect_cnt overflow in the , should be last, iteration. But i feel it is circumventive and less straight forward. The variable causing the spill over is sect_cnt, there for it should be the one that being tested against its own boundaries.
My 'nitpick' is about the for loop becoming longer than 80chars, and hence being wrapped to 3 lines which makes it harder to read. I'd prefer code that keeps the for(;;) statement short for readability.
g.
On 7/4/07, Grant Likely grant.likely@secretlab.ca wrote:
On 7/4/07, eran liberty eran.liberty@gmail.com wrote:
On 7/3/07, Grant Likely grant.likely@secretlab.ca wrote:
On 7/3/07, eran.liberty@gmail.com eran.liberty@gmail.com wrote:
--- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69) @@ -1281,7 +1281,9 @@ erase_region_count = (tmp & 0xffff) + 1; debug ("erase_region_count = %d erase_region_size = %d\n", erase_region_count, erase_region_size);
for (j = 0; j < erase_region_count; j++) {
for (j = 0;j < erase_region_count&& sect_cnt < CFG_MAX_FLASH_SECT; j++) { info->start[sect_cnt] = sector; sector += (erase_region_size * size_ratio);Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.
If I had not missed anything, your suggestion tie together two variables which are not necessarily connected. True, your suggestion will prevent the j variable from letting the sect_cnt overflow in the , should be last, iteration. But i feel it is circumventive and less straight forward. The variable causing the spill over is sect_cnt, there for it should be the one that being tested against its own boundaries.
My 'nitpick' is about the for loop becoming longer than 80chars, and hence being wrapped to 3 lines which makes it harder to read. I'd prefer code that keeps the for(;;) statement short for readability.
Will taking the check inside the loop and breaking out, make us happy?
for (j = 0; j < erase_region_count; j++) { if (CFG_MAX_FLASH_SECT <= sect_cnt) break; ... ... }
g.
-- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. grant.likely@secretlab.ca (403) 399-0195
On 7/4/07, eran liberty eran.liberty@gmail.com wrote:
On 7/4/07, Grant Likely grant.likely@secretlab.ca wrote:
My 'nitpick' is about the for loop becoming longer than 80chars, and hence being wrapped to 3 lines which makes it harder to read. I'd prefer code that keeps the for(;;) statement short for readability.
Will taking the check inside the loop and breaking out, make us happy?
for (j = 0; j < erase_region_count; j++) { if (CFG_MAX_FLASH_SECT <= sect_cnt) break; ... ... }
I prefer the other approach, but as long as it's readable I'm happy. (It's that whole 'taste' think that Linus likes to talk about)
Cheers, g.
Hi,
I have another thing that might be added in cfi_flash.c.
Please correct me if I am wrong, but the monitor protection must not be applied (at least not in this way) when U-Boot is running from RAM. The original test is probably not a problem on the architectures where the flash address range is above (SD)RAM, but for the architectures where flash has to start at 0x0 (e.g. Coldfire), I would ask for the patch below to be applied:
--- drivers/cfi_flash.c.sicher 2007-07-04 08:35:43.000000000 +0000 +++ drivers/cfi_flash.c 2007-07-04 08:35:37.000000000 +0000 @@ -397,7 +397,7 @@ }
/* Monitor protection ON by default */ -#if (CFG_MONITOR_BASE >= CFG_FLASH_BASE) +#if !defined(CONFIG_MONITOR_IS_IN_RAM) && (CFG_MONITOR_BASE >= CFG_FLASH_BASE) flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, CFG_MONITOR_BASE + monitor_flash_len - 1,
Best regards, Wolfgang
participants (4)
-
eran liberty -
eran.liberty＠gmail.com
-
Grant Likely -
w.wegner＠astro-kom.de