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

[Wolfgang Denk <wd@denx.de>]

Dear York Sun,

In message <1456439779-4792-2-git-send-email-york.sun@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?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
Nearly everyone is in favor of going  to  heaven  but  too  many  are
hoping  they'll  live  long  enough  to see an easing of the entrance
requirements. Never appeal to a man's "better nature." he  might  not
have one.