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

[Simon Glass]

Hi Masahiro,

On 27 September 2017 at 11:11, Masahiro Yamada
 wrote:
> Hi Simon,
>
>
> 2017-09-27 4:10 GMT+09:00 Simon Glass :
>> Hi Masahiro,
>>
>> On 20 September 2017 at 11:19, Masahiro Yamada
>>  wrote:
>>> Hi Simon,
>>>
>>>
>>> 2017-09-20 22:49 GMT+09:00 Simon Glass :
 Hi Masahiro,

 On 19 September 2017 at 20:51, Masahiro Yamada
  wrote:
> Hi Simon,
>
>
> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>
>>
>> +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.
>>>
>>> I will explain it in another mail.
>>>
>>>
>
>
>
>
>> +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?
>>>
>>>
>>> In fact, Linux has one more for debug.
>>>  dev_vdbg() is widely used in Linux.
>>>
>>> If you like, we can add one more level:
>>>
>>>  8 (KERN_VDEBUG)   verbose debug messages
>>>
>>>
>>> Perhaps, logging every access to an MMC peripheral
>>> might belong to the vdbg level.
>>
>> I like the idea of having a log level for message contents (bytes) and
>> another for I/O access. So I will add two more in v2.
>>
>>>
>>>
>>>
>>> BTW, what do you mean "INFO is what I plan to use for normal printf() 
>>> output"
>>>
>>> Is that mean printf() is equivalent to pr_info()?
>>> If loglevel is 6 or smaller, will all print() be silent?
>>> If so, probably we can not use 

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

[Masahiro Yamada]

Hi Simon,


2017-09-27 4:10 GMT+09:00 Simon Glass :
> Hi Masahiro,
>
> On 20 September 2017 at 11:19, Masahiro Yamada
>  wrote:
>> Hi Simon,
>>
>>
>> 2017-09-20 22:49 GMT+09:00 Simon Glass :
>>> Hi Masahiro,
>>>
>>> On 19 September 2017 at 20:51, Masahiro Yamada
>>>  wrote:
 Hi Simon,


 2017-09-17 6:23 GMT+09:00 Simon Glass :

>
> +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.
>>
>> I will explain it in another mail.
>>
>>




> +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?
>>
>>
>> In fact, Linux has one more for debug.
>>  dev_vdbg() is widely used in Linux.
>>
>> If you like, we can add one more level:
>>
>>  8 (KERN_VDEBUG)   verbose debug messages
>>
>>
>> Perhaps, logging every access to an MMC peripheral
>> might belong to the vdbg level.
>
> I like the idea of having a log level for message contents (bytes) and
> another for I/O access. So I will add two more in v2.
>
>>
>>
>>
>> BTW, what do you mean "INFO is what I plan to use for normal printf() output"
>>
>> Is that mean printf() is equivalent to pr_info()?
>> If loglevel is 6 or smaller, will all print() be silent?
>> If so, probably we can not use command line interface.
>
> I mean that I want to (later) add a feature that logs normal printf()
> output. If the console is silent then it would still be logged. Maybe
> one day log functions will be used instead of printf(), but for now
> this provides a useful way to make 

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

[Masahiro Yamada]

Hi Philipp,


2017-09-21 2:51 GMT+09:00 Dr. Philipp Tomsich
:
> Masahiro,
>
>> On 20 Sep 2017, at 19:34, Masahiro Yamada  
>> wrote:
>>
>> 2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich
>> :
>>> Masahiro & Simon,
>>>
 On 20 Sep 2017, at 15:49, Simon Glass  wrote:

 Hi Masahiro,

 On 19 September 2017 at 20:51, Masahiro Yamada
  wrote:
> Hi Simon,
>
>
> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>
>>
>> +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
>
> Note that this was retro-actively defined to be
> TPL -> SPL -> U-Boot full
> by Tom at some point and reiterated in
> https://lists.denx.de/pipermail/u-boot/2017-July/299266.html



Thanks.  I did not know that this flip had already happened.


In fact, I am probably the first man who suggested it.


Here is the history.


1. Scott Wood introduced TPL to support some freescale chip,
   which had only 4KB memory footprint for the second loader
   https://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2013/tpl-presentation.pdf

   At this point, the boot order was:  SPL -> TPL -> U-Boot
   And TPL means "Tertiary Program Loader".


2.  I imported Kbuild and Kconfig to U-Boot.


3.  During the migration of Kconfig, I noticed
switching the order of SPL / TPL has advantages.

The "Tiny Program Loader" was mentioned first in this mail:
https://lists.denx.de/pipermail/u-boot/2015-August/222900.html


4.  When Simon started to move CONFIG_TPL,
I suggested it once again
http://patchwork.ozlabs.org/patch/662396/



So, I'd like to make "TPL -> SPL" legitimate.


More ideally, I hope this is done outside of luxury frameworks.
No DM, no OF_CONTROL, then CONFIG_TPL_* are all gone.






-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

Hi Masahiro,

On 20 September 2017 at 11:19, Masahiro Yamada
 wrote:
> Hi Simon,
>
>
> 2017-09-20 22:49 GMT+09:00 Simon Glass :
>> Hi Masahiro,
>>
>> On 19 September 2017 at 20:51, Masahiro Yamada
>>  wrote:
>>> Hi Simon,
>>>
>>>
>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>>>

 +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.
>
> I will explain it in another mail.
>
>
>>>
>>>
>>>
>>>
 +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?
>
>
> In fact, Linux has one more for debug.
>  dev_vdbg() is widely used in Linux.
>
> If you like, we can add one more level:
>
>  8 (KERN_VDEBUG)   verbose debug messages
>
>
> Perhaps, logging every access to an MMC peripheral
> might belong to the vdbg level.

I like the idea of having a log level for message contents (bytes) and
another for I/O access. So I will add two more in v2.

>
>
>
> BTW, what do you mean "INFO is what I plan to use for normal printf() output"
>
> Is that mean printf() is equivalent to pr_info()?
> If loglevel is 6 or smaller, will all print() be silent?
> If so, probably we can not use command line interface.

I mean that I want to (later) add a feature that logs normal printf()
output. If the console is silent then it would still be logged. Maybe
one day log functions will be used instead of printf(), but for now
this provides a useful way to make things wok.

>
>
>
>
>
>>>
>>>
>>>
>>>
 +
 +/**
 + * log_dispatch() - Send a log record to all log devices for processing
 + *
 + * The log record is sent to each log device in turn, skipping those 

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

[Simon Glass]

Hi Bin,

On 22 September 2017 at 07:37, Bin Meng  wrote:
> Hi Simon,
>
> On Thu, Sep 21, 2017 at 12:58 PM, Simon Glass  wrote:
>> Hi Bin,
>>
>> On 20 September 2017 at 08:41, Bin Meng  wrote:
>>>
>>> Hi Simon,
>>>
>>> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass  wrote:
>>> > Hi Bin,
>>> >
>>> > On 17 September 2017 at 21:45, Bin Meng  wrote:
>>> >> Hi Simon,
>>> >>
>>> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
>>> >>> Add the logging header file and implementation with some configuration
>>> >>> options to control it.
>>> >>>
>>> >>> Signed-off-by: Simon Glass 
>>> >>> ---
>>> >>>
>>> >>>  MAINTAINERS   |   9 ++
>>> >>>  common/Kconfig|  56 +
>>> >>>  common/Makefile   |   1 +
>>> >>>  common/log.c  | 246 
>>> >>> +
>>> >>>  include/asm-generic/global_data.h |   5 +
>>> >>>  include/log.h | 247 
>>> >>> --
>>> >>>  6 files changed, 555 insertions(+), 9 deletions(-)
>>> >>>  create mode 100644 common/log.c
>>> >>>
>>> >>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> >>> index 04acf2b89d..eb420afa8d 100644
>>> >>> --- a/MAINTAINERS
>>> >>> +++ b/MAINTAINERS
>>> >>> @@ -290,6 +290,15 @@ S: Maintained
>>> >>>  T: git git://git.denx.de/u-boot-i2c.git
>>> >>>  F: drivers/i2c/
>>> >>>
>>> >>> +LOGGING
>>> >>> +M: Simon Glass 
>>> >>> +S: Maintained
>>> >>> +T: git git://git.denx.de/u-boot.git
>>> >>> +F: common/log.c
>>> >>> +F: cmd/log.c
>>> >>> +F: test/log/log_test.c
>>> >>> +F: test/py/tests/test_log.py
>>> >>
>>> >> test/log/log_test.c and test/py/tests/test_log.py have not been
>>> >> introduced at this point.
>>> >
>>> > OK I can tweak that,
>>> > [..]
>>> >
>>> >>> +/**
>>> >>> + * struct log_driver - a driver which accepts and processes log records
>>> >>> + *
>>> >>> + * @name: Name of driver
>>> >>> + */
>>> >>> +struct log_driver {
>>> >>> +   const char *name;
>>> >>> +   /**
>>> >>> +* emit() - emit a log record
>>> >>> +*
>>> >>> +* Called by the log system to pass a log record to a 
>>> >>> particular driver
>>> >>> +* for processing. The filter is checked before calling this 
>>> >>> function.
>>> >>> +*/
>>> >>> +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
>>> >>> +};
>>> >>> +
>>> >>
>>> >> So we are creating a new type of non-DM driver which is log-specific?
>>> >> How about we add this emit to the existing uclass driver that can be
>>> >> used as the log driver? (eg: blk devices with file system?)
>>> >
>>> > Yes that's right. I think I can link it to DM once it starts up, but a
>>> > logging of DM start-up is useful.
>>> >
>>> > Re your idea are you saying add an emit() method to UCLASS_BLK?
>>> >
>>>
>>> Yep, something like
>>>
>>> #ifdef CONFIG_LOG
>>> .log_emit = xxx_log_emit,
>>> #endif
>>>
>>> Probably we need a flag to find out which driver provides such log
>>> functionality.
>>
>> So what would the block driver do in that function? I guess I don't
>> understand what it is for. Can you please give me more information?
>>
>
> The blk driver can save the logs to a predefined block address for
> permanent storage. Or file system driver can send the logs to a file.

OK I see. IMO this is not really a function of a block device. I don't
see what an emit() method in a USB block device (for example) would do
differently from one in a SATA block device.

We could have a single 'log_blk' driver which:

- allows configuration of what blocks are used for the log
- writes blocks there (perhaps wrapping if it runs out of space)

For file systems we could have a 'log_fs' driver which:

- allows configuration of log file path
- writes blocks into that file (perhaps starting a new file and
rotating the log if things get over a limit

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Bin Meng]

Hi Simon,

On Thu, Sep 21, 2017 at 12:58 PM, Simon Glass  wrote:
> Hi Bin,
>
> On 20 September 2017 at 08:41, Bin Meng  wrote:
>>
>> Hi Simon,
>>
>> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass  wrote:
>> > Hi Bin,
>> >
>> > On 17 September 2017 at 21:45, Bin Meng  wrote:
>> >> Hi Simon,
>> >>
>> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
>> >>> Add the logging header file and implementation with some configuration
>> >>> options to control it.
>> >>>
>> >>> Signed-off-by: Simon Glass 
>> >>> ---
>> >>>
>> >>>  MAINTAINERS   |   9 ++
>> >>>  common/Kconfig|  56 +
>> >>>  common/Makefile   |   1 +
>> >>>  common/log.c  | 246 
>> >>> +
>> >>>  include/asm-generic/global_data.h |   5 +
>> >>>  include/log.h | 247 
>> >>> --
>> >>>  6 files changed, 555 insertions(+), 9 deletions(-)
>> >>>  create mode 100644 common/log.c
>> >>>
>> >>> diff --git a/MAINTAINERS b/MAINTAINERS
>> >>> index 04acf2b89d..eb420afa8d 100644
>> >>> --- a/MAINTAINERS
>> >>> +++ b/MAINTAINERS
>> >>> @@ -290,6 +290,15 @@ S: Maintained
>> >>>  T: git git://git.denx.de/u-boot-i2c.git
>> >>>  F: drivers/i2c/
>> >>>
>> >>> +LOGGING
>> >>> +M: Simon Glass 
>> >>> +S: Maintained
>> >>> +T: git git://git.denx.de/u-boot.git
>> >>> +F: common/log.c
>> >>> +F: cmd/log.c
>> >>> +F: test/log/log_test.c
>> >>> +F: test/py/tests/test_log.py
>> >>
>> >> test/log/log_test.c and test/py/tests/test_log.py have not been
>> >> introduced at this point.
>> >
>> > OK I can tweak that,
>> > [..]
>> >
>> >>> +/**
>> >>> + * struct log_driver - a driver which accepts and processes log records
>> >>> + *
>> >>> + * @name: Name of driver
>> >>> + */
>> >>> +struct log_driver {
>> >>> +   const char *name;
>> >>> +   /**
>> >>> +* emit() - emit a log record
>> >>> +*
>> >>> +* Called by the log system to pass a log record to a particular 
>> >>> driver
>> >>> +* for processing. The filter is checked before calling this 
>> >>> function.
>> >>> +*/
>> >>> +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
>> >>> +};
>> >>> +
>> >>
>> >> So we are creating a new type of non-DM driver which is log-specific?
>> >> How about we add this emit to the existing uclass driver that can be
>> >> used as the log driver? (eg: blk devices with file system?)
>> >
>> > Yes that's right. I think I can link it to DM once it starts up, but a
>> > logging of DM start-up is useful.
>> >
>> > Re your idea are you saying add an emit() method to UCLASS_BLK?
>> >
>>
>> Yep, something like
>>
>> #ifdef CONFIG_LOG
>> .log_emit = xxx_log_emit,
>> #endif
>>
>> Probably we need a flag to find out which driver provides such log
>> functionality.
>
> So what would the block driver do in that function? I guess I don't
> understand what it is for. Can you please give me more information?
>

The blk driver can save the logs to a predefined block address for
permanent storage. Or file system driver can send the logs to a file.

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

Hi Bin,

On 20 September 2017 at 08:41, Bin Meng  wrote:
>
> Hi Simon,
>
> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass  wrote:
> > Hi Bin,
> >
> > On 17 September 2017 at 21:45, Bin Meng  wrote:
> >> Hi Simon,
> >>
> >> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
> >>> Add the logging header file and implementation with some configuration
> >>> options to control it.
> >>>
> >>> Signed-off-by: Simon Glass 
> >>> ---
> >>>
> >>>  MAINTAINERS   |   9 ++
> >>>  common/Kconfig|  56 +
> >>>  common/Makefile   |   1 +
> >>>  common/log.c  | 246 
> >>> +
> >>>  include/asm-generic/global_data.h |   5 +
> >>>  include/log.h | 247 
> >>> --
> >>>  6 files changed, 555 insertions(+), 9 deletions(-)
> >>>  create mode 100644 common/log.c
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 04acf2b89d..eb420afa8d 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -290,6 +290,15 @@ S: Maintained
> >>>  T: git git://git.denx.de/u-boot-i2c.git
> >>>  F: drivers/i2c/
> >>>
> >>> +LOGGING
> >>> +M: Simon Glass 
> >>> +S: Maintained
> >>> +T: git git://git.denx.de/u-boot.git
> >>> +F: common/log.c
> >>> +F: cmd/log.c
> >>> +F: test/log/log_test.c
> >>> +F: test/py/tests/test_log.py
> >>
> >> test/log/log_test.c and test/py/tests/test_log.py have not been
> >> introduced at this point.
> >
> > OK I can tweak that,
> > [..]
> >
> >>> +/**
> >>> + * struct log_driver - a driver which accepts and processes log records
> >>> + *
> >>> + * @name: Name of driver
> >>> + */
> >>> +struct log_driver {
> >>> +   const char *name;
> >>> +   /**
> >>> +* emit() - emit a log record
> >>> +*
> >>> +* Called by the log system to pass a log record to a particular 
> >>> driver
> >>> +* for processing. The filter is checked before calling this 
> >>> function.
> >>> +*/
> >>> +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
> >>> +};
> >>> +
> >>
> >> So we are creating a new type of non-DM driver which is log-specific?
> >> How about we add this emit to the existing uclass driver that can be
> >> used as the log driver? (eg: blk devices with file system?)
> >
> > Yes that's right. I think I can link it to DM once it starts up, but a
> > logging of DM start-up is useful.
> >
> > Re your idea are you saying add an emit() method to UCLASS_BLK?
> >
>
> Yep, something like
>
> #ifdef CONFIG_LOG
> .log_emit = xxx_log_emit,
> #endif
>
> Probably we need a flag to find out which driver provides such log
> functionality.

So what would the block driver do in that function? I guess I don't
understand what it is for. Can you please give me more information?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Dr. Philipp Tomsich]

Masahiro,

> On 20 Sep 2017, at 19:34, Masahiro Yamada  
> wrote:
> 
> 2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich
> :
>> Masahiro & Simon,
>> 
>>> On 20 Sep 2017, at 15:49, Simon Glass  wrote:
>>> 
>>> Hi Masahiro,
>>> 
>>> On 19 September 2017 at 20:51, Masahiro Yamada
>>>  wrote:
 Hi Simon,
 
 
 2017-09-17 6:23 GMT+09:00 Simon Glass :
 
> 
> +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

Note that this was retro-actively defined to be
TPL -> SPL -> U-Boot full
by Tom at some point and reiterated in
https://lists.denx.de/pipermail/u-boot/2017-July/299266.html

> I'd rather want to see:
>->  SPL  ->  U-Boot full

I would agree, that the stage before the SPL could be chip-specific.
However, this is unlikely to happen quickly as some of the infrastructure 
(e.g. OF_PLATDATA) would have to be easily available to this new TPL
framework.

>  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 .
> 
> 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.

Other RK3368 boards do not have DRAM init support yet (they still use
the proprietary vendor blobs and then boot the full U-Boot stage).  This
is gated by the resource availability on Rockchip’s end to add support
for those features of the DRAM controller that can not be tested on Lion.

> Why does that board need TPL?

The RK3368’s boot-ROM loads only 0x7000 bytes as its first stage and
requires this to (a) set up clocking and (b) initialise the DRAM controller.
The SPL-stage on the RK3368 then runs out of DRAM (still loaded by
the boot-ROM), but the size-limit is somewhat relaxed.

On the RK3368, we take the liberty of reusing as much framework code
as possible in the TPL (resulting in 21k binary) and using OF_PLATDATA:
the features reused include DM, DM_REGMAP, DM_SYSCON, DM_CLK, 
DM_RAM and DM_TIMER.  With this, we can both use the same drivers
as for SPL and full U-Boot and then have a SPL stage that does not rely
of OF_PLATDATA (but has full OF_CONTROL).

Note that TPL is required for most of the Rockchip boards, if we want to 
do the DRAM initialisation in U-Boot… those chips that already have their
DRAM controller drivers merged, usually have a TPL stage (with the
exception of the RK3399, which supports 192kB for its first stage).

Regards,
Philipp.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Masahiro Yamada]

2017-09-20 23:37 GMT+09:00 Dr. Philipp Tomsich
:
> Masahiro & Simon,
>
>> On 20 Sep 2017, at 15:49, Simon Glass  wrote:
>>
>> Hi Masahiro,
>>
>> On 19 September 2017 at 20:51, Masahiro Yamada
>>  wrote:
>>> Hi Simon,
>>>
>>>
>>> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>>>

 +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:
->  SPL  ->  U-Boot full


 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 .

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() 

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

[Masahiro Yamada]

Hi Simon,


2017-09-20 22:49 GMT+09:00 Simon Glass :
> Hi Masahiro,
>
> On 19 September 2017 at 20:51, Masahiro Yamada
>  wrote:
>> Hi Simon,
>>
>>
>> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>>
>>>
>>> +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.

I will explain it in another mail.


>>
>>
>>
>>
>>> +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?


In fact, Linux has one more for debug.
 dev_vdbg() is widely used in Linux.

If you like, we can add one more level:

 8 (KERN_VDEBUG)   verbose debug messages


Perhaps, logging every access to an MMC peripheral
might belong to the vdbg level.



BTW, what do you mean "INFO is what I plan to use for normal printf() output"

Is that mean printf() is equivalent to pr_info()?
If loglevel is 6 or smaller, will all print() be silent?
If so, probably we can not use command line interface.





>>
>>
>>
>>
>>> +
>>> +/**
>>> + * log_dispatch() - Send a log record to all log devices for processing
>>> + *
>>> + * The log record is sent to each log device in turn, skipping those which 
>>> have
>>> + * filters which block the record
>>> + *
>>> + * @rec: Log record to dispatch
>>> + * @return 0 (meaning success)
>>> + */
>>> +static int log_dispatch(struct log_rec *rec)
>>> +{
>>> +   struct log_device *ldev;
>>> +
>>> +   list_for_each_entry(ldev, >log_head, sibling_node) {
>>> +   if (log_passes_filters(ldev, rec))
>>> +   ldev->drv->emit(ldev, rec);
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,
>>> +int line, const char *func, const char *fmt, ...)
>>> +{
>>> +   char 

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

[Dr. Philipp Tomsich]

> On 20 Sep 2017, at 16:41, Bin Meng  wrote:
> 
> Hi Simon,
> 
> On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass  wrote:
>> Hi Bin,
>> 
>> On 17 September 2017 at 21:45, Bin Meng  wrote:
>>> Hi Simon,
>>> 
>>> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
 Add the logging header file and implementation with some configuration
 options to control it.
 
 Signed-off-by: Simon Glass 
 ---
 
 MAINTAINERS   |   9 ++
 common/Kconfig|  56 +
 common/Makefile   |   1 +
 common/log.c  | 246 
 +
 include/asm-generic/global_data.h |   5 +
 include/log.h | 247 
 --
 6 files changed, 555 insertions(+), 9 deletions(-)
 create mode 100644 common/log.c
 
 diff --git a/MAINTAINERS b/MAINTAINERS
 index 04acf2b89d..eb420afa8d 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -290,6 +290,15 @@ S: Maintained
 T: git git://git.denx.de/u-boot-i2c.git
 F: drivers/i2c/
 
 +LOGGING
 +M: Simon Glass 
 +S: Maintained
 +T: git git://git.denx.de/u-boot.git
 +F: common/log.c
 +F: cmd/log.c
 +F: test/log/log_test.c
 +F: test/py/tests/test_log.py
>>> 
>>> test/log/log_test.c and test/py/tests/test_log.py have not been
>>> introduced at this point.
>> 
>> OK I can tweak that,
>> [..]
>> 
 +/**
 + * struct log_driver - a driver which accepts and processes log records
 + *
 + * @name: Name of driver
 + */
 +struct log_driver {
 +   const char *name;
 +   /**
 +* emit() - emit a log record
 +*
 +* Called by the log system to pass a log record to a particular 
 driver
 +* for processing. The filter is checked before calling this 
 function.
 +*/
 +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
 +};
 +
>>> 
>>> So we are creating a new type of non-DM driver which is log-specific?
>>> How about we add this emit to the existing uclass driver that can be
>>> used as the log driver? (eg: blk devices with file system?)
>> 
>> Yes that's right. I think I can link it to DM once it starts up, but a
>> logging of DM start-up is useful.
>> 
>> Re your idea are you saying add an emit() method to UCLASS_BLK?
>> 
> 
> Yep, something like
> 
> #ifdef CONFIG_LOG
>.log_emit = xxx_log_emit,
> #endif
> 
> Probably we need a flag to find out which driver provides such log
> functionality.

I had started to sketch (but have been able to fully flesh this out, due to 
other priorities intervening) a mechanism for MISC devices that might
be a good fit for this.

My work centers around the idea of having devices comply to “protocols”
and was aimed at distinguishing between an efuse-device and a GbE 
RGMII-control (one where we need to adjust the RGMII clock depending
on the link rate the PHY negotiated) device.

I.e. I had started to implement something along the lines of:

/**
 * misc_supports - tests if a misc device complies to a given protocol
 *
 * protocols can either be ‘how data is processed after calling 
write()’,
 * ‘how data is presented for a read()’ or ‘what ioctl-values are 
supported’.
 * The assumption is that protocols can be versioned and higher versions
 * offer full backward compatibility (i.e. a client for “Ethernet PHY 
control, v1”
 * can work with a driver implementing v1 or any higher version).
 */
bool misc_supports(struct udevice *dev, const char *protocol, u32 
version);

and a way to register a list of protocols as part of the misc uclass-priv for
any given driver.

This would allow us to enumerate all MISC devices until we find one that
complies to the ‘logging’-protocol and then invoke write or ioctl on it.

Regards,
Philipp.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Bin Meng]

Hi Simon,

On Wed, Sep 20, 2017 at 9:50 PM, Simon Glass  wrote:
> Hi Bin,
>
> On 17 September 2017 at 21:45, Bin Meng  wrote:
>> Hi Simon,
>>
>> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
>>> Add the logging header file and implementation with some configuration
>>> options to control it.
>>>
>>> Signed-off-by: Simon Glass 
>>> ---
>>>
>>>  MAINTAINERS   |   9 ++
>>>  common/Kconfig|  56 +
>>>  common/Makefile   |   1 +
>>>  common/log.c  | 246 
>>> +
>>>  include/asm-generic/global_data.h |   5 +
>>>  include/log.h | 247 
>>> --
>>>  6 files changed, 555 insertions(+), 9 deletions(-)
>>>  create mode 100644 common/log.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 04acf2b89d..eb420afa8d 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -290,6 +290,15 @@ S: Maintained
>>>  T: git git://git.denx.de/u-boot-i2c.git
>>>  F: drivers/i2c/
>>>
>>> +LOGGING
>>> +M: Simon Glass 
>>> +S: Maintained
>>> +T: git git://git.denx.de/u-boot.git
>>> +F: common/log.c
>>> +F: cmd/log.c
>>> +F: test/log/log_test.c
>>> +F: test/py/tests/test_log.py
>>
>> test/log/log_test.c and test/py/tests/test_log.py have not been
>> introduced at this point.
>
> OK I can tweak that,
> [..]
>
>>> +/**
>>> + * struct log_driver - a driver which accepts and processes log records
>>> + *
>>> + * @name: Name of driver
>>> + */
>>> +struct log_driver {
>>> +   const char *name;
>>> +   /**
>>> +* emit() - emit a log record
>>> +*
>>> +* Called by the log system to pass a log record to a particular 
>>> driver
>>> +* for processing. The filter is checked before calling this 
>>> function.
>>> +*/
>>> +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
>>> +};
>>> +
>>
>> So we are creating a new type of non-DM driver which is log-specific?
>> How about we add this emit to the existing uclass driver that can be
>> used as the log driver? (eg: blk devices with file system?)
>
> Yes that's right. I think I can link it to DM once it starts up, but a
> logging of DM start-up is useful.
>
> Re your idea are you saying add an emit() method to UCLASS_BLK?
>

Yep, something like

#ifdef CONFIG_LOG
.log_emit = xxx_log_emit,
#endif

Probably we need a flag to find out which driver provides such log
functionality.

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Dr. Philipp Tomsich]

Masahiro & Simon,

> On 20 Sep 2017, at 15:49, Simon Glass  wrote:
> 
> Hi Masahiro,
> 
> On 19 September 2017 at 20:51, Masahiro Yamada
>  wrote:
>> Hi Simon,
>> 
>> 
>> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>> 
>>> 
>>> +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…)

> 
>> 
>> 
>> 
>> 
>>> +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).

Regards,
Philipp.

> 
>> 
>> 
>> 
>> 
>>> +config LOG_SPL_MAX_LEVEL
>>> +   int "Maximum log level to record in SPL"
>>> +   depends on SPL_LOG
>>> +   default 3
>>> +   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
>>> 
>> 
>> 
>> If you want to use CONFIG_VAL(LOG_MAX_LEVEL),
>> this must be SPL_LOG_MAX_LEVEL.
>> (this coding mistake is now hidden by another mistake)
> 
> Oops, yes.
> 
>> 
>> 

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

[Simon Glass]

Hi Masahiro,

On 19 September 2017 at 20:51, Masahiro Yamada
 wrote:
> Hi Simon,
>
>
> 2017-09-17 6:23 GMT+09:00 Simon Glass :
>
>>
>> +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.

>
>
>
>
>> +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?

>
>
>
>
>> +config LOG_SPL_MAX_LEVEL
>> +   int "Maximum log level to record in SPL"
>> +   depends on SPL_LOG
>> +   default 3
>> +   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
>>
>
>
> If you want to use CONFIG_VAL(LOG_MAX_LEVEL),
> this must be SPL_LOG_MAX_LEVEL.
> (this coding mistake is now hidden by another mistake)

Oops, yes.

>
> Again, you will probably end up with TPL_LOG_MAX_LEVEL.
>
>
>
>
>> +
>> +/**
>> + * log_dispatch() - Send a log record to all log devices for processing
>> + *
>> + * The log record is sent to each log device in turn, skipping those which 
>> have
>> + * filters which block the record
>> + *
>> + * @rec: Log record to dispatch
>> + * @return 0 (meaning success)
>> + */
>> +static int log_dispatch(struct log_rec *rec)
>> +{
>> +   struct log_device *ldev;
>> +
>> +   list_for_each_entry(ldev, >log_head, sibling_node) {
>> +   if (log_passes_filters(ldev, rec))
>> +   ldev->drv->emit(ldev, rec);
>> +   }
>> +
>> +   return 0;
>> +}
>> +
>> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,
>> +int line, const char *func, const char *fmt, ...)
>> 

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

[Simon Glass]

Hi Bin,

On 17 September 2017 at 21:45, Bin Meng  wrote:
> Hi Simon,
>
> On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
>> Add the logging header file and implementation with some configuration
>> options to control it.
>>
>> Signed-off-by: Simon Glass 
>> ---
>>
>>  MAINTAINERS   |   9 ++
>>  common/Kconfig|  56 +
>>  common/Makefile   |   1 +
>>  common/log.c  | 246 
>> +
>>  include/asm-generic/global_data.h |   5 +
>>  include/log.h | 247 
>> --
>>  6 files changed, 555 insertions(+), 9 deletions(-)
>>  create mode 100644 common/log.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 04acf2b89d..eb420afa8d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -290,6 +290,15 @@ S: Maintained
>>  T: git git://git.denx.de/u-boot-i2c.git
>>  F: drivers/i2c/
>>
>> +LOGGING
>> +M: Simon Glass 
>> +S: Maintained
>> +T: git git://git.denx.de/u-boot.git
>> +F: common/log.c
>> +F: cmd/log.c
>> +F: test/log/log_test.c
>> +F: test/py/tests/test_log.py
>
> test/log/log_test.c and test/py/tests/test_log.py have not been
> introduced at this point.

OK I can tweak that,
[..]

>> +/**
>> + * struct log_driver - a driver which accepts and processes log records
>> + *
>> + * @name: Name of driver
>> + */
>> +struct log_driver {
>> +   const char *name;
>> +   /**
>> +* emit() - emit a log record
>> +*
>> +* Called by the log system to pass a log record to a particular 
>> driver
>> +* for processing. The filter is checked before calling this 
>> function.
>> +*/
>> +   int (*emit)(struct log_device *ldev, struct log_rec *rec);
>> +};
>> +
>
> So we are creating a new type of non-DM driver which is log-specific?
> How about we add this emit to the existing uclass driver that can be
> used as the log driver? (eg: blk devices with file system?)

Yes that's right. I think I can link it to DM once it starts up, but a
logging of DM start-up is useful.

Re your idea are you saying add an emit() method to UCLASS_BLK?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Masahiro Yamada]

Hi Simon,


2017-09-17 6:23 GMT+09:00 Simon Glass :

>
> +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.




> +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




> +config LOG_SPL_MAX_LEVEL
> +   int "Maximum log level to record in SPL"
> +   depends on SPL_LOG
> +   default 3
> +   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
>


If you want to use CONFIG_VAL(LOG_MAX_LEVEL),
this must be SPL_LOG_MAX_LEVEL.
(this coding mistake is now hidden by another mistake)

Again, you will probably end up with TPL_LOG_MAX_LEVEL.




> +
> +/**
> + * log_dispatch() - Send a log record to all log devices for processing
> + *
> + * The log record is sent to each log device in turn, skipping those which 
> have
> + * filters which block the record
> + *
> + * @rec: Log record to dispatch
> + * @return 0 (meaning success)
> + */
> +static int log_dispatch(struct log_rec *rec)
> +{
> +   struct log_device *ldev;
> +
> +   list_for_each_entry(ldev, >log_head, sibling_node) {
> +   if (log_passes_filters(ldev, rec))
> +   ldev->drv->emit(ldev, rec);
> +   }
> +
> +   return 0;
> +}
> +
> +int _log(enum log_category_t cat, enum log_level_t level, const char *file,
> +int line, const char *func, const char *fmt, ...)
> +{
> +   char buf[CONFIG_SYS_CBSIZE];
> +   struct log_rec rec;
> +   va_list args;
> +
> +   rec.cat = cat;
> +   rec.level = level;
> +   rec.file = file;
> +   rec.line = line;
> +   rec.func = func;
> +   va_start(args, fmt);
> +   vsnprintf(buf, sizeof(buf), fmt, args);
> +   va_end(args);
> +   rec.msg = buf;
> +   if (!gd || !(gd->flags & GD_FLG_LOG_READY)) {
> +   if (gd)
> +   gd->log_drop_count++;
> +   return -ENOSYS;
> +   }
> +   log_dispatch();
> +
> +   return 0;
> +}
> +
> +int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
> +  enum log_level_t max_level, const char *file_list)
> +{
> +   struct log_filter *filt;
> +   struct log_device *ldev;
> +   int i;
> +
> +   ldev = log_device_find_by_name(drv_name);
> +   if (!ldev)
> +   return -ENOENT;
> +   filt = (struct log_filter *)calloc(1, sizeof(*filt));
> +   if (!filt)
> +   return -ENOMEM;
> +
> +   if (cat_list) {
> +   filt->flags |= LOGFF_HAS_CAT;
> +   for (i = 

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

[Bin Meng]

Hi Simon,

On Sun, Sep 17, 2017 at 5:23 AM, Simon Glass  wrote:
> Add the logging header file and implementation with some configuration
> options to control it.
>
> Signed-off-by: Simon Glass 
> ---
>
>  MAINTAINERS   |   9 ++
>  common/Kconfig|  56 +
>  common/Makefile   |   1 +
>  common/log.c  | 246 +
>  include/asm-generic/global_data.h |   5 +
>  include/log.h | 247 
> --
>  6 files changed, 555 insertions(+), 9 deletions(-)
>  create mode 100644 common/log.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 04acf2b89d..eb420afa8d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -290,6 +290,15 @@ S: Maintained
>  T: git git://git.denx.de/u-boot-i2c.git
>  F: drivers/i2c/
>
> +LOGGING
> +M: Simon Glass 
> +S: Maintained
> +T: git git://git.denx.de/u-boot.git
> +F: common/log.c
> +F: cmd/log.c
> +F: test/log/log_test.c
> +F: test/py/tests/test_log.py

test/log/log_test.c and test/py/tests/test_log.py have not been
introduced at this point.

> +
>  MICROBLAZE
>  M: Michal Simek 
>  S: Maintained
> diff --git a/common/Kconfig b/common/Kconfig
> index 4d8cae9610..cbccc8ae26 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -384,6 +384,62 @@ config SYS_STDIO_DEREGISTER
>
>  endmenu
>
> +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.
> +
> +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
> +
> +config LOG_SPL_MAX_LEVEL
> +   int "Maximum log level to record in SPL"
> +   depends on SPL_LOG
> +   default 3
> +   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
> +
> +endmenu
> +
>  config DTB_RESELECT
> bool "Support swapping dtbs at a later point in boot"
> depends on FIT_EMBED
> diff --git a/common/Makefile b/common/Makefile
> index 1b56cf9a70..d37c8d5636 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -128,5 +128,6 @@ obj-y += cli.o
>  obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o
>  obj-$(CONFIG_CMD_DFU) += dfu.o
>  obj-y += command.o
> +obj-$(CONFIG_$(SPL_)LOG) += log.o
>  obj-y += s_record.o
>  obj-y += xyzModem.o
> diff --git a/common/log.c b/common/log.c
> new file mode 100644
> index 00..6bf2219d38
> --- /dev/null
> +++ b/common/log.c
> @@ -0,0 +1,246 @@
> +/*
> + * Logging support
> + *
> + * Copyright (c) 2017 Google, Inc
> + * Written by Simon Glass 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static struct log_device *log_device_find_by_name(const char *drv_name)
> +{
> +   struct log_device *ldev;
> +
> +   list_for_each_entry(ldev, >log_head, sibling_node) {
> +   if (!strcmp(drv_name, ldev->drv->name))
> +   return ldev;
> +   }
> +
> +   return NULL;
> +}
> +
> +/**
> + * log_has_cat() - check if a log category exists within a list
> + *
> + * @cat_list: List of categories to check, at most LOGF_MAX_CATEGORIES 
> entries
> + * long, terminated by LC_END if fewer
> + * @cat: Category to search for
> + * @return true if @cat is in @cat_list, else false
> + */
> +static bool log_has_cat(enum log_category_t cat_list[], enum log_category_t 
> cat)
> +{
>