Hi Jonas,
On Mon, 10 Feb 2025 at 14:52, Jonas Karlman <jo...@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2025-02-09 22:14, Simon Glass wrote:
> > Move the FIT description into a template so that it can (later) be used
> > in multiple places in the image.
> >
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> > arch/arm/dts/rockchip-u-boot.dtsi | 57 ++++++++++++++++++-------------
> > 1 file changed, 34 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
> > b/arch/arm/dts/rockchip-u-boot.dtsi
> > index 64a6b57d5fd..ca666ba0cd4 100644
> > --- a/arch/arm/dts/rockchip-u-boot.dtsi
> > +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> > @@ -19,6 +19,10 @@
> > #define COMP "none"
> > #endif
> >
> > +#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) ||
> > defined(CONFIG_SPL_OPTEE_IMAGE))
> > +#define HAS_FIT
> > +#endif
> > +
> > / {
> > binman: binman {
> > multiple-images;
> > @@ -27,28 +31,9 @@
> >
> > #ifdef CONFIG_SPL
> > &binman {
> > - simple-bin {
> > - filename = "u-boot-rockchip.bin";
> > - pad-byte = <0xff>;
> > -
> > - mkimage {
> > - filename = "idbloader.img";
> > - args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> > - multiple-data-files;
> > -
> > -#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
> > - rockchip-tpl {
> > - };
> > -#elif defined(CONFIG_TPL)
> > - u-boot-tpl {
> > - };
> > -#endif
> > - u-boot-spl {
> > - };
> > - };
> > -
> > -#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) ||
> > defined(CONFIG_SPL_OPTEE_IMAGE))
> > - fit: fit {
> > +#ifdef HAS_FIT
> > + common_part: template-1 {
> > + type = "fit";
> > #ifdef CONFIG_ARM64
> > description = "FIT image for U-Boot with bl31 (TF-A)";
> > #else
> > @@ -59,7 +44,6 @@
> > filename = "u-boot.itb";
>
> Please move the filename prop to the fit node in this patch.
>
> There are lots of guides and Linux image building scripts that depend on
> the u-boot.itb file, so we should continue to keep this file to be
> backward compatible for a few more releases.
>
> > fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> > fit,align = <512>;
> > - offset = <CONFIG_SPL_PAD_TO>;
> > images {
> > u-boot {
> > description = "U-Boot";
> > @@ -164,6 +148,33 @@
> > fit,loadables;
> > };
> > };
> > + };
> > +#endif /* HAS_FIT */
> > +
> > + simple-bin {
> > + filename = "u-boot-rockchip.bin";
> > + pad-byte = <0xff>;
> > +
> > + mkimage {
> > + filename = "idbloader.img";
> > + args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> > + multiple-data-files;
> > +
> > +#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
> > + rockchip-tpl {
> > + };
> > +#elif defined(CONFIG_TPL)
> > + u-boot-tpl {
> > + };
> > +#endif
> > + u-boot-spl {
> > + };
> > + };
> > +
> > +#ifdef HAS_FIT
> > + fit {
> > + offset = <CONFIG_SPL_PAD_TO>;
> > + insert-template = <&common_part>;
> > };
> > #else
> > u-boot-img {
>
> There is one more fit node in simple-bin-spi that should change to use
> #ifdef HAS_FIT. Also for consistency with fit/u-boot-img node in
> simple-bin-spi I recommend we move the #else/#endif to not include the
> offset and closing bracket.
>
> I suggest something like the following fixup to be included in this patch:
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
> b/arch/arm/dts/rockchip-u-boot.dtsi
> index c3013e75ed10..460bf15e0032 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -41,7 +41,6 @@
> #endif
> #address-cells = <1>;
> fit,fdt-list = "of-list";
> - filename = "u-boot.itb";
> fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> fit,align = <512>;
> images {
> @@ -173,14 +172,13 @@
>
> #ifdef HAS_FIT
> fit {
> - offset = <CONFIG_SPL_PAD_TO>;
> + filename = "u-boot.itb";
> insert-template = <&common_part>;
> - };
> #else
> u-boot-img {
> +#endif
> offset = <CONFIG_SPL_PAD_TO>;
> };
> -#endif
>
> fdtmap {
> };
> @@ -207,7 +205,7 @@
> };
> };
>
> -#if defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE)
> +#ifdef HAS_FIT
> fit {
> type = "blob";
> filename = "u-boot.itb";
>
>
> With something like that applied this is:
Thanks for the patch, that helped me figure out what you wanted with
the braces. I have tended to avoid #ifdef around open braces, but I
agree it seems easier this way in this case.
>
> Reviewed-by: Jonas Karlman <jo...@kwiboo.se>
Regards,
Simon
