[PATCH 0/2] Add support for loading images above 4GB

[PATCH 0/2] Add support for loading images above 4GB

[Michal Simek]

Hi,

We have several use cases where customers want to partition memory by
putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
others CPU in the systems (like R5) or for secure sw.
Currently there is limitation in SPL to record load/entry addresses in
64bit format because they are recorded in 32bit only.
This series add support for it.
Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
images generated by binman. Because u-boot is using
CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
and there is no real need to build it to certain offset.

Thanks,
Michal


Michal Simek (2):
  spl: Use standard FIT entries
  spl: fdt: Record load/entry fit-images entries in 64bit format

 common/fdt_support.c | 9 ++-------
 common/spl/spl_atf.c | 7 ++++---
 common/spl/spl_fit.c | 6 +++++-
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.28.0

[PATCH 1/2] spl: Use standard FIT entries

[Michal Simek]

SPL is creating fit-images DT node when loadables are recorded in selected
configuration. Entries which are created are using entry-point and
load-addr property names. But there shouldn't be a need to use non standard
properties because entry/load are standard FIT properties. But using
standard FIT properties enables option to use generic FIT functions to
descrease SPL size. Here is result for ZynqMP virt configuration:
xilinx_zynqmp_virt: spl/u-boot-spl:all -82 spl/u-boot-spl:rodata -22 spl/u-boot-spl:text -60

The patch causes change in run time fit image record.
Before:
fit-images {
        uboot {
                os = "u-boot";
                type = "firmware";
                size = <0xfd520>;
                entry-point = <0x8000000>;
                load-addr = <0x8000000>;
        };
};

After:
fit-images {
        uboot {
                os = "u-boot";
                type = "firmware";
                size = <0xfd520>;
                entry = <0x8000000>;
                load = <0x8000000>;
        };
};

Replacing calling fdt_getprop_u32() by fit_image_get_entry/load() also
enables support for reading entry/load properties recorded in 64bit format.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Would be good to know history of fit-images and it's property names but
there shouldn't be a need to use non standard names where we have
FIT_*_PROP recorded as macros in include/image.h.

Concern regarding backward compatibility is definitely valid but not sure
how many systems can be affected by this change.

Adding temporary support for entry-point/load-addr is also possible.
Or second way around is to create new wrappers as
fit_image_get_entry_point()/fit_image_get_load_addr() or
call fit_image_get_address() directly.

---
 common/fdt_support.c | 4 ++--
 common/spl/spl_atf.c | 7 ++++---
 common/spl/spl_fit.c | 6 +++++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index a565b470f81e..b8a8768a2147 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -616,9 +616,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name,
 	 * However, spl_fit.c is not 64bit safe either: i.e. we should not
 	 * have an issue here.
 	 */
-	fdt_setprop_u32(blob, node, "load-addr", load_addr);
+	fdt_setprop_u32(blob, node, "load", load_addr);
 	if (entry_point != -1)
-		fdt_setprop_u32(blob, node, "entry-point", entry_point);
+		fdt_setprop_u32(blob, node, "entry", entry_point);
 	fdt_setprop_u32(blob, node, "size", size);
 	if (type)
 		fdt_setprop_string(blob, node, "type", type);
diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
index b54b4f0d22e2..9bd25f6b32c1 100644
--- a/common/spl/spl_atf.c
+++ b/common/spl/spl_atf.c
@@ -132,10 +132,11 @@ static int spl_fit_images_find(void *blob, int os)
 uintptr_t spl_fit_images_get_entry(void *blob, int node)
 {
 	ulong  val;
+	int ret;
 
-	val = fdt_getprop_u32(blob, node, "entry-point");
-	if (val == FDT_ERROR)
-		val = fdt_getprop_u32(blob, node, "load-addr");
+	ret = fit_image_get_entry(blob, node, &val);
+	if (ret)
+		ret = fit_image_get_load(blob, node, &val);
 
 	debug("%s: entry point 0x%lx\n", __func__, val);
 	return val;
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 365104fe0288..b69b3948841f 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -332,9 +332,13 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 	}
 
 	if (image_info) {
+		ulong entry_point;
+
 		image_info->load_addr = load_addr;
 		image_info->size = length;
-		image_info->entry_point = fdt_getprop_u32(fit, node, "entry");
+
+		if (!fit_image_get_entry(fit, node, &entry_point))
+			image_info->entry_point = entry_point;
 	}
 
 	return 0;
-- 
2.28.0

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Michal Simek]

The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
problem to record these addresses in 64bit format.
The patch adds support for systems which need to load images above 4GB.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 common/fdt_support.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index b8a8768a2147..5ae75df3c658 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name,
 	if (node < 0)
 		return node;
 
-	/*
-	 * We record these as 32bit entities, possibly truncating addresses.
-	 * However, spl_fit.c is not 64bit safe either: i.e. we should not
-	 * have an issue here.
-	 */
-	fdt_setprop_u32(blob, node, "load", load_addr);
+	fdt_setprop_u64(blob, node, "load", load_addr);
 	if (entry_point != -1)
-		fdt_setprop_u32(blob, node, "entry", entry_point);
+		fdt_setprop_u64(blob, node, "entry", entry_point);
 	fdt_setprop_u32(blob, node, "size", size);
 	if (type)
 		fdt_setprop_string(blob, node, "type", type);
-- 
2.28.0

[PATCH 0/2] Add support for loading images above 4GB

[Heinrich Schuchardt]

On 9/3/20 1:03 PM, Michal Simek wrote:
> Hi,
>
> We have several use cases where customers want to partition memory by
> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
> others CPU in the systems (like R5) or for secure sw.
> Currently there is limitation in SPL to record load/entry addresses in
> 64bit format because they are recorded in 32bit only.
> This series add support for it.
> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
> images generated by binman. Because u-boot is using
> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
> and there is no real need to build it to certain offset.
>
> Thanks,
> Michal

Hello Michal,

does this series require changes to doc/uImage.FIT/source_file_format.txt?

Best regards

Heinrich

>
>
> Michal Simek (2):
>   spl: Use standard FIT entries
>   spl: fdt: Record load/entry fit-images entries in 64bit format
>
>  common/fdt_support.c | 9 ++-------
>  common/spl/spl_atf.c | 7 ++++---
>  common/spl/spl_fit.c | 6 +++++-
>  3 files changed, 11 insertions(+), 11 deletions(-)
>

[PATCH 0/2] Add support for loading images above 4GB

[Michal Simek]

Hi,

On 03. 09. 20 13:16, Heinrich Schuchardt wrote:
> On 9/3/20 1:03 PM, Michal Simek wrote:
>> Hi,
>>
>> We have several use cases where customers want to partition memory by
>> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
>> others CPU in the systems (like R5) or for secure sw.
>> Currently there is limitation in SPL to record load/entry addresses in
>> 64bit format because they are recorded in 32bit only.
>> This series add support for it.
>> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
>> images generated by binman. Because u-boot is using
>> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
>> and there is no real need to build it to certain offset.
>>
>> Thanks,
>> Michal
> 
> Hello Michal,
> 
> does this series require changes to doc/uImage.FIT/source_file_format.txt?

I am not changing fit format. I am just changing how SPL records
loadables in DT fit-images node. And also series is using FIT functions
for reading these properties (at least in this version).

Thanks,
Michal

[PATCH 1/2] spl: Use standard FIT entries

[Simon Glass]

Hi Michal,

On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>
> SPL is creating fit-images DT node when loadables are recorded in selected
> configuration. Entries which are created are using entry-point and
> load-addr property names. But there shouldn't be a need to use non standard
> properties because entry/load are standard FIT properties. But using
> standard FIT properties enables option to use generic FIT functions to
> descrease SPL size. Here is result for ZynqMP virt configuration:
> xilinx_zynqmp_virt: spl/u-boot-spl:all -82 spl/u-boot-spl:rodata -22 spl/u-boot-spl:text -60
>
> The patch causes change in run time fit image record.
> Before:
> fit-images {
>         uboot {
>                 os = "u-boot";
>                 type = "firmware";
>                 size = <0xfd520>;
>                 entry-point = <0x8000000>;
>                 load-addr = <0x8000000>;
>         };
> };
>
> After:
> fit-images {
>         uboot {
>                 os = "u-boot";
>                 type = "firmware";
>                 size = <0xfd520>;
>                 entry = <0x8000000>;
>                 load = <0x8000000>;
>         };
> };
>
> Replacing calling fdt_getprop_u32() by fit_image_get_entry/load() also
> enables support for reading entry/load properties recorded in 64bit format.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---

Reviewed-by: Simon Glass <sjg@chromium.org>

Isn't there a test that could be updated here?

>
> Would be good to know history of fit-images and it's property names but
> there shouldn't be a need to use non standard names where we have
> FIT_*_PROP recorded as macros in include/image.h.

I agree.

> Concern regarding backward compatibility is definitely valid but not sure
> how many systems can be affected by this change.

Me neither. Probably a good idea to fix it.

>
> Adding temporary support for entry-point/load-addr is also possible.
> Or second way around is to create new wrappers as
> fit_image_get_entry_point()/fit_image_get_load_addr() or
> call fit_image_get_address() directly.
>
> ---
>  common/fdt_support.c | 4 ++--
>  common/spl/spl_atf.c | 7 ++++---
>  common/spl/spl_fit.c | 6 +++++-
>  3 files changed, 11 insertions(+), 6 deletions(-)
>

Regards,
SImon

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Simon Glass]

Hi Michal,

On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>
> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
> problem to record these addresses in 64bit format.
> The patch adds support for systems which need to load images above 4GB.

But what about 32-bit systems who read this as a 32-bit number?
Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT?

>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  common/fdt_support.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index b8a8768a2147..5ae75df3c658 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name,
>         if (node < 0)
>                 return node;
>
> -       /*
> -        * We record these as 32bit entities, possibly truncating addresses.
> -        * However, spl_fit.c is not 64bit safe either: i.e. we should not
> -        * have an issue here.
> -        */
> -       fdt_setprop_u32(blob, node, "load", load_addr);
> +       fdt_setprop_u64(blob, node, "load", load_addr);
>         if (entry_point != -1)
> -               fdt_setprop_u32(blob, node, "entry", entry_point);
> +               fdt_setprop_u64(blob, node, "entry", entry_point);
>         fdt_setprop_u32(blob, node, "size", size);
>         if (type)
>                 fdt_setprop_string(blob, node, "type", type);
> --
> 2.28.0
>

Regards,
SImon

[PATCH 0/2] Add support for loading images above 4GB

[Simon Glass]

Hi Michal,

On Thu, 3 Sep 2020 at 06:30, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi,
>
> On 03. 09. 20 13:16, Heinrich Schuchardt wrote:
> > On 9/3/20 1:03 PM, Michal Simek wrote:
> >> Hi,
> >>
> >> We have several use cases where customers want to partition memory by
> >> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
> >> others CPU in the systems (like R5) or for secure sw.
> >> Currently there is limitation in SPL to record load/entry addresses in
> >> 64bit format because they are recorded in 32bit only.
> >> This series add support for it.
> >> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
> >> images generated by binman. Because u-boot is using
> >> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
> >> and there is no real need to build it to certain offset.
> >>
> >> Thanks,
> >> Michal
> >
> > Hello Michal,
> >
> > does this series require changes to doc/uImage.FIT/source_file_format.txt?
>
> I am not changing fit format. I am just changing how SPL records
> loadables in DT fit-images node. And also series is using FIT functions
> for reading these properties (at least in this version).

Well I think there should be some mention of the 32/64 bit issue added
to the FIT docs. How about adding an example with 64-bit addresses?

Regards,
Simon
'

[PATCH 1/2] spl: Use standard FIT entries

[Michal Simek]

Hi Simon,

On 07. 09. 20 3:43, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> SPL is creating fit-images DT node when loadables are recorded in selected
>> configuration. Entries which are created are using entry-point and
>> load-addr property names. But there shouldn't be a need to use non standard
>> properties because entry/load are standard FIT properties. But using
>> standard FIT properties enables option to use generic FIT functions to
>> descrease SPL size. Here is result for ZynqMP virt configuration:
>> xilinx_zynqmp_virt: spl/u-boot-spl:all -82 spl/u-boot-spl:rodata -22 spl/u-boot-spl:text -60
>>
>> The patch causes change in run time fit image record.
>> Before:
>> fit-images {
>>         uboot {
>>                 os = "u-boot";
>>                 type = "firmware";
>>                 size = <0xfd520>;
>>                 entry-point = <0x8000000>;
>>                 load-addr = <0x8000000>;
>>         };
>> };
>>
>> After:
>> fit-images {
>>         uboot {
>>                 os = "u-boot";
>>                 type = "firmware";
>>                 size = <0xfd520>;
>>                 entry = <0x8000000>;
>>                 load = <0x8000000>;
>>         };
>> };
>>
>> Replacing calling fdt_getprop_u32() by fit_image_get_entry/load() also
>> enables support for reading entry/load properties recorded in 64bit format.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Isn't there a test that could be updated here?

Are we testing SPL flow?

> 
>>
>> Would be good to know history of fit-images and it's property names but
>> there shouldn't be a need to use non standard names where we have
>> FIT_*_PROP recorded as macros in include/image.h.
> 
> I agree.
> 
>> Concern regarding backward compatibility is definitely valid but not sure
>> how many systems can be affected by this change.
> 
> Me neither. Probably a good idea to fix it.

Fix means keep existing code with warning and add new one next to it.

M

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Michal Simek]

On 07. 09. 20 3:43, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
>> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
>> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
>> problem to record these addresses in 64bit format.
>> The patch adds support for systems which need to load images above 4GB.
> 
> But what about 32-bit systems who read this as a 32-bit number?
> Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT?

The code for reading doesn't really care if value is 32bit or 64bit.
The fit_image_get_entry() and fit_image_get_load() read number of cells
used and based on that read 32 or 64 bit values.

Thanks,
Michal

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Michal Simek]

+andestech.com

On 07. 09. 20 3:43, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
>> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
>> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
>> problem to record these addresses in 64bit format.
>> The patch adds support for systems which need to load images above 4GB.
> 
> But what about 32-bit systems who read this as a 32-bit number?
> Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT?
> 
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  common/fdt_support.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index b8a8768a2147..5ae75df3c658 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name,
>>         if (node < 0)
>>                 return node;
>>
>> -       /*
>> -        * We record these as 32bit entities, possibly truncating addresses.
>> -        * However, spl_fit.c is not 64bit safe either: i.e. we should not
>> -        * have an issue here.
>> -        */
>> -       fdt_setprop_u32(blob, node, "load", load_addr);
>> +       fdt_setprop_u64(blob, node, "load", load_addr);
>>         if (entry_point != -1)
>> -               fdt_setprop_u32(blob, node, "entry", entry_point);
>> +               fdt_setprop_u64(blob, node, "entry", entry_point);
>>         fdt_setprop_u32(blob, node, "size", size);
>>         if (type)
>>                 fdt_setprop_string(blob, node, "type", type);
>> --
>> 2.28.0
>>

riscv: Your code is also using entry-point/load-addr and should be also
fixed based on this series. Can you please take a look at this series?
I am interested how riscv users are keeping in sync SPL and full U-Boot.

Thanks,
Michal

[PATCH 0/2] Add support for loading images above 4GB

[Michal Simek]

Hi Simon,

On 07. 09. 20 3:43, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 3 Sep 2020 at 06:30, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Hi,
>>
>> On 03. 09. 20 13:16, Heinrich Schuchardt wrote:
>>> On 9/3/20 1:03 PM, Michal Simek wrote:
>>>> Hi,
>>>>
>>>> We have several use cases where customers want to partition memory by
>>>> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
>>>> others CPU in the systems (like R5) or for secure sw.
>>>> Currently there is limitation in SPL to record load/entry addresses in
>>>> 64bit format because they are recorded in 32bit only.
>>>> This series add support for it.
>>>> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
>>>> images generated by binman. Because u-boot is using
>>>> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
>>>> and there is no real need to build it to certain offset.
>>>>
>>>> Thanks,
>>>> Michal
>>>
>>> Hello Michal,
>>>
>>> does this series require changes to doc/uImage.FIT/source_file_format.txt?
>>
>> I am not changing fit format. I am just changing how SPL records
>> loadables in DT fit-images node. And also series is using FIT functions
>> for reading these properties (at least in this version).
> 
> Well I think there should be some mention of the 32/64 bit issue added
> to the FIT docs. How about adding an example with 64-bit addresses?

git grep "fit-images" doc/
and output is 0. It means we really don't have any documentation for
fit-images embed node.
It means we should create one.

/fit-images are related to loadables property that's why I think make
sense to document them together.

What about doc/uImage.FIT/howto.txt?

Thanks,
Michal

Do we have any doc which describe

[PATCH 0/2] Add support for loading images above 4GB

[Simon Glass]

Hi Michal,

On Mon, 7 Sep 2020 at 03:01, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi Simon,
>
> On 07. 09. 20 3:43, Simon Glass wrote:
> > Hi Michal,
> >
> > On Thu, 3 Sep 2020 at 06:30, Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> Hi,
> >>
> >> On 03. 09. 20 13:16, Heinrich Schuchardt wrote:
> >>> On 9/3/20 1:03 PM, Michal Simek wrote:
> >>>> Hi,
> >>>>
> >>>> We have several use cases where customers want to partition memory by
> >>>> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
> >>>> others CPU in the systems (like R5) or for secure sw.
> >>>> Currently there is limitation in SPL to record load/entry addresses in
> >>>> 64bit format because they are recorded in 32bit only.
> >>>> This series add support for it.
> >>>> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
> >>>> images generated by binman. Because u-boot is using
> >>>> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
> >>>> and there is no real need to build it to certain offset.
> >>>>
> >>>> Thanks,
> >>>> Michal
> >>>
> >>> Hello Michal,
> >>>
> >>> does this series require changes to doc/uImage.FIT/source_file_format.txt?
> >>
> >> I am not changing fit format. I am just changing how SPL records
> >> loadables in DT fit-images node. And also series is using FIT functions
> >> for reading these properties (at least in this version).
> >
> > Well I think there should be some mention of the 32/64 bit issue added
> > to the FIT docs. How about adding an example with 64-bit addresses?
>
> git grep "fit-images" doc/
> and output is 0. It means we really don't have any documentation for
> fit-images embed node.
> It means we should create one.
>
> /fit-images are related to loadables property that's why I think make
> sense to document them together.
>
> What about doc/uImage.FIT/howto.txt?

Sounds good.

Regards,
Simon

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Simon Glass]

Hi Michal,

On Mon, 7 Sep 2020 at 02:29, Michal Simek <michal.simek@xilinx.com> wrote:
>
>
>
> On 07. 09. 20 3:43, Simon Glass wrote:
> > Hi Michal,
> >
> > On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
> >> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
> >> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
> >> problem to record these addresses in 64bit format.
> >> The patch adds support for systems which need to load images above 4GB.
> >
> > But what about 32-bit systems who read this as a 32-bit number?
> > Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT?
>
> The code for reading doesn't really care if value is 32bit or 64bit.
> The fit_image_get_entry() and fit_image_get_load() read number of cells
> used and based on that read 32 or 64 bit values.

Sorry I wrote that before looking at the function. The functions
should have header-file comments that indicate what they do.

Regards,
Simon

[PATCH 1/2] spl: Use standard FIT entries

[Simon Glass]

Hi Michal,

On Mon, 7 Sep 2020 at 02:27, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi Simon,
>
> On 07. 09. 20 3:43, Simon Glass wrote:
> > Hi Michal,
> >
> > On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> SPL is creating fit-images DT node when loadables are recorded in selected
> >> configuration. Entries which are created are using entry-point and
> >> load-addr property names. But there shouldn't be a need to use non standard
> >> properties because entry/load are standard FIT properties. But using
> >> standard FIT properties enables option to use generic FIT functions to
> >> descrease SPL size. Here is result for ZynqMP virt configuration:
> >> xilinx_zynqmp_virt: spl/u-boot-spl:all -82 spl/u-boot-spl:rodata -22 spl/u-boot-spl:text -60
> >>
> >> The patch causes change in run time fit image record.
> >> Before:
> >> fit-images {
> >>         uboot {
> >>                 os = "u-boot";
> >>                 type = "firmware";
> >>                 size = <0xfd520>;
> >>                 entry-point = <0x8000000>;
> >>                 load-addr = <0x8000000>;
> >>         };
> >> };
> >>
> >> After:
> >> fit-images {
> >>         uboot {
> >>                 os = "u-boot";
> >>                 type = "firmware";
> >>                 size = <0xfd520>;
> >>                 entry = <0x8000000>;
> >>                 load = <0x8000000>;
> >>         };
> >> };
> >>
> >> Replacing calling fdt_getprop_u32() by fit_image_get_entry/load() also
> >> enables support for reading entry/load properties recorded in 64bit format.
> >>
> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> >> ---
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Isn't there a test that could be updated here?
>
> Are we testing SPL flow?
>
> >
> >>
> >> Would be good to know history of fit-images and it's property names but
> >> there shouldn't be a need to use non standard names where we have
> >> FIT_*_PROP recorded as macros in include/image.h.
> >
> > I agree.
> >
> >> Concern regarding backward compatibility is definitely valid but not sure
> >> how many systems can be affected by this change.
> >
> > Me neither. Probably a good idea to fix it.
>
> Fix means keep existing code with warning and add new one next to it.

No, I mean use your code as here and document with a test, etc. If it
breaks anything, it can be fixed.

Regards,
SImon

[PATCH 2/2] spl: fdt: Record load/entry fit-images entries in 64bit format

[Michal Simek]

On 07. 09. 20 15:57, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 7 Sep 2020 at 02:29, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>>
>>
>> On 07. 09. 20 3:43, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>
>>>> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which
>>>> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe.
>>>> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a
>>>> problem to record these addresses in 64bit format.
>>>> The patch adds support for systems which need to load images above 4GB.
>>>
>>> But what about 32-bit systems who read this as a 32-bit number?
>>> Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT?
>>
>> The code for reading doesn't really care if value is 32bit or 64bit.
>> The fit_image_get_entry() and fit_image_get_load() read number of cells
>> used and based on that read 32 or 64 bit values.
> 
> Sorry I wrote that before looking at the function. The functions
> should have header-file comments that indicate what they do.

kernel-doc format is in common/image-fit.c already.

M

[PATCH 0/2] Add support for loading images above 4GB

[Michal Simek]

Hi Simon,

On 07. 09. 20 15:57, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 7 Sep 2020 at 03:01, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Hi Simon,
>>
>> On 07. 09. 20 3:43, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Thu, 3 Sep 2020 at 06:30, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 03. 09. 20 13:16, Heinrich Schuchardt wrote:
>>>>> On 9/3/20 1:03 PM, Michal Simek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> We have several use cases where customers want to partition memory by
>>>>>> putting NS images above 4GB. On Xilinx arm 64bit SOC 0-2GB can be used for
>>>>>> others CPU in the systems (like R5) or for secure sw.
>>>>>> Currently there is limitation in SPL to record load/entry addresses in
>>>>>> 64bit format because they are recorded in 32bit only.
>>>>>> This series add support for it.
>>>>>> Patches have been tested on Xilinx ZynqMP zcu102 board in SD bootmode with
>>>>>> images generated by binman. Because u-boot is using
>>>>>> CONFIG_POSITION_INDEPENDENT it can be put to others 4k aligned addresses
>>>>>> and there is no real need to build it to certain offset.
>>>>>>
>>>>>> Thanks,
>>>>>> Michal
>>>>>
>>>>> Hello Michal,
>>>>>
>>>>> does this series require changes to doc/uImage.FIT/source_file_format.txt?
>>>>
>>>> I am not changing fit format. I am just changing how SPL records
>>>> loadables in DT fit-images node. And also series is using FIT functions
>>>> for reading these properties (at least in this version).
>>>
>>> Well I think there should be some mention of the 32/64 bit issue added
>>> to the FIT docs. How about adding an example with 64-bit addresses?
>>
>> git grep "fit-images" doc/
>> and output is 0. It means we really don't have any documentation for
>> fit-images embed node.
>> It means we should create one.
>>
>> /fit-images are related to loadables property that's why I think make
>> sense to document them together.
>>
>> What about doc/uImage.FIT/howto.txt?
> 
> Sounds good.

v2 sent.

Thanks,
Michal