[U-Boot] [RFC PATCH v5 1/4] common: Convert ulong to phys_addr_t for image addresses

[york sun]

On 02/25/2016 03:05 PM, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1456439779-4792-2-git-send-email-york.sun at nxp.com> you 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.
> 
> You talk here about using "phys_addr_t"...
> 
>> -			   ulong, ulong, ulong))images->ep;
>> +			   ulong, ulong, ulong))(uintptr_t)images->ep;
> 
> ...but here you use  uintptr_t  , hich is something different?
> 
>> -		ulong, ulong, ulong))images->ep)(images->ft_addr,
>> +		ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
> 
> Ditto.
> 
>> +	phys_addr_t os_data;
>> +	ulong os_len;
>>  	void *data = NULL;
>>  	size_t len;
>>  	int ret;
>> @@ -87,11 +89,10 @@ static int boot_prep_linux(bootm_headers_t *images)
>>  	if (images->legacy_hdr_valid) {
>>  		hdr = images->legacy_hdr_os;
>>  		if (image_check_type(hdr, IH_TYPE_MULTI)) {
>> -			ulong os_data, os_len;
> 
> Why do you moe the declarations out of this block?  The variables are
> only used within this block so there is no need for a wider scope?
> 
>> -			data = (void *)os_data;
>> +			data = (void *)(uintptr_t)os_data;
> 
> This double cast looks scary to me, and you don;t explain it in the
> commit message.  Why exactly is this needed?
> 
>> -		cmd_line_dest = (void *)images->ep + COMMAND_LINE_OFFSET;
>> +		cmd_line_dest = (void *)(uintptr_t)images->ep +
>> +				COMMAND_LINE_OFFSET;
> 
> Ditto.
> 
>> -	printf("Setup at %#08lx\n", images->ep);
>> -	ret = setup_zimage((void *)images->ep, cmd_line_dest,
>> +	printf("Setup at %#08" PRIpa "\n", images->ep);
> 
> This is really ugly...
> 
>> +	ret = setup_zimage((void *)(uintptr_t)images->ep, cmd_line_dest,
> 
> See before.
> 
>> -	debug("## Transferring control to Linux (at address %08lx, kernel %08lx) ...\n",
>> +	debug("## Transferring control to Linux (at address %#08" PRIpa
>> +	      ", kernel %#08" PRIpa ") ...\n",
> 
> See before...
> 
>> -		debug("*  kernel: cmdline image address = 0x%08lx\n",
>> -			images->ep);
>> +		debug("*  kernel: cmdline image address = %#08" PRIpa "\n",
>> +		      images->ep);
> 
> Ditto.  etc. etc.
> 
>> +	/*
>> +	 * In this function, data is decalred as phys_addr_t type.
> 
> s/decalred/declared/
> 
>> +	 * On some systems (eg. ARM, PowerPC) phys_addr_t can be
>> +	 * "unsigned long", or "unsigned long long", depending on
>> +	 * CONFIG_PHYS_64BIT.  It is safe to cast 64-bit phys_addr_t
>> +	 * to 32-bit pointer for image handling because the actual
>> +	 * address the image is loaded is within 32-bit space.
> 
> Who guarantees that?
> 
>> -		data = (ulong)fit_data;
>> +		data = (phys_addr_t)(uintptr_t)fit_data;
> 
> This double cast looks strange to me.  Why is it needed?
> 
>> -				void *from = (void *)data;
>> +				void *from = (void *)(uintptr_t)data;
> 
> Ditto.
> 
>> -			memmove((char *) dest, (char *)data, len);
>> +			memmove((char *)dest, (char *)(uintptr_t)data, len);
> 
> Ditto. etc. etc.
> 
> 
> All these double casts look somewhat wrong to me.  Why are they
> needed?

Dear Wolfgang,

I can use some serious help here. What I am really trying to achieve is the last
two patches in this set. I didn't want to use replace ulong with phys_addr_t. I
am not proud with the change I proposed, but I didn't come up with a smarter
solution. My specific trouble is to build ARMv8 targets on 32-bit Ubuntu host.
Some code is shared between the target and host tool (mkimage). I started from
small changes, but it gets wider and wider when I tried to get rid of the
compiling warnings.

York