On 02/26/2016 09:31 AM, Simon Glass wrote:
> Hi York,
>
> On 26 February 2016 at 10:22, york sun <york....@nxp.com> wrote:
>> On 02/25/2016 03:05 PM, Wolfgang Denk wrote:
>>> 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?
>>
>> 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
>>
>
> I suggest just documenting it better with comments and in the commit
> message. It's mostly the same comment I made.
>
> One concern is that if you cast to uintptr_t on a 32-bit host machine,
> won't you end up dropping the top 32 bits?
>
Simon,
Some code is shared between targets and hosts, but not all. The ugly casting is
used by targets only. Maybe I should take another look at the issue and back
away from converting ulong to phys_addr_t totally. It is only broken on 32-bit
host tool and seems nobody really cares. Sandbox is also broken on 32-bit host
(compiling warnings), and yet no one complains. Am I the only one living in old
age? I can upgrade to 64-bit Ubuntu and we all can forget about this mess I
created. (Getting exhausted here...)
York
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
