Re: [U-Boot] [PATCH 06/13] log: Add an implemention of logging

20 Sep 2017


      2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich
philipp.tomsich@theobroma-systems.com:
...
Masahiro & Simon,
...
On 20 Sep 2017, at 15:49, Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
On 19 September 2017 at 20:51, Masahiro Yamada
yamada.masahiro@socionext.com wrote:
...
Hi Simon,
2017-09-17 6:23 GMT+09:00 Simon Glass sjg@chromium.org:
...
+menu "Logging"


+config LOG

  bool "Enable logging support"


  help


    This enables support for logging of status and debug messages. These


    can be displayed on the console, recorded in a memory buffer, or


    discarded if not needed. Logging supports various categories and


    levels of severity.


+config SPL_LOG

  bool "Enable logging support in SPL"


  help


    This enables support for logging of status and debug messages. These


    can be displayed on the console, recorded in a memory buffer, or


    discarded if not needed. Logging supports various categories and


    levels of severity.


Please note CONFIG_IS_ENABLED(LOG) is never enabled for TPL_BUILD.
Since commit f1c6e1922eb57f4a212c09709801a1cc7920ffa9,
CONFIG_IS_ENABLED(LOG) is expanded to CONFIG_TPL_LOG
when building for TPL.
Since that commit, if you add SPL_ prefixed option,
you need to add a TPL_ one as well.
I cannot believe why such a commit was accepted.
Well either way is strange. it is strange that SPL is enabled for TPL
when really they are separate.
We could revert that commit. But how do you think all of this SPL/TPL
control should actually work? What is intended?
But I'm OK with not having logging in TPL until we need it.
If we don’t differentiate between TPL_ and SPL_, we’ll eventually run into
size issues for TPL and the $(SPL_TPL_) mechanism will not match the
CONFIG_IS_ENABLED() mechanism.
I don’t think that anyone will miss this much in TPL and that this can be
safely left off for TPL (if space was not at a premium in TPL, then why
have a TPL at all…)
The motivation of TPL is
the image size is really limited
for the secondary boot loader in some cases.
Instead of:
  SPL -> TPL -> U-Boot full
I'd rather want to see:
  <something>  ->  SPL  ->  U-Boot full
<something> is implemented in an ad-hoc way under board or soc directory.
If the space is premium, there is no room for DM, DT-ctrl in the <something>.
Then, remove the current TPL support.
It was only some old freescale boards that used TPL,
so I was expecting they would die out sooner or later.
Recently, lion-rk3368_defconfig was added wit TPL.
Other rk3368 boards do not enable TPL.
Why does that board need TPL?
...
...
...
...
+config LOG_MAX_LEVEL

  int "Maximum log level to record"


  depends on LOG


  default 5


  help


    This selects the maximum log level that will be recorded. Any value


    higher than this will be ignored. If possible log statements below


    this level will be discarded at build time. Levels:


      0 - panic


      1 - critical


      2 - error


      3 - warning


      4 - note


      5 - info


      6 - detail


      7 - debug


Please do not invent our own for U-Boot.
Just use Linux log level.
                   0 (KERN_EMERG)          system is unusable
                   1 (KERN_ALERT)          action must be taken immediately
                   2 (KERN_CRIT)           critical conditions
                   3 (KERN_ERR)            error conditions
                   4 (KERN_WARNING)        warning conditions
                   5 (KERN_NOTICE)         normal but significant condition
                   6 (KERN_INFO)           informational
                   7 (KERN_DEBUG)          debug-level messages

Yes I looked hard at that. The first three seem hard to distinguish in
U-Boot, but we can keep them I suppose. But most of my problem is with
the last two. INFO is what I plan to use for normal printf() output.
DEBUG is obviously for debugging and often involves vaste amounts of
stuff (e.g. logging every access to an MMC peripheral). We need
something in between. It could list the accesses to device at a high
level (e.g API calls) but not every little register access.
So I don't think the Linux levels are suitable at the high end. We
could go up to 8 I suppose, instead of trying to save one at the
bottom?
I would expect KERN_NOTICE to be used for normal printf output, KERN_INFO
for more verbose output and DEBUG would be the granularity for tracing through
drivers (i.e. the MMC example given).
In my opinion, printf() and pr_*() should be different concept.
printf() (and puts(), putc(), etc.) should be turned on/off
by CONFIG_SILENT_CONSOLE.
These are direct access to console,
so they should always and immediately print messages
unless CONFIG_SILENT_CONSOLE is defined.
We may want to use the command interface regardless of
CONFIG_LOG / CONFIG_LOGLEVEL.
pr_*() are log functions that are controlled by CONFIG_LOGLEVEL.
These can send the messages to the console directly,
or to the ring buffer, or wherever implemented in the log device.
-- 
Best Regards
Masahiro Yamada