| Message ID | 20220722113505.3875669-2-foss+uboot@0leil.net |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | migrate u-boot-rockchip.bin to binman and generate an image for SPI | expand |
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! === 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>;
сб, 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>;
El Sat, Jul 23, 2022 at 02:07:30PM +0200, Johan Jonker deia: > > > + 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 > The input for mkimage can be tpl alone. I think this case is for rksd. For rksd one can simply concatenate the output of mkimage -n rk... -T rksd tpl with the spl binary. That's why u-boot-spl is out of mkimage when there's u-boot-tpl. for rksd I think mkimage just adds a header in front of the binary, so you can put the next binary right behind. You don't have to give spl to mkimage if you give it tpl.
Hi Johan, On 7/23/22 14:07, Johan Jonker wrote: > 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! > Fair enough. > === > > 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. > I personally care very little what kind of information one finds outside of U-Boot upstream documentation. Considering that U-Boot has on-going deep changes, specifically related to DM and conversion from C headers to Kconfig options, old tutorial or answers on StackOverflow will anyway become outdated. This to me is not a valid argument (though updating the docs should have been done). > > 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 { > }; > }; > }; > > === > I'll add "filename" DT property support for binman mkimage entry instead so that it creates idbloader.img binary again. The issue is that binman creates images in parallel, so you cannot rely on the idbloader.img binary being creates by binman before another section uses it. I also don't want to duplicate the mkimage node outside of u-boot-rockchip.bin. It'd be nice to have support for phandles in order to explicit dependencies between images, e.g. something like: ``` &binman { idbloader_img: idbloader-img { filename = "idbloader.img"; mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; #ifdef CONFIG_TPL multiple-data-files; u-boot-tpl { }; #endif u-boot-spl { }; }; }; simple-bin { filename "u-boot-rockchip.bin"; blob { content = <&idbloader_img>; }; u-boot-img { }; }; }; ``` But I couldn't find something allowing this? The collection entry seems to be close to what I'd like but: ``` &binman { collection { filename = "idbloader.img"; content = <&idbloader_img>; }; simple-bin { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; idbloader_img: mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; #ifdef CONFIG_TPL multiple-data-files; u-boot-tpl { }; #endif u-boot-spl { }; }; u-boot-img {}; }; ``` creates an empty idbloader.img. I'm sure I'm just missing out on something obvious but cannot find it right now. > After this patch serie "idbloader.img" is not removed. > Yes, there's no need to remove files that aren't generated anymore, since they won't exist. This is up to the user to do some clean-up before building a newer version of U-Boot. > 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 Yup, will do. > 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 > Those predates the patch series (I only have the last one though). > > 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. > Technically not needed, can be appended right after the TPL. But since you want the idbloader.img binary back, I'll do what you suggest otherwise it wouldn't contain the same content as before this patch series. >> + 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. > I have an issue with the assumption of what u-boot-rockchip.bin is supposed to be. What does it actually represent? I have three possible boot media on my board: SPI-NOR, eMMC and SD Card. Both MMC devices can use the same offsets and images, so that's fine for me to have something like u-boot-rockchip-mmc.bin. However, SPI-NOR has a different offset for U-Boot proper than MMC devices. It would be ridiculous to have two defconfigs with the only difference being the value of SPL_PAD_TO option. Hence why there's a u-boot-rockchip-spi.bin image now and also why I argue in this patch series that using SPL_PAD_TO is incorrect. I replaced SPL_PAD_TO with the formula that was originally used to define the padding, see https://source.denx.de/u-boot/u-boot/-/commit/79030a486128bdb6888059113711a6ae66ff89c5. I understand this change is not ok, but I cannot use SPL_PAD_TO either. I would like to have some opinion/answer on what I asked in the paragraph above. > See my patch serie that still is in need for review: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220508150825.21711-2D7-2Djbx6244-40gmail.com_&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=G-BQeahyEOXOd4k4qm8FOKAAi0q_GYVKUH7r8DETdouey2AzEq1fMNzHfSSoLb5P&s=rDJyHW_vIS5_pzBJbCANS9y47hKX5KlTmVBqlI2rb_k&e= > It's been almost three months and I don't see reviews on it, you should send a mail asking people to review it (you should probably rebase and resend though as I assume some changes were made to U-Boot that either creates conflict when applying or some header variable made it to Kconfig option now). > 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! > Fair enough. I would rename u-boot-rockchip.bin into u-boot-rockchip-mmc.bin and add an #ifdef on CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR. In your series, you'll be able to add a u-boot-rockchip-nand.bin I guess? > === > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_a1458a7b-2D2043-2D6397-2D3107-2D2d1fdf08c8e1-40gmail.com_&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=G-BQeahyEOXOd4k4qm8FOKAAi0q_GYVKUH7r8DETdouey2AzEq1fMNzHfSSoLb5P&s=wXUk74zHNoAwgPlPvlDZbNTBdqpDy2UTuSt9MDyuKiw&e= > > 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 > Where is loader1 partition located in your NAND device? We do not use a partition table with our board (and we do not use Rockchip default offsets either) and I think U-Boot shouldn't assume users will flash into a named partition, because there's no guarantee the location of the partition will be the same for all devices and users. Thanks for the review, Cheers, Quentin
Note: I removed a few recipients from Cc: with a quite random criteria, just to avoid error messages from the list software that there were too many addresses in Cc:. I hope those will get it from the list anyway and sorry if this is a problem. > I'm sure I'm just missing out on something obvious but cannot find it right > now. > I have an issue with the assumption of what u-boot-rockchip.bin is supposed > to be. What does it actually represent? > > I have three possible boot media on my board: SPI-NOR, eMMC and SD Card. > Both MMC devices can use the same offsets and images, so that's fine for me > to have something like u-boot-rockchip-mmc.bin. > However, SPI-NOR has a different offset for U-Boot proper than MMC devices. > It would be ridiculous to have two defconfigs with the only difference being > the value of SPL_PAD_TO option. Hence why there's a u-boot-rockchip-spi.bin > image now and also why I argue in this patch series that using SPL_PAD_TO is > incorrect. I replaced SPL_PAD_TO with the formula that was originally used > to define the padding, see https://source.denx.de/u-boot/u-boot/-/commit/79030a486128bdb6888059113711a6ae66ff89c5. > I understand this change is not ok, but I cannot use SPL_PAD_TO either. I > would like to have some opinion/answer on what I asked in the paragraph > above. The difference between u-boot-rockchip-mmc.bin and u-boot-rockchip-spi.bin is not only the offset. The image for SPI has the parts loaded by bootrom (tpl and spl) written in 8Kb chunks separated by 8Kb empty space (or something like this, I don't remember). That's why there are different -T rksd and -T rkspi options to mkimage. This is so at least for RK3399. I have no idea whether nand images need this special format or not, or whether it depends on the SoC model or what. If it's only the offset, then maybe we can avoid a 3rd image and reuse the MMC one. I don't know.
On 7/25/22 13:05, Xavier Drudis Ferran wrote: > Note: I removed a few recipients from Cc: with a quite random criteria, > just to avoid error messages from the list software that there were > too many addresses in Cc:. I hope those will get it from the list anyway > and sorry if this is a problem. > > >> I'm sure I'm just missing out on something obvious but cannot find it right >> now. >> I have an issue with the assumption of what u-boot-rockchip.bin is supposed >> to be. What does it actually represent? >> >> I have three possible boot media on my board: SPI-NOR, eMMC and SD Card. >> Both MMC devices can use the same offsets and images, so that's fine for me >> to have something like u-boot-rockchip-mmc.bin. >> However, SPI-NOR has a different offset for U-Boot proper than MMC devices. >> It would be ridiculous to have two defconfigs with the only difference being >> the value of SPL_PAD_TO option. Hence why there's a u-boot-rockchip-spi.bin >> image now and also why I argue in this patch series that using SPL_PAD_TO is >> incorrect. I replaced SPL_PAD_TO with the formula that was originally used >> to define the padding, see https://source.denx.de/u-boot/u-boot/-/commit/79030a486128bdb6888059113711a6ae66ff89c5. >> I understand this change is not ok, but I cannot use SPL_PAD_TO either. I >> would like to have some opinion/answer on what I asked in the paragraph >> above. > > The difference between u-boot-rockchip-mmc.bin and > u-boot-rockchip-spi.bin is not only the offset. The image for SPI has > the parts loaded by bootrom (tpl and spl) written in 8Kb chunks > separated by 8Kb empty space (or something like this, I don't > remember). That's why there are different -T rksd and -T rkspi > options to mkimage. This is so at least for RK3399. > > I have no idea whether nand images need this special format or not, > or whether it depends on the SoC model or what. If it's only the > offset, then maybe we can avoid a 3rd image and reuse the MMC one. > I don't know. > The pattern for NAND depends on the chip. Have NOT found a good logic for lsb modes other then a lookup list. No need for a 3rd image as idbloader.img contains everything (header/TPL/SPL/RC4 support) for further processing elsewhere. Johan
On Fri, Jul 22, 2022 at 5:05 PM Quentin Schulz <foss+uboot@0leil.net> 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"; Binman might be first priority boot image to be used but not mandatory for all cases. Some boards use SPL or TPL+SPL and U-Boot proper separately in order to change the SPL or TPL+SPL for supporting Multi DTB on a variety of boards without updating U-Boot proper from the factory. idbloader.img can be a legacy image name but it is derived from rockchip and many boards are already used it and it is simply recognized by rockchip board vendors. Thanks, Jagan.
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"; + mkimage { + args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; +#ifndef CONFIG_TPL + u-boot-spl { + }; }; +#else + u-boot-tpl { + }; + }; + + u-boot-spl { + }; +#endif u-boot-img { offset = <CONFIG_SPL_PAD_TO>;