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

[Heinrich Schuchardt]

On 4/14/20 9:46 AM, Michal Simek wrote:
> On 14. 04. 20 1:38, Heinrich Schuchardt wrote:
>> On 4/13/20 10:03 AM, 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.
>>
>> Please, describe why this patch is needed.
>>
>>>
>>> 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);
>>
>> This change does not relate to the commit message. You are not adjusting
>> the alignment of the device tree location. You are adjusting the size of
>> the memory available for the device tree.
>
> Let me correct that description in v2.
> It is additional size for actual FDT size. I am also not sure why this
> is needed.

In image_setup_libfdt() additional fields are added to the device tree.
There must be some space available to accommodate these. Then we pass
the device tree to the operating system or bootloader which again may
make changes.

In the case of UEFI booting we make a copy of the original device tree
before updating and currently provide 12 KiB of headspace. This copy is
needed because the UEFI binary may return to U-Boot and when we call the
next binary we want to use the original device tree. The alignment of
the copy is 4 KiB.

Best regards

Heinrich

>
>
>>
>> reserve_fdt() is called after reserve_global_data() where we have:
>> gd->start_addr_sp -= sizeof(gd_t).
>>
>> A typical debug output from reserve_global_data() is:
>> Reserving 544 Bytes for Global Data at: bf740d98
>>
>> So in this case the device tree will have an alignment of 8 no matter
>> what power of 2 (>= 8) you choose for CONFIG_FDT_MEM_ALIGN_SIZE.
>
> fdt_size should be aligned by 32 bytes as is written above.
>
>
>>
>> What about other places where we allocate memory for device trees like
>> copy_fdt() in cmd/bootefi.?
>
> This is interesting. You are using 0x3000 alignment.
>
> This address is also interesting:
> 127         new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 +
> 128                                              fdt_size, 0);
>
> Anyway. If you are happy to use 0x1000 here we can also include it.
> Also if you are aware about any other location which is related to this
> we are happy to include it.
>
>
>
>>
>>> 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
>>
>> %s/is used to set the default alignment/sets the alignment/
>>
>> Please, mention if this value should be a power of 2.
>
> And is it really necessary? I think the question is why there is 0x1000
> additional space. It was added by Simon in 2013. Is this space even used
> for something?
> I expect that you don't run fdt resize on in this space anyway.
>
> Thanks,
> Michal
>