[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[Michal Simek]

From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

There is no technical reason to add additional 4k space for FDT. This space
is completely unused and just increase memory requirements. This is
problematic on systems with limited memory resources as Xilinx Zynq
CSE/ZynqMP mini and Versal mini configurations.

The patch is removing additional 4k space.

EFI code is using copy_fdt() which copy FDT to different location.
And all boot commands in case of using U-Boot's FDT pointed by
$fdtcontroladdr are copying FDT to different locations by
image_setup_libfdt().
That's why in proper flow none should modified DTB used by U-Boot that's
why there is no need for additional space.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3:
- Remove alignment change and keep it as 32

Changes in v2:
- Change subject (was: common: Add Kconfig option for FDT mem alignment)
- Remove Kconfig symbol
- Extend description

I have tested it on zcu104.
Outcome from v2 review was to remove that alignment part and if that works
on all boards then it is ready to go. The best as early as possible.

Tom: Would be good to add it to your queue for testing to spot any issue
earlier.

---
 common/board_f.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index 01194eaa0e4d..dcad551ae434 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -537,7 +537,7 @@ 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), 32);
 
 		gd->start_addr_sp = reserve_stack_aligned(gd->fdt_size);
 		gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);
-- 
2.27.0

[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[Heinrich Schuchardt]

On 6/18/20 10:51 AM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>
> There is no technical reason to add additional 4k space for FDT. This space
> is completely unused and just increase memory requirements. This is
> problematic on systems with limited memory resources as Xilinx Zynq
> CSE/ZynqMP mini and Versal mini configurations.
>
> The patch is removing additional 4k space.
>
> EFI code is using copy_fdt() which copy FDT to different location.
> And all boot commands in case of using U-Boot's FDT pointed by
> $fdtcontroladdr are copying FDT to different locations by
> image_setup_libfdt().
> That's why in proper flow none should modified DTB used by U-Boot that's
> why there is no need for additional space.
>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v3:
> - Remove alignment change and keep it as 32
>
> Changes in v2:
> - Change subject (was: common: Add Kconfig option for FDT mem alignment)
> - Remove Kconfig symbol
> - Extend description
>
> I have tested it on zcu104.
> Outcome from v2 review was to remove that alignment part and if that works
> on all boards then it is ready to go. The best as early as possible.
>
> Tom: Would be good to add it to your queue for testing to spot any issue
> earlier.
>
> ---
>   common/board_f.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 01194eaa0e4d..dcad551ae434 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -537,7 +537,7 @@ 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), 32);

reserve_stack_aligned() already rounds the size up to a multiple of 16.
The ALIGN() here is simply wasting 16 bytes of memory in half of the cases.

Best regards

Heinrich

>
>   		gd->start_addr_sp = reserve_stack_aligned(gd->fdt_size);
>   		gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);
>

[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[Michal Simek]

On 18. 06. 20 12:48, Heinrich Schuchardt wrote:
> On 6/18/20 10:51 AM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>
>> There is no technical reason to add additional 4k space for FDT. This
>> space
>> is completely unused and just increase memory requirements. This is
>> problematic on systems with limited memory resources as Xilinx Zynq
>> CSE/ZynqMP mini and Versal mini configurations.
>>
>> The patch is removing additional 4k space.
>>
>> EFI code is using copy_fdt() which copy FDT to different location.
>> And all boot commands in case of using U-Boot's FDT pointed by
>> $fdtcontroladdr are copying FDT to different locations by
>> image_setup_libfdt().
>> That's why in proper flow none should modified DTB used by U-Boot that's
>> why there is no need for additional space.
>>
>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v3:
>> - Remove alignment change and keep it as 32
>>
>> Changes in v2:
>> - Change subject (was: common: Add Kconfig option for FDT mem alignment)
>> - Remove Kconfig symbol
>> - Extend description
>>
>> I have tested it on zcu104.
>> Outcome from v2 review was to remove that alignment part and if that
>> works
>> on all boards then it is ready to go. The best as early as possible.
>>
>> Tom: Would be good to add it to your queue for testing to spot any issue
>> earlier.
>>
>> ---
>> ? common/board_f.c | 2 +-
>> ? 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index 01194eaa0e4d..dcad551ae434 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -537,7 +537,7 @@ 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), 32);
> 
> reserve_stack_aligned() already rounds the size up to a multiple of 16.
> The ALIGN() here is simply wasting 16 bytes of memory in half of the cases.

As we discussed in v2. Let's just deal with alignment in separate patch.

Thanks,
Michal

[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[Stephen Warren]

On 6/18/20 2:51 AM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> 
> There is no technical reason to add additional 4k space for FDT. This space
> is completely unused and just increase memory requirements. This is
> problematic on systems with limited memory resources as Xilinx Zynq
> CSE/ZynqMP mini and Versal mini configurations.
> 
> The patch is removing additional 4k space.
> 
> EFI code is using copy_fdt() which copy FDT to different location.
> And all boot commands in case of using U-Boot's FDT pointed by
> $fdtcontroladdr are copying FDT to different locations by
> image_setup_libfdt().
> That's why in proper flow none should modified DTB used by U-Boot that's
> why there is no need for additional space.

Acked-by: Stephen Warren <swarren@nvidia.com>

[PATCH v3] common: fdt: Remove additional 4k space for fdt allocation

[Michal Simek]

?t 18. 6. 2020 v 10:51 odes?latel Michal Simek <michal.simek@xilinx.com> napsal:
>
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>
> There is no technical reason to add additional 4k space for FDT. This space
> is completely unused and just increase memory requirements. This is
> problematic on systems with limited memory resources as Xilinx Zynq
> CSE/ZynqMP mini and Versal mini configurations.
>
> The patch is removing additional 4k space.
>
> EFI code is using copy_fdt() which copy FDT to different location.
> And all boot commands in case of using U-Boot's FDT pointed by
> $fdtcontroladdr are copying FDT to different locations by
> image_setup_libfdt().
> That's why in proper flow none should modified DTB used by U-Boot that's
> why there is no need for additional space.
>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v3:
> - Remove alignment change and keep it as 32
>
> Changes in v2:
> - Change subject (was: common: Add Kconfig option for FDT mem alignment)
> - Remove Kconfig symbol
> - Extend description
>
> I have tested it on zcu104.
> Outcome from v2 review was to remove that alignment part and if that works
> on all boards then it is ready to go. The best as early as possible.
>
> Tom: Would be good to add it to your queue for testing to spot any issue
> earlier.
>
> ---
>  common/board_f.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 01194eaa0e4d..dcad551ae434 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -537,7 +537,7 @@ 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), 32);
>
>                 gd->start_addr_sp = reserve_stack_aligned(gd->fdt_size);
>                 gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);
> --
> 2.27.0
>

Applied.
M

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs