[PATCH] common: Add Kconfig option for FDT mem alignment

[Michal Simek]

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.soma at 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.soma at xilinx.com>
>>>>> Signed-off-by: Michal Simek <michal.simek at 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