[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys
include/flash.h was commented to say that the address in flash_info->start was a physical address. However, from u-boot's point of view, and looking at most flash code, it makes more sense for this to be a virtual address. So I corrected the comment to indicate that this was a virtual address.
The only flash driver that was actually treating the address as physical was the mtd/cfi_flash driver. However, this code was using it inconsistently as it actually directly dereferenced the "start" element, while it used map_physmem to get a virtual address in other places. I changed this driver so that the code which initializes the info->start field calls map_physmem to get a virtual address, eliminating the need for further map_physmem calls. The code is now consistent.
The *only* place a physical address should be used is when defining the flash banks list that is used to initialize the flash_info struct, usually found in the board config file.
Signed-off-by: Becky Bruce beckyb@kernel.crashing.org --- drivers/mtd/cfi_flash.c | 53 +++++++++++++++++++++------------------------- include/flash.h | 2 +- 2 files changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 84ff7e8..4cb5fb5 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -305,17 +305,12 @@ flash_map (flash_info_t * info, flash_sect_t sect, uint offset) { unsigned int byte_offset = offset * info->portwidth;
- return map_physmem(info->start[sect] + byte_offset, - flash_sector_size(info, sect) - byte_offset, - MAP_NOCACHE); + return (void *)(info->start[sect] + byte_offset); }
static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, unsigned int offset, void *addr) { - unsigned int byte_offset = offset * info->portwidth; - - unmap_physmem(addr, flash_sector_size(info, sect) - byte_offset); }
/*----------------------------------------------------------------------- @@ -802,13 +797,11 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr) static int flash_write_cfiword (flash_info_t * info, ulong dest, cfiword_t cword) { - void *dstaddr; + void *dstaddr = (void *)dest; int flag; flash_sect_t sect = 0; char sect_found = 0;
- dstaddr = map_physmem(dest, info->portwidth, MAP_NOCACHE); - /* Check if Flash is (sufficiently) erased */ switch (info->portwidth) { case FLASH_CFI_8BIT: @@ -827,10 +820,8 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest, flag = 0; break; } - if (!flag) { - unmap_physmem(dstaddr, info->portwidth); + if (!flag) return ERR_NOT_ERASED; - }
/* Disable interrupts which might cause a timeout here */ flag = disable_interrupts (); @@ -873,8 +864,6 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest, if (flag) enable_interrupts ();
- unmap_physmem(dstaddr, info->portwidth); - if (!sect_found) sect = find_sector (info, dest);
@@ -890,7 +879,7 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, int cnt; int retcode; void *src = cp; - void *dst = map_physmem(dest, len, MAP_NOCACHE); + void *dst = dest; void *dst2 = dst; int flag = 0; uint offset = 0; @@ -1052,7 +1041,6 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, }
out_unmap: - unmap_physmem(dst, len); return retcode; } #endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */ @@ -1301,7 +1289,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt) /* handle unaligned start */ if ((aln = addr - wp) != 0) { cword.l = 0; - p = map_physmem(wp, info->portwidth, MAP_NOCACHE); + p = (uchar *)wp; for (i = 0; i < aln; ++i) flash_add_byte (info, &cword, flash_read8(p + i));
@@ -1313,7 +1301,6 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt) flash_add_byte (info, &cword, flash_read8(p + i));
rc = flash_write_cfiword (info, wp, cword); - unmap_physmem(p, info->portwidth); if (rc != 0) return rc;
@@ -1372,14 +1359,13 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt) * handle unaligned tail bytes */ cword.l = 0; - p = map_physmem(wp, info->portwidth, MAP_NOCACHE); + p = (uchar *)wp; for (i = 0; (i < info->portwidth) && (cnt > 0); ++i) { flash_add_byte (info, &cword, *src++); --cnt; } for (; i < info->portwidth; ++i) flash_add_byte (info, &cword, flash_read8(p + i)); - unmap_physmem(p, info->portwidth);
return flash_write_cfiword (info, wp, cword); } @@ -1618,7 +1604,7 @@ static void flash_read_jedec_ids (flash_info_t * info) * board_flash_get_legacy needs to fill in at least: * info->portwidth, info->chipwidth and info->interface for Jedec probing. */ -static int flash_detect_legacy(ulong base, int banknum) +static int flash_detect_legacy(phys_addr_t base, int banknum) { flash_info_t *info = &flash_info[banknum];
@@ -1634,7 +1620,10 @@ static int flash_detect_legacy(ulong base, int banknum)
for (i = 0; i < sizeof(modes) / sizeof(modes[0]); i++) { info->vendor = modes[i]; - info->start[0] = base; + info->start[0] = + (ulong)map_physmem(base, + info->portwith, + MAP_NOCACHE); if (info->portwidth == FLASH_CFI_8BIT && info->interface == FLASH_CFI_X8X16) { info->addr_unlock1 = 0x2AAA; @@ -1648,8 +1637,11 @@ static int flash_detect_legacy(ulong base, int banknum) info->manufacturer_id, info->device_id, info->device_id2); - if (jedec_flash_match(info, base)) + if (jedec_flash_match(info, info->start[0])) break; + else + unmap_physmem(info->start[0], + MAP_NOCACHE); } }
@@ -1671,7 +1663,7 @@ static int flash_detect_legacy(ulong base, int banknum) return 0; /* use CFI */ } #else -static inline int flash_detect_legacy(ulong base, int banknum) +static inline int flash_detect_legacy(phys_addr_t base, int banknum) { return 0; /* use CFI */ } @@ -1826,12 +1818,12 @@ static void flash_fixup_stm(flash_info_t *info, struct cfi_qry *qry) * The following code cannot be run from FLASH! * */ -ulong flash_get_size (ulong base, int banknum) +ulong flash_get_size (phys_addr_t base, int banknum) { flash_info_t *info = &flash_info[banknum]; int i, j; flash_sect_t sect_cnt; - unsigned long sector; + phys_addr_t sector; unsigned long tmp; int size_ratio; uchar num_erase_regions; @@ -1847,7 +1839,7 @@ ulong flash_get_size (ulong base, int banknum) info->legacy_unlock = 0; #endif
- info->start[0] = base; + info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);
if (flash_detect_cfi (info, &qry)) { info->vendor = le16_to_cpu(qry.p_id); @@ -1939,7 +1931,10 @@ ulong flash_get_size (ulong base, int banknum) printf("ERROR: too many flash sectors\n"); break; } - info->start[sect_cnt] = sector; + info->start[sect_cnt] = + (ulong)map_physmem(sector, + info->portwidth, + MAP_NOCACHE); sector += (erase_region_size * size_ratio);
/* @@ -2016,7 +2011,7 @@ unsigned long flash_init (void) char *s = getenv("unlock"); #endif
-#define BANK_BASE(i) (((unsigned long [CFI_MAX_FLASH_BANKS])CONFIG_SYS_FLASH_BANKS_LIST)[i]) +#define BANK_BASE(i) (((phys_addr_t [CFI_MAX_FLASH_BANKS])CONFIG_SYS_FLASH_BANKS_LIST)[i])
/* Init: no FLASHes known */ for (i = 0; i < CONFIG_SYS_MAX_FLASH_BANKS; ++i) { diff --git a/include/flash.h b/include/flash.h index 6e2981c..02c6a04 100644 --- a/include/flash.h +++ b/include/flash.h @@ -33,7 +33,7 @@ typedef struct { ulong size; /* total bank size in bytes */ ushort sector_count; /* number of erase units */ ulong flash_id; /* combined device & manufacturer code */ - ulong start[CONFIG_SYS_MAX_FLASH_SECT]; /* physical sector start addresses */ + ulong start[CONFIG_SYS_MAX_FLASH_SECT]; /* virtual sector start address */ uchar protect[CONFIG_SYS_MAX_FLASH_SECT]; /* sector protection status */ #ifdef CONFIG_SYS_FLASH_CFI uchar portwidth; /* the width of the port */
If the VA and PA of the flash aren't the same, the banks list should be initialized to hold the physical address. Correct this.
Signed-off-by: Becky Bruce beckyb@kernel.crashing.org --- include/configs/MPC8641HPCN.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/configs/MPC8641HPCN.h b/include/configs/MPC8641HPCN.h index 5a83296..ceb8e54 100644 --- a/include/configs/MPC8641HPCN.h +++ b/include/configs/MPC8641HPCN.h @@ -186,7 +186,7 @@ extern unsigned long get_board_sys_clk(unsigned long dummy); #define CONFIG_SYS_FLASH_BASE_PHYS (CONFIG_SYS_FLASH_BASE \ | CONFIG_SYS_PHYS_ADDR_HIGH)
-#define CONFIG_SYS_FLASH_BANKS_LIST {CONFIG_SYS_FLASH_BASE} +#define CONFIG_SYS_FLASH_BANKS_LIST {CONFIG_SYS_FLASH_BASE_PHYS}
#define CONFIG_SYS_BR0_PRELIM (BR_PHYS_ADDR(CONFIG_SYS_FLASH_BASE_PHYS) \ | 0x00001001) /* port size 16bit */
Dear Becky Bruce,
In message 1233614092-5480-2-git-send-email-beckyb@kernel.crashing.org you wrote:
If the VA and PA of the flash aren't the same, the banks list should be initialized to hold the physical address. Correct this.
Signed-off-by: Becky Bruce beckyb@kernel.crashing.org
include/configs/MPC8641HPCN.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/configs/MPC8641HPCN.h b/include/configs/MPC8641HPCN.h index 5a83296..ceb8e54 100644 --- a/include/configs/MPC8641HPCN.h +++ b/include/configs/MPC8641HPCN.h @@ -186,7 +186,7 @@ extern unsigned long get_board_sys_clk(unsigned long dummy); #define CONFIG_SYS_FLASH_BASE_PHYS (CONFIG_SYS_FLASH_BASE \ | CONFIG_SYS_PHYS_ADDR_HIGH)
-#define CONFIG_SYS_FLASH_BANKS_LIST {CONFIG_SYS_FLASH_BASE} +#define CONFIG_SYS_FLASH_BANKS_LIST {CONFIG_SYS_FLASH_BASE_PHYS}
#define CONFIG_SYS_BR0_PRELIM (BR_PHYS_ADDR(CONFIG_SYS_FLASH_BASE_PHYS) \ | 0x00001001) /* port size 16bit */
Applied, thanks.
Best regards,
Wolfgang Denk
Becky Bruce wrote:
include/flash.h was commented to say that the address in flash_info->start was a physical address. However, from u-boot's point of view, and looking at most flash code, it makes more sense for this to be a virtual address. So I corrected the comment to indicate that this was a virtual address.
The only flash driver that was actually treating the address as physical was the mtd/cfi_flash driver. However, this code was using it inconsistently as it actually directly dereferenced the "start" element, while it used map_physmem to get a virtual address in other places. I changed this driver so that the code which initializes the info->start field calls map_physmem to get a virtual address, eliminating the need for further map_physmem calls. The code is now consistent.
The *only* place a physical address should be used is when defining the flash banks list that is used to initialize the flash_info struct, usually found in the board config file.
Signed-off-by: Becky Bruce beckyb@kernel.crashing.org
drivers/mtd/cfi_flash.c | 53 +++++++++++++++++++++------------------------- include/flash.h | 2 +- 2 files changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 84ff7e8..4cb5fb5 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -305,17 +305,12 @@ flash_map (flash_info_t * info, flash_sect_t sect, uint offset) { unsigned int byte_offset = offset * info->portwidth;
- return map_physmem(info->start[sect] + byte_offset,
flash_sector_size(info, sect) - byte_offset,MAP_NOCACHE);
- return (void *)(info->start[sect] + byte_offset);
}
static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, unsigned int offset, void *addr) {
- unsigned int byte_offset = offset * info->portwidth;
- unmap_physmem(addr, flash_sector_size(info, sect) - byte_offset);
}
<snip>
Since "flash_unmap(flash_info_t *info, flash_sect_t sect,unsigned int offset, void *addr)" function is empty after applying the patch, should all function calls in the /drivers/mtd/cfi_flash.c file not also be removed?
cheers pieter
On Feb 3, 2009, at 2:53 AM, Pieter wrote:
static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, unsigned int offset, void *addr) {
- unsigned int byte_offset = offset * info->portwidth;
- unmap_physmem(addr, flash_sector_size(info, sect) - byte_offset);
}
<snip>
Since "flash_unmap(flash_info_t *info, flash_sect_t sect,unsigned int offset, void *addr)" function is empty after applying the patch, should all function calls in the /drivers/mtd/cfi_flash.c file not also be removed?
cheers pieter
I think its best to leave them alone for now. If in the future we really think flash_unmap isn't going to get called we can remove them. Stefan?
- k
On Feb 3, 2009, at 8:48 AM, Kumar Gala wrote:
On Feb 3, 2009, at 2:53 AM, Pieter wrote:
static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, unsigned int offset, void *addr) {
- unsigned int byte_offset = offset * info->portwidth;
- unmap_physmem(addr, flash_sector_size(info, sect) - byte_offset);
}
<snip>
Since "flash_unmap(flash_info_t *info, flash_sect_t sect,unsigned int offset, void *addr)" function is empty after applying the patch, should all function calls in the /drivers/mtd/cfi_flash.c file not also be removed?
cheers pieter
I think its best to leave them alone for now. If in the future we really think flash_unmap isn't going to get called we can remove them. Stefan?
I left those in case there might later be something that needs to be done there, so that flash_map/unmap remain symmetric. But I can remove them if Stefan wants.
Cheers, B
On Tuesday 03 February 2009, Kumar Gala wrote:
On Feb 3, 2009, at 2:53 AM, Pieter wrote:
static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, unsigned int offset, void *addr) {
- unsigned int byte_offset = offset * info->portwidth;
- unmap_physmem(addr, flash_sector_size(info, sect) - byte_offset);
}
<snip>
Since "flash_unmap(flash_info_t *info, flash_sect_t sect,unsigned int offset, void *addr)" function is empty after applying the patch, should all function calls in the /drivers/mtd/cfi_flash.c file not also be removed?
cheers pieter
I think its best to leave them alone for now. If in the future we really think flash_unmap isn't going to get called we can remove them. Stefan?
Fine with me.
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@denx.de =====================================================================
On Monday 02 February 2009, Becky Bruce wrote:
include/flash.h was commented to say that the address in flash_info->start was a physical address. However, from u-boot's point of view, and looking at most flash code, it makes more sense for this to be a virtual address. So I corrected the comment to indicate that this was a virtual address.
The only flash driver that was actually treating the address as physical was the mtd/cfi_flash driver. However, this code was using it inconsistently as it actually directly dereferenced the "start" element, while it used map_physmem to get a virtual address in other places. I changed this driver so that the code which initializes the info->start field calls map_physmem to get a virtual address, eliminating the need for further map_physmem calls. The code is now consistent.
The *only* place a physical address should be used is when defining the flash banks list that is used to initialize the flash_info struct, usually found in the board config file.
Applied to cfi-flash/master. Thanks.
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@denx.de =====================================================================
participants (5)
-
Becky Bruce -
Kumar Gala -
Pieter -
Stefan Roese -
Wolfgang Denk