[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Becky Bruce]

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		*/
-- 
1.5.6.6

[U-Boot] [PATCH 2/2] mpc8641hpcn: Use physical address in flash banks defintion

[Becky Bruce]

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 */
-- 
1.5.6.6

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Pieter]

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

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Kumar Gala]

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

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Becky Bruce]

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

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH 1/2] flash/cfi_flash: Use virtual sector start address, not phys

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH 2/2] mpc8641hpcn: Use physical address in flash banks defintion

[Wolfgang Denk]

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

-- 
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: wd at denx.de
A secure program has to be robust: it  must  be  able  to  deal  with
conditions  that "can't happen", whether user input, program error or
library/etc. This is basic damage  control.  Buffer  overflow  errors
have nothing to do with security, but everything with stupidity.
                 -- Wietse Venema in <5cnqm3$8r9@spike.porcupine.org>