[U-Boot] [PATCHv2 5/5] env: Finish migration of common ENV options

Wed Nov 20 19:56:19 UTC 2019

On Wed, Nov 20, 2019 at 08:39:27PM +0100, Simon Goldschmidt wrote:
> Am 19.11.2019 um 22:52 schrieb Tom Rini:
> > On Tue, Nov 19, 2019 at 10:39:18PM +0100, Simon Goldschmidt wrote:
> > > Am 19.11.2019 um 02:02 schrieb Tom Rini:> - In ARMv8 NXP Layerscape
> > > platforms we also need to make use of
> > > >     CONFIG_SYS_RELOC_GD_ENV_ADDR now, do so.
> > > > - On ENV_IS_IN_REMOTE, CONFIG_ENV_OFFSET is never used, drop the define
> > > >     to 0.
> > > > - Add Kconfig entry for ENV_ADDR.
> > > > - Make ENV_ADDR / ENV_OFFSET depend on the env locations that use it.
> > > > - Add ENV_xxx_REDUND options that depend on their primary option and
> > > >     SYS_REDUNDAND_ENVIRONMENT
> > > > - On a number of PowerPC platforms, use SPL_ENV_ADDR not CONFIG_ENV_ADDR
> > > >     for the pre-main-U-Boot environment location.
> > > > - On ENV_IS_IN_SPI_FLASH, check not for CONFIG_ENV_ADDR being set but
> > > >     rather it being non-zero, as it will now be zero by default.
> > > > - Rework the env_offset absolute in env/embedded.o to not use
> > > >     CONFIG_ENV_OFFSET as it was the only use of ENV_OFFSET within
> > > >     ENV_IS_IN_FLASH.
> > > > - Migrate all platforms.
> > > > 
> > > > Cc: Wolfgang Denk <wd at denx.de>
> > > > Cc: Joe Hershberger <joe.hershberger at ni.com>
> > > > Cc: Patrick Delaunay <patrick.delaunay at st.com>
> > > > Cc: uboot-stm32 at st-md-mailman.stormreply.com
> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > ---
> > > 
> > > <snip>
> > > 
> > > > diff --git a/include/configs/socfpga_common.h
> > > b/include/configs/socfpga_common.h
> > > > index baa214399ff9..05bfef75c0df 100644
> > > > --- a/include/configs/socfpga_common.h
> > > > +++ b/include/configs/socfpga_common.h
> > > > @@ -157,21 +157,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
> > > >    /*
> > > >     * U-Boot environment
> > > >     */
> > > > -#if !defined(CONFIG_ENV_SIZE)
> > > > -#define CONFIG_ENV_SIZE			(8 * 1024)
> > > > -#endif
> > > > 
> > > >    /* Environment for SDMMC boot */
> > > > -#if defined(CONFIG_ENV_IS_IN_MMC) && !defined(CONFIG_ENV_OFFSET)
> > > > +#if defined(CONFIG_ENV_IS_IN_MMC)
> > > >    #define CONFIG_SYS_MMC_ENV_DEV		0 /* device 0 */
> > > > -#define CONFIG_ENV_OFFSET		(34 * 512) /* just after the GPT */
> > > >    #endif
> > > > 
> > > >    /* Environment for QSPI boot */
> > > > -#if defined(CONFIG_ENV_IS_IN_SPI_FLASH) && !defined(CONFIG_ENV_OFFSET)
> > > > -#define CONFIG_ENV_OFFSET		0x00100000
> > > > -#define CONFIG_ENV_SECT_SIZE		(64 * 1024)
> > > > -#endif
> > > 
> > > Removing paragraphs like this one will break configs that haven't made it to
> > > a mainline defconfig. E.g. for socfpga_socrates_defconfig, you can chose for
> > > the ENV to be saved in SPI instead of MMC as the config supports booting
> > > from all sources.
> > > 
> > > How do we proceed with such things? I know that might be non-mainline, but I
> > > think throwing this info away is a step-back, not an improvement.
> > > 
> > > [And don't get me wrong: this doesn't affect my downstream boards, they
> > > don't save/load env due to secure boot reasons anyway.]
> > 
> > So, I would be happy to see follow up series that add default values for
> > locations for various SoCs.  That would address the type of problem
> > you're talking about, I believe.
> 
> You mean adding lines like this to env/Kconfig at config ENV_OFFSET
> 	default 0x00100000 if ARCH_SOCFPGA && ENV_IS_IN_SPI_FLASH
> 
> Does that even work? And how well does it scale? I know it's a more
> fundamental problem, but in my opinion, scattering those defaults for every
> arch or board throughout all Kconfig files is not the best solution.
> Although I have to admit I don't have an idea of how to solve it better
> right now...

That does work, it's about what we do for other platforms today, it's
not the best and doesn't scale well.

> > I'd be even happier if someone looked at how Zephyr takes a dts file and
> > generates a header and adapt that to a way for us to have some values be
> > read from a dts* file and turned into a define we can use at build time
> > (not just the OF_PLAT data).  That would be a real nice step forward :)
> > 
> 
> If we have that ENV information in the devicetree, we could just use it and
> dump the defines. No need to convert dts to defines beforehand. Only if dts
> cannot be used, we need something different. But in that case (mostly real
> small SPL, I guess), do we really enable the ENV loading code?

I mention the above mainly because there's a lot of other examples of
this type of problem where yes, we can put a value in Kconfig and have a
default but it's really not the best way forward.

> Anyway, thinking this over, I don't see a way out to keep the SPI env
> locations for socfpga when moving to Kconfig, so for socfpga:
> 
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

Thanks!

> BTW: From looking at the include/configs/* changes, CONFIG_SYS_MMC_ENV_DEV
> could be moved to Kconfig, too...

Yes, there's a bunch of other changes that could/should be moved as
well.  I'm going down this particular rabbit hole so that I can take the
MTD Makefile/Kconfig clean-up and verify that those changes are still
size neutral.  I want to circle back to the env stuff to change a few
things soon including seeing about getting the rest of the easy
migrations done.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191120/126ade6b/attachment.sig>