| Message ID | 9f4859ccaf3f75d68325fd8918dea0c1149e6dbc.1599130989.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for loading images above 4GB | expand |
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
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
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
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;
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(-)