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.sch...@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+ub...@0leil.net>
Signed-off-by: Quentin Schulz <quentin.sch...@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
