Dear York Sun,
In message <1456439779-4792-2-git-send-email-york....@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: w...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
