[U-Boot] [PATCH v2 1/9] fdt_support: fdt reservations on the sandbox

[Heinrich Schuchardt]

On 11/13/18 10:35 PM, Simon Glass wrote:
> Hi,
> 
> On 13 November 2018 at 12:39, Alexander Graf <agraf at suse.de> wrote:
>>
>>
>> On 12.11.18 18:55, Heinrich Schuchardt wrote:
>>> On the sandbox the memory addresses in the device tree refer to the virtual
>>> address space of the sandbox. This implies that the memory reservations for
>>> the fdt also have to be converted to this address space.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>> v2:
>>>       no change
>>> ---
>>>  common/fdt_support.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index e6daa67990d..ec6af92dbba 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -7,6 +7,7 @@
>>>   */
>>>
>>>  #include <common.h>
>>> +#include <mapmem.h>
>>>  #include <stdio_dev.h>
>>>  #include <linux/ctype.h>
>>>  #include <linux/types.h>
>>> @@ -240,7 +241,8 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end)
>>>               }
>>>       }
>>>
>>> -     err = fdt_add_mem_rsv(fdt, initrd_start, initrd_end - initrd_start);
>>> +     err = fdt_add_mem_rsv((void *)(uintptr_t)map_to_sysmem(fdt),
>>
>> This sounds like an API bug to me. Either fdt_add_mem_rsv() takes an
>> address (ulong) or a pointer (void*). But taking void * while expecting
>> an address to get passed in sounds pretty broken to me.
> 
> Yes that can't be right. If it, we need to change the first param of
> fdt_initrd() to a ulong and adjust.

Why? The first parameter is where address where the fdt lives. For any
board but sandbox the current API is fine. And the sandbox is not the
world but the playground.

> 
> We should use addresses in U-Boot code, not pointers.
> We should use addresses in U-Boot code, not pointers.
> We should use addresses in U-Boot code, not pointers.

Your repetitive poetry does not catch. A pointer holds an address in
memory. Usage of different C types for the same content is just a way to
confuse ones mind.

Regards

Heinrich

> 
> It avoids so much confusion.
> 
> Regards,
> Simon
> 
>>
>> Simon?
>>
>>
>> Alex
>>
>>> +                           initrd_start, initrd_end - initrd_start);
>>>       if (err < 0) {
>>>               printf("fdt_initrd: %s\n", fdt_strerror(err));
>>>               return err;
>>> @@ -633,7 +635,8 @@ int fdt_shrink_to_minimum(void *blob, uint extrasize)
>>>       fdt_set_totalsize(blob, actualsize);
>>>
>>>       /* Add the new reservation */
>>> -     ret = fdt_add_mem_rsv(blob, (uintptr_t)blob, actualsize);
>>> +     ret = fdt_add_mem_rsv((void *)(uintptr_t)map_to_sysmem(blob),
>>> +                           (uintptr_t)blob, actualsize);
>>>       if (ret < 0)
>>>               return ret;
>>>
>>>
>