Re: [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses

2016-02-23 Thread Simon Glass 
Hi York,

On 23 February 2016 at 13:36, york sun  wrote:
> On 02/16/2016 08:01 AM, Simon Glass wrote:
>> Hi York,
>>
>> On 12 February 2016 at 13:59, York Sun  wrote:
>>> When dealing with image addresses, ulong has been used. Some files
>>> are used by both host and target. It is OK for the target, but not
>>> always enough for host tools including mkimage. This patch replaces
>>> "ulong" with "phys_addr_t" to make sure addresses are correct for
>>> both the target and the host.
>>>
>>> This issue was found on 32-bit host when compiling for 64-bit target
>>> to support images with address higher than 32-bit space.
>>>
>>> Signed-off-by: York Sun 
>>>
>>> ---
>>>
>>> Changes in v4:
>>>   New patch, separated from fixing FIT image.
>>>
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>  arch/powerpc/lib/bootm.c |4 ++--
>>>  cmd/ximg.c   |9 +
>>>  common/bootm.c   |   21 +++--
>>>  common/bootm_os.c|   14 --
>>>  common/image-android.c   |6 +++---
>>>  common/image-fdt.c   |   16 
>>>  common/image-fit.c   |   27 ++-
>>>  common/image.c   |   17 ++---
>>>  include/bootm.h  |6 +++---
>>>  include/image.h  |   30 ++
>>>  tools/default_image.c|2 +-
>>>  11 files changed, 83 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
>>> index ef15e7a..794382a 100644
>>> --- a/arch/powerpc/lib/bootm.c
>>> +++ b/arch/powerpc/lib/bootm.c
>>> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
>>>  #endif
>>>
>>> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
>>> -  ulong, ulong, ulong))images->ep;
>>> +  ulong, ulong, ulong))(uintptr_t)images->ep;
>>> debug ("## Transferring control to Linux (at address %08lx) ...\n",
>>> (ulong)kernel);
>>>
>>> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
>>> WATCHDOG_RESET();
>>>
>>> ((void (*)(void *, ulong, ulong, ulong,
>>> -   ulong, ulong, ulong))images->ep)(images->ft_addr,
>>> +   ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
>>> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
>>>  }
>>>  #endif
>>> diff --git a/cmd/ximg.c b/cmd/ximg.c
>>> index d033c15..70d6d14 100644
>>> --- a/cmd/ximg.c
>>> +++ b/cmd/ximg.c
>>> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char 
>>> * const argv[])
>>>  {
>>> ulong   addr = load_addr;
>>> ulong   dest = 0;
>>> -   ulong   data, len;
>>> +   phys_addr_t data;
>>> +   ulong   len;
>>> int verify;
>>> int part = 0;
>>>  #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>>> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, 
>>> char * const argv[])
>>> return 1;
>>> }
>>>
>>> -   data = (ulong)fit_data;
>>> +   data = (phys_addr_t)(uintptr_t)fit_data;
>>> len = (ulong)fit_len;
>>> break;
>>>  #endif
>>> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, 
>>> char * const argv[])
>>> }
>>>  #else  /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
>>> printf("   Loading part %d ... ", part);
>>> -   memmove((char *) dest, (char *)data, len);
>>> +   memmove((char *)dest, (char *)(uintptr_t)data, len);
>>>  #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
>>> break;
>>>  #ifdef CONFIG_GZIP
>>> case IH_COMP_GZIP:
>>> printf("   Uncompressing part %d ... ", part);
>>> if (gunzip((void *) dest, unc_len,
>>> -  (uchar *) data, ) != 0) {
>>> +  (uchar *)(uintptr_t)data, ) != 0) {
>>
>> The uintptr_t cast presumably defeats the warning. Is the intention to
>> convert a 64-bit value to 32-bit?
>
> Yes. Before this patch, all these addresses are ulong. It is enough to hold 
> the
> addresses _used_ for images on 32- and 64-bit targets. Please note, the key 
> here
> is the address used. On system with 36 or more bits for physical addresses, 
> the
> phys_addr_t is "unsigned long long". If the core is in 32-bit, the images are
> actually put in 32-bit space. So it is safe to case the 64-bit phys_addr_t to 
> a
> 32-bit pointer.
>
> The idea behind this change is to make the host tool (such as mkimge) to 
> support
> 64-bit address, even on a 32-bit host. My solution is to always use 64-bit
> variable on host. I didn't find a better way to deal with this issue.
>
>>
>>

Re: [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses

2016-02-23 Thread york sun 
On 02/16/2016 08:01 AM, Simon Glass wrote:
> Hi York,
> 
> On 12 February 2016 at 13:59, York Sun  wrote:
>> When dealing with image addresses, ulong has been used. Some files
>> are used by both host and target. It is OK for the target, but not
>> always enough for host tools including mkimage. This patch replaces
>> "ulong" with "phys_addr_t" to make sure addresses are correct for
>> both the target and the host.
>>
>> This issue was found on 32-bit host when compiling for 64-bit target
>> to support images with address higher than 32-bit space.
>>
>> Signed-off-by: York Sun 
>>
>> ---
>>
>> Changes in v4:
>>   New patch, separated from fixing FIT image.
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  arch/powerpc/lib/bootm.c |4 ++--
>>  cmd/ximg.c   |9 +
>>  common/bootm.c   |   21 +++--
>>  common/bootm_os.c|   14 --
>>  common/image-android.c   |6 +++---
>>  common/image-fdt.c   |   16 
>>  common/image-fit.c   |   27 ++-
>>  common/image.c   |   17 ++---
>>  include/bootm.h  |6 +++---
>>  include/image.h  |   30 ++
>>  tools/default_image.c|2 +-
>>  11 files changed, 83 insertions(+), 69 deletions(-)
>>
>> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
>> index ef15e7a..794382a 100644
>> --- a/arch/powerpc/lib/bootm.c
>> +++ b/arch/powerpc/lib/bootm.c
>> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
>>  #endif
>>
>> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
>> -  ulong, ulong, ulong))images->ep;
>> +  ulong, ulong, ulong))(uintptr_t)images->ep;
>> debug ("## Transferring control to Linux (at address %08lx) ...\n",
>> (ulong)kernel);
>>
>> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
>> WATCHDOG_RESET();
>>
>> ((void (*)(void *, ulong, ulong, ulong,
>> -   ulong, ulong, ulong))images->ep)(images->ft_addr,
>> +   ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
>> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
>>  }
>>  #endif
>> diff --git a/cmd/ximg.c b/cmd/ximg.c
>> index d033c15..70d6d14 100644
>> --- a/cmd/ximg.c
>> +++ b/cmd/ximg.c
>> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char 
>> * const argv[])
>>  {
>> ulong   addr = load_addr;
>> ulong   dest = 0;
>> -   ulong   data, len;
>> +   phys_addr_t data;
>> +   ulong   len;
>> int verify;
>> int part = 0;
>>  #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, 
>> char * const argv[])
>> return 1;
>> }
>>
>> -   data = (ulong)fit_data;
>> +   data = (phys_addr_t)(uintptr_t)fit_data;
>> len = (ulong)fit_len;
>> break;
>>  #endif
>> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, 
>> char * const argv[])
>> }
>>  #else  /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
>> printf("   Loading part %d ... ", part);
>> -   memmove((char *) dest, (char *)data, len);
>> +   memmove((char *)dest, (char *)(uintptr_t)data, len);
>>  #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
>> break;
>>  #ifdef CONFIG_GZIP
>> case IH_COMP_GZIP:
>> printf("   Uncompressing part %d ... ", part);
>> if (gunzip((void *) dest, unc_len,
>> -  (uchar *) data, ) != 0) {
>> +  (uchar *)(uintptr_t)data, ) != 0) {
> 
> The uintptr_t cast presumably defeats the warning. Is the intention to
> convert a 64-bit value to 32-bit?

Yes. Before this patch, all these addresses are ulong. It is enough to hold the
addresses _used_ for images on 32- and 64-bit targets. Please note, the key here
is the address used. On system with 36 or more bits for physical addresses, the
phys_addr_t is "unsigned long long". If the core is in 32-bit, the images are
actually put in 32-bit space. So it is safe to case the 64-bit phys_addr_t to a
32-bit pointer.

The idea behind this change is to make the host tool (such as mkimge) to support
64-bit address, even on a 32-bit host. My solution is to always use 64-bit
variable on host. I didn't find a better way to deal with this issue.

> 
> Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target?

No. On some arch (eg arm, mips, powerpc), phys_addr_t can be either "unsigned
long long", or "unsigned long".

> 
>>

Re: [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses

2016-02-16 Thread Simon Glass 
Hi York,

On 12 February 2016 at 13:59, York Sun  wrote:
> When dealing with image addresses, ulong has been used. Some files
> are used by both host and target. It is OK for the target, but not
> always enough for host tools including mkimage. This patch replaces
> "ulong" with "phys_addr_t" to make sure addresses are correct for
> both the target and the host.
>
> This issue was found on 32-bit host when compiling for 64-bit target
> to support images with address higher than 32-bit space.
>
> Signed-off-by: York Sun 
>
> ---
>
> Changes in v4:
>   New patch, separated from fixing FIT image.
>
> Changes in v3: None
> Changes in v2: None
>
>  arch/powerpc/lib/bootm.c |4 ++--
>  cmd/ximg.c   |9 +
>  common/bootm.c   |   21 +++--
>  common/bootm_os.c|   14 --
>  common/image-android.c   |6 +++---
>  common/image-fdt.c   |   16 
>  common/image-fit.c   |   27 ++-
>  common/image.c   |   17 ++---
>  include/bootm.h  |6 +++---
>  include/image.h  |   30 ++
>  tools/default_image.c|2 +-
>  11 files changed, 83 insertions(+), 69 deletions(-)
>
> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
> index ef15e7a..794382a 100644
> --- a/arch/powerpc/lib/bootm.c
> +++ b/arch/powerpc/lib/bootm.c
> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
>  #endif
>
> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
> -  ulong, ulong, ulong))images->ep;
> +  ulong, ulong, ulong))(uintptr_t)images->ep;
> debug ("## Transferring control to Linux (at address %08lx) ...\n",
> (ulong)kernel);
>
> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
> WATCHDOG_RESET();
>
> ((void (*)(void *, ulong, ulong, ulong,
> -   ulong, ulong, ulong))images->ep)(images->ft_addr,
> +   ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
>  }
>  #endif
> diff --git a/cmd/ximg.c b/cmd/ximg.c
> index d033c15..70d6d14 100644
> --- a/cmd/ximg.c
> +++ b/cmd/ximg.c
> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * 
> const argv[])
>  {
> ulong   addr = load_addr;
> ulong   dest = 0;
> -   ulong   data, len;
> +   phys_addr_t data;
> +   ulong   len;
> int verify;
> int part = 0;
>  #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char 
> * const argv[])
> return 1;
> }
>
> -   data = (ulong)fit_data;
> +   data = (phys_addr_t)(uintptr_t)fit_data;
> len = (ulong)fit_len;
> break;
>  #endif
> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, 
> char * const argv[])
> }
>  #else  /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
> printf("   Loading part %d ... ", part);
> -   memmove((char *) dest, (char *)data, len);
> +   memmove((char *)dest, (char *)(uintptr_t)data, len);
>  #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
> break;
>  #ifdef CONFIG_GZIP
> case IH_COMP_GZIP:
> printf("   Uncompressing part %d ... ", part);
> if (gunzip((void *) dest, unc_len,
> -  (uchar *) data, ) != 0) {
> +  (uchar *)(uintptr_t)data, ) != 0) {

The uintptr_t cast presumably defeats the warning. Is the intention to
convert a 64-bit value to 32-bit?

Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target?

> puts("GUNZIP ERROR - image not loaded\n");
> return 1;
> }
> diff --git a/common/bootm.c b/common/bootm.c
> index 99d574d..785858c 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
>
>  static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>char * const argv[], bootm_headers_t 
> *images,
> -  ulong *os_data, ulong *os_len);
> +  phys_addr_t *os_data, ulong *os_len);
>
>  #ifdef CONFIG_LMB
>  static void boot_start_lmb(bootm_headers_t *images)
> @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t 
> uncomp_size,
> return BOOTM_ERR_RESET;
>  }
>
> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
> -