diff mbox series

common: Add Kconfig option for FDT mem alignment

Message ID b6e99dd460d741a86cdbae18dd3f71ba072ea8f3.1586764982.git.michal.simek@xilinx.com
State New
Headers show
Series common: Add Kconfig option for FDT mem alignment | expand

Commit Message

Michal Simek April 13, 2020, 8:03 a.m. UTC 
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(-)

Comments

Heinrich Schuchardt April 13, 2020, 11:38 p.m. UTC | #1 
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.

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.

What about other places where we allocate memory for device trees like
copy_fdt() in cmd/bootefi.?

> 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.

Best regards

Heinrich

> +	  for fdt.
> +
>  config OF_LIST
>  	string "List of device tree files to include for DT control"
>  	depends on SPL_LOAD_FIT || MULTI_DTB_FIT
>
Michal Simek April 14, 2020, 7:46 a.m. UTC | #2 
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.


> 
> 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
Tom Rini April 16, 2020, 3:39 p.m. UTC | #3 
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!
Michal Simek April 16, 2020, 3:47 p.m. UTC | #4 
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?

Thanks,
Michal
Heinrich Schuchardt April 16, 2020, 4:45 p.m. UTC | #5 
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
>
Tom Rini April 16, 2020, 5:06 p.m. UTC | #6 
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?
Stephen Warren April 16, 2020, 5:21 p.m. UTC | #7 
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.

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.
Michal Simek April 17, 2020, 6:49 a.m. UTC | #8 
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
Simon Glass April 19, 2020, 11:37 p.m. UTC | #9 
On Mon, 13 Apr 2020 at 02:03, Michal Simek <michal.simek at xilinx.com> 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(-)

+Marek since he is doing a similar thing



>
> 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.
> +
>  config OF_LIST
>         string "List of device tree files to include for DT control"
>         depends on SPL_LOAD_FIT || MULTI_DTB_FIT
> --
> 2.26.0
>
Michal Simek April 20, 2020, 8:28 a.m. UTC | #10 
On 20. 04. 20 1:37, Simon Glass wrote:
> On Mon, 13 Apr 2020 at 02:03, Michal Simek <michal.simek at xilinx.com> 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(-)
> 
> +Marek since he is doing a similar thing

I talked to Marek and his approach is different
https://lists.denx.de/pipermail/u-boot/2020-April/406438.html

He is adding padding to DTB and based on our chat he wants to touch dtb
before this space is allocated. That's why I still don't think that this
4k space is really used anywhere.

It means I am heading to v2 where DTB start address should be aligned to
start at 64bit aligned address and remove this extra unused space. If
someone requires to have more space it should be done by adding padding
before DTB is built.

What do you think?

Thanks,
Michal
diff mbox series

Patch

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.
+
 config OF_LIST
 	string "List of device tree files to include for DT control"
 	depends on SPL_LOAD_FIT || MULTI_DTB_FIT