[U-Boot] [RFC] Make U-Boot log great again

[Tom Rini]

On Wed, Feb 21, 2018 at 02:55:29PM +0100, Maxime Ripard wrote:
> On Wed, Feb 21, 2018 at 08:35:47AM -0500, Tom Rini wrote:
> > On Tue, Feb 20, 2018 at 10:59:33AM +0100, Lukasz Majewski wrote:
> > > On Tue, 20 Feb 2018 09:00:56 +0100
> > > Maxime Ripard <maxime.ripard at bootlin.com> wrote:
> > > 
> > > > On Mon, Feb 19, 2018 at 07:47:52PM +0200, Sam Protsenko wrote:
> > > > > On 18 February 2018 at 23:22, Wolfgang Denk <wd at denx.de> wrote:  
> > > > > > Dear Sam,
> > > > > >
> > > > > > In message
> > > > > > <CAKaJLVsWKpGeEuS=iZ7QCtZrDfUSA=8GZo3zJDr-VgW-MUCFzA at mail.gmail.com>
> > > > > > you wrote:  
> > > > > >>
> > > > > >> Right now U-Boot and SPL logs are cluttered with bogus warnings
> > > > > >> like these (on X15 board, but I'm pretty sure it should appear
> > > > > >> on many others):
> > > > > >>
> > > > > >>     Loading Environment from FAT...
> > > > > >>     *** Warning - bad CRC, using default environment  
> > > > > >
> > > > > > I donpt want to question the purpose of your patch series in
> > > > > > genral, but:
> > > > > >  
> > > > > 
> > > > > Oh, it's merely a discussion, not a patch series. I probably
> > > > > shouldn't have been added that RFC tag, it's confusing, sorry.
> > > > >   
> > > > > > This is NOT a bogus warning - actually it is something which is
> > > > > > not supposed to happen on any sane system.  If it does on your
> > > > > > board even after first boot and running "env save" at least once,
> > > > > > then you have some problem either in the design or implementation
> > > > > > of your board code.
> > > > > >
> > > > > > So this is a very valid warning which means: FIX ME!
> > > > > >  
> > > > > 
> > > > > AFAIU, that behavior was changed in the mentioned patch (please see
> > > > > my original message). Let me elaborate a bit. In v2018.01 everything
> > > > > works fine and none errors/warnings are present on my boards (AM57x
> > > > > EVM and X15 board). The problem appears after commit fb69464eae1e
> > > > > ("env: Allow to build multiple environments in Kconfig"). Now U-Boot
> > > > > tries to load the environment from SD card first (uEnv.txt file on
> > > > > FAT partition), and then from eMMC partition. In case when SD card
> > > > > is not inserted, I observe mentioned errors. So I'm not sure how to
> > > > > handle this properly, that's why I created this thread... Let me
> > > > > try and explain my concerns better:
> > > > >  1. On the one hand, it's good to check the environment on both SD
> > > > > card and eMMC (that was done in mentioned patch). This case seems to
> > > > > be legit (at least as far as I understand it), i.e. when SD card is
> > > > > not inserted, it's fine, we just check the env on eMMC
> > > > >  2. But on the other hand, errors shouldn't appear in boot log, if
> > > > > it's legit case, it's confusing the user  
> > > > 
> > > > That patch intent was to keep the current behaviour as is for all
> > > > users, so the fact that you now have the FAT environment enabled is an
> > > > unwanted side-effect.
> > > 
> > > The same situation is on Beagle Bone Black. Even though with OE it is
> > > built to use eMMC for storing its envs, by default it also has envs in
> > > FAT support enabled.
> > > 
> > > For that reason, u-boot on this board looks for envs in FAT first and
> > > similar message is printed.
> > > 
> > > IMHO, we now have (unintentionally) the situation where implicitly
> > > reading envs from FAT has the highest priority.
> > 
> > It's not so much unintentional but rather that the mechanism to define
> > the priority order isn't being provided specifically by
> > board/ti/am335x/board.c so we get the default order.
> 
> It really was unintentional to me :)
> 
> The point of that commit really was to not introduce new environments
> to anyone. The fact that we now have FAT being higher priority than
> MMC is a side effect of that since we now have two environments
> enabled. If we only had the MMC, we wouldn't have any issue with it,
> and that was my intent.
> 
> Is there an easy way (one in tools/ ?) to try to diff two configs
> between revisions?

So, what happened here is that the defconfig file says ENV_IS_IN_MMC but
we also have 'default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS' in
env/Kconfig.

> > And one thing that I think does need to happen now is that the error
> > messages about "didn't find valid environment in ..." need to be
> > rethought a bit.  It would probably also make sense to move from every
> > env operation tries every possible env location to env init finds the
> > first valid location, tells the user clearly it's using that, and then
> > always uses it.
> 
> Not all environments have an init callback, so we'd rather need to do
> that at load time. However, I guess we also want to inform users if a
> higher priority load has failed somehow.

Riffing off what I just said to Simon, I was thinking that at env_init()
we set the default env_driver, and then introduce a new sub-command to
'env' so that a user can specify where they want the env to be stored
(or if you do env default -f -a, re-read).

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