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

[Maxime Ripard]

On Wed, Feb 21, 2018 at 09:54:41AM -0500, Tom Rini wrote:
> On Wed, Feb 21, 2018 at 03:38:42PM +0100, Simon Goldschmidt wrote:
> > On 21.02.2018 14:35, 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.
> > >
> > >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.
> > 
> > But it's like that already, isnt' it? Env load selects the first valid
> > location in the list and env save uses that location. All other env
> > operations work on the environment in memory.
> 
> For saveenv we have:
> for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
> ...

In the default case, env_driver_lookup will return the location where
the environment with the highest priority has been loaded.

> > Also, for transitioning e.g. from MMC to FAT, we would need a mechanism to
> > store to an environment place other than the one selected at load time.
> 
> Ah, so we have different valid use cases.  Maybe a new env sub-command,
> switch?

We implemented it by overwriting the priority look up.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- 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/20180221/8f84f0d0/attachment.sig>