| Message ID | 1475055308-24080-6-git-send-email-marcel.ziswiler@toradex.com |
|---|---|
| State | Accepted |
| Commit | 74b19ad1c180fe32a01bccc35b0be1425132d927 |
| Delegated to: | Tom Warren |
| Headers | show |
On 09/28/2016 03:35 AM, Marcel Ziswiler wrote: > Now with the config block handling in place move the U-Boot environment > location before the config block at the end of 1st "boot sector" as > deployed during production using our downstream BSP. > diff --git a/include/configs/apalis_t30.h b/include/configs/apalis_t30.h > -/* Environment in eMMC, at the end of 2nd "boot sector" */ > +/* Environment in eMMC, before config block at the end of 1st "boot sector" */ > #define CONFIG_ENV_IS_IN_MMC > -#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE) > +#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE + \ > + CONFIG_TDX_CFG_BLOCK_OFFSET) I'm not convinced that ENV_OFFSET actually points where the description says it does. (-CONFIG_ENV_SIZE + CONFIG_TDX_CFG_BLOCK_OFFSET) is (-8192 + -512). The original comment states that an ENV_OFFSET of -8192 locates the environment at the end of a sector, so shifting it 512 bytes earlier in the flash can't possible now align it with a different sector? Should ENV_OFFSET be shifted down by a whole sector size by this patch, i.e. should +CONFIG_TXT_CFG_BLOCK_OFFSET be rounded up to a sector size to match the comment?
On Wed, 2016-09-28 at 12:11 -0600, Stephen Warren wrote: > On 09/28/2016 03:35 AM, Marcel Ziswiler wrote: > > > > Now with the config block handling in place move the U-Boot > > environment > > location before the config block at the end of 1st "boot sector" as > > deployed during production using our downstream BSP. > > > > diff --git a/include/configs/apalis_t30.h > > b/include/configs/apalis_t30.h > > > > -/* Environment in eMMC, at the end of 2nd "boot sector" */ > > +/* Environment in eMMC, before config block at the end of 1st > > "boot sector" */ > > #define CONFIG_ENV_IS_IN_MMC > > -#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE) > > +#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE + \ > > + CONFIG_TDX_CFG_BLOCK_OFFS > > ET) > I'm not convinced that ENV_OFFSET actually points where the > description > says it does. (-CONFIG_ENV_SIZE + CONFIG_TDX_CFG_BLOCK_OFFSET) is (- > 8192 > + -512). The original comment states that an ENV_OFFSET of -8192 > locates > the environment at the end of a sector, so shifting it 512 bytes > earlier > in the flash can't possible now align it with a different sector? Why not? > Should > ENV_OFFSET be shifted down by a whole sector size by this patch, > i.e. > should +CONFIG_TXT_CFG_BLOCK_OFFSET be rounded up to a sector size > to > match the comment? Yes, a block, sector or whatever you wana call it is actually 512 bytes in size on all eMMCs or even MMC/SD cards that I have seen so far. I guess while it would be kind of nice having this enforced to some intrinsic size in software ultimately this is a board configuration option concerning our Toradex factory configuration block as programmed during production. We just want to make sure that one is preserved and the environment is stored right before.
On 09/30/2016 05:44 AM, Marcel Ziswiler wrote: > On Wed, 2016-09-28 at 12:11 -0600, Stephen Warren wrote: >> On 09/28/2016 03:35 AM, Marcel Ziswiler wrote: >>> >>> Now with the config block handling in place move the U-Boot >>> environment >>> location before the config block at the end of 1st "boot sector" as >>> deployed during production using our downstream BSP. >>> >>> diff --git a/include/configs/apalis_t30.h b/include/configs/apalis_t30.h >>> >>> -/* Environment in eMMC, at the end of 2nd "boot sector" */ >>> +/* Environment in eMMC, before config block at the end of 1st "boot sector" */ >>> #define CONFIG_ENV_IS_IN_MMC >>> -#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE) >>> +#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE + \ >>> + CONFIG_TDX_CFG_BLOCK_OFFSET) >> I'm not convinced that ENV_OFFSET actually points where the description >> says it does. (-CONFIG_ENV_SIZE + CONFIG_TDX_CFG_BLOCK_OFFSET) is (-8192 >> + -512). The original comment states that an ENV_OFFSET of -8192 locates >> the environment at the end of a sector, so shifting it 512 bytes earlier >> in the flash can't possible now align it with a different sector? > > Why not? If -8192 is aligned to the last sector boundary, the sectors must be 8192 bytes in size (or the last sector isn't a whole sector, which is highly unlikely). If so, then 512 bytes away from that isn't also at a sector boundary. >> Should ENV_OFFSET be shifted down by a whole sector size by this patch, i.e. should >> +CONFIG_TXT_CFG_BLOCK_OFFSET be rounded up to a sector size to match the comment? > > Yes, a block, sector or whatever you wana call it is actually 512 bytes > in size on all eMMCs or even MMC/SD cards that I have seen so far. > > I guess while it would be kind of nice having this enforced to some > intrinsic size in software ultimately this is a board configuration > option concerning our Toradex factory configuration block as programmed > during production. We just want to make sure that one is preserved and > the environment is stored right before. Ah, I see. the existing comment uses the term "sector" incorrectly, which is where the confusion arises. eMMC do indeed have 512-byte sectors. I suppose the old (and new) comments should refer to HW boot partitions, not "sectors"?
diff --git a/include/configs/apalis_t30.h b/include/configs/apalis_t30.h index 1a22973..5be770c 100644 --- a/include/configs/apalis_t30.h +++ b/include/configs/apalis_t30.h @@ -33,11 +33,12 @@ #define CONFIG_GENERIC_MMC #define CONFIG_TEGRA_MMC -/* Environment in eMMC, at the end of 2nd "boot sector" */ +/* Environment in eMMC, before config block at the end of 1st "boot sector" */ #define CONFIG_ENV_IS_IN_MMC -#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE) +#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE + \ + CONFIG_TDX_CFG_BLOCK_OFFSET) #define CONFIG_SYS_MMC_ENV_DEV 0 -#define CONFIG_SYS_MMC_ENV_PART 2 +#define CONFIG_SYS_MMC_ENV_PART 1 /* USB host support */ #define CONFIG_USB_EHCI diff --git a/include/configs/colibri_t30.h b/include/configs/colibri_t30.h index 6b6496b..95cba12 100644 --- a/include/configs/colibri_t30.h +++ b/include/configs/colibri_t30.h @@ -33,11 +33,12 @@ #define CONFIG_GENERIC_MMC #define CONFIG_TEGRA_MMC -/* Environment in eMMC, at the end of 2nd "boot sector" */ +/* Environment in eMMC, before config block at the end of 1st "boot sector" */ #define CONFIG_ENV_IS_IN_MMC -#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE) +#define CONFIG_ENV_OFFSET (-CONFIG_ENV_SIZE + \ + CONFIG_TDX_CFG_BLOCK_OFFSET) #define CONFIG_SYS_MMC_ENV_DEV 0 -#define CONFIG_SYS_MMC_ENV_PART 2 +#define CONFIG_SYS_MMC_ENV_PART 1 /* USB host support */ #define CONFIG_USB_EHCI
Now with the config block handling in place move the U-Boot environment location before the config block at the end of 1st "boot sector" as deployed during production using our downstream BSP. Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> --- Changes in v2: - fixed common.h include mess in board/toradex/common by renaming common* to tdx-common - renamed TRDX to TDX and trdx to tdx as in common use internally - consolidated makefiles and changed copyright message - renamed configblock* to tdx-cfg-block* analogous to Kconfig symbols - moved board/toradex/common/Kconfig sourcing from arch/arm/Kconfig into our own board Kconfig files - use a named choice for the config block location for above to work without issuing any warnings (undocumented kbuild feature curtsey Arnaud Lacombe) - added CUSTOM_BOARDINFO into our common Kconfig to avoid recent no_new_adhoc_configs_check error - add Max' patch colibri_vf: usb gadget: toradex pid is now set generically include/configs/apalis_t30.h | 7 ++++--- include/configs/colibri_t30.h | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-)