[PATCH v2 2/2] riscv: dts: starfive: generate u-boot-spl.bin.normal.out

[Massimo Pegorer]

Il giorno dom 17 set 2023 alle ore 09:14 Massimo Pegorer
<massimo.pegorer+oss at gmail.com> ha scritto:
>
> Il giorno dom 17 set 2023 alle ore 02:01 Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> ha scritto:
> >
> >
> >
> > On 9/17/23 00:19, Jonas Karlman wrote:
> > > On 2023-09-06 14:00, Heinrich Schuchardt wrote:
> > >> The StarFive VisionFive 2 board cannot load spl/u-boot-spl.bin but needs a
> > >> prefixed header. We have referring to a vendor tool (spl_tool) for this
> > >> task. 'mkimage -T sfspl' can generate the prefixed file.
> > >>
> > >> Use binman to invoke mkimage for the generation of file
> > >> spl/u-boot-spl.bin.normal.out.
> > >>
> > >> Update the documentation.
> > >>
> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > >> Tested-by: Chanho Park <chanho61.park at samsung.com>
> > >> ---
> > >> v2:
> > >>      Fix a typo in a comment in tools/sfspl.c
> > >>      Add Tested-by credit
> > >> ---
> > >>   .../dts/jh7110-starfive-visionfive-2-u-boot.dtsi   | 10 ++++++++++
> > >>   doc/board/starfive/visionfive2.rst                 | 14 ++------------
> > >>   2 files changed, 12 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/arch/riscv/dts/jh7110-starfive-visionfive-2-u-boot.dtsi b/arch/riscv/dts/jh7110-starfive-visionfive-2-u-boot.dtsi
> > >> index 13f69da31e..defe2b605f 100644
> > >> --- a/arch/riscv/dts/jh7110-starfive-visionfive-2-u-boot.dtsi
> > >> +++ b/arch/riscv/dts/jh7110-starfive-visionfive-2-u-boot.dtsi
> > >> @@ -103,4 +103,14 @@
> > >>                      };
> > >>              };
> > >>      };
> > >> +    u-boot-spl {
> > >> +            filename = "spl/u-boot-spl.bin.normal.out";
> > >> +
> > >> +            mkimage {
> > >> +                    args = "-T sfspl";
> > >> +                    blob {
> > >> +                            filename = "spl/u-boot-spl.bin";
> > >> +                    };
> > >> +            };
> > >> +    };
> > >
> > > This should probably be:
> > >
> > > mkimage {
> > >       filename = "spl/u-boot-spl.bin.normal.out";
> > >       args = "-T sfspl";
> > >
> > >       u-boot-spl {
> > >       };
> > > };
> >
> > @Jonas
> > If I replace the node u-boot-spl by the suggested mkimage node, I get a
> > file spl/u-boot-spl.bin.normal.out which is identical to
> > spl/u-boot-spl.bin. It lacks the header that mkimage should create.

Images and entries are different things for binman. It expects an
image at first level, while mkimage is an entry. If you wrap Jonas'
suggestion in an image declaration, you will get what you need.
Something like:

anyname {
    mkimage {
        filename = "spl/u-boot-spl.bin.normal.out";
        args = "-T sfspl";

        u-boot-spl {
        };
    };
};

My personal and questionable suggestions:
 - Do not make binman/mkimage working in spl subfolder.
 - Do not use a generic term like "normal" in filename.
 - Do not use .out filename extension instead of .bin

Thus I would suggest something like:

u-boot-spl.starfive {
    mkimage {
        filename = "u-boot-spl.starfive.bin";
        args = "-T sfspl";

        u-boot-spl {
        };
    };
};

Regards,
Massimo


> >
> > Replacing the blob node by a u-boot-spl node is possible. Whether it is
> > better readable is a matter of taste.
>
> It is also a matter of default/standard assumptions and conventions,
> and thus opportunities. If SPL binary filename will change in the
> future for any reason, a single fix to binman would be enough for
> everybody using u-boot-spl node, while blob node users should fix each
> one of the -u-boot.dtsi files.
>
> >
> > @Simon:
> > Could you, please, have a look at doc/develop/package/entries.rst. It
> > seems not to fully describe how binman is controlled via the device-tree.
> >
> > * A blob sub-node for the mkimage node is not described.
> > * A mkimage node which is not a direct subnode of binman is not mentioned.
> > * Why the target filename must be outside of the mkimage node in my case
> > is not evident.
> >
> > Maybe a more formal description of the schema in a yaml file would help.
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > Regards,
> > > Jonas
> > >
> > >>   };
> > >> diff --git a/doc/board/starfive/visionfive2.rst b/doc/board/starfive/visionfive2.rst
> > >> index 941899a0a4..f5575ab68b 100644
> > >> --- a/doc/board/starfive/visionfive2.rst
> > >> +++ b/doc/board/starfive/visionfive2.rst
> > >> @@ -65,18 +65,8 @@ Now build the U-Boot SPL and U-Boot proper
> > >>      make starfive_visionfive2_defconfig
> > >>      make OPENSBI=$(opensbi_dir)/opensbi/build/platform/generic/firmware/fw_dynamic.bin
> > >>
> > >> -This will generate spl/u-boot-spl.bin and FIT image (u-boot.itb)
> > >> -
> > >> -u-boot-spl.bin cannot be used directly on StarFive VisionFive2,we need
> > >> -to convert the u-boot-spl.bin to u-boot-spl.bin.normal.out with
> > >> -the below command:
> > >> -
> > >> -    ./spl_tool -c -f $(Uboot_PATH)/spl/u-boot-spl.bin
> > >> -
> > >> -More detailed description of spl_tool,please refer spl_tool documenation.
> > >> -(Note: spl_tool git repo is at https://github.com/starfive-tech/Tools/tree/master/spl_tool)
> > >> -
> > >> -This will generate u-boot-spl.bin.normal.out file.
> > >> +This will generate the U-Boot SPL image (spl/u-boot-spl.bin.normal.out) as well
> > >> +as the FIT image (u-boot.itb) with OpenSBI and U-Boot.
> > >>
> > >>   Flashing
> > >>   ~~~~~~~~
> > >