[PATCH] odroid-go2: remove setting SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR

[Heiko Stuebner]

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 at 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