On 16. 04. 20 19:21, Stephen Warren wrote:
> On 4/16/20 11:06 AM, Tom Rini wrote:
>> On Thu, Apr 16, 2020 at 05:47:41PM +0200, Michal Simek wrote:
>>> On 16. 04. 20 17:39, Tom Rini wrote:
>>>> On Mon, Apr 13, 2020 at 10:03:04AM +0200, Michal Simek wrote:
>>>>
>>>>> From: Ashok Reddy Soma <ashok.reddy.s...@xilinx.com>
>>>>>
>>>>> FDT memory is aligned by 4KB. This is hardcoded in common/board_f.c.
>>>>> Add Kconfig option, assign default value of 0x1000 and enable option to
>>>>> change this value.
>>>>>
>>>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.s...@xilinx.com>
>>>>> Signed-off-by: Michal Simek <michal.si...@xilinx.com>
>>>>> ---
>>>>>
>>>>> common/board_f.c | 3 ++-
>>>>> dts/Kconfig | 7 +++++++
>>>>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>> index 82a164752aa3..928874e03555 100644
>>>>> --- a/common/board_f.c
>>>>> +++ b/common/board_f.c
>>>>> @@ -546,7 +546,8 @@ static int reserve_fdt(void)
>>>>> * will be relocated with other data.
>>>>> */
>>>>> if (gd->fdt_blob) {
>>>>> - gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
>>>>> + gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) +
>>>>> + CONFIG_FDT_MEM_ALIGN_SIZE, 32);
>>>>>
>>>>> gd->start_addr_sp -= gd->fdt_size;
>>>>> gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);
>>>>> diff --git a/dts/Kconfig b/dts/Kconfig
>>>>> index 046a54a17366..696c0b71afaf 100644
>>>>> --- a/dts/Kconfig
>>>>> +++ b/dts/Kconfig
>>>>> @@ -121,6 +121,13 @@ config DEFAULT_DEVICE_TREE
>>>>> It can be overridden from the command line:
>>>>> $ make DEVICE_TREE=<device-tree-name>
>>>>>
>>>>> +config FDT_MEM_ALIGN_SIZE
>>>>> + hex "FDT memory alignment size"
>>>>> + default 0x1000
>>>>> + help
>>>>> + This option is used to set the default alignment when reserving memory
>>>>> + for fdt.
>>>>
>>>> I'm not sure this is clear enough of a description. At first I thought
>>>> this was about where the start address needs to be, and that is 8 bytes
>>>> and non-configurable (both arm32 and arm64 require 64bit aligned). What
>>>> I don't see is where the size of the overall blob needs to be aligned.
>>>> We need to point at that requirement somewhere in the help so that we
>>>> don't have people using bad values and then working around it without
>>>> realizing the problem. Thanks!
>>>
>>> Definitely description should be better as we agreed in second review.
>>> But the point is do we really need such a big FDT alignment here?
>>> It was added by Simon long time ago without any explanation as the part
>>> of bigger patch.
>>> But are we allowing resizing FDT that we need additional "4k" alignment?
>>
>> So, I stumbled on an even older thread asking about this where it seems
>> like part of the reason we do something here is so that there's room to
>> add bootargs, etc to /chosen. The start address must be 64bit-aligned,
>> but the size seems like it just needs to be big enough for everything we
>> could have in it, and at this point in the code we don't know just how
>> much that is?
>
> Adding space for DT modifications certainly makes sense. This isn't
> alignment though, it's just adding space.
yes but 4k additional space pointed by $fdtcontroladdr.
As Heinrich pointed out UEFI is doing own copy.
I tried to run booti 80000 - $fdtcontroladdr (bootm should be the same)
and DT is copied to different location anyway (based on fdt_high) which
is reported by
"Loading Device Tree to 000000000fff3000, end 000000000fffffff ... OK"
And on this copy image_setup_libfdt() is called.
And 4k is not enough for playing games with DT overlays anyway.
That's why my question remains. Do we use this additional 4k space in GD
structure for any purpose?
>
> Looking back at the original patch, there was no change to alignment of
> the start or end of the DT, just the ability to configure the amount of
> extra space added to the DT. Nothing should refer to that as alignment,
> since it's not.
That description will be fixed when we finish this discussion.
Thanks,
Michal
