Re: [PATCH v2 1/7] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM

["Matwey V. Kornilov" <matwey.kornilov@gmail.com>]

сб, 23 июл. 2022 г. в 15:07, Johan Jonker <jbx6244@gmail.com>:
>
> Hi Quentin and others,
>
> Some comments. Have a look if it's useful.
> It works, but is in need for some improvement...
>
> Johan
>
> On 7/22/22 13:34, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >
> > idbloader.img content - currently created by way of Makefile - can be
> > created by binman directly.
> >
> > So let's do that for Rockchip ARM platforms.
> >
> > Cc: Quentin Schulz <foss+uboot@0leil.net>
> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > ---
> >  Makefile                          |  2 +-
> >  arch/arm/dts/rockchip-u-boot.dtsi | 15 +++++++++++++--
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index d76ec69b52..f780bfe211 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1005,7 +1005,7 @@ endif
> >  else
> >  ifeq ($(CONFIG_SPL),y)
> >  # Generate these inputs for binman which will create the output files
> > -INPUTS-y += idbloader.img u-boot.img
> > +INPUTS-y += u-boot.img
> >  endif
> >  endif
> >  endif
> > diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> > index eae3ee715d..0362c97e0b 100644
> > --- a/arch/arm/dts/rockchip-u-boot.dtsi
> > +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> > @@ -17,9 +17,20 @@
> >               filename = "u-boot-rockchip.bin";
> >               pad-byte = <0xff>;
> >
>
> > -             blob {
> > -                     filename = "idbloader.img";
>
> "u-boot-rockchip.bin" is a combination image of mkimage(TPL/SPL) + "u-boot.img". Not everyone suites this fixed GPT format.
> People may still want to write them separate while testing or whatever, so "idbloader.img" and "u-boot.img" must be kept available after this change!

I agree. For instance, RockChip proprietary idbloader.img is known to
work with u-boot u-boot.img as a payload. It may be useful for testing
purposes then.

>
> ===
>
> rockchip.rst and README.rockchip and elsewhere on the internet still refer to "idbloader.img" so it should come back, but then made by binman.
>
>
>         idbloader {
>                 filename = "idbloader.img";
>
>                 mkimage {
>                         args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> #ifdef CONFIG_TPL
>                         multiple-data-files;
>
>                         u-boot-tpl {
>                         };
> #endif
>                         u-boot-spl {
>                         };
>                 };
>         };
>
> ===
>
> After this patch serie "idbloader.img" is not removed.
>
> make clean
>
> After:
>   CLEAN   include/generated/env.in u-boot-nodtb.bin u-boot.lds u-boot.cfg.configs u-boot.bin u-boot-dtb.bin u-boot.img u-boot-dtb.img u-boot.sym u-boot.map u-boot.srec u-boot.cfg u-boot.dtb.out u-boot-tpl.dtb.out u-boot u-boot-spl.dtb.out u-boot.dtb u-boot-rockchip.bin System.map
>
> Before:
>   CLEAN   include/generated/env.in u-boot-nodtb.bin u-boot.lds u-boot.cfg.configs u-boot.bin u-boot-dtb.bin u-boot.img u-boot-dtb.img u-boot.sym u-boot.map u-boot.srec u-boot.cfg u-boot.dtb.out u-boot-tpl.dtb.out u-boot u-boot-spl.dtb.out u-boot.dtb u-boot-rockchip.bin System.map idbloader.img
>
> ===
>
> Your serie generates zombie files. PLease remove after use.
> Same for others like:
>
> simple-bin.map
> mkimage-out.rom.mkimage
> mkimage.rom.mkimage
> rom.map
> tools/boot/bootm.c
> tools/boot/fdt_region.c
> tools/boot/image-cipher.c
> tools/boot/image-fit-sig.c
> tools/boot/image-fit.c
> tools/boot/image-host.c
> tools/boot/image.c
> u_boot_logo.S
>
>
> Untracked files:
>   (use "git add <file>..." to include in what will be committed)
>
>         mkimage-out.simple-bin.mkimage
>         mkimage.simple-bin.mkimage
>
> ===
>
>
>
> ===
>
> > +             mkimage {
> > +                     args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
>
> > +#ifndef CONFIG_TPL
> > +                     u-boot-spl {
> > +                     };
> >               };
> > +#else
> > +                     u-boot-tpl {
> > +                     };
> > +             };
> > +
>
> u-boot-spl is the input for mkimage and should be a subnode.
>
> > +             u-boot-spl {
> > +             };
> > +#endif
>
> Fix your dts format:
>
>         simple-bin {
>                 filename = "u-boot-rockchip.bin";
>                 pad-byte = <0xff>;
>
>                 mkimage {
>                         args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> #ifdef CONFIG_TPL
>                         multiple-data-files;
>
>                         u-boot-tpl {
>                         };
> #endif
>                         u-boot-spl {
>                         };
>                 };
>
> #ifdef CONFIG_ARM64
>                 blob {
>                         filename = "u-boot.itb";
> #else
>                 u-boot-img {
> #endif
>                         offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512)>;
>                 };
>         };
> ===
>
> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR is only related to MMC!
>
> There are other boot mediums like NAND or USB, so don't assume that CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR is defined.
>
> See my patch serie that still is in need for review:
>
> https://lore.kernel.org/u-boot/20220508150825.21711-7-jbx6244@gmail.com/
>
> Without it generates a warning:
>
> Error: arch/arm/dts/rockchip-u-boot.dtsi:54.16-17 syntax error
> FATAL ERROR: Unable to parse input tree
>
> Add more compile conditions!
>
> ===
>
> RK3066:
> For NAND the "idbloader.img" might be useful for my serie (in need for review) when it gets TPL/SPL and rc4 right:
>
> [PATCH v2 00/11] Add Rockchip IDB device
> https://lore.kernel.org/u-boot/a1458a7b-2043-6397-3107-2d1fdf08c8e1@gmail.com/
>
> In mk808_defconfig change:
>
> CONFIG_TPL_TEXT_BASE=0x10080C04
>
> to:
>
> CONFIG_TPL_TEXT_BASE=0x10080C00
>
> In rockchip.rst change:
>
>         printf "RK30" > tplspl.bin
>         dd if=u-boot-tpl.bin >> tplspl.bin
>
> to:
>         printf "RK30" > tplspl.bin
>         dd if=u-boot-tpl.bin ibs=1 skip=4 >> tplspl.bin
>
> The NAND can be programmed simular to MMC with:
>
> rkdeveloptool wlx loader1 idbloader.img
>
> TODO:
>
> rk30 usbplug (open source)
> ===
> >
> >               u-boot-img {
> >                       offset = <CONFIG_SPL_PAD_TO>;



-- 
With best regards,
Matwey V. Kornilov