| Message ID | 20210213155901.10247-1-royger@FreeBSD.org |
|---|---|
| State | Accepted |
| Commit | 19c6b8cf8bea6d3634594bcb6a26768ecf4e9b96 |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | odroid-go2: remove setting SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR | expand |
Hi Roger, Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > From: Roger Pau Monné <royger@FreeBSD.org> > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > Removing the setting from the defconfig allows U-Boot to load > successfully. Hmm, I'd disagree slightly. In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ #define CONFIG_SPL_PAD_TO 8355840 so it's a static value but based on the MMCSD_RAW_MODE... config option. So instead of mandating one specific MMCSD_RAW_MODE... value that CONFIG_SPL_PAD_TO should be defined based on the the actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR and not some static number that then gets enforced for all boards. Heiko > > Signed-off-by: Roger Pau Monné <royger@FreeBSD.org> > --- > Cc: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > --- > configs/odroid-go2_defconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/configs/odroid-go2_defconfig b/configs/odroid-go2_defconfig > index 853890901a..5632b15611 100644 > --- a/configs/odroid-go2_defconfig > +++ b/configs/odroid-go2_defconfig > @@ -33,7 +33,6 @@ CONFIG_SPL_BOOTROM_SUPPORT=y > # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set > CONFIG_SPL_STACK_R=y > # CONFIG_TPL_BANNER_PRINT is not set > -CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x200 > CONFIG_SPL_CRC32_SUPPORT=y > CONFIG_SPL_I2C_SUPPORT=y > CONFIG_SPL_POWER_SUPPORT=y >
On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > Hi Roger, > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > Removing the setting from the defconfig allows U-Boot to load > > successfully. > > Hmm, I'd disagree slightly. > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > #define CONFIG_SPL_PAD_TO 8355840 > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > So instead of mandating one specific MMCSD_RAW_MODE... value > that CONFIG_SPL_PAD_TO should be defined based on the the > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > and not some static number that then gets enforced for all boards. Sure, I can fix CONFIG_SPL_PAD_TO to be calculated based on MMCSD_RAW_MODE..., but why does this board use a different sector? Is it just to use less space? I guess it's not possible to add a comment to odroid-go2_defconfig to clarify this, as it would get dropped by updates to the file? Thanks, Roger.
On Sun, Feb 14, 2021 at 08:25:27AM +0100, Roger Pau Monné wrote: > On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > > Hi Roger, > > > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > > Removing the setting from the defconfig allows U-Boot to load > > > successfully. > > > > Hmm, I'd disagree slightly. > > > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > > #define CONFIG_SPL_PAD_TO 8355840 > > > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > > > So instead of mandating one specific MMCSD_RAW_MODE... value > > that CONFIG_SPL_PAD_TO should be defined based on the the > > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > and not some static number that then gets enforced for all boards. > > Sure, I can fix CONFIG_SPL_PAD_TO to be calculated based on > MMCSD_RAW_MODE... So that's easier said that done. CONFIG_SPL_PAD_TO ends up being used by the call to objcopy, so using an expression instead of an absolute number leads to: aarch64-none-elf-objcopy --gap-fill=0xff -j .text -j .secure_text -j .secure_data -j .rodata -j .data -j .u_boot_list -j .rela.dyn -j .got -j .got.plt -j .binman_sym_table -j .text_rest -j .dtb.i nit.rodata -I binary -O binary --pad-to="((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512)" --gap-fill=0xff idbloader.img u-boot-rockchip.bin && cat u-boot.itb >> u-boot-rockchip.bin || { rm - f u-boot-rockchip.bin; false; } aarch64-none-elf-objcopy: --pad-to: bad number: ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) And fixing this seems like a much bigger issue to tackle than the original patch. The current way to propagate CONFIG_ values to Makefiles is by the output of `cc -E -dM`, which won't perform the arithmetic. We could create a small tool using HOSTCC to print the values of the macros, but as said above that's not trivial IMO. Thanks, Roger.
On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > Hi Roger, > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > Removing the setting from the defconfig allows U-Boot to load > > successfully. > > Hmm, I'd disagree slightly. > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > #define CONFIG_SPL_PAD_TO 8355840 > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > So instead of mandating one specific MMCSD_RAW_MODE... value > that CONFIG_SPL_PAD_TO should be defined based on the the > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > and not some static number that then gets enforced for all boards. So, what does CONFIG_SPL_PAD_TO actually mean, in this case? And SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR ? What I'm getting at is that we generally have some required to be fixed (by the SoC/ROM/etc) locations some parts of our SPL/TPL/U-Boot need to be at and then the rest of the values are (supposed to be) well and carefully chosen offsets and not changed around. So with the above comment in the code to explain where 8355840 came from, it also shouldn't and nor should CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR be changed without a compelling reason.
Hi Tom, Am Dienstag, 16. Februar 2021, 15:26:52 CET schrieb Tom Rini: > On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > > Hi Roger, > > > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > > Removing the setting from the defconfig allows U-Boot to load > > > successfully. > > > > Hmm, I'd disagree slightly. > > > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > > #define CONFIG_SPL_PAD_TO 8355840 > > > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > > > So instead of mandating one specific MMCSD_RAW_MODE... value > > that CONFIG_SPL_PAD_TO should be defined based on the the > > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > and not some static number that then gets enforced for all boards. > > So, what does CONFIG_SPL_PAD_TO actually mean, in this case? And > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR ? What I'm getting at is that we > generally have some required to be fixed (by the SoC/ROM/etc) locations > some parts of our SPL/TPL/U-Boot need to be at and then the rest of the > values are (supposed to be) well and carefully chosen offsets and not > changed around. So with the above comment in the code to explain where > 8355840 came from, it also shouldn't and nor should > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR be changed without a compelling > reason. Normally Rockchip platforms have two loader binaries: - idbLoader.img (tpl + spl, or only spl), loving at offset 64 of a sd-card This is mandated by the bootrom - u-boot.itb (u-boot, atf, etc) living at SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR As SPL will load from the location specified in the config, this location can be set depending on emmc/sd-card/whatever needs. It looks like recently a new binary creating method was added that creates a u-boot-rockchip.bin combining these somewhat automatically: idbLoader.img + SPL_PAD_TO + u-boot.itb So that only that binary needs to be flashed to the boot medium instead of two. So the SPL_PAD_TO essentially would mandate one specific SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR for every board. For the odroid-go2 itself it doesn't really matter I guess, but there are other boards with different requirements, so mandating one specific place for the main uboot for all boards that will ever exist seems a bit counter- intuitive to me. Heiko
On Wed, Feb 17, 2021 at 02:42:21AM +0100, Heiko Stuebner wrote: > Hi Tom, > > Am Dienstag, 16. Februar 2021, 15:26:52 CET schrieb Tom Rini: > > On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > > > Hi Roger, > > > > > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > > > Removing the setting from the defconfig allows U-Boot to load > > > > successfully. > > > > > > Hmm, I'd disagree slightly. > > > > > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > > > > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > > > #define CONFIG_SPL_PAD_TO 8355840 > > > > > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > > > > > So instead of mandating one specific MMCSD_RAW_MODE... value > > > that CONFIG_SPL_PAD_TO should be defined based on the the > > > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > > and not some static number that then gets enforced for all boards. > > > > So, what does CONFIG_SPL_PAD_TO actually mean, in this case? And > > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR ? What I'm getting at is that we > > generally have some required to be fixed (by the SoC/ROM/etc) locations > > some parts of our SPL/TPL/U-Boot need to be at and then the rest of the > > values are (supposed to be) well and carefully chosen offsets and not > > changed around. So with the above comment in the code to explain where > > 8355840 came from, it also shouldn't and nor should > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR be changed without a compelling > > reason. > > Normally Rockchip platforms have two loader binaries: > > - idbLoader.img (tpl + spl, or only spl), loving at offset 64 of a sd-card > This is mandated by the bootrom > - u-boot.itb (u-boot, atf, etc) living at SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > As SPL will load from the location specified in the config, this location > can be set depending on emmc/sd-card/whatever needs. > > It looks like recently a new binary creating method was added that creates > a u-boot-rockchip.bin combining these somewhat automatically: > > idbLoader.img > + SPL_PAD_TO > + u-boot.itb > > So that only that binary needs to be flashed to the boot medium > instead of two. > > So the SPL_PAD_TO essentially would mandate one specific > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR for every board. > > > For the odroid-go2 itself it doesn't really matter I guess, but there are > other boards with different requirements, so mandating one specific place > for the main uboot for all boards that will ever exist seems a bit counter- > intuitive to me. I would say that yes, it's quite intentional that all boards for a given SoC (or SoC family) would use the same value for SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR and NOT leave it up to be per-board. It should be a matter of kilobytes being potentially wasted which is (often or most likely) worth sacrificing in the name of consistency and ease of future use / development. In other cases this ends up being something around "ROM will only load something of $X size, round that up a little bit, place U-Boot there, as it's the next thing to load".
Hi Tom, Am Mittwoch, 17. Februar 2021, 03:21:25 CET schrieb Tom Rini: > On Wed, Feb 17, 2021 at 02:42:21AM +0100, Heiko Stuebner wrote: > > Hi Tom, > > > > Am Dienstag, 16. Februar 2021, 15:26:52 CET schrieb Tom Rini: > > > On Sat, Feb 13, 2021 at 11:45:50PM +0100, Heiko Stuebner wrote: > > > > Hi Roger, > > > > > > > > Am Samstag, 13. Februar 2021, 16:59:01 CET schrieb Roger Pau Monne: > > > > > From: Roger Pau Monné <royger@FreeBSD.org> > > > > > > > > > > Using a non-default SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR setting makes the > > > > > resulting u-boot-rockchip.bin unbootable, as it gets stuck after SPL. > > > > > Removing the setting from the defconfig allows U-Boot to load > > > > > successfully. > > > > > > > > Hmm, I'd disagree slightly. > > > > > > > > In the rockchip-common.h the CONFIG_SPL_PAD_TO is defined as > > > > > > > > /* ((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512) */ > > > > #define CONFIG_SPL_PAD_TO 8355840 > > > > > > > > so it's a static value but based on the MMCSD_RAW_MODE... config option. > > > > > > > > So instead of mandating one specific MMCSD_RAW_MODE... value > > > > that CONFIG_SPL_PAD_TO should be defined based on the the > > > > actual config value of CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > > > and not some static number that then gets enforced for all boards. > > > > > > So, what does CONFIG_SPL_PAD_TO actually mean, in this case? And > > > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR ? What I'm getting at is that we > > > generally have some required to be fixed (by the SoC/ROM/etc) locations > > > some parts of our SPL/TPL/U-Boot need to be at and then the rest of the > > > values are (supposed to be) well and carefully chosen offsets and not > > > changed around. So with the above comment in the code to explain where > > > 8355840 came from, it also shouldn't and nor should > > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR be changed without a compelling > > > reason. > > > > Normally Rockchip platforms have two loader binaries: > > > > - idbLoader.img (tpl + spl, or only spl), loving at offset 64 of a sd-card > > This is mandated by the bootrom > > - u-boot.itb (u-boot, atf, etc) living at SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > As SPL will load from the location specified in the config, this location > > can be set depending on emmc/sd-card/whatever needs. > > > > It looks like recently a new binary creating method was added that creates > > a u-boot-rockchip.bin combining these somewhat automatically: > > > > idbLoader.img > > + SPL_PAD_TO > > + u-boot.itb > > > > So that only that binary needs to be flashed to the boot medium > > instead of two. > > > > So the SPL_PAD_TO essentially would mandate one specific > > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR for every board. > > > > > > For the odroid-go2 itself it doesn't really matter I guess, but there are > > other boards with different requirements, so mandating one specific place > > for the main uboot for all boards that will ever exist seems a bit counter- > > intuitive to me. > > I would say that yes, it's quite intentional that all boards for a given > SoC (or SoC family) would use the same value for > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR and NOT leave it up to be per-board. > It should be a matter of kilobytes being potentially wasted which is > (often or most likely) worth sacrificing in the name of consistency and > ease of future use / development. In other cases this ends up being > something around "ROM will only load something of $X size, round that up > a little bit, place U-Boot there, as it's the next thing to load". Ok ... then I'guess I'll not stand in the way ;-) . Though we're in the megabyte range with CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR being 0x4000 * 512 . But I guess with current emmc sizes that might not matter too much. But should there be some sort of warning when the CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR doesn't match the expected value? Because for example rk3399-puma and rk3368-lion historical use that 0x200 instead of 0x4000 block offset and I think at least their default firmware also expects u-boot to not reach that far into the emmc. Heiko
On Wed, Feb 17, 2021 at 02:08:55PM +0100, Heiko Stuebner wrote: > Hi Tom, > > Am Mittwoch, 17. Februar 2021, 03:21:25 CET schrieb Tom Rini: > > I would say that yes, it's quite intentional that all boards for a given > > SoC (or SoC family) would use the same value for > > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR and NOT leave it up to be per-board. > > It should be a matter of kilobytes being potentially wasted which is > > (often or most likely) worth sacrificing in the name of consistency and > > ease of future use / development. In other cases this ends up being > > something around "ROM will only load something of $X size, round that up > > a little bit, place U-Boot there, as it's the next thing to load". > > Ok ... then I'guess I'll not stand in the way ;-) . > > Though we're in the megabyte range with > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR being 0x4000 * 512 . > But I guess with current emmc sizes that might not matter too much. > > But should there be some sort of warning when the > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR doesn't match the expected > value? Because for example rk3399-puma and rk3368-lion historical use that > 0x200 instead of 0x4000 block offset and I think at least their default > firmware also expects u-boot to not reach that far into the emmc. Hello, It's not clear to me whether there's anything else expected from my side to get this accepted. Could you please clarify if the current patch is OK, or if it needs any changes? Thanks, Roger.
Ping? On Mon, Feb 22, 2021 at 10:15:34AM +0100, Roger Pau Monné wrote: > On Wed, Feb 17, 2021 at 02:08:55PM +0100, Heiko Stuebner wrote: > > Hi Tom, > > > > Am Mittwoch, 17. Februar 2021, 03:21:25 CET schrieb Tom Rini: > > > I would say that yes, it's quite intentional that all boards for a given > > > SoC (or SoC family) would use the same value for > > > SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR and NOT leave it up to be per-board. > > > It should be a matter of kilobytes being potentially wasted which is > > > (often or most likely) worth sacrificing in the name of consistency and > > > ease of future use / development. In other cases this ends up being > > > something around "ROM will only load something of $X size, round that up > > > a little bit, place U-Boot there, as it's the next thing to load". > > > > Ok ... then I'guess I'll not stand in the way ;-) . > > > > Though we're in the megabyte range with > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR being 0x4000 * 512 . > > But I guess with current emmc sizes that might not matter too much. > > > > But should there be some sort of warning when the > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR doesn't match the expected > > value? Because for example rk3399-puma and rk3368-lion historical use that > > 0x200 instead of 0x4000 block offset and I think at least their default > > firmware also expects u-boot to not reach that far into the emmc. > > Hello, > > It's not clear to me whether there's anything else expected from my side > to get this accepted. Could you please clarify if the current patch is > OK, or if it needs any changes? > > Thanks, Roger.
diff --git a/configs/odroid-go2_defconfig b/configs/odroid-go2_defconfig index 853890901a..5632b15611 100644 --- a/configs/odroid-go2_defconfig +++ b/configs/odroid-go2_defconfig @@ -33,7 +33,6 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x200 CONFIG_SPL_CRC32_SUPPORT=y CONFIG_SPL_I2C_SUPPORT=y CONFIG_SPL_POWER_SUPPORT=y