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

[Masahiro Yamada]

2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com>:
> Masahiro & Simon,
>
>> On 20 Sep 2017, at 15:49, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Masahiro,
>>
>> On 19 September 2017 at 20:51, Masahiro Yamada
>> <yamada.masahiro at socionext.com> wrote:
>>> Hi Simon,
>>>
>>>
>>> 2017-09-17 6:23 GMT+09:00 Simon Glass <sjg at 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